Use useFocusZone hook in ActionList

[bolonio]

What are you trying to accomplish?

Some accessibility issues in the Primer React ActionList component have been discovered during the Primer React SelectPanel component accessibility engineering review, and since the SelectPanel uses the component in the body.

This PR is to fix the focus and navigation issue in the Primer React ActionList component.

• https://github.com/github/primer/issues/1045

What approach did you choose and why?

Use useFocusZone hook in ActionList

const {containerRef} = useFocusZone({bindKeys: FocusKeys.ArrowVertical | FocusKeys.HomeAndEnd})

Now the ActionList is one single tab stop and the user will navigate the elements using up/down arrows or home/end keys

Screenshots

N/A

Merge checklist

• [ ] Added/updated tests
• [ ] Added/updated documentation
• [x] Tested in Chrome
• [ ] Tested in Firefox
• [x] Tested in Safari
• [ ] Tested in Edge

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: 0d3dfacfb474c051b38cc6765fa1a12a9bad41c3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions[bot]]

size-limit report 📦

Path	Size
dist/browser.esm.js	67.68 KB (+0.1% 🔺)
dist/browser.umd.js	68.05 KB (+0.11% 🔺)

[siddharthkp]

A few questions here:

1. Should disabled items get focus?

2. In Action list with links, it looks like it requires 2 keypresses to move through the list, because the focus also goes to the anchor tag as well <a>

   repro: (see live story)

   https://user-images.githubusercontent.com/1863771/176439258-fad80b1f-1357-4626-a45a-fc5be7ca2505.mov

3. Action menu composes Action list and implements a focus zone, should we disable focus zone on Action list if it's part of a menu? (we can tell by looking at container in ActionListContainerContext view source.

   on main branch: (see live story)

   https://user-images.githubusercontent.com/1863771/176440516-7463a904-60b3-49c4-b0de-92ce2137db05.mov

   on feature branch: (see live story)

   https://user-images.githubusercontent.com/1863771/176440550-22a4d14f-722a-4977-85fa-70c1ebc5f61e.mov

[bolonio]

@siddharthkp Thanks for your questions :)

Let me answer them:

Should disabled items get focus?

As @jscholes mentioned a few times (and actually strongly against it), listbox should not include disabled items. It is an a11y antipattern and it makes no sense. We could suggest though to add a described by are saying something like Some labels have been hidden and don’t apply to this list.

In Action list with links, it looks like it requires 2 keypresses to move through the list, because the focus also goes to the anchor tag as well <a>

We mentioned it during one of our meetings with @jscholes . The ActionList is a component for single or multi selection, so including a link inside the <li> element won’t work. So arrow keys should navigate through all ActionList items. After arriving to an item, you can press Space to select/deselect it. If you want links inside, then you would need a different structure (James mentioned plain list with checkboxes and links. But that’s something design needs to review it.

Action menu composes Action list and implements a focus zone, should we disable focus zone on Action list if it's part of a menu? (we can tell by looking at container in ActionListContainerContext view source.

That use case for the ActionMenu should have 2 tab stops. 1) The input 2) The listbox. Once inside the listbox, then navigations should be done using up/down arrows.

[siddharthkp]

As @jscholes mentioned a few times (and actually strongly against it), listbox should not include disabled items. It is an a11y antipattern and it makes no sense. We could suggest though to add a described by are saying something like Some labels have been hidden and don’t apply to this list.

Would be nice to catch any existing usage of ActionList with disabled items so that we can remedy them as well.

Follow up, does the same apply to role=menu as well?

The ActionList is a component for single or multi selection, so including a link inside the

element won’t work.

That's not entirely true 😅 ActionList is used as a building block for a bunch of components. [see also: design guidelines and NavList.

Would it be wrong to add keyboard navigation for NavList or similar list of links?

That use case for the ActionMenu should have 2 tab stops. 1) The input 2) The listbox. Once inside the listbox, then navigations should be done using up/down arrows.

I can see how that would work! Do we include that in this pull request? The current behavior doesn't implement tab stops either but does jump to the input, so we're breaking something here 😅

[bolonio]

Would be nice to catch any existing usage of ActionList with disabled items so that we can remedy them as well. Follow up, does the same apply to role=menu as well?

Good question, we should consult to @jscholes before.

That's not entirely true 😅 ActionList is used as a building block for a bunch of components. [see also: design guidelines and NavList. Would it be wrong to add keyboard navigation for NavList or similar list of links?

You're right, I meant an ActionList build as <ul role="listbox"> with items <li role="option">. The NavList component doesn't have any role, so it's effectively just a <ul> with <li> containing <a>. We might need to rethink the focus behaviour or not apply the useFocus hook in this case. I might consult @jscholes for that.

I can see how that would work! Do we include that in this pull request? The current behavior doesn't implement tab stops either but does jump to the input, so we're breaking something here 😅

This PR only updates the ActionList component. I guess the Primer team can take a look at every component that uses the ActionList to adapt them. What do you think @hectahertz?

[jscholes]

Having disabled items in a menu is fine. Menus contain actions, and one or more of those actions may simply not be available at a specific point. A listbox contains items that you can do things with (in this case, select/de-select), and those items are not simple actions as they would be in a menu. So disabling them will not make sense to users, and a screen reader may not convey the state.

In terms of everything else here, I've repeatedly said that Action List, as a concept, is being completely overloaded. A list of links, listbox of multiselect items, and menu of actions are completely different constructs with unique semantics and acceptable behaviours. It is not possible, or accessible, to make decisions relating to those semantics or behaviours that apply to all uses of Action List.

[bolonio]

In terms of everything else here, I've repeatedly said that Action List, as a concept, is being completely overloaded. A list of links, listbox of multiselect items, and menu of actions are completely different constructs with unique semantics and acceptable behaviours. It is not possible, or accessible, to make decisions relating to those semantics or behaviours that apply to all uses of Action List.

I agree with you @jscholes, that's why we're having issues like @siddharthkp raised about using the NavList. We might want to re-evaluate the uses of the ActionList, and maybe creating new components for the different behaviors, roles, or components we use.

I will keep this PR open for the sake of documentation. It might be helpful for people like @hectahertz who's doing accessibility reviews of the Primer components.

Thanks @siddharthkp and @jscholes for the help :)

[LisaKr]

hey @hectahertz, we in cybercats are working on a PR which is based off of this PR (incorporating the new and improved ActionList). @bolonio has notified us that you are taking over this work from him. We are wondering what is the timeline of this getting merged so we understand the implications for our work and whether the plan for this is still to be merged in the near future? cc @traumverloren @reneexeener @azenmatt @lukewar

[LisaKr]

and another question we thought of @hectahertz, do you imagine the final state of the new AL (after you have worked on this PR) to be significantly different from what the current state of this PR? We are refactoring a custom element into an AL and we want to understand what behavior we can approximately expect from it in the future. thanks! cc @reneexeener

[hectahertz]

Hey @LisaKr! I'm glad to hear this work served as a base for your changes.

ActionList is quite a complex component to modify because it's being used to build other components. As for now, we don't have plans of merging this as-is, as it requires further analysis to ensure all components that use it keep working as expected. It's hard to predict how this will end up working, but I don't think it should change a lot.

As @bolonio mentioned, we'll keep this PR open for now as this is something we'd like to reassess, but it's not top of the list at the moment.

[LisaKr]

Hey @LisaKr! I'm glad to hear this work served as a base for your changes.

ActionList is quite a complex component to modify because it's being used to build other components. As for now, we don't have plans of merging this as-is, as it requires further analysis to ensure all components that use it keep working as expected. It's hard to predict how this will end up working, but I don't think it should change a lot.

As @bolonio mentioned, we'll keep this PR open for now as this is something we'd like to reassess, but it's not top of the list at the moment.

thank you for the update! then just please let us know once there is any progress on it and thanks! :) cc @traumverloren @reneexeener @azenMatt

[github-actions[bot]]

Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days.