fix: prevent leaving multiple submenus open at a time #2335

Westbrook · 2022-06-01T18:56:05Z

Description

Rather than hack the Menu/Menu Item relationship to enforce open/close rules on a Menu with multiple submenus, add a root concept to the overlay stack that prevents multiple overlays from the same root, even if they have different triggers.

prevent leaving things open
prevent passing focus back to the "root"
reduce code needed to manage submenus
doesn't have a test for duplicate submenus being open because while I'm "certain ™️" that the issue comes from the way setCloseSelfAsSubmenu and setCloseOpenSubmenu were being used, I could never isolate a reproduction case to base a new test on
needs a test for closing super decedent submenus when changing two+ levels up

Related issue(s)

Motivation and context

Things should "just work" 😉

How has this been tested?

Test case 1
1. Go here
2. Note: the menus open wonky by default.
3. Mouse around in an attempt to break it and have a submenu not open or have more than one submenu for the same menu open or have sub-submenus not close, etc.
4. Rejoice.

Types of changes

Bug fix (non-breaking change which fixes an issue)

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

github-actions · 2022-06-01T19:00:07Z

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:

najikahalsema

This is so beautiful. I'm crying right now. Nice work!

github-actions · 2022-06-01T19:20:45Z

Tachometer results

Chrome

action-bar permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	434 kB	40.21ms - 40.62ms	-	unsure 🔍 -0% - +1% -0.11ms - +0.35ms
branch	443 kB	40.18ms - 40.41ms	unsure 🔍 -1% - +0% -0.35ms - +0.11ms	-

action-menu permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	951 kB	395.86ms - 414.23ms	-	unsure 🔍 -3% - +3% -11.82ms - +13.68ms
branch	961 kB	395.27ms - 412.95ms	unsure 🔍 -3% - +3% -13.68ms - +11.82ms	-

menu permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	534 kB	460.22ms - 473.33ms	-	unsure 🔍 -2% - +2% -11.10ms - +10.24ms
branch	542 kB	458.79ms - 475.62ms	unsure 🔍 -2% - +2% -10.24ms - +11.10ms	-

overlay permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	561 kB	96.83ms - 97.70ms	-	unsure 🔍 -1% - +1% -0.57ms - +0.54ms
branch	571 kB	96.94ms - 97.62ms	unsure 🔍 -1% - +1% -0.54ms - +0.57ms	-

picker permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	713 kB	2137.52ms - 2200.92ms	-	unsure 🔍 -1% - +2% -30.31ms - +52.10ms
branch	723 kB	2132.00ms - 2184.64ms	unsure 🔍 -2% - +1% -52.10ms - +30.31ms	-

popover permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	428 kB	42.63ms - 42.87ms	-	faster ✔ 0% - 1% 0.06ms - 0.37ms
branch	438 kB	42.86ms - 43.06ms	slower ❌ 0% - 1% 0.06ms - 0.37ms	-

split-button permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	820 kB	1964.72ms - 1967.54ms	-	unsure 🔍 -0% - +0% -3.81ms - +0.39ms
branch	830 kB	1966.29ms - 1969.39ms	unsure 🔍 -0% - +0% -0.39ms - +3.81ms	-

tooltip permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	436 kB	46.50ms - 47.11ms	-	unsure 🔍 -0% - +1% -0.16ms - +0.55ms
branch	446 kB	46.42ms - 46.79ms	unsure 🔍 -1% - +0% -0.55ms - +0.16ms	-

Firefox

action-bar permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	434 kB	164.03ms - 174.73ms	-	unsure 🔍 -4% - +5% -6.94ms - +8.42ms
branch	443 kB	163.13ms - 174.15ms	unsure 🔍 -5% - +4% -8.42ms - +6.94ms	-

action-menu permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	951 kB	630.86ms - 648.98ms	-	unsure 🔍 -2% - +3% -10.29ms - +18.09ms
branch	961 kB	625.10ms - 646.94ms	unsure 🔍 -3% - +2% -18.09ms - +10.29ms	-

menu permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	534 kB	778.94ms - 798.10ms	-	unsure 🔍 -2% - +1% -19.31ms - +9.31ms
branch	542 kB	782.89ms - 804.15ms	unsure 🔍 -1% - +2% -9.31ms - +19.31ms	-

overlay permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	561 kB	293.95ms - 304.45ms	-	unsure 🔍 -3% - +2% -8.69ms - +7.29ms
branch	571 kB	293.88ms - 305.92ms	unsure 🔍 -2% - +3% -7.29ms - +8.69ms	-

picker permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	713 kB	1736.03ms - 1774.45ms	-	unsure 🔍 -1% - +2% -14.46ms - +37.22ms
branch	723 kB	1726.58ms - 1761.14ms	unsure 🔍 -2% - +1% -37.22ms - +14.46ms	-

popover permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	428 kB	144.86ms - 171.06ms	-	unsure 🔍 -2% - +24% -2.07ms - +32.79ms
branch	438 kB	131.11ms - 154.09ms	unsure 🔍 -20% - +1% -32.79ms - +2.07ms	-

split-button permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	820 kB	2289.26ms - 2310.30ms	-	unsure 🔍 -0% - +1% -5.21ms - +26.33ms
branch	830 kB	2277.48ms - 2300.96ms	unsure 🔍 -1% - +0% -26.33ms - +5.21ms	-

tooltip permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	436 kB	141.00ms - 154.40ms	-	unsure 🔍 -4% - +7% -6.21ms - +10.57ms
branch	446 kB	140.47ms - 150.57ms	unsure 🔍 -7% - +4% -10.57ms - +6.21ms	-

hunterloftis · 2022-06-02T18:31:20Z

Is it intended behavior to have the user click multiple times to close a nested menu? Checking since that's not typically how these sorts of menus behave:

multi-click-close-menu.mov

Westbrook · 2022-06-02T19:15:37Z

Nice catch @hunterloftis! It helped me to catch another issue, as well. Added tests for both of them now that they are fixed.

packages/overlay/src/overlay-stack.ts

Westbrook requested review from hunterloftis, loredanaspataru and najikahalsema June 1, 2022 18:56

Westbrook requested a review from spdev3000 June 1, 2022 19:10

najikahalsema previously approved these changes Jun 1, 2022

View reviewed changes

Westbrook dismissed najikahalsema’s stale review via ef53304 June 1, 2022 21:47

Westbrook force-pushed the submenu branch 3 times, most recently from 0b1a7eb to 6015254 Compare June 1, 2022 22:06

hunterloftis self-assigned this Jun 2, 2022

Westbrook force-pushed the submenu branch 2 times, most recently from 6db8f41 to 80d56a7 Compare June 2, 2022 18:07

Westbrook force-pushed the submenu branch from 80d56a7 to f886426 Compare June 2, 2022 19:11

Westbrook requested a review from najikahalsema June 2, 2022 19:44

hunterloftis previously approved these changes Jun 2, 2022

View reviewed changes

packages/overlay/src/overlay-stack.ts Outdated Show resolved Hide resolved

fix: prevent leaving multiple submenus open at a time

1635933

Westbrook dismissed hunterloftis’s stale review via 1635933 June 3, 2022 13:58

Westbrook force-pushed the submenu branch from f886426 to 1635933 Compare June 3, 2022 13:58

Westbrook merged commit d2bfbb2 into main Jun 3, 2022

Westbrook deleted the submenu branch June 3, 2022 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: prevent leaving multiple submenus open at a time #2335

fix: prevent leaving multiple submenus open at a time #2335

Westbrook commented Jun 1, 2022 •

edited

Loading

github-actions bot commented Jun 1, 2022

najikahalsema left a comment

github-actions bot commented Jun 1, 2022 •

edited

Loading

action-bar permalink

action-menu permalink

menu permalink

overlay permalink

picker permalink

popover permalink

split-button permalink

tooltip permalink

action-bar permalink

action-menu permalink

menu permalink

overlay permalink

picker permalink

popover permalink

split-button permalink

tooltip permalink

hunterloftis commented Jun 2, 2022

Westbrook commented Jun 2, 2022

fix: prevent leaving multiple submenus open at a time #2335

fix: prevent leaving multiple submenus open at a time #2335

Conversation

Westbrook commented Jun 1, 2022 • edited Loading

Description

Related issue(s)

Motivation and context

How has this been tested?

Types of changes

Checklist

github-actions bot commented Jun 1, 2022

Branch Preview

najikahalsema left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 1, 2022 • edited Loading

Tachometer results

action-bar permalink

action-menu permalink

menu permalink

overlay permalink

picker permalink

popover permalink

split-button permalink

tooltip permalink

action-bar permalink

action-menu permalink

menu permalink

overlay permalink

picker permalink

popover permalink

split-button permalink

tooltip permalink

hunterloftis commented Jun 2, 2022

Westbrook commented Jun 2, 2022

Westbrook commented Jun 1, 2022 •

edited

Loading

github-actions bot commented Jun 1, 2022 •

edited

Loading