feat(menu): add mobile drill-down submenu navigation#6063
feat(menu): add mobile drill-down submenu navigation#6063
Conversation
🦋 Changeset detectedLatest commit: 99dc87c The changes in this PR will be included in the next version bump. This PR includes changesets to release 83 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📚 Branch Preview Links🔍 First Generation Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
|
|
||
| private get _mobileViewRoot(): Menu | null { | ||
| if (this.slot === 'mobile-submenu') { | ||
| return this.parentElement as Menu | null; |
There was a problem hiding this comment.
Could we use this.closest('sp-menu[is-mobile-view]') instead?
There was a problem hiding this comment.
Changed to this.closest('sp-menu[mobile-view]') (also renamed the attribute from is-mobile-view to mobile-view per your other comment).
| } | ||
|
|
||
| item._mobileSubmenuProjected = true; | ||
| this._mobileSubmenuOriginalParents.set(submenuEl, submenuEl.parentElement!); |
There was a problem hiding this comment.
Can we avoid the ! assertion? Maybe just find a way to return if parentElement is null?
There was a problem hiding this comment.
Added a null guard — we now extract parentElement into a const and return early if it's null.
|
|
||
| this._removeMobileBackElements(submenuEl); | ||
|
|
||
| const submenu = submenuEl as unknown as Menu; |
There was a problem hiding this comment.
Can we avoid this double cast? I see us doing this multiple times in the file.
There was a problem hiding this comment.
Added a private asMenu(element) helper method and replaced all 5 occurrences of as unknown as Menu with it.
|
|
||
| const backItem = document.createElement('sp-menu-item') as MenuItem; | ||
| backItem.setAttribute('data-mobile-back', ''); | ||
| backItem.textContent = 'Back'; |
There was a problem hiding this comment.
Do we need to allow this to be localized? e.g., add one more property for this?
There was a problem hiding this comment.
Added a mobileBackLabel property (attribute: mobile-back-label) with a default of "Back". Consumers can set it for localization.
|
|
||
| private _mobileBackElements = new Map<HTMLElement, HTMLElement[]>(); | ||
|
|
||
| private _injectMobileBackElements(submenuEl: HTMLElement): void { |
There was a problem hiding this comment.
Can we not do this imperatively? Why don't we just create this template in the render()? This would let us delete _injectMobileBackElements, _removeMobileBackElements, and the _mobileBackElements Map. We can move the inline styles to CSS, use a method reference for the click handler (e.g., handleMobileBack), simplify _focusProjectedSubmenu (instead of looking up the back item from a Map, we can mb use this.shadowRoot.querySelector('.mobile-back-button').
There was a problem hiding this comment.
Also this prob bypasses Lit's lifecycle methods, which sounds problematic :P
|
|
||
| const icon = document.createElement('sp-icon-arrow500'); | ||
| icon.slot = 'icon'; | ||
| icon.style.transform = 'rotate(180deg)'; |
There was a problem hiding this comment.
Also, we need to rotate it for RTL too!
There was a problem hiding this comment.
Done! The icon now checks this.dir === 'rtl' and uses rotate(0deg) for RTL vs rotate(180deg) for LTR. Also added an RTL navigation test.
| @property({ type: Boolean, attribute: 'is-mobile-view', reflect: true }) | ||
| public isMobileView = false; | ||
|
|
||
| @property({ attribute: false }) |
There was a problem hiding this comment.
This shouldn't be a prop?
There was a problem hiding this comment.
Good catch! Changed from @property({ attribute: false }) to @state() since this is internal reactive state, not a public API.
| public handleSubmenuClosed = (event: Event): void => { | ||
| if (this.isMobileView) { | ||
| this.resetMobileSubmenus(); | ||
| return; |
There was a problem hiding this comment.
Aren't we skipping the stopPropagation in mobile?
There was a problem hiding this comment.
Good catch! Moved event.stopPropagation() to before the mobile-view early return so it always fires regardless of the code path.
| if (event.defaultPrevented || !this.rovingTabindexController) { | ||
| return; | ||
| } | ||
| if (this.isMobileView && this._mobileSubmenuStack.length > 0) { |
There was a problem hiding this comment.
Can we also test a menu with selection / create a story for it? I'm not sure if this handleKeydown isn't eating more than it should :P
There was a problem hiding this comment.
Added a test (handleKeydown does not eat ArrowDown/ArrowUp at the root level) that verifies keyboard navigation still works correctly with mobile-view enabled — ArrowDown/ArrowUp are not swallowed by the mobile handleKeydown logic at the root level.
| * of opening a flyout overlay. | ||
| */ | ||
| @property({ type: Boolean, attribute: 'is-mobile-view', reflect: true }) | ||
| public isMobileView = false; |
There was a problem hiding this comment.
Do we want the is-? or just mobile-view?
There was a problem hiding this comment.
Also... how does this work for a11y when we're drilling down, does this need to be an aria-live region?
There was a problem hiding this comment.
Agreed, dropped the is- prefix. Renamed to mobile-view attribute / mobileView property across all files (Menu.ts, MenuItem.ts, menu.css, stories, tests).
There was a problem hiding this comment.
Done! Added role="region" and aria-live="polite" to the mobile submenu animation wrapper so screen readers announce content changes when drilling down or navigating back.
rubencarvalho
left a comment
rubencarvalho
left a comment
There was a problem hiding this comment.
Overall the UI looks really nice and I can confirm it works for the basic cases! I left some comments to try and avoid imperative things we're doing.
| expect(this.rootItem.open, 'should stay closed').to.be.false; | ||
| }); | ||
| }); | ||
| describe('mobile view', () => { |
There was a problem hiding this comment.
Maybe also add an RTL test?
There was a problem hiding this comment.
And Escape / leaf item selection too?
There was a problem hiding this comment.
Done! Added an RTL test (navigates correctly in RTL mode) that verifies ArrowRight closes the submenu in RTL (opposite of LTR).
There was a problem hiding this comment.
Added an Escape key test (closes submenu via Escape key). Leaf item selection in mobile view needs further investigation as part of the declarative refactor — the reparenting currently affects how change events propagate.
Rajdeepc
force-pushed
the
rajdeep/swc-89
branch
from
a91344c to
ff0acc5
Compare
- Rename `is-mobile-view` attribute to `mobile-view` for consistency - Use `this.closest()` instead of slot/parentElement check in _mobileViewRoot - Replace `!` assertion on parentElement with null guard - Add `asMenu()` helper to avoid repeated double casts - Add `mobileBackLabel` property for localization of back button text - Handle RTL icon rotation for back arrow - Change `_mobileSubmenuStack` from `@property` to `@state` - Move `event.stopPropagation()` before mobile-view early return in handleSubmenuClosed - Add `aria-live="polite"` region for screen reader announcements - Add tests for Escape key, RTL navigation, and keyboard handling with mobile view Made-with: Cursor
Made-with: Cursor
…ender() Use Lit's `render()` to declaratively render the mobile back button template into the projected submenu element instead of imperative `document.createElement` calls. The back button remains inside the same `<sp-menu>` so `RovingTabindexController` manages keyboard navigation naturally. Also adds JSDoc to all mobile submenu methods. Made-with: Cursor
caseyisonit
added
the
High priority PR review
rubencarvalho
left a comment
rubencarvalho
left a comment
There was a problem hiding this comment.
Missing a changeset! Left a couple of comments too 😄
| if (!submenuEl) { | ||
| return; | ||
| } | ||
| const backItem = submenuEl.querySelector( |
There was a problem hiding this comment.
Could we use @query?
There was a problem hiding this comment.
Looked at this — @query doesn't quite fit here because the back button isn't in Menu's own shadow root. The flow is:
- Parent
<sp-menu mobile-view>projects the user's<sp-menu slot="submenu">into its own light DOM (slot="mobile-submenu"). - We then call
_renderMobileBackElements(submenuEl)which usesLit.render(html\<sp-menu-item class="mobile-back-button" ...>`, container)to insert the back row into the **projected's light DOM** (so it lands in that submenu's default slot and becomes part of *its*childItems`/RTI).
So the back button lives inside whichever projected <sp-menu> is on top of the drill-down stack — it's never in this.shadowRoot. @query('.mobile-back-button') from Menu would always return null.
@queryAssignedElements({ slot: 'mobile-submenu' }) would get us the projected submenu element itself, but not the back button inside it. We'd still need the submenuEl.querySelector('.mobile-back-button') step (or move to a tagged-name approach like [data-mobile-back] if you'd prefer that signal over the class).
If/when we switch to the fully declarative back-row refactor (see imperative-render thread) and the back row becomes a first-class child item of the projected submenu, we could grab it via childItems.find(i => i.classList.contains('mobile-back-button')) and drop the querySelector entirely. Happy to do that as part of that follow-up.
| return; | ||
| } | ||
| await backItem.updateComplete; | ||
| const submenu = this.asMenu(submenuEl); |
There was a problem hiding this comment.
Still not sure what to think about these casts 😅 maybe we just check instanceof Menu? or directly cast it?
There was a problem hiding this comment.
Kept the asMenu() helper as the single chokepoint for the cast in 6e78ef2:
private asMenu(element: HTMLElement): Menu {
return element as HTMLElement as Menu;
}The reason for going through a helper rather than instanceof Menu: every site that needs the cast already starts from item.submenuElement (an HTMLElement | undefined returned by MenuItem). At runtime we know it's always an <sp-menu> because that's the only thing we project into slot="submenu", but TypeScript only sees HTMLElement. An instanceof Menu runtime guard would force a null branch on every caller for a case that can't actually happen, and it would also re-introduce a circular-ish import shape (MenuItem → Menu) that we currently avoid by keeping the cast type-only.
The double cast (as HTMLElement as Menu) is just to satisfy TS strictness about going from one element type to another without overlap — the helper localizes that ugliness in one place so it's auditable, and every site that needs a Menu reads as this.asMenu(submenuEl) which is reasonably self-documenting.
If you'd rather have a runtime guard with instanceof, the cleanest version would be:
private asMenu(element: HTMLElement): Menu {
if (!(element instanceof Menu)) {
throw new Error(
'Expected Menu element in mobile-submenu projection slot'
);
}
return element;
}That gives us a loud failure if the invariant is ever broken. Happy to switch to that if you'd prefer the safety over the leniency.
| * animation finishes, preventing the animation from replaying on | ||
| * subsequent layout changes. | ||
| */ | ||
| private _handleTransitionEnd = (event: AnimationEvent): void => { |
There was a problem hiding this comment.
nit:
| private _handleTransitionEnd = (event: AnimationEvent): void => { | |
| private _handleAnimationEnd = (event: AnimationEvent): void => { |
There was a problem hiding this comment.
Done in 6e78ef2 — renamed _handleTransitionEnd → _handleAnimationEnd. The event being listened to is animationend (CSS keyframes, not a CSS transition), so the new name is also more accurate semantically.
| public overlayElement!: Overlay; | ||
|
|
||
| private submenuElement?: HTMLElement; | ||
| public submenuElement?: HTMLElement; |
There was a problem hiding this comment.
nit: does this need to be public?
There was a problem hiding this comment.
It does need to be reachable from outside MenuItem, but I agree the public was unmotivated. In 6e78ef2 I left it as public but added an @internal JSDoc making the contract explicit:
/**
* Reference to the slotted submenu element, captured by `manageSubmenu`.
* Public so the parent `<sp-menu>` can project this submenu for mobile
* drill-down and so tests can assert on its contents.
*
* @internal
*/
public submenuElement?: HTMLElement;The two callers are:
Menu.ts— projectsitem.submenuElementinto the mobile-submenu slot during drill-down, restores it on close, and queries it for the back button.submenu.test.ts— asserts onrootItem.submenuElement?.querySelector('[data-mobile-back]').
MenuItem and Menu aren't in a subclass relationship, so protected would block Menu's access. The cleanest "really private" refactor would be to expose access via a method like getSubmenuElement(): HTMLElement | undefined and keep the field private — happy to do that in a follow-up if you'd prefer; it just touches a few sites in Menu.ts and the tests.
| * the mobile-submenu slot. Guards against manageSubmenu clearing | ||
| * hasSubmenu when the slot appears empty during projection. | ||
| */ | ||
| public _mobileSubmenuProjected = false; |
There was a problem hiding this comment.
nit: same here, can this be private (I mean the _ implies it)?
There was a problem hiding this comment.
Same situation as submenuElement — the _ was aspirational but Menu.ts writes it during projection (item._mobileSubmenuProjected = true) and reads it during restore. In 6e78ef2 I made the contract explicit with @internal:
/**
* Set by the parent Menu when the submenu element is projected to the
* mobile-submenu slot. Guards against `manageSubmenu` clearing
* `hasSubmenu` when the slot appears empty during projection.
* Cross-class implementation detail; do not depend on it from outside
* the `@spectrum-web-components/menu` package.
*
* @internal
*/
public _mobileSubmenuProjected = false;Truly private would need a setMobileSubmenuProjected(value: boolean) method on MenuItem. Happy to do that follow-up if you'd prefer the harder boundary — just say the word.
|
|
||
| if (this.isMobileView) { | ||
| this._mobileRootMenu?.openMobileSubmenu(this); | ||
| setTimeout(() => { |
There was a problem hiding this comment.
Maybe a small comment here explaining why we need to defer this?
There was a problem hiding this comment.
Done in 6e78ef2 — added inline comments at both deferral sites in MenuItem.handleTouchSubmenuToggle:
if (this.isMobileView) {
this._mobileRootMenu?.openMobileSubmenu(this);
// Defer clearing the flag until after the synthetic `click`
// dispatched by the same touch sequence has been handled, so
// `handleSubmenuClick` can suppress that duplicate activation.
setTimeout(() => {
this._touchHandledViaPointerup = false;
}, 0);
return;
}The deferral is what lets handleSubmenuClick see _touchHandledViaPointerup === true for the trailing click and bail; without it the click runs the open path a second time on touch devices.
| animation: mobile-slide-in-right 200ms ease-out; | ||
| } | ||
|
|
||
| .mobile-submenu-animation-wrapper[mobile-transition="back"] { |
There was a problem hiding this comment.
I can't recall what we usually do here, but should we respect prefers-reduced-motion: no-preference for these animations?
There was a problem hiding this comment.
Done in 6e78ef2. Added a prefers-reduced-motion: reduce block in menu.css that disables the slide-in animation on the wrapper:
@media (prefers-reduced-motion: reduce) {
.mobile-submenu-animation-wrapper[mobile-transition="forward"],
.mobile-submenu-animation-wrapper[mobile-transition="back"] {
animation: none;
}
}The drill-down still works (content swaps in immediately), it just skips the keyframe.
| <div | ||
| class="mobile-submenu-animation-wrapper" | ||
| role="region" | ||
| aria-live="polite" |
There was a problem hiding this comment.
OK, I retested this and it's super noisy 😆 We're already announcing the menu-items on focus so it should be enough!! It's announcing all of the things again 😛 Also we were missing an accessible name for the region anyway. I think we can safely get rid of it. Sorry!
There was a problem hiding this comment.
Done in 6e78ef2 — dropped both role="region" and aria-live="polite" from .mobile-submenu-animation-wrapper. Agreed it was double-announcing on top of the menu-item focus announcements; the back button + the change in focus already convey the level transition for AT users without an extra polite live region.
| this.setAttribute('role', this.ownRole); | ||
| } | ||
| this.updateComplete.then(() => this.updateItemFocus()); | ||
| this.addEventListener('animationend', this._handleTransitionEnd); |
There was a problem hiding this comment.
Could we bind @animationend declaratively to element that triggers it?
There was a problem hiding this comment.
Done in 6e78ef2. The handler is now bound declaratively on the wrapper:
<div
class="mobile-submenu-animation-wrapper"
@animationend=${this._handleAnimationEnd}
>
<slot name="mobile-submenu"></slot>
</div>Also dropped the addEventListener('animationend', ...) / removeEventListener pair from connectedCallback / disconnectedCallback, and switched to event.currentTarget inside the handler so it can never accidentally fire for some other animation that bubbles up to the host.
rubencarvalho
left a comment
rubencarvalho
left a comment
There was a problem hiding this comment.
Looking at this again, I think we need to figure out documentation around the pattern and/or build this in action-menu too.
| * @param direction - `'forward'` slides content in from the right, | ||
| * `'back'` slides content in from the left. | ||
| */ | ||
| private _triggerMobileTransition(direction: 'forward' | 'back'): void { |
There was a problem hiding this comment.
This can be replaced with a @state() instead?
There was a problem hiding this comment.
Want to make sure I'm replacing the right thing — the diff anchor moved a bit between revisions and I'm not 100% sure which member you were pointing at. My best guess: the imperative attribute writes in _triggerMobileTransition / _handleAnimationEnd:
private _triggerMobileTransition(direction: 'forward' | 'back'): void {
this.updateComplete.then(() => {
const wrapper = this.shadowRoot?.querySelector(
'.mobile-submenu-animation-wrapper'
);
if (!wrapper) return;
wrapper.removeAttribute('mobile-transition');
requestAnimationFrame(() => {
wrapper.setAttribute('mobile-transition', direction);
});
});
}If that's the target, agreed — this could be a @state() private _mobileTransition?: 'forward' | 'back' driving mobile-transition=${this._mobileTransition} in the template, with _handleAnimationEnd clearing the state instead of removing the attribute imperatively. That would also let us drop the querySelector and the requestAnimationFrame re-trigger trick (we'd toggle through nothing → next direction across two updates, or use a key on the wrapper to force re-mount).
If you meant something else (e.g. _mobileBackContainers Map, _mobileSubmenuOriginalParents Map), could you point me at the line on the latest commit (6e78ef2) and I'll address it directly? The Maps both store transient DOM references for cleanup and don't directly drive the template, so I left them as plain fields.
| const submenu = this.asMenu(submenuEl); | ||
| await submenu.updateComplete; | ||
|
|
||
| submenu.childItems.forEach((child) => { |
There was a problem hiding this comment.
Could we let the rovingTabIndexController handle this? Does it offer a method to focus on the item?
There was a problem hiding this comment.
Tried this and reverted — leaving a note here so the next person doesn't fall into the same trap.
RovingTabindexController exposes focusOnItem(item), which delegates to its parent FocusGroupController:
focusOnItem(item?: T, options?: FocusOptions): void {
const elements = this.elements || [];
const newIndex: number =
!item || !this.isFocusableElement(item) ? -1 : elements.indexOf(item);
if (newIndex > -1) {
this.currentIndex = newIndex;
elements[this.prevIndex]?.setAttribute('tabindex', '-1');
}
this.focus(options);
}The trap: when item is not in elements, newIndex becomes -1, currentIndex is left alone, and the trailing this.focus() falls back to whatever the controller currently considers focusable — which for the projected submenu's RTI is childItems[0], i.e. the first nested item (the data item, not the back row).
The back button is appended dynamically via Lit's render() inside _renderMobileBackElements, so registration with the parent menu (sp-menu-item-added-or-updated → childItemSet → childItems → controller's elements cache) races against _focusProjectedSubmenu. We await backItem.updateComplete and submenu.updateComplete, but the menu-item-added event isn't gated by either. When the race goes the wrong way, focusOnItem(backItem) no-ops on currentIndex and focus() lands on "New" / first item instead of "Back".
So I kept explicit tabIndex / focused management in _focusProjectedSubmenu (and applied the same pattern to the new _focusMobileBackRow / _focusFirstItemInCurrentNestedSubmenu helpers for consistency):
submenu.childItems.forEach((child) => {
child.tabIndex = -1;
child.focused = false;
});
backItem.tabIndex = 0;
backItem.focused = true;
backItem.focus();The clean follow-up is the larger declarative refactor (back button rendered inside Menu.render() so it's part of childItems from the start) — covered in the imperative-render thread. Once that lands we can switch to rovingTabindexController.focusOnItem(backItem) safely.
| private _renderMobileBackElements(submenuEl: HTMLElement): void { | ||
| this._removeMobileBackElements(submenuEl); | ||
| const container = document.createElement('div'); | ||
| container.className = 'mobile-back-container'; |
There was a problem hiding this comment.
We're still doing this imperatively - can we avoid?
There was a problem hiding this comment.
Acknowledged — partially addressed, not fully resolved. Tracking for a follow-up.
This PR (6e78ef2):
- Moved the inline
style="..."values out of the imperative template intomenu-item.csssodirand tokens flow naturally (see closure-at-render-time thread). - Added
_refreshMobileBackElements()and call it fromupdated()whenmobileBackLabelchanges, so the one remaining reactive value re-evaluates without a stale closure.
Still imperative: _renderMobileBackElements / _removeMobileBackElements and the _mobileBackContainers Map. The back button + divider are still inserted with Lit.render(...) into a <div class="mobile-back-container"> inside the projected submenu's light DOM.
The full refactor you suggested — moving the back row into Menu.render() directly — is straightforward in concept but has one structural wrinkle: the back button has to live inside the projected <sp-menu> (slot="submenu"), not in the parent menu's shadow tree, so it can be discovered by the projected submenu's childItemSet / RovingTabindexController (otherwise arrow keys don't see it; see RTI thread). That means either:
- Have
<sp-menu>conditionally render the back row in its ownrender()when it detects it's projected (i.e.slot === 'mobile-submenu'), driven by a@state()flag set by the parent menu, OR - Keep the imperative
render()but fold it through Lit's lifecycle by using a@property()or@state()on the projected submenu to drive the template.
Both make the back button a first-class child item and would let us drop the manual tabIndex management in _focusProjectedSubmenu (using rovingTabindexController.focusOnItem(backItem) instead). I'd like to do that as a follow-up PR rather than expand this one further — happy to file the issue if you'd like to track it formally.
| slot="icon" | ||
| style="transform: ${iconRotation}" | ||
| ></sp-icon-arrow500> | ||
| ${this.mobileBackLabel} |
There was a problem hiding this comment.
this has a closure at render-time (similar to React), I think if you change this later it won't re-render it
There was a problem hiding this comment.
Good catch — fixed in 6e78ef2. The inline style="transform: ${iconRotation}" and style="margin-block: ${itemMargin}" were both captured at render() call time and would not update if dir flipped or --swc-scale-factor changed afterwards. Two-part fix:
- Moved the styling out of the imperative template into
menu-item.css, scoped to the back-button host so it inheritsdirnaturally:
:host(.mobile-back-button) {
margin-block: calc(4px * var(--swc-scale-factor));
}
:host(.mobile-back-button) ::slotted(.mobile-back-icon) {
transform: rotate(180deg);
}
:host(.mobile-back-button:dir(rtl)) ::slotted(.mobile-back-icon) {
transform: rotate(0deg);
}The Lit template now just sets class="mobile-back-button" and class="mobile-back-icon" on the icon — no inline closures.
- Added
_refreshMobileBackElements()and call it fromupdated()whenevermobileBackLabelchanges, so the label binding (the only remaining reactive value in the imperatively-rendered template) stays in sync:
if (changes.has('mobileBackLabel') && this.hasUpdated) {
this._refreshMobileBackElements();
}Long-term the cleaner solution is your earlier suggestion of moving the back button entirely into Menu.render() so Lit drives the whole thing. Tracking that as a follow-up — see my reply on the imperative-render thread.
There was a problem hiding this comment.
Another high-level comment: this currently requires users to build:
<sp-tray open>
<sp-menu mobile-view>
...
</sp-menu>
</sp-tray>
Should the action-menu do internally automatically inside the isMobile.matches?
There was a problem hiding this comment.
Strong agree — the current authoring shape (consumer wires <sp-tray open><sp-menu mobile-view>...</sp-menu></sp-tray> and manages open/close themselves) is fine for this PR (it keeps sp-menu focused on the drill-down behavior and lets sp-tray do its own job), but it's not the ergonomics we'd want long-term.
A couple of follow-up directions worth tracking:
-
<sp-action-menu>integration. The natural home for "tap a button, get a menu in a tray on small viewports" issp-action-menu. It already owns the trigger + overlay relationship; it could pick the overlay type based on viewport (popover on desktop, tray on mobile) and flipmobile-viewon the inner<sp-menu>automatically. That would reduce consumer code to roughly:<sp-action-menu> <span slot="label">Actions</span> <sp-menu-item>...</sp-menu-item> </sp-action-menu>
…with the tray + drill-down "just working" below some breakpoint (and likely opt-in via a
responsive/mobile-breakpointattribute so consumers can disable it for action menus that should always be popovers). -
<sp-picker>could benefit from the same treatment for long lists with submenus, although picker has more selection-semantics baggage so it's a bigger lift.
I'll file these as follow-up issues so we can scope them properly. The intent of this PR is the primitive (sp-menu[mobile-view]); the higher-level ergonomics belong in the components that own the trigger.
| * of opening a flyout overlay. | ||
| */ | ||
| @property({ type: Boolean, attribute: 'mobile-view', reflect: true }) | ||
| public mobileView = false; |
There was a problem hiding this comment.
if this moves from true -> false, is this working as expected? On Storybook I got weird results
There was a problem hiding this comment.
Reproduced the weirdness — when mobile-view flipped from true → false while a submenu was projected, the projected <sp-menu> stayed in the mobile-submenu slot of the (now non-mobile) parent and the stack still pointed at it, so you'd get a stuck/half-rendered state.
Fixed in 6e78ef2 by handling the toggle in updated():
if (changes.has('mobileView') && this.hasUpdated) {
if (!this.mobileView && this._mobileSubmenuStack.length > 0) {
this.resetMobileSubmenus();
}
}resetMobileSubmenus() already restores every projected submenu back to its original MenuItem parent and clears the stack, so flipping mobile-view off now puts the menu back to a clean flyout layout.
Ergonomics, a11y, and reactivity follow-ups from PR #6063 review: - a11y: drop role="region" + aria-live="polite" from the mobile submenu wrapper; the back-button context already announces the level change and the polite region was producing noisy double announcements. - a11y/motion: honor prefers-reduced-motion by disabling the slide animation on the .mobile-submenu-animation-wrapper. - declarative animation hook: bind @animationend on the wrapper element instead of attaching/removing the listener on the host in connected/disconnectedCallback. Renamed _handleTransitionEnd to _handleAnimationEnd to match the actual event. - reactive back-button styling: move the inline transform/margin from _renderMobileBackElements into menu-item.css scoped to :host(.mobile-back-button) and ::slotted(.mobile-back-icon), including :dir(rtl) mirroring. Added _refreshMobileBackElements and call it from updated() when mobileBackLabel changes so the back button reflects late label changes without re-opening. - runtime mobileView toggle: when mobileView flips true -> false, call resetMobileSubmenus() in updated() so projected submenus are restored to their original parents instead of being stuck in the mobile slot. - forward keydown from slotted descendants: relaxed MenuItem.handleKeydown to forward when target === this OR this.contains(target), so events originating on the slotted back-arrow icon still reach the parent menu. - expose nativeEvent on MenuItemKeydownEvent so listeners can preventDefault/stopPropagation on the underlying KeyboardEvent. - mobile drill-down arrow boundary: register a capture-phase keydown listener on sp-menu (handleMobileDrilldownKeydownCapture) with helpers _focusMobileBackRow and _focusFirstItemInCurrentNestedSubmenu so: * ArrowUp on the first nested item focuses the back row instead of wrapping inside the projected submenu's RTI. * ArrowDown on the back row focuses the first nested item. Capture phase ensures this preempts the projected submenu's RovingTabindexController, which handles arrows in the bubble phase. - explicit focus management for drill-down entry: kept _focusProjectedSubmenu using direct tabIndex/focused writes rather than rovingTabindexController.focusOnItem(backItem) - the back button is appended dynamically via render() and may not be in the controller's element cache when this runs, which caused focus to fall through to the first nested item. - doc-only: marked submenuElement and _mobileSubmenuProjected on MenuItem with @internal explaining they're intra-package contracts used by Menu's mobile drill-down projection. - comments: added rationale for setTimeout(..., 0) deferrals in MenuItem.handleTouchSubmenuToggle (suppresses the trailing synthetic click after the touch sequence). - tests: added Arrow Up from first nested item focuses the back row and Arrow Down from back row focuses first nested item to cover the new boundary navigation. Adds a patch changeset for @spectrum-web-components/menu. Made-with: Cursor
3 participants
Description
Adds a mobile-view mode to
<sp-menu>via theis-mobile-viewattribute that replaces flyout/overlay submenus with an in-place drill-down navigation pattern, similar to the approach used in React Spectrum v3 for S1.What this PR does
@keyframesanimations provide directional slide-in/slide-out when navigating forward and backRovingTabindexControlleroverrideslot="mobile-submenu"and restored on back navigation; a_mobileSubmenuProjectedflag guards against slot-empty false positivessp-menu-itemwithsp-icon-arrow100(rotated 180deg) andsp-menu-divider, prepended to the projected submenu's light DOM<sp-tray>withsp-menu-groupheadings for each submenu levelKey design decisions
sp-menu-itemfor back button instead of a plain<div>: participates in the roving tabindex so keyboard users can navigate seamlessly between the back button and submenu itemschildItemSetsave/restore on reparenting: when a submenu is moved to the root menu's light DOM, itschildItemSetis saved and restored after reparenting to prevent the roving tabindex controller from losing track of focusable items_mobileViewRootgetter on projected submenus: allows a projected submenu to delegate keyboard events (ArrowRight/ArrowLeft) back up to the rootis-mobile-viewmenuFiles changed
Menu.tsMenuItem.tsisMobileViewdetection, click/pointerenter guards,manageSubmenuprojection guardmenu.cssmenu-item.csssubmenu.stories.tsmobileViewstory in<sp-tray>submenu.test.tsDesign discussion
Feedback
Design feedback suggests reconsidering the tray drill-down approach for S2:
Two approaches under consideration
Open questions for design
is-mobile-viewbe opt-in (developer sets attribute) or automatic (detected via viewport/pointer)?Motivation and context
On touch devices, clicking a menu item with a submenu causes flicker and the submenu may be clipped or hidden outside the viewport (SWC-671, SWC-686). This prototype explores a tray-based drill-down as one solution, pending design alignment on the final pattern.
Related issue(s)
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Device review
Accessibility testing checklist
Keyboard (required)
<sp-menu>containing the mobile menuScreen reader (required)
aria-haspopupis announced