spectrum-web-components

spectrum-web-components copied to clipboard

Code of conduct

• [X] I agree to follow this project's code of conduct.

Impacted component(s)

Menu

Expected behavior

Insert of space charactars into textfields wrapped by sp-menus is possible, detection of lastFocusedItem handles also undefined state.

Actual behavior

Entering space characters into a textfield inside a sp-menu crashes the menu logic of handling of lastFocusedItem.

TypeError: Cannot read properties of undefined (reading 'hasSubmenu')
    at Menu.handleKeydown (https://srv.divriots.com/packd/@spectrum-web-components/action-menu/sp-action-menu.js,@spectrum-web-components/menu/sp-menu-divider.js,@spectrum-web-components/menu/sp-menu-item.js,@spectrum-web-components/menu/sp-menu.js,@spectrum-web-components/story-decorator/decorator.js,@spectrum-web-components/textfield/sp-textfield.js,lit-element,lit-html,tslib:632:6381)

Screenshots

No response

What browsers are you seeing the problem in?

Firefox, Chrome

How can we reproduce this issue?

1. Go to https://studio.webcomponents.dev/edit/rjLEOAa2OyhLlWzxllP9
2. Click on action menu to open menu
3. Click on submenu
4. Enter text into textfield
5. Check console
6. See error

Sample code that illustrates the problem

public handleKeydown(event: KeyboardEvent): void {
        const { code } = event;
        if (code === 'Tab') {
            this.prepareToCleanUp();
            return;
        }
        if (code === 'Space') {
            const lastFocusedItem = this.childItems[this.focusedItemIndex];
            if (lastFocusedItem.hasSubmenu) {
                // Remove focus while opening overlay from keyboard or the visible focus
                // will slip back to the first item in the menu.
                this.blur();
                lastFocusedItem.openOverlay();
                return;
            }
        }

Can be fixed by using

 if (lastFocusedItem?.hasSubmenu) {

Logs taken while reproducing problem

No response

I menu is not technically allowed to have this type of content. Can you share some more information about the larger interaction at play here so that we might address this at a higher level>

One initial though, as <sp-textfield> doesn't really belong directly in an <sp-menu>... still pretty hypothetical at this point, however. https://studio.webcomponents.dev/edit/XtzQqgirl81eYNsWe41Z/src/index.ts?p=stories

The interaction at play aside, I do think that the code itself should have a check for lastFocusedItem to exist before querying to hasSubmenu. If you want to throw an error or warning there, I think that makes sense but we should still protect the statement from failing.

Since this is patched in PSWeb, I took a stab at a minimal fix for SWC: https://github.com/adobe/spectrum-web-components/pull/2589

Happy to add any necessary messaging, etc. Please advise.

This is an interesting one, because it IS an error and the code should never be allowed to get that far. In raw HTML it would be a parser error that would push the <sp-textfield> out of the <sp-menu> without telling you. I'm much more prone to do similar here, maybe ::slotted(:not(sp-menu-item)) { display: none }.

@spdev3000 you're demo no longer seems to exhibit the erroneous code that id did before. Can you resubmit the repro and share a little more as to what you're looking to achieve in app with this approach?

While if (lastFocusedItem?.hasSubmenu) { will prevent an error from surfacing, it does open the path towards even more focus bearing elements being slotted into this context where the accessibility model delivered doesn't adequately support them at current.

@Westbrook I have restructured the demo to create that error: https://studio.webcomponents.dev/edit/rjLEOAa2OyhLlWzxllP9 The difference is that we are currently using a real <sp-menu slot="submenu"><sp-textfield goes here...></sp-menu> which creates the error. I have seen that in https://studio.webcomponents.dev/edit/XtzQqgirl81eYNsWe41Z/src/index.ts?p=stories you just used <div slot="submenu">? Does this work with all the events and use cases the current submenu system offers?

<div slot="submenu"> certainly does not, but neither does <sp-menu><sp-textfield>, e.g. just removing the error above doesn't throw focus into the Textfield when navigating with the keyboard (at least in your current demo) and attempting to edit the Textfield with the Left Arrow key also closes the "submenu", etc. as you might expect when opening a "submenu". What's more is there's a high likelihood of this "off label" usage facing unexpected bugs in the future for not being a tested part of the API.

Are you able to share more of the goals and expectations of structuring a menu like this so we can look into adequately supporting this use case. Questions that will be important to answer:

• will this form of submenu have any non-Textfield content
• will there be mixed Menu Item and Textfield content
• will this form of submenu have multiple tab stops within its content (e.g. more than one Textfield). This could be a particularly tricky point to cover in the way that your project downgrades overlays from modals, as modal content is the only content that is fully certified to have more than one overlaying tab stop as of today.
• what sort of "submit" actions would occur in these "submenus". At current click submits the change event of a submenu, would there be a similar interaction for form-like content?
  • A submit button?
  • Simple input tracking on the element?
  • What (if any) of these interactions should cause the entire menu system to close in the way that a Menu Item click does?

I'll probably have more questions as this comes together, but this should be enough to get us started.

If you feel like the proposed PR gives you enough support short term, we can definitely get some tests added and ship that while we work towards fully supporting your goals here.

@Westbrook Oh yes and I do have tests started for this but haven't pushed it up yet - was waiting for the reproduction example.

The repro in https://studio.webcomponents.dev/edit/rjLEOAa2OyhLlWzxllP9 no longer exhibits this issue. Please feel free to reopen if you find an updated path to approach this error.