Request for comments / Proof-of-concept towards submenu support #1410
Conversation
This is a Request For Comments to seek directional guidance towards implementing the submenu slot of menu-item. Includes: - SubmenuController to manage event listeners on menu-item. - Example usage in menu-item documentation. - Trivial tests to check rendering. Outstanding questions include: - Accessibility concerns. E.g. where to handle 'ArrowRight', 'ArrowLeft'? - Should selection of menu-item denoting submenu be possible or customizable? - How to parameterize contained popup? - Implementation concerns: - Use of ref / id - delegation of some rendering to the controller - What to test Related to [shoelace-style#620](shoelace-style#620).
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Removed extraneous `console.log()`.
| } | ||
| </style> | ||
| <sl-popup | ||
| ${ref(this.popupRef)} |
There was a problem hiding this comment.
This is a bit of a nit, but can we use @query instead? This is the only thing importing ref, but many things import query so that will be one less module we have to worry about bringing in.
There was a problem hiding this comment.
At this location, this is a ReactiveController, and the query directive wants this to be a ReactiveElement (or at least supporting .querySelector()). I don't believe the @query directive supports changing the renderRoot / querySelector root, but I'm happy to be shown otherwise.
Access to this element could be wrapped in a call to .querySelector() (which is what @query does anyway). ?
I'm slightly surprised there's no non-decorator equivalent I could use, i.e. in the manner this uses createRef(); maybe I just don't know where it is.
I'm happy to change it.
@query source here
(Intuitively, refs should have better performance, right?)
There was a problem hiding this comment.
🤔 I think the ref / queries may be unnecessary. If it were me, I'd probably just write the value of isActive into the template.
private handleMouseOver = () => {
clearTimeout(this.mouseOutTimer);
if (this.hasSlotController.test('submenu') && this.isActive === false) {
this.isActive = true;
this.host.requestUpdate()
}
}
render () {
return html`<sl-popup
?active=${this.isActive}
placement=${isLtr ? 'right-start' : 'left-start'}
anchor="anchor"
flip
strategy="fixed"
>
<slot name="submenu"></slot>
</sl-popup>`
}The requestUpdate() is important because itll force the re-render since ReactiveControllers dont have reactive properties.
|
Thanks for submitting this! I've wanted to add submenus for awhile, but I do have one reservation. By introducing submenus, we inevitably allow (or will be asked to allow) sub-submenus and sub-sub-submenus, which are usually pretty awful. I'd be OK to scope the feature to one level of submenus, but that might confuse users. If it's technically possible to allow nesting more than one level without collisions, we should at least call it out as a bad practice in the docs. On mobile, the submenu opens as shown below. The sub-submenu is correctly visible, but the first one goes out of the viewport. Ideally, it would always how within the viewport, even if that means overlapping (which we don't normally want when space is available). There are a few properties of that might be helpful here.
I've left some feedback in a review. The approach seems fine, but I really want to make sure we get positioning right as well as:
Thanks again! |
|
Thoughts on following the ARIA Authoring Practices Guide (APG) here, specifically their Menu and Menubar Pattern > Keyboard Interaction guidance? |
|
That sounds about right!
There’s some type to select logic in |
? |
|
Forgot the backticks, so GitHub stripped the tag. I've updated my previous comment. |
(Didn't mean to publish this.) This reverts commit be04e9a.
|
I didn't realize pushing to the branch I created the PR from would update the PR. Please ignore for now. |
|
Thoughts on mouseover behavior when another element has focus? (In the image, the focus is on an item in the lower submenu, and the mouse is hovering over the first submenu.) |
|
I see the submenus are a little lower than the parent item, is that intentional? I would suggest aligning them for visual balance and a more predictable pathway for mouse users
|
|
@mitchray: That is intentional. Even just pulling down menus in my chrome session, the alignment of the first item in the submenu with the "head" of the submenu is consistent (see new attached screenshots). Apologies if you meant something else that I have missed. My question specifically is about handling mouse-hover highlighting and focus together. In my screenshot, you can see two submenus open because focus and mouseover are acting independently, however, in Chrome application menus, there is not a separate mouse-hover highlight and focus: the most recent action takes focus / highlight. Maybe that is the way forward? But is that correct from an accessibility standpoint? 
|
|
Apologies didn't mean to divert the conversation, just wanted to query the alignment situation To clarify, this is the alignment of your original image (in green below)
Is the image with red lines a new screenshot of live code or an edit of the mockup image I shared? |
|
Your initial "screenshot" now makes more sense — which I based mine off of. I agree with your view that the submenus should be in alignment as in your mockup. Thanks for pointing it out! |
- Submenus now close on change-of-focus, not a timeout. - Keyboard navigation support added. - Skidding fix for better alignment. - Submenu documentation moved to Menu page. - Tests for accessibility, right and left arrow keys.
|
A comment is probably in order about the behaviour change: From Content on Hover or Focus (Level AA) (W3C's Web Content Accessibility Guidelines), I figured that the most accessible thing to do would be to persist the submenu until the user takes an action that would cause it to be dismissed. This is what my OS does for application menus, but, I have noticed that at least one other web-component library does not do this, and immediately dismisses the submenu if the mouse is moved away from it. To implement this persisting behaviour, I used the Also, the popover skidding alignment adjustment (affecting the submenu's popover location) doesn't work with Firefox due to a lack of |
I think following the OS behavior makes the most sense. I suspect a big reason nested dropdowns are considered bad usability is because submenus tend to be too eager to close. I like how macOS menus works:
If there's no reasonable polyfill, is there maybe a better way to position them? If necessary, I'm even open to using Floating UI directly instead of |
I don't know if you mean that it looks fine either-way, or something else? There's a mix of screen-shots above, so I'll try to clarify. @mitchray pointed it out best in green above, so I'll just highlight it in their image:
Floating UI's |
1. Close submenu on click explicitly, so this occurs even if the menu is not inside of an sl-dropdown. 2. In menu, ignore clicks that do not explicitly target a menu-item. Clicks that were on (e.g. a menu-border) were emitting select events.
Menu's handleKeyDown calls item.click (to emit the selection). Propagating the keyboard event on Enter / space would the cause re-entry into a submenu, so prevent the needless propagation.
|
Wow, this is looking great! Looking strictly at the example, I'm only seeing a couple visual things that we should figure out. Submenu hover state is lost when mousing outWhen mousing out of the submenu, the hover states are lost (because of nesting). We might need to add some kind of pseudo state for menu items to prevent it. Mimicking macOS, it should probably match the light gray hover color, not the blue focus color. CleanShot.2023-07-13.at.13.29.13.mp4I wish we had custom states but, in lieu of that, perhaps we simulate them with data attributes like we're doing with form control validation. This would prevent us from introducing a new pattern and give us a path for backwards compatibility when custom states become available in the future. Add an opening delaySubmenus appear immediately on hover, which feels especially weird when you have more than one submenu at the same level. CleanShot.2023-07-13.at.13.58.09.mp4Contrast this to OS menus, which have a slight delay before opening. It should be quite subtle, perhaps 100–200ms. I don't think it needs to be user configurable for this PR. Move submenus into a separate exampleI'm happy to do this, just leaving it here as a TODO. I'm torn between showing the example on the Menu page, the Menu Item page, or maybe both. Either way, it probably shouldn't live in the first example. Cosmetic thingsThe chevrons could use more spacing on the start. This is more obvious when menu items have long labels. 
We need to add shadows when menus are used in dropdowns. 
Let me know what you're willing to tackle and what you want us to do. I really appreciate your effort to bring this into the library! Thanks again! |
|
Another visual thing — should we have submenus slightly overlap like this?
And circling back to the topic of delaying menus on hover, I wonder if adding a subtle animation would be useful here. We could potentially delay it using keyframes and this would also make it customizable. If you want to go this route, let me know and I'll be happy to explain my thoughts further. |
- 100 ms delay when opening submenus on mouseover - Shadows added - Distance added to popup to have submenus overlap menu slightly.
|
Just pushed a change for a few of these.
Hmmm. I used 100 ms for now — for mouseover openings. Should we delay opening on keyboard-based activation? (Enabling the submenu could then return a promise so that focus could
Added shadows and overlap. Overlap should probably be dynamic (convert from
Yeah, there are some remaining issues:
|
|
Can we control the "submenu open" state directly instead of messing with hover/focus? Again, there's a pattern we use for form control validation that might make sense here. I like this pattern because, in the future (pending browser support), we can use For example, what if menu items received a data attribute when the submenu is open? <sl-menu-item data-submenu-open>
...
</sl-menu-item>And that state mapped to the same hover style in CSS: /* menu-item.styles.ts */
:host(:hover:not([aria-disabled='true'], :focus-visible)) .menu-item,
:host([data-submenu-open]) {
background-color: var(--sl-color-neutral-100);
color: var(--sl-color-neutral-1000);
}I think that will finish off this PR. |
|
Just adding my $0.02, this looks really solid. Mostly code nit-picky stuff, but overall love the look + feel of the submenu seems really nice. Ideally as @claviska stated I'd like to see the submenu appear underneath instead of to the left on smaller screens. Other than that, I really don't have too much to add. 👍 |
|
@ecyrb Sorry, I just noticed you requested a review on this. My comment above sums up what I think is remaining. Let me know if you'd like help with this or if you'd like to finish it off on your own :) Thanks again! |
| @@ -49,19 +48,21 @@ export default class SlMenu extends ShoelaceElement { | |||
| if (event.key === 'Enter' || event.key === ' ') { | |||
| const item = this.getCurrentItem(); | |||
| event.preventDefault(); | |||
| event.stopPropagation(); | |||
There was a problem hiding this comment.
I'm kind of curious why this stopPropagation() was adding? Were the events bubbling causing the parent menus to catch the event?
| const item = target.closest('sl-menu-item'); | ||
|
|
||
| if (!item || item.disabled || item.inert) { | ||
| if (!(event.target instanceof SlMenuItem)) { |
There was a problem hiding this comment.
I know previously we used target.closest('sl-menu-item') so this isn't a huge deal, but we should think of defining common interfaces for cases like this so we don't need to check instance types.
This is purely a comment for the future, and shouldn't affect this PR.
| const items = this.getAllItems(); | ||
| const activeItem = this.getCurrentItem(); | ||
| let index = activeItem ? items.indexOf(activeItem) : 0; | ||
|
|
||
| if (items.length > 0) { | ||
| event.preventDefault(); | ||
| event.stopPropagation(); |
There was a problem hiding this comment.
Im assuming the same thing here around parents catching the event.
|
We decided to pick this up and finish it off. Thank you so much for adding this! Really excited to finally ship submenus. Follow #1527 for updates. |
Apologies in advance.
tl;dr: I'm looking for directional guidance on implementing submenus. Is this the right direction? If not, what can I do to be headed in the right direction? First Github PR, and first use of TypeScript in anger, so apologies for the goofs.
Includes:
Outstanding questions include:
Related to #620.