Skip to content

[ContextMenu][DropdownMenu] Add submenu support #682

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

Merged
merged 19 commits into from
Jun 15, 2021
Merged

[ContextMenu][DropdownMenu] Add submenu support #682

merged 19 commits into from
Jun 15, 2021

Conversation

andy-hook
Copy link
Contributor

@andy-hook andy-hook commented Jun 3, 2021

Closes #383

This PR adds submenu support to ContextMenu and DropdownMenu

While there are variations in submenu behaviour across desktop and web, we've generally attempted to stick close to the native macos implementation. See original discussion here

Demo

CleanShot.2021-06-04.at.19.03.00.mp4

TODO

Andy Hook and others added 2 commits June 1, 2021 18:16
* Scope typeahead via collection

* Exclude only on typeahead space key

* Simplify typeahead

* Prevent continuous `updateSearch` call

* Inline `onExclusiveKeyDown` functionality

* Clear search buffer when leaving

* Move `MenuSubTrigger` code below `MenuContent`

* `isTypingAhead` conditional clarity
@benoitgrelard benoitgrelard changed the title [Draft] Add submenus [ContextMenu][DropdownMenu] Add submenu support Jun 3, 2021
Andy Hook and others added 5 commits June 3, 2021 16:44
…enus (#688)

* [ContextMenu][DropdownMenu] delay sub trigger open via pointer

* Add pointer area grace

* Use clientX/Y rather than pageX/Y because getBoundingClientRect doesn't include scroll

* Update DropdownMenu.stories.tsx

* feedback

* onItemEnter

* Update Menu.tsx

* more explicit open timer ref

* Remove content rect caching
@andy-hook
Copy link
Contributor Author

I'm so pleased with how this feels now, especially after #688 👏 , I did notice one very minor detail and that's when we enter and leave within the 100ms window of the pointer grace, the focus sticks:

CleanShot.2021-06-04.at.21.26.14.mp4

@benoitgrelard
Copy link
Contributor

benoitgrelard commented Jun 4, 2021

I'm so pleased with how this feels now, especially after #688 👏 , I did notice one very minor detail and that's when we enter and leave within the 100ms window of the pointer grace, the focus sticks:

CleanShot.2021-06-04.at.21.26.14.mp4

ah yep good catch.
I've added to the list up there. ☝️

@andy-hook
Copy link
Contributor Author

I'm so pleased with how this feels now, especially after #688 👏 , I did notice one very minor detail and that's when we enter and leave within the 100ms window of the pointer grace, the focus sticks:
CleanShot.2021-06-04.at.21.26.14.mp4

ah yep good catch.

I don't think it was introduced in that change, but just exposed by this new state it can be in. Will take a look during a full reg test and cross browser bizzo.

Jenna Smith added 4 commits June 10, 2021 13:59
* Add `aria-labelledby`

* Follow `aria-expanded` spec for submenu triggers

* Updates based on feedback

* Rename root context type
* Focus first item when all content types open for keyboard users

* Initial feedback addressed

* Add `pointerdown` to keyboard user logic

* Remove capture

* Re-apply capture events with comment
* Share grace timer ref in content

* Align pointer behaviour closer with native

* Simplify base story for easier test

* Remove export of private func

* Tidy from feedback

* Minor naming adjustment

* Apply `useCallback`

Co-authored-by: Andy Hook <[email protected]>
@andy-hook
Copy link
Contributor Author

andy-hook commented Jun 14, 2021

Did a final round of browser testing, here is what I've found.

You cannot enter a submenu with keyboard if it was originally opened via the pointer

CleanShot.2021-06-14.at.09.24.13.mp4

If you navigate to a submenu two levels deep and click the root trigger, the open state sticks

CleanShot.2021-06-14.at.09.30.12.mp4

With touch inputs there is some odd behaviour when layered click targets overlap e.g. after a collision adjustment (seen on IOS but can reproduce in emulated dev tool)

CleanShot.2021-06-14.at.09.37.20.mp4

…707)

* Fix menu focus after pointer interaction

* Add explainer
@benoitgrelard benoitgrelard marked this pull request as ready for review June 14, 2021 16:03
Copy link
Contributor

@benoitgrelard benoitgrelard left a comment

Choose a reason for hiding this comment

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

I've done one more thorough review of the whole change overall and I'm very very happy with it!
I've found just one thing we could do to simplify things a little.

👏

@benoitgrelard benoitgrelard merged commit f12c7e3 into main Jun 15, 2021
@benoitgrelard benoitgrelard deleted the submenu branch June 15, 2021 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New component: Submenu
2 participants