spectrum-web-components Fix controlled menuitems

Description

"A" (probably not "the") fix for the currently-broken controlled action-menu items:

https://opensource.adobe.com/spectrum-web-components/storybook/?path=/story/action-menu--controlled

(opening and closing, without clicking on any items, will clear all items' selected states) (it probably should not trigger the "action" popup immediately; that may be a separate issue)

The root cause is that PickerBase always assigns its child <sp-menu>'s .selected array. Since ActionMenu extends PickerBase, it also always assigns the selected array. So an action menu like this:

<sp-action-menu>
  <sp-menu-item selected>Red</sp-menu-item>
  <sp-menu-item selected>Green</sp-menu-item>
  <sp-menu-item selected>Blue</sp-menu-item>
</sp-action-menu>

...will render correctly, once (all items visibly selected), but upon opening & closing the menu, the action-menu will reset the .selected array to [] which will clear the selected state from every item. This behavior started here:

https://github.com/adobe/spectrum-web-components/pull/3669

This demonstration fix sidesteps the issue by not setting selected on unmanaged action menus. That change can't be applied directly to PickerBase because then there are side effects for other components that inherit from PickerBase, and even for action menus that are intended to be used by assigning values to .selected.

Sharing this imperfect/hacky fix in order to start a conversation about what a long-term fix could look like.

Feb 16 '24 20:02 hunterloftis

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:

Feb 16 '24 20:02 github-actions[bot]

Lighthouse scores

Category	Latest (report)	Main (report)	Branch (report)
Performance	0.94	0.97	0.98
Accessibility	1	1	1
Best Practices	1	1	1
SEO	1	0.92	0.92
PWA	1	1	1

What is this?

Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on main ("Main"). Higher scores are better, but note that the SEO scores on Netlify URLs are artifically constrained to 0.92.

Transfer Size

Category	Latest	Main	Branch
Total	243.641 kB	230.019 kB	230.016 kB 🏆
Scripts	60.538 kB	54.028 kB	53.97 kB 🏆
Stylesheet	50.612 kB	44.14 kB	44.138 kB 🏆
Document	5.749 kB	5.109 kB 🏆	5.166 kB
Third Party	126.742 kB	126.742 kB	126.742 kB

Request Count

Category	Latest	Main	Branch
Total	42	42	42
Scripts	34	34	34
Stylesheet	5	5	5
Document	1	1	1
Third Party	2	2	2

Feb 16 '24 20:02 github-actions[bot]

Tachometer results

Chrome

action-menu permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	655 kB	162.75ms - 166.01ms	-	unsure 🔍 -1% - +1% -2.36ms - +2.24ms
branch	647 kB	162.82ms - 166.06ms	unsure 🔍 -1% - +1% -2.24ms - +2.36ms	-

picker permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	518 kB	599.32ms - 614.30ms	-	unsure 🔍 -2% - +2% -10.49ms - +10.45ms
branch	509 kB	599.52ms - 614.15ms	unsure 🔍 -2% - +2% -10.45ms - +10.49ms	-

split-button permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	721 kB	1856.96ms - 1859.57ms	-	unsure 🔍 -0% - +0% -1.07ms - +2.68ms
branch	713 kB	1856.12ms - 1858.80ms	unsure 🔍 -0% - +0% -2.68ms - +1.07ms	-

Firefox

action-menu permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	655 kB	341.25ms - 349.59ms	-	unsure 🔍 -2% - +2% -5.48ms - +7.84ms
branch	647 kB	339.05ms - 349.43ms	unsure 🔍 -2% - +2% -7.84ms - +5.48ms	-

picker permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	518 kB	1050.66ms - 1057.62ms	-	unsure 🔍 -2% - +1% -19.44ms - +10.56ms
branch	509 kB	1043.99ms - 1073.17ms	unsure 🔍 -1% - +2% -10.56ms - +19.44ms	-

split-button permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	721 kB	1570.62ms - 1574.58ms	-	unsure 🔍 -0% - +0% -3.10ms - +2.54ms
branch	713 kB	1570.86ms - 1574.90ms	unsure 🔍 -0% - +0% -2.54ms - +3.10ms	-

Feb 16 '24 20:02 github-actions[bot]

Could similar be achieved by moving the application of selected from the declarative renderer to something like:

    protected override updated(): void {
        if (this.selects) {
            (this.shadowRoot.querySelector('sp-menu') as Menu).selected= this.value ? [this.value] : [];
        }
    }

Types are not so happy about it at current, but you can seemingly do this declarative with:

    .selected=${this.selects ? (this.value ? [this.value] : []) : undefined}

Feb 16 '24 20:02 Westbrook

I'm not sure what the contract is exactly between selects and selected - if selects is undefined, is the component allowed to ignore selected?

If so then 👍 that's the way to go.

Feb 16 '24 20:02 hunterloftis

When the selects attribute is not present a value will not be maintained and the Menu Item children of this Menu will not have their selected state managed.

No idea whether there are tests that behave differently or not on this.

Feb 16 '24 20:02 Westbrook

No idea whether there are tests that behave differently or not on this.

It looks like there are tests in menu for when selects is actually set, but no tests for situations where it's undefined?

Feb 16 '24 20:02 najikahalsema

No idea whether there are tests that behave differently or not on this.

It causes this test to fail: https://github.com/adobe/spectrum-web-components/blob/4a31c5b407e3cb4f3d684363d8d35008488ec617/packages/action-menu/test/index.ts#L499

So far I've only come up with fixes that have a breaking API change: either requiring selects to be some value in order for selected to apply, or requiring selects to be some special sentinel value (or adding another sentinel like unmanaged) in order for selected to not apply.

Feb 16 '24 20:02 hunterloftis

What happens if you change https://github.com/adobe/spectrum-web-components/blob/main/packages/menu/src/Menu.ts#L95 to be if (selected === this.selected || !this.selects) {`?

There's also likely a reasonable path to retiring a test or two if we need to in order to have an understandable API for this....

Feb 16 '24 21:02 Westbrook

@Westbrook that also fails the "allows top-level selection state to change" since right now, setting menu.selected works regardless of the value of selects & that's how it's tested. I think it would be reasonable to change that, but I'm not sure what to, since the values available to selects seems to mean "manage selected automatically/internally" whereas setting selected means "I'll set this explicitly/externally."

Maybe a new value like selects = "selected"? That's a breaking API change but it seems like a decent path forward.

The alternative is the inverse, like selects = "none", which would be a sentinel to block any internal management of the selected value. I slightly prefer that option because it seems a little clearer & easier to explain.

Feb 20 '24 18:02 hunterloftis

Just to add my 2 cents here, as I also try to extend current ActionMenu to a CustomActionMenu for different reasons, and the selection behavior was one problem to solve here, as we want to control the selection states of the menu-items from the outside. So selects of the ActionMenu is undefined and setting sp-menu-item to selected and then open/close ActionMenu removes the selection state. Can we maybe try to only set the .selected value of PickerBase.renderMenu if the ActionMenu has a selects value !== undefined? Maybe you can skip the unmanaged property and just compare for this.selects resulting in:

${this.selects ? this.renderMenu : this.renderUnmanagedMenu}

Feb 22 '24 09:02 spdev3000

Would be better to confirm that selection is appropriately managed when using event.preventDefault() on the change event rather than expanding the API here. Closing in preference of a solution in that shape.

May 10 '24 13:05 Westbrook

spectrum-web-components spectrum-web-components copied to clipboard

Fix controlled menuitems

Description

Branch preview

Lighthouse scores

Transfer Size

Request Count

Tachometer results

action-menu permalink

picker permalink

split-button permalink

action-menu permalink

picker permalink

split-button permalink

spectrum-web-components
spectrum-web-components copied to clipboard