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

[List] Add keyboard navigation #15421

Closed
wants to merge 1 commit into from

Conversation

mbrookes
Copy link
Member

@mbrookes mbrookes commented Apr 20, 2019

Continues #15334 (I must have accidentally clicked "Close" when replying to @eps1lon.)

This is still very much a discussion draft / WIP, as the tabIndex code would also need to move, tests fixed up etc. I wish "draft" was the default PR option, since you can convert a draft PR to open, but not the other way around!

@mbrookes
Copy link
Member Author

mbrookes commented Apr 20, 2019

@eps1lon Regarding:

One issue is that it doesn't work for deploy-preview-15334--material-ui.netlify.com/demos/lists/#checkbox.

I might be missing what the problem is, but it seems to behave the same as in next, other than the change in keyboard handling from tab to arrow keys?

@eps1lon
Copy link
Member

eps1lon commented Apr 20, 2019

https://deploy-preview-15421--material-ui.netlify.com/demos/lists/#checkbox:

first example

  1. tab into list
  2. listitem has selected state
  3. pressing enter toggles checkbox state
    4a. Tab would focus the comment? action
    4b. arrow keys do nothing, expected select next item

2nd example

  1. tab into list
  2. listitem has selected state
  3. pressing enter does nothing? expected the checkbox to change state like the 1st example
    4a. tab focuses the checkbox
    4aa. pressing enter does not change checkbox state
    4b. arrows keys do nothing. expected next listitem to become focused

Ideally both demos would have checkboxes that can be toggled via keyboard and arrow navigation would work.

@eps1lon eps1lon added accessibility a11y component: list This is the name of the generic UI component, not the React module! new feature New feature or request labels Apr 20, 2019
@ryancogswell
Copy link
Contributor

ryancogswell commented Apr 21, 2019

My changes in #15360 move some logic out of MenuList into Menu such that the keyboard navigation is just about the only thing left in MenuList. This means that someone could fairly easily opt-in to this functionality by simply using MenuList instead of List. I wonder if that may be an easier-to-maintain solution (keeping this encapsulated within MenuList with that being the primary responsibility of MenuList) rather than trying to share this functionality between List and MenuList.

The key handling is about to get more complicated since I've now started working on #8191 which will require remembering some information across key events. This means that the handleKeyDown function will be leveraging a ref, so to share this between List and MenuList it would have to become a custom hook (e.g. useHandleKeyDown). This would be doable, but seems more complicated than keeping this encapsulated within MenuList.

Another option would be to just move the functionality from MenuList into List so that MenuList becomes a nearly empty layer that just passes all its props through to List.

@mbrookes
Copy link
Member Author

mbrookes commented Apr 21, 2019

rather than trying to share this functionality between List and MenuList.

It isn't shared any more, that was just a poor abstraction on my part. The keyboard functionality now resides in List only on my local branch, which MenuList picks up by wrapping List.

Another option would be to just move the functionality from MenuList into List so that MenuList becomes a nearly empty layer that just passes all its props through to List.

Oops, should have read the whole way down. Yes, exactly this.

I think trying to manage these separate efforts as discrete PRs and merging them later may be more work than combining them at the outset. If you're happy to include this change as part of your PR, then we can close this one.

@ryancogswell
Copy link
Contributor

I think trying to manage these separate efforts as discrete PRs and merging them later may be more work than combining them at the outset. If you're happy to include this change as part of your PR, then we can close this one.

I definitely agree that this should be done with the changes in #15360 as the starting point, since there are significant changes to the keyboard focus logic. I'm happy to take care of the changes, but I'm not sure I want to add it into #15360 since there are already a number of different things in it, and I don't want to complicate its review and derail it getting merged by adding more changes to it.

I have some further thoughts/questions/concerns around the details of how this should work. I'll take some time to write up my thoughts within the next day or two.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility a11y component: list This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants