Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keyboard Control In Scene Tree Doesn't Select Correct Node #36291

Closed
cybereality opened this issue Feb 17, 2020 · 16 comments
Closed

Keyboard Control In Scene Tree Doesn't Select Correct Node #36291

cybereality opened this issue Feb 17, 2020 · 16 comments

Comments

@cybereality
Copy link
Contributor

Godot version:
3.2 stable (Steam)

OS/device including version:
Ubuntu Linux 64-bit 19.10

Issue description:
When using the arrow keys on the keyboard to navigate through the scene tree, the correct node is not selected. This behavior seems to be based on class type. For example, if all the nodes are Node2D, then it works, but by adding a CanvasLayer, then it doesn't work. This is a problem because if you press Delete on the keyboard, it now deletes the incorrect node (especially important for people with impaired sight and relying on keyboard and screen readers).

Steps to reproduce:
Create a new project, add two Node2D objects, then add a CanvasLayer in-between them. Try pressing up and down on the arrow keys and you will see the correct node is not selected (and pressing delete will delete the previous node, not the current one).

Minimal reproduction project:
Attached is a minimal project with the problem.
KeyTest.zip

@KoBeWi
Copy link
Member

KoBeWi commented Feb 17, 2020

Yeah, this is a problem with focus being different than "selected" item. It causes other issues like this #35876

This probably works like intended, but I wonder if the focus thing is necessary.

@ndarilek
Copy link
Contributor

If by "focus thing" you mean updating tree selection based on keyboard input, it is definitely needed for my Godot accessibility work. Without it, using Godot as a blind developer is very difficult as I have to hand-edit .tscn files, update resource/subresource IDs manually, etc. Even outside of development, I assume this may be broken for other arbitrary trees, ruling out the possibility of building a screen-reader-accessible Godot game that needs a tree.

@KoBeWi
Copy link
Member

KoBeWi commented Feb 17, 2020

If by "focus thing" you mean updating tree selection based on keyboard input

No, I mean that you have separate selection and focus, resulting in two rectangles on the Tree node, with the focused item being used for the operations.
image
The worst is that it's inconsistent, you can properly select nodes when they are 2D nodes as noted in the OP.

@ndarilek
Copy link
Contributor

ndarilek commented Feb 17, 2020 via email

@ndarilek
Copy link
Contributor

I'm taking a crack at fixing this, but don't fully understand the failure case.

Is the issue that arrowing through the tree in the included sample project unifies focus and selection for a time, but as soon as focus passes the canvas, it splits into focus and selection? Deleting behaves inconsistently, but attempting to rename nodes with Enter always seems reliable.

@cybereality
Copy link
Contributor Author

Okay, awesome.

So download the project I posted in the OP. Click on the root node (Game). Then use the down arrow key on the keyboard to move down the tree. Notice on the first 3 nodes, it works like expected, pressing down will focus the next node on the list.

But when you get to the CanvasLayer, you only get a outline around the node, not a full change of color (I guess this is the difference between focus and selection, which I don't understand). Now if you press delete on the keyboard, it deletes Sprite, not CanvasLayer, which seems like a bug.

Renaming does seem to work, and the Inspector shows CanvasLayer (not Sprite) so I am unsure of why there needs to be a separate focus and selection (maybe there is a good reason I don't understand). If would seem to be more straightforward if selection and focus were the same thing.

The problem does not happen as long as the nodes in question are of the same class (or sharing a parent class). For example, going from Node2D to Sprite works, but moving from Node2D to Timer does not work.

Please let me know if that is enough information. Thanks.

@ndarilek
Copy link
Contributor

ndarilek commented Apr 27, 2020 via email

@cybereality
Copy link
Contributor Author

So there are two states.

One is with the node shown with a solid filled in rectangle around the name. It is what is shown as the node in the Inspector (this is always correct). This is also the one that you will delete when pressing Delete. You can also rename this one while it is the only thing selected. To me, this state is the one that is working, and the one we should model after.

The second is shown with an empty rectangle around the name. This is the one that is can be renamed when pressing Enter. It is also shown in the Inspector. But when pressing Delete it will delete the previous node with the solid rectangle (usually one above or below, depending on what you pressed on the keyboard).

As mentioned, the Inspector is always showing the correct node, so you may want to see what changes the Inspector and then change the selection/focus to that. Exposing them through signals seems like a valid approach, as there may be some reason for the selection/focus feature that I don't understand. I mean, there is definitely a bug there, but it's hard to fix if we don't know what the intended behavior is.

Hope that helps.

@cybereality
Copy link
Contributor Author

To answer your other question, it does happen on other trees. I tested in the File System box (the one with the project files) and it seemed to show similar behavior (and I'm not 100% it was working either, it did seem kind of strange).

@ndarilek
Copy link
Contributor

ndarilek commented Apr 27, 2020 via email

@cybereality
Copy link
Contributor Author

I'd be willing to help you fix this, but I don't understand what the intended functionality is. We really need someone familiar with the code base to explain why there are selection and focus and how it's supposed to work.

@KoBeWi
Copy link
Member

KoBeWi commented Apr 27, 2020

You could try going to godot-devel IRC and asking there. With that many issues on the repo, it's the easiest way to get some answers.

@follower
Copy link
Contributor

follower commented Aug 4, 2020

Hey, so, I've spent a ton of time debugging issues with the Tree control and keyboard navigation for a project of mine.

I'd started writing some of them up in an issue but hadn't yet finished because I have far more issues to write up & projects to finish than I have time/money. :/

But I've just seen the comments that the Tree control is affecting @ndarilek's accessibility work (which I think is really important & really want to support) so I have just posted my draft/wip as it is right now: #41014

I also included a quick copy/paste of some code I wrote to workaround some of the issues--I don't know if any of it is going to be any use in the its current form but hopefully it's better than nothing.

My suggestion for trying to get a handle on things is to make a build of the engine as it was before the commit (linked in #41014) that changed the keyboard navigation from direct keyboard input to the action input, test what works & what doesn't and then look at the changes made in the subsequent commit.

Overlay personal disclosure: I'm sorry that I can't promise to be responsive to questions about the issues because (a) ADHD & (b) I'm trying to get some projects out the door to try to (maybe :/ ) improve things financially but I'll try to provide further help if I can.

follower referenced this issue in lightsoutgames/godot-accessibility Aug 4, 2020
Trees appear to eat "ui_accept" before `gui_input` receives it. This makes certain use cases--triggering buttons in cells--impossible via the keyboard.

Here, `ScreenReader` attempts to intercept recent presses of "ui_accept" and, if a button in a cell should receive focus, redirects the `InputEvent` to the custom handler for `Tree`.

Unfortunately, this loses the ability to click buttons in cells in the editor's node tree for some reason. It restores, or maybe gets working in the first place, clicking on the _Remove_ button in the input mapping screen. Not sure how to restore the former functionality--these trees are complex and not well-documented.
@abraaofilho10
Copy link

I didn't see this post and I posted a proposal here some time ago. I didn't know if it was a bug or something missing. I think the current selection behavior is very confusing.

godotengine/godot-proposals#1043

@follower
Copy link
Contributor

follower commented Sep 3, 2021

If you would like to help this issue to be fixed (without writing code)

If someone would like to help move this issue forward (particularly if you'd like to help but can't do so directly on the C++ coding side) here are three tasks that would be extremely helpful toward fixing this & a number of other issues with the Tree control and help prevent future similar regressions:

  1. Create a table/grid/matrix (maybe in a spreadsheet, maybe just something in an issue comment) with a slot for each permutation of UI interaction with the Tree control. (Or, at least, as many as you can manage, I think there's roughly sixty billionty permutations. :D )
    For example, you could start with:
    1. Actions: Select item, de-select item, move to previous item, move to next item, etc
    2. Input device: Mouse, keyboard (e.g. "left click single click", "left click double click", "right single click", "select with space bar", "select with enter", then all those with "shift"/"ctrl"/"alt"/"command"/"option" modifier keys,)
    3. Selection mode: Cell select, Row select, Multi-select
    4. Control type: "Standard" controls, custom controls, non-selectable controls
  2. Then, with a Tree control (either one in the editor itself, or one in a test project) in, say, the Godot 3.3 release, go through and test each permutation of the above 4 categories and note whether or not it works as expected.
  3. Either build or download a Godot version released before GUI elements ui_action usage, improvements #16947 was merged, perform the same tests in the earlier version and note the differences in behaviour.

What would completing these tasks accomplish?

Completing these tasks will:

  1. Enable us to determine what was broken by the changes so we can at least get the previous level of functionality back.
  2. Potentially identify other permutations that were broken then & still are now.
  3. Provide a test suite that can be used to assess whether any future changes actually fix a particular bug without creating more (which is almost inevitable with a control this complex without a test suite).

(This is essentially a restatement of my suggestion here in a hopefully more useful form: #41014 (comment))

Helping create this test matrix & collect the results would be a great help toward fixing the accessibility & usability & correctness of the Tree control.

My thanks to anyone who considers taking some or all of these tasks on. :)

@KoBeWi
Copy link
Member

KoBeWi commented Jan 3, 2022

Closing as duplicate of #12544

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants