-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
CustomSelectControl V2: keep legacy arrow down behavior only for legacy wrapper #62919
Conversation
Size Change: 0 B Total Size: 1.75 MB ℹ️ View Unchanged
|
5321e8e
to
38c1546
Compare
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Hey @t-hamano , if I'm not mistaken you use a Windows machine regularly. I have found this comment in the V1 component: gutenberg/packages/components/src/custom-select-control/index.js Lines 26 to 29 in b76de86
Are you aware of this behavior? Would it be ok if, in the V2 (non legacy) version of the component, we relied on the arrow down key to open the select popover? Thank you 🙏 |
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.
Looks good overall 👍
We still have to merge #62907 and rebase, but left some feedback in the meantime.
packages/components/src/custom-select-control-v2/legacy-component/test/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/custom-select-control-v2/legacy-component/test/index.tsx
Show resolved
Hide resolved
packages/components/src/custom-select-control-v2/test/index.tsx
Outdated
Show resolved
Hide resolved
79b0a3d
to
a518d2f
Compare
packages/components/src/custom-select-control-v2/custom-select.tsx
Outdated
Show resolved
Hide resolved
This is the first time I've seen this code, but it looks like it's intended to simulate the behavior of the native select element in Windows OS. On Windows OS, the native select elements behave as follows:
97784a2cb54294de6a164c9d92cb160d.mp4This behavior is the same for On the other hand, in this PR, the up and down arrow keys always open the dropdown. db00db5095c3c769669578cecf4b915f.mp4I'm concerned that behavior that differs from the Windows OS standard may be a bit confusing for keyboard users. I don't really understand the history of this PR being submitted, so sorry if I'm off the mark 🙏 |
Thank you, @t-hamano ! Always appreciate your help :D
It seems that there isn't a standard behavior across OSs — on MacOS, pressing arrow down on a closed Looking into the WAI-ARIA APG, it looks like the recommended behavior is to open the dropdown when pressing arrow down:
That is a valid concern, but unfortunately, one that we can't easily solve across all OSs unless we try to sniff the OS (which I'd like to avoid). In this scenario, I'd suggest we follow the WAI-ARIA APG — that would mean that, in |
c2f7c59
to
3ed4327
Compare
I see, if that’s the case then I think it’s okay 👍 |
…lProps` + `<button />`
3ed4327
to
1b388cc
Compare
Hey folks, I think this PR should be ready for a final round of review, if we agree on the approach taken |
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.
Working as expected and the code looks good 👍 🚀
From what I understand, @t-hamano agrees with following the WAI-ARIA recommendations, so I believe this is good to ship 🚢
/** | ||
* External dependencies | ||
*/ | ||
// eslint-disable-next-line no-restricted-imports |
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.
🤔 Shouldn't we allow Ariakit imports within the components package? Sounds like it could be an exception to the ESLint rule.
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.
…cy wrapper (WordPress#62919) * Use isLelacy flag to enable/disable showOnKeyDown behavior * Add unit tests * CHANGELOG * Unit tests: Add `sleep()` before pressing `Tab` key * Use label when checking that the listbox hasn't opened * Use label variable instead of hardcoded string * Use `Ariakit.Select` prop types instead of `CustomSelectButtonInternalProps` + `<button />`
What?
Tracked in #55023
Change the result of pressing the arrow down key when focussing the trigger button on V2
CustomSelectControl
(non-legacy). While the V1 and the V2 legacy wrapper select previous/next option item (with the select popover hidden), the V2 (non legacy) component opens the select popover instead.Why?
The V1 behavior felt non-standard, but we see value in retaining the original behavior in the V2 legacy wrapper.
Having a separate V2 allows us to switch to a better, more standard default.
How?
showOnKeyDown
prop to internal users of theButton
isLegacy
prop to affect theshowOnKeyDown
behaviorTesting Instructions
CustomSelectControl
. Focus the trigger button and press the arrow down key. V1 and V2 (legacy wrappers) should keep the select popover closed and change item selection, while V2 (default) should open the select popover.