spectrum-web-components
spectrum-web-components copied to clipboard
adobe
fix(menu): allow keyboard events to propagate from opened menu
Description
An opened sp-menu should allow application keyboard shortcuts.
Related issue(s)
- #4786
Motivation and context
An opened sp-menu doesn't allow application keyboard shortcuts, stopping the propagation of almost all keydown events.
How has this been tested?
-
Added a test and it passed ;)
-
[x] Did it pass in Desktop?
-
[ ] Did it pass in Mobile?
-
[ ] Did it pass in iPad?
Screenshots (if appropriate)
Types of changes
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)
Checklist
- [x] I have signed the Adobe Open Source CLA.
- [x] My code follows the code style of this project.
- [x] If my change required a change to the documentation, I have updated the documentation in this pull request.
- [x] I have read the CONTRIBUTING document.
- [x] I have added tests to cover my changes.
- [x] All new and existing tests passed.
- [x] I have reviewed at the Accessibility Practices for this feature, see: Aria Practices
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.
Branch preview
Visual regression test results
When a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
- High Contrast Mode | Medium | LTR
- Spectrum | Lightest | Medium | LTR
- Spectrum | Lightest | Medium | RTL
- Spectrum | Lightest | Large | LTR
- Spectrum | Lightest | Large | RTL
- Spectrum | Light | Medium | LTR
- Spectrum | Light | Medium | RTL
- Spectrum | Light | Large | LTR
- Spectrum | Light | Large | RTL
- Spectrum | Dark | Medium | LTR
- Spectrum | Dark | Medium | RTL
- Spectrum | Dark | Large | LTR
- Spectrum | Dark | Large | RTL
- Spectrum | Darkest | Medium | LTR
- Spectrum | Darkest | Medium | RTL
- Spectrum | Darkest | Large | LTR
- Spectrum | Darkest | Large | RTL
- Express | Lightest | Medium | LTR
- Express | Lightest | Medium | RTL
- Express | Lightest | Large | LTR
- Express | Lightest | Large | RTL
- Express | Light | Medium | LTR
- Express | Light | Medium | RTL
- Express | Light | Large | LTR
- Express | Light | Large | RTL
- Express | Dark | Medium | LTR
- Express | Dark | Medium | RTL
- Express | Dark | Large | LTR
- Express | Dark | Large | RTL
- Express | Darkest | Medium | LTR
- Express | Darkest | Medium | RTL
- Express | Darkest | Large | LTR
- Express | Darkest | Large | RTL
- Spectrum-two | Light | Medium | LTR
- Spectrum-two | Light | Medium | RTL
- Spectrum-two | Light | Large | LTR
- Spectrum-two | Light | Large | RTL
- Spectrum-two | Dark | Medium | LTR
- Spectrum-two | Dark | Medium | RTL
- Spectrum-two | Dark | Large | LTR
- Spectrum-two | Dark | Large | RTL
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Lighthouse scores
| Category | Latest (report) | Main (report) | Branch (report) |
|---|---|---|---|
| Performance | 0.99 | 0.99 | 0.99 |
| Accessibility | 1 | 1 | 1 |
| Best Practices | 1 | 1 | 1 |
| SEO | 1 | 0.92 | 0.92 |
| PWA | 1 | 1 | 1 |
What is this?
Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on main ("Main"). Higher scores are better, but note that the SEO scores on Netlify URLs are artifically constrained to 0.92.
Transfer Size
| Category | Latest | Main | Branch |
|---|---|---|---|
| Total | 250.292 kB | 236.581 kB | 216.63 kB 🏆 |
| Scripts | 60.788 kB | 54.134 kB | 51.957 kB 🏆 |
| Stylesheet | 53.666 kB | 47.913 kB | 30.157 kB 🏆 |
| Document | 6.208 kB | 5.453 kB 🏆 | 5.455 kB |
| Font | 126.786 kB | 126.605 kB 🏆 | 126.623 kB |
Request Count
| Category | Latest | Main | Branch |
|---|---|---|---|
| Total | 52 | 52 | 52 |
| Scripts | 41 | 41 | 41 |
| Stylesheet | 5 | 5 | 5 |
| Document | 1 | 1 | 1 |
| Font | 2 | 2 | 2 |
Pull Request Test Coverage Report for Build 11680202255
Details
- 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
- No unchanged relevant lines lost coverage.
- Overall coverage decreased (-0.002%) to 98.187%
| Totals | |
|---|---|
| Change from base Build 11676245986: | -0.002% |
| Covered Lines: | 32319 |
| Relevant Lines: | 32741 |
💛 - Coveralls
Tachometer results
Chrome
action-menu permalink
test-basic
| Version | Bytes | Avg Time | vs remote | vs branch |
|---|---|---|---|---|
| npm latest | 960 kB | 139.70ms - 143.06ms | - | faster ✔ 6% - 9% 8.76ms - 13.40ms |
| branch | 917 kB | 150.85ms - 154.05ms | slower ❌ 6% - 10% 8.76ms - 13.40ms |
- |
test-directive permalink
| Version | Bytes | Avg Time | vs remote | vs branch |
|---|---|---|---|---|
| npm latest | 917 kB | 70.57ms - 71.68ms | - | faster ✔ 6% - 8% 4.71ms - 6.52ms |
| branch | 874 kB | 76.03ms - 77.45ms | slower ❌ 7% - 9% 4.71ms - 6.52ms |
- |
test-lazy permalink
| Version | Bytes | Avg Time | vs remote | vs branch |
|---|---|---|---|---|
| npm latest | 916 kB | 69.34ms - 70.57ms | - | faster ✔ 7% - 10% 5.65ms - 8.00ms |
| branch | 873 kB | 75.77ms - 77.78ms | slower ❌ 8% - 11% 5.65ms - 8.00ms |
- |
test-open-close-directive permalink
| Version | Bytes | Avg Time | vs remote | vs branch |
|---|---|---|---|---|
| npm latest | 1.09 MB | 1876.03ms - 1879.37ms | - | unsure 🔍 -0% - +0% -4.05ms - +0.35ms |
| branch | 1.05 MB | 1878.11ms - 1880.97ms | unsure 🔍 -0% - +0% -0.35ms - +4.05ms |
- |
test-open-close permalink
| Version | Bytes | Avg Time | vs remote | vs branch |
|---|---|---|---|---|
| npm latest | 1.09 MB | 1880.67ms - 1883.99ms | - | unsure 🔍 -0% - +0% -2.38ms - +2.06ms |
| branch | 1.05 MB | 1881.01ms - 1883.97ms | unsure 🔍 -0% - +0% -2.06ms - +2.38ms |
- |
breadcrumbs permalink
basic-test
| Version | Bytes | Avg Time | vs remote | vs branch |
|---|---|---|---|---|
| npm latest | 979 kB | 526.43ms - 531.95ms | - | faster ✔ 1% - 3% 4.67ms - 15.29ms |
| branch | 936 kB | 534.64ms - 543.70ms | slower ❌ 1% - 3% 4.67ms - 15.29ms |
- |
combobox permalink
basic-test
| Version | Bytes | Avg Time | vs remote | vs branch |
|---|---|---|---|---|
| npm latest | 1 MB | 42.28ms - 43.44ms | - | faster ✔ 1% - 5% 0.60ms - 2.15ms |
| branch | 957 kB | 43.71ms - 44.75ms | slower ❌ 1% - 5% 0.60ms - 2.15ms |
- |
light-dom-test permalink
| Version | Bytes | Avg Time | vs remote | vs branch |
|---|---|---|---|---|
| npm latest | 1 MB | 400.29ms - 407.12ms | - | faster ✔ 1% - 3% 2.94ms - 14.31ms |
| branch | 958 kB | 407.79ms - 416.87ms | slower ❌ 1% - 4% 2.94ms - 14.31ms |
- |
menu permalink
test-basic
| Version | Bytes | Avg Time | vs remote | vs branch |
|---|---|---|---|---|
| npm latest | 741 kB | 210.64ms - 213.83ms | - | faster ✔ 2% - 3% 3.44ms - 7.51ms |
| branch | 716 kB | 216.45ms - 218.97ms | slower ❌ 2% - 4% 3.44ms - 7.51ms |
- |
picker permalink
basic-test
| Version | Bytes | Avg Time | vs remote | vs branch |
|---|---|---|---|---|
| npm latest | 817 kB | 510.98ms - 518.29ms | - | faster ✔ 3% - 5% 13.66ms - 27.17ms |
| branch | 774 kB | 529.37ms - 540.72ms | slower ❌ 3% - 5% 13.66ms - 27.17ms |
- |
Firefox
action-menu permalink
test-basic
| Version | Bytes | Avg Time | vs remote | vs branch |
|---|---|---|---|---|
| npm latest | 960 kB | 286.05ms - 293.83ms | - | faster ✔ 10% - 13% 30.73ms - 42.63ms |
| branch | 917 kB | 322.12ms - 331.12ms | slower ❌ 10% - 15% 30.73ms - 42.63ms |
- |
test-directive permalink
| Version | Bytes | Avg Time | vs remote | vs branch |
|---|---|---|---|---|
| npm latest | 917 kB | 148.50ms - 152.02ms | - | faster ✔ 2% - 5% 3.21ms - 8.43ms |
| branch | 874 kB | 154.16ms - 158.00ms | slower ❌ 2% - 6% 3.21ms - 8.43ms |
- |
test-lazy permalink
| Version | Bytes | Avg Time | vs remote | vs branch |
|---|---|---|---|---|
| npm latest | 916 kB | 136.85ms - 138.99ms | - | faster ✔ 3% - 5% 4.10ms - 7.10ms |
| branch | 873 kB | 142.47ms - 144.57ms | slower ❌ 3% - 5% 4.10ms - 7.10ms |
- |
test-open-close-directive permalink
| Version | Bytes | Avg Time | vs remote | vs branch |
|---|---|---|---|---|
| npm latest | 1.09 MB | 1899.01ms - 1902.27ms | - | slower ❌ 0% - 1% 5.80ms - 10.44ms |
| branch | 1.05 MB | 1890.87ms - 1894.17ms | faster ✔ 0% - 1% 5.80ms - 10.44ms |
- |
test-open-close permalink
| Version | Bytes | Avg Time | vs remote | vs branch |
|---|---|---|---|---|
| npm latest | 1.09 MB | 1903.89ms - 1906.91ms | - | faster ✔ 1% - 1% 12.72ms - 20.44ms |
| branch | 1.05 MB | 1918.43ms - 1925.53ms | slower ❌ 1% - 1% 12.72ms - 20.44ms |
- |
breadcrumbs permalink
basic-test
| Version | Bytes | Avg Time | vs remote | vs branch |
|---|---|---|---|---|
| npm latest | 979 kB | 834.26ms - 855.70ms | - | faster ✔ 0% - 3% 1.93ms - 25.71ms |
| branch | 936 kB | 853.64ms - 863.96ms | slower ❌ 0% - 3% 1.93ms - 25.71ms |
- |
combobox permalink
basic-test
| Version | Bytes | Avg Time | vs remote | vs branch |
|---|---|---|---|---|
| npm latest | 1 MB | 70.13ms - 73.47ms | - | unsure 🔍 -2% - +3% -1.73ms - +1.89ms |
| branch | 957 kB | 71.03ms - 72.41ms | unsure 🔍 -3% - +2% -1.89ms - +1.73ms |
- |
light-dom-test permalink
| Version | Bytes | Avg Time | vs remote | vs branch |
|---|---|---|---|---|
| npm latest | 1 MB | 714.60ms - 734.64ms | - | slower ❌ 1% - 4% 7.74ms - 31.34ms |
| branch | 958 kB | 698.85ms - 711.31ms | faster ✔ 1% - 4% 7.74ms - 31.34ms |
- |
menu permalink
test-basic
| Version | Bytes | Avg Time | vs remote | vs branch |
|---|---|---|---|---|
| npm latest | 741 kB | 392.01ms - 403.19ms | - | faster ✔ 0% - 4% 0.25ms - 16.79ms |
| branch | 716 kB | 400.02ms - 412.22ms | slower ❌ 0% - 4% 0.25ms - 16.79ms |
- |
picker permalink
basic-test
| Version | Bytes | Avg Time | vs remote | vs branch |
|---|---|---|---|---|
| npm latest | 817 kB | 980.16ms - 989.44ms | - | faster ✔ 5% - 8% 51.08ms - 86.96ms |
| branch | 774 kB | 1036.49ms - 1071.15ms | slower ❌ 5% - 9% 51.08ms - 86.96ms |
- |
Let us further discuss if this is the most adequate approach to solving this issue. We are diverging from the native
selectelement behaviour which, when opened, captures focus and directs keyboard events specifically towards interacting with its options (even though we currently have not implemented the "interacting with its options" part).
Good point, I didn't realize <select> eats all keyboard events when opened, even listeners added with capture: true! However, I can still use keyboard shortcuts such as Command W and Command + when focused on and interacting with a <select>.
Can we check if consumers can solve this on their side by adding an event listener in the capture phase instead of relying solely on the bubbling phase?
This would work and was actually my first inclination. I suppose we should speak with accessibility and ask what they think the best behavior is here.
Hey @rubencarvalho, unfortunately, we're unable to use a capture listener on our side to work around this bug.
This behavior in SWC was actually introduced 4 months ago in a feature PR that was adding support for action menus in action bars, apparently to prevent a potential bug:
It was never intentionally added to model behavior after <select>, and this is clearly an unintended side effect of that change.
I suggest we merge this PR after testing that the original but this change was meant to address is no longer happening. Thank you!
@rubencarvalho @nikkimk @TarunAdobe can we confirm if the menu omnibus accessibility work addressed this issue? or what conversations do we need to have to update the status on this PR?
For this particular case, we agreed that consumers should handle it on the application level (resolved in Slack thread). There is a similar issue opened here about other components: https://github.com/adobe/spectrum-web-components/issues/5099