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

Tabbing into a selectableLazyColumn with no selections pre-made does not allow keyboard navigation #357

Closed
YasserDbeis opened this issue Apr 15, 2024 · 8 comments · Fixed by #372
Assignees
Labels
bug Something isn't working

Comments

@YasserDbeis
Copy link
Collaborator

If the user has not selected anything from a SelectableLazyColumn, and navigates to the SelectableLazyColumn using tab, then the arrow keys do not seem to be detected. I was able to reproduce this issue by using the ide-sample's Compose Panel tab's list of releases as the SelectableLazyColumn.

Note: this has been encountered with #347 merged.

@rock3r rock3r added the bug Something isn't working label Apr 16, 2024
@fscarponi
Copy link
Collaborator

This behavior is a known issue in the swing feature pair. You can verify it in the IDE: when a "LazyColumnLike" component (such as the project tree) gains focus, the first item is selected. This allows you to navigate freely using arrow keys. The same applies to SLC if you set a selection either through clicking or programmatically.

Why is the scenario of "selecting the first element" not desirable?
Unlike Swing, the lazy column is not fully loaded into memory, so it's possible that an element with key k is selected even if it's not currently in the visible range... In that case? should the list scroll or what? Other possible scenarios come to mind that would need to be addressed...

However, nothing prevents the user from setting the selected element by manipulating the state during the construction of the list!
It would also be possible to add the key of the first element to the set of selected items if it's empty, but this would result in losing support for "nothing selected."

Feel free to suggest any other solution!

@fscarponi fscarponi added the question Further information is requested label Apr 22, 2024
@rock3r
Copy link
Collaborator

rock3r commented Apr 22, 2024

I think Swing selects the first visible item, or the last selected item, when a list gains focus. We should do the same thing.

If the last selected item key doesn't exist anymore, I'd select the first visible one. I think that's what Swing does (but will double check). I agree with Yasser that focusing the component should allow you to keyboard navigate immediately

@rock3r
Copy link
Collaborator

rock3r commented Apr 22, 2024

Screen recording of Swing list behaviour

So, I checked and the Swing behaviour is as follows:

  1. When focusing a list, either the previous selection is kept, or the first item gets selected (if there is any item in the list)
  2. When the current selection is gone (e.g., item removed), the item above (i.e., selectedIndex - 1) is selected
  3. If the item was the first one, then the next one is selected (i.e., the selection remains on selectedIndex even if the item is now different)
  4. If there are no items left, the selection is null
  5. The selection can only be null when the list is empty, or if it has never received focus before

On multi-select lists, it pretty much follows the same rules. The only difference is that when there are multiple items selected and one of them disappears, the item also disappears from the selection, but other selected items remain selected. If all selected items are removed, the behaviour is the same as for the single-select list.

We should align to this behaviour. If the list is empty when it receives focus, it'll have no selection, but as soon as the user uses the keyboard to move the selection, a selection is created on the first item. If apps want to create a selection once they add data to the list, they're free to do so, but we won't do it for them.

@YasserDbeis
Copy link
Collaborator Author

I agree with the proposed solution. Thanks for the investigation!

@rock3r rock3r removed the question Further information is requested label Apr 27, 2024
@rock3r
Copy link
Collaborator

rock3r commented Apr 27, 2024

@fscarponi please look into this when you have time 🙏

@fscarponi
Copy link
Collaborator

Hi Folks,
there is not too much effort to get the desired result, except for point 2...
The current implementation of the data structure is not minded to store ordered keys, so I'm not sure if it is possible to achieve this behavior without rethinking the key management, we need to investigate deeper. (keep in mind that the SLC supports selectable AND not-selectable elements, such as the headers)
The problem is that I'm busy at the moment and can't provide an ETA. If you need this asap, I can try to discuss/schedule this task with my TL

@YasserDbeis
Copy link
Collaborator Author

@fscarponi The only thing on my end needed ASAP is the ability to tab into the selectableLazyColumn and use arrow keys when there is no selection made before (the original problem). I say this because the Profiler needs to support full keybaord navigation; one of the major components on opening the UI is a selectableLazyColumn, and the user cannot make their selection if they are purely using the keyboard. I am okay with the other behavior coming later if that is alright.

@fscarponi fscarponi linked a pull request Apr 30, 2024 that will close this issue
@fscarponi
Copy link
Collaborator

fscarponi commented Apr 30, 2024

#372 add an hotfix for the point 1

When focusing a list, either the previous selection is kept, or the first item gets selected (if there is any item in the list)

to implement other behaviors is possible but requires more investigations, keeping this state easily savable and consistent.
I will take care to create a new issue for these features

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants