-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
NavigableContainer: use code instead of keyCode for keyboard events #43606
Conversation
Size Change: -10 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
431dda7
to
eae2bb3
Compare
const targetHasMenuItemRole = | ||
MENU_ITEM_ROLES.includes( targetRole ); | ||
|
||
// `preventDefault()` on tab to avoid having the browser move the focus | ||
// after this component has already moved it. | ||
const isTab = event.code === 'Tab'; | ||
|
||
if ( targetHasMenuItemRole || isTab ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from changing the keyCode
s to code
s, this is the only other runtime change and it's a bug fix (see code comments and PR description for more details)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work 👍
Needs a rebase, but other than that, works well and code looks great 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great improvements!
<button role="menuitem">Item 1</button> | ||
<button role="menuitem">Item 2</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we should have a more realistic example for this story, in the sense that we likely never want items in a NavigableMenu
to all be tabbable. There should only be one tab stop for the entire menu, and the menu items should only be navigable by arrow keys. (I was actually a little confused by this story at first because I was just tabbing through all the items.)
What do you think? I don't have a strong opinion about the actual menuitem implementation (e.g. divs/buttons), but I think the tabIndex
handling required for using this component should be part of the example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense — currently, "Item 4" is the only non-tabbable item that can be focused with arrow keys.
I will swap some button
s for other non-tabbable items (e.g. divs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So — it looks like the NavigableMenu
only moves focus across focusable
elements, while the TabbableContainer
element only moves focus across tabbable
components:
const finder = onlyBrowserTabstops ? focus.tabbable : focus.focusable; |
In this case, it seems that focusable elements = tabbable elements + anything with tabindex = 0. I tried to improve slightly the story in 37be0d4, but honestly I just think that these components are veeery niche and I'm not that sure about their real utility
@@ -0,0 +1,467 @@ | |||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why the NavigableMenu and TabbableContainer tests are merged into a single file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not particularly. I think the main issue is that there's one folder, but actually two different components being exported — and so I didn't know which approach to take.
I'll split the tests into two separate files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a thought — did we not recently make some changes to some config where it would only consider index.[ts]s{x}
files, either for storybook or test files? Would that matter for this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for test/story files, .js
files are excluded from type checking:
gutenberg/packages/components/tsconfig.json
Lines 39 to 40 in 9620bae
"src/**/stories/**.js", // only exclude js files, tsx files should be checked | |
"src/**/test/**.js", // only exclude js files, ts{x} files should be checked |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in d1455f7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a thought — did we not recently make some changes to some config where it would only consider
index.[ts]s{x}
files, either for storybook or test files? Would that matter for this PR?
That's actually what I had in mind — these changes proposed in the update to Storybook V7
const targetHasMenuItemRole = | ||
MENU_ITEM_ROLES.includes( targetRole ); | ||
|
||
// `preventDefault()` on tab to avoid having the browser move the focus | ||
// after this component has already moved it. | ||
const isTab = event.code === 'Tab'; | ||
|
||
if ( targetHasMenuItemRole || isTab ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, so if I understand correctly, this component kind of assumes that all navigable children will have a menuitem* role? 🤔 So in a NavigableMenu, the "prevent scroll containers from scrolling" part will not work if children don't have a menuitem* role. This seems like an important detail that is yet undocumented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it — it was a behaviour that existed before this PR, and it was not documented (it looks like NavigableMenu
is not used frequently in the codebase, and TabbableContainer
is not used at all).
I will add some documentation about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 9c38277
I explicitly didn't mention the preventDefault
behaviour explicitly because it felt like a bit of an implementation detail, which I didn't want to expose in official docs.
… would move the focus by two tabbable elements (instead of one)
b2e1098
to
37be0d4
Compare
79f3d52
to
d1455f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
What?
NavigableContainer
component to rely oncode
instead ofkeyCode
for keyboard eventsuser-event
TabbableContainer
where pressingtab
orshift tab
would end up skipping one tabbable element (because both the component and the browser would move focus to the next tabbable)Note that
TabbableContainer
doesn't seem to be used in the repo.Why?
keyCode
is deprecated, and replaced bycode
Testing Instructions
Interact with the Storybook examples, make sure that the components behave as expected.
Inspect the unit tests, make sure that: