Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(menu): allow keyboard events to propagate from opened menu #4793

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(menu): allow keyboard events to propagate from opened menu #4793

wants to merge 2 commits into from

Conversation

TarunAdobe
Copy link
Contributor

@TarunAdobe TarunAdobe commented Oct 1, 2024 •
edited
Loading

Description

An opened sp-menu should allow application keyboard shortcuts.

Related issue(s)

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 ;)

  • Did it pass in Desktop?

  • Did it pass in Mobile?

  • Did it pass in iPad?

Screenshots (if appropriate)

Types of changes

  • 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

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • If my change required a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • 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.

@TarunAdobe TarunAdobe requested a review from a team as a code owner October 1, 2024 09:06
@TarunAdobe TarunAdobe self-assigned this Oct 1, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 1, 2024

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:

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 1, 2024 •
edited
Loading

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

@coveralls
Copy link
Collaborator

coveralls commented Oct 1, 2024 •
edited
Loading

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 Coverage Status
Change from base Build 11676245986: -0.002%
Covered Lines: 32319
Relevant Lines: 32741
💛 - Coveralls

@TarunAdobe
fix(menu): allow keyboard events to propogate from menu-items
47aa1f4 
chore(menu): added tests

fix(menu): allow keyboard events to propagate from opened menu
@TarunAdobe TarunAdobe force-pushed the fix/menu-keyboard-event-propagation branch from f986c6d to 47aa1f4 Compare October 1, 2024 09:17
@TarunAdobe TarunAdobe linked an issue Oct 1, 2024 that may be closed by this pull request
[Bug]: menu opened stops page keyboard shortcuts from working #4786
Open
1 task
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 1, 2024 •
edited
Loading

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		 -

blunteshwar
blunteshwar approved these changes Oct 1, 2024
View reviewed changes
Copy link
Collaborator

@blunteshwar blunteshwar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🔥

rubencarvalho
rubencarvalho requested changes Oct 1, 2024
View reviewed changes
Copy link
Collaborator

@rubencarvalho rubencarvalho left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let us further discuss if this is the most adequate approach to solving this issue. We are diverging from the native select element 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).

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?

@lazd
Copy link
Member

lazd commented Oct 1, 2024

Let us further discuss if this is the most adequate approach to solving this issue. We are diverging from the native select element 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.

@Rajdeepc Rajdeepc self-requested a review November 5, 2024 08:30
@rubencarvalho
Copy link
Collaborator

Hello @lazd, can you confirm if you could solve this on your side?

@lazd
Copy link
Member

lazd commented Nov 11, 2024

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:

image

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rubencarvalho rubencarvalho rubencarvalho requested changes

@blunteshwar blunteshwar blunteshwar approved these changes

@Rajdeepc Rajdeepc Awaiting requested review from Rajdeepc

Requested changes must be addressed to merge this pull request.

Assignees

@TarunAdobe TarunAdobe

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: menu opened stops page keyboard shortcuts from working
6 participants
@TarunAdobe @coveralls @lazd @rubencarvalho @Rajdeepc @blunteshwar