ActionList: Enable focusZone for roles listbox and menu #4795
Conversation
siddharthkp
added
the
integration-tests: recommended
🦋 Changeset detectedLatest commit: fa13d5a The changes in this PR will be included in the next version bump. 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 |
size-limit report 📦
|
|
Identified some of the failing tests in dotcom as coming from the release candidate. Will wait for next dotcom upgrade before going too deep in debugging Update: dotcom upgrade was merged, creating integration tests again |
siddharthkp
changed the title
siddharthkp
added
integration-tests: passing
| let enableFocusZone = false | ||
| if (enableFocusZoneFromContainer !== undefined) enableFocusZone = enableFocusZoneFromContainer | ||
| else if (listRole) enableFocusZone = ['menu', 'menubar', 'listbox'].includes(listRole) | ||
|
|
||
| useFocusZone({ |
There was a problem hiding this comment.
I'm wondering if we'd want to add focusOutBehavior: 'wrap' by default, or allow consumers to add that themselves?
There was a problem hiding this comment.
I'm wondering if we'd want to add focusOutBehavior: 'wrap' by default
Honestly, not sure, what do you think?
or allow consumers to add that themselves?
That's the part that's missing right now. I think I'd like to wait for feedback/requests before introducing focusZoneSettings like we have on other focus-zoning components
There was a problem hiding this comment.
For role=menu, we'd definitely want it to wrap by default. With role=listbox there may be cases where it doesn't need to wrap. I'd say if we can conditionally set it to wrap based on if it's a menu or not would be a safe bet, at least for now!
|
|
||
| let enableFocusZone = false | ||
| if (enableFocusZoneFromContainer !== undefined) enableFocusZone = enableFocusZoneFromContainer | ||
| else if (listRole) enableFocusZone = ['menu', 'menubar', 'listbox'].includes(listRole) |
There was a problem hiding this comment.
Non-blocking: I see we have have menubar, I am wondering if there'd be a case where role=menubar is used with ActionList 🤔
There was a problem hiding this comment.
We don't have one yet, but I think it's possible. The issue linked to menu + menubar pattern, so I included it just in case
| const listRef = useProvidedRefOrCreate(forwardedRef as React.RefObject<HTMLUListElement>) | ||
|
|
||
| let enableFocusZone = false |
There was a problem hiding this comment.
If there's an existing focus zone being used for an ActionList, would the one being used here replace it? I saw an instance where there was an ActionList inside of an AnchoredOverlay that had an implicit focus zone attached:
<AnchoredOverlay
renderAnchor={props => <Button {...props}>Button</Button>}
focusZoneSettings={{
focusOutBehavior: 'wrap',
}}
>
<ActionList role="listbox">
<ActionList.Item>Item 1</ActionList.Item>
<ActionList.Item>Item 2</ActionList.Item>
</ActionList>
</AnchoredOverlay>I'm wondering if this would be an issue or not 🤔. I think the existing cases are rare, so doesn't seem like it would be.
There was a problem hiding this comment.
focus zones can only be additive, not removed. So, if the parent AnchoredOverlay has a focus zone, that would work on all its children. Setting it to false on ActionList would not exclude it from the parent's focus zone.
Tested with ActionMenu, Autocomplete, etc. because they also use AnchoredOverlay (and it's focus zone)
|
Updated focusOutBehavior to wrap, will test with dotcom once again before merging |
siddharthkp
removed
the
integration-tests: passing
siddharthkp
added
the
integration-tests: recommended
|
👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/337581 Update: Only license job failed, everything else is good to go! |
siddharthkp
added
integration-tests: passing
Rollout strategy
Testing & Reviewing
Merge checklist