Adixon 1189 menu prep #1475

adixon-adobe · 2021-05-22T16:57:19Z

Continued work on selects attribute in sp-menu & sp-menu-group

Preview link: https://adixon-1189-menu-prep--spectrum-web-components.netlify.app/

Description

For this round I've tried to keep the changes as scoped to sp-menu as possible, given that the reparenting in PickerBase is going to be tricky to work through.

I'm stuck on initialization for sub-menus though -- prepItems isn't getting called once the slotted items in the underlying sp-menu-group's sp-menu are present (meaning that after initializing menu groups always think they have 0 menu items). I've got some new unittests that are failing, and it's pretty easy to see in the storybook for "Menu Group Selects Multiple"

The only (known) work left here outside of that is getting the documentation work.

Related Issue

Initial work for #1189
fixes #715

Motivation and Context

Gets us closer to more useful menus

How Has This Been Tested?

Storybook & unit tests.

Here's an example of what's failing: the "Oakland" menu group should be single-select but it initializes wrong. If you keep using the menu it eventually starts working properly once prepItems has been called correctly.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
[ x ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[ x ] I have signed the Adobe Open Source CLA.
[ x ] My code follows the code style of this project.
[ x ] My change requires a change to the documentation.
I have updated the documentation accordingly.
[ x ] I have read the CONTRIBUTING document.
[ x ] I have added tests to cover my changes.
All new and existing tests passed.

… nested menus

packages/picker/src/Picker.ts

packages/menu/src/MenuItem.ts

packages/menu/src/Menu.ts

adixon-adobe · 2021-05-25T14:55:46Z

I think I have a bit more work to do with inherits & selection styles here as well -- I bet we're not applying no selection styles in some menu group cases we should.

Westbrook

This is a code only pass. I'll take a closer look at the actual experience, particularly as it relates to your questions either later today or tomorrow morning.

Westbrook · 2021-05-25T18:49:48Z

packages/menu/src/Menu.ts

+
+ // For the multiple select case, we'll join the value strings together
+ // for the value property with this separator
+ @property({ type: String })


Should this have a non-camelcase attribute? Or should this be only a property?

Switched to non-camelCase. I should also add some tests around this (and a bit more for selection in general).

I don't see a reason for it not to be an attribute as long as it's a string.

packages/menu/src/Menu.ts

Westbrook · 2021-05-25T19:43:59Z

packages/menu/src/MenuGroup.ts

@@ -41,13 +46,38 @@ export class MenuGroup extends SpectrumElement {
 MenuGroup.instances += 1;
 }

+ private get menuRole() {
+ if (this.selects) {
+ return 'menu';


I'm not sure this can ever see children through the slot in a screen reader, so the role here may actually go somewhere else. I'll have to visit the preview site to confirm.

let me know if there's anything to do here.

Thanks for reminding me, this looks good right now!

packages/menu/src/MenuGroup.ts

packages/menu/src/MenuItem.ts

packages/picker/src/Picker.ts

adixon-adobe · 2021-06-01T22:20:00Z

@Westbrook pushed up my WIP since I think I've finally got a solid approach to managing item ownership based on our discussions.

Now "sp-menu-item-added" and "sp-menu-item-removed" events are handled to pick up the menu items a parent menu should own. To get the right DOM order we do a debounced update. Looks like some focus issues got introduced, at least in the Picker context, and there's likely more issues with this that need to be fixed for menus with groups that have different ownership

packages/menu/src/MenuItem.ts

adixon-adobe · 2021-06-01T22:30:13Z

packages/menu/src/Menu.ts

+ private addItem(item: MenuItem) {
+ this.menuItemSet.add(item);
+ this.updateMenuItems();
+ }


This and the updateMenuItems method are the bulk of the new management logic. While there's a lot more to clean up in this PR, it'd be helpful to get some feedback on this approach.

IIUC, the this.menuItems array that is built in this.updateMenuItems() only needs to be maintained in the original sp-menu descendent, right? Like in this case:

<sp-menu> // I'm a menu. <sp-menu-group> // I own a menu <sp-menu-item></sp-menu-item> </sp-menu-group> <sp-menu-group> // I also own a meni <sp-menu-item></sp-menu-item> </sp-menu-group> <sp-menu> // I'm also a menu <sp-menu-item></sp-menu-item> </sp-menu> </sp-menu>

While technically there would be 4 sp-menu elements in the various shadow trees, only the one at the root would need to coalesce this data in order to manage focus/arrow keys/etc. Do we need additional logic to skip that work, or am I missing something?

If a sub-menu manages it's own selection things get trickier, since we still want arrow keys to behave sensibly. I haven't addressed that yet (realized this case right after I pushed up yesterday). So tweaking your example slightly:

<sp-menu> // I'm a menu. <sp-menu-group> // I own a menu <sp-menu-item></sp-menu-item> </sp-menu-group> <sp-menu-group selects="single"> // I'm a menu that manages my own selection <sp-menu-item></sp-menu-item> </sp-menu-group> <sp-menu> // I'm also a menu <sp-menu-item></sp-menu-item> </sp-menu> </sp-menu>

One question I have here is whether I need to manage 2 lists. One for focus (every item the menu is a parent of), and one for just the items the menu directly manages.

This is actually what I was thinking. There are ways to not do this, but I'm pretty certain that while the selection context is handled locally, the focus management has to happen globally. There is likely some tricky accessibility concerns with how the screen reader relates to the data here...

You could also have a more complicated single list?

type ChildItem = { element: MenuItem, // reference to DOM element managed: boolean, // whether this item is part of the selection pool active: boolean, // whether this is the "focused" item or not } type ChildItems = ChildItem[];

Okay, this is helpful!

Would only the top-level parent menu need to manage the focus list (at least until we support nested menus)?

I believe so.

For all intents and purposes, each nested menu will act as its own root: https://webcomponents.dev/edit/Aze4atcpZc7EsxqnCBve/src/index.ts anyways.

packages/menu/src/Menu.ts

Westbrook · 2021-06-02T12:30:46Z

packages/menu/src/Menu.ts

+ private addItem(item: MenuItem) {
+ this.menuItemSet.add(item);
+ this.updateMenuItems();
+ }


IIUC, the this.menuItems array that is built in this.updateMenuItems() only needs to be maintained in the original sp-menu descendent, right? Like in this case:

<sp-menu> // I'm a menu. <sp-menu-group> // I own a menu <sp-menu-item></sp-menu-item> </sp-menu-group> <sp-menu-group> // I also own a meni <sp-menu-item></sp-menu-item> </sp-menu-group> <sp-menu> // I'm also a menu <sp-menu-item></sp-menu-item> </sp-menu> </sp-menu>

While technically there would be 4 sp-menu elements in the various shadow trees, only the one at the root would need to coalesce this data in order to manage focus/arrow keys/etc. Do we need additional logic to skip that work, or am I missing something?

Westbrook · 2021-06-02T12:34:20Z

packages/menu/src/Menu.ts

+ ] as MenuItem[]);
+ for (const childItem of childItems) {
+ if (this.menuItemSet.has(childItem)) {
+ this.menuItems.push(childItem);


Related to the below comment around who owns this.menuItems to manage keyboard and focus work, it seems like this.menuItems is a larger collection that this.menuItemSet (which is about selection, right?) in way that questions this logic...

thought I'd responded yesterday. With the approach here, this.menuItemSet is used to manage what items are owned, while this.menuItems has thee same items in the right DOM order.

I can't say I like it, and one thing I'm thinking is that moving more of the logic here into a menuItems getter (along with an invalidation flag), might help clean this up, and address some of the other feedback you have here.

Let me know what you think of the way this is re-worked now.

Westbrook · 2021-06-02T12:35:50Z

packages/menu/src/Menu.ts

+ }
+ }
+
+ this.updateSelectedItemIndex();


Possibly nit as O(1n) and O(2n) are kind of the same...

This walks the array we just built again, is there room to merge the work?

there might be, but there are times we only need to do updateSelectedItemIndex. I'll take a look once I get more of the changes here cleaned up.

packages/menu/src/Menu.ts

packages/menu/src/MenuItem.ts

adixon-adobe · 2021-06-03T15:30:53Z

packages/menu/src/Menu.ts

+ let selected: string[] = [];
+ let selectedItems: MenuItem[] = [];


that's weird. I'd expect the linter to have at least caught this at commit time.

adixon-adobe · 2021-06-03T15:36:42Z

packages/menu/src/Menu.ts

+ ] as MenuItem[]);
+ for (const childItem of childItems) {
+ if (this.menuItemSet.has(childItem)) {
+ this.menuItems.push(childItem);


thought I'd responded yesterday. With the approach here, this.menuItemSet is used to manage what items are owned, while this.menuItems has thee same items in the right DOM order.

I can't say I like it, and one thing I'm thinking is that moving more of the logic here into a menuItems getter (along with an invalidation flag), might help clean this up, and address some of the other feedback you have here.

adixon-adobe · 2021-06-03T15:39:01Z

packages/menu/src/Menu.ts

+ private addItem(item: MenuItem) {
+ this.menuItemSet.add(item);
+ this.updateMenuItems();
+ }


Okay, this is helpful!

Would only the top-level parent menu need to manage the focus list (at least until we support nested menus)?

adixon-adobe · 2021-06-03T15:42:54Z

packages/picker/test/picker.test.ts

@@ -679,7 +693,7 @@ describe('Picker', () => {
 const el = await pickerFixture();
 el.addEventListener('change', (event: Event) => {
 const { value } = event.target as Picker;
- console.log('change', value);
+ //console.log('change', value);


these seemed noisy, but probably worth leaving in the code since it's useful for debugging. Happy to re-enable though!

adixon-adobe · 2021-06-03T15:47:24Z

packages/menu/test/menu-group.test.ts


 describe('Menu group', () => {
 it('renders', async () => {
 const el = await fixture<Menu>(
 html`
- <sp-menu>
+ <sp-menu selects="none">


Right now this outer menu won't manage the child sp-menu-group elements without selects being one of none, single, or multiple, but I'm thinking it might be better for the behavior to default to selects="none" for the outer parent menu (i.e. if there's nothing to inherit from the parent).

I also realize that we need a way to have an "unmanaged" state for the PickerBase case, since at least for this round PickerBase is still managing it's own selection.

Sub-note, styling for the Menu in a Picker will currently require a selects attribute (value not withstanding) in order to display correctly when going between having a selected child and not. That also means that if <sp-menu selects="none"> actually doesn't take a selected child that it will be visual "incorrect".

This likely doesn't help. Can you clarify what information would be good for me to share my thoughts on here?

Oh yeah -- I even looked at that styling earlier today. I think I also need to fix styling for nested menus so I'll leave this open until that's resolved.

It'd be good to get your thoughts on having the default behavior be that selection is unmanaged (if no parent is managing selection). I think this also means that by default the styling should allow for selection. As I think about it more it seems reasonable, but we could certainly go a different direction.

I don't understand this. Can you share a couple of code samples and the expected output?

It's probably easiest to think about the difference between selects="none" and no selects.

For selects="none" we don't allow selection (and in the menu I'm capturing the click event, though that might be unnecessary now). This means that clients should add click events to the individual menu items here, at least the way it's coded now. When adding/updating items we also might clear the selected state on any items marked as selected.

When selects isn't present, selection is totally unmanaged. Click events are passed up so either a parent menu or PickerBase can handle it in the way they need. In general this probably isn't the state you want if there's not a parent managing things.

The one piece of this I haven't thought through is whether the default styling with no selects should be that the items have the selectable styling or not. Keeping it selectable is handy for the PickerBase case.

Also add a still-failing test related to selects changing

Also reworked the way menuItems updates to make it clearer it's derived from menuItemSet.

adixon-adobe · 2021-06-07T14:01:04Z

@Westbrook I'm seeing one unit test failure on Firefox I might need some help with. picker.test "opens without visible focus on a menu item on click" -- I don't see the same in storybook, but the test says it is getting visible focus (also possible I just need to rebase).

Westbrook · 2021-06-14T15:36:21Z

Closing in favor of adixon-1189-menu-selects

adixon-adobe added 4 commits May 21, 2021 14:57

feat: start adding single & multi-select logic to menu

f016388

feat: start adding selects support to menu group

a34bea8

fix: (from Westbrook) sp-opened was triggering before focus had resolved

9090908

feat: update aria-roles and almost get menus initialized properly for…

df8bb59

… nested menus

adixon-adobe added the Component: Menu label May 22, 2021

adixon-adobe requested a review from Westbrook May 22, 2021 16:57

adixon-adobe commented May 22, 2021

View reviewed changes

packages/picker/src/Picker.ts Outdated Show resolved Hide resolved

packages/menu/src/MenuItem.ts Outdated Show resolved Hide resolved

packages/menu/src/Menu.ts Outdated Show resolved Hide resolved

packages/menu/src/Menu.ts Outdated Show resolved Hide resolved

Westbrook reviewed May 25, 2021

View reviewed changes

Westbrook mentioned this pull request May 28, 2021

Support an "sp-action-menu" w/o value #350

Closed

adixon-adobe force-pushed the adixon-1189-menu-prep branch from f98d2aa to be243a6 Compare June 1, 2021 22:24

fix: remove egregious debugging line

a3d8826

adixon-adobe commented Jun 1, 2021

View reviewed changes

packages/menu/src/MenuItem.ts Show resolved Hide resolved

adixon-adobe commented Jun 1, 2021

View reviewed changes

Westbrook reviewed Jun 2, 2021

View reviewed changes

chore: clean up some dead code and address some minor code feedback

02bbfac

Westbrook mentioned this pull request Jun 3, 2021

sp-menu-group is overkill #1505

Closed

feat: re-work selects to be consistent between sp-menu and sp-menu-group

db9b131

adixon-adobe commented Jun 3, 2021

View reviewed changes

adixon-adobe added 4 commits June 3, 2021 11:42

chore: cache resolved selects, and improve the item-added comment

f19774a

Also add a still-failing test related to selects changing

fix: properly update menu items & roles on selects changes

8bcb5c4

Also reworked the way menuItems updates to make it clearer it's derived from menuItemSet.

chore: forgot to add this to the previous commit

bd556eb

feat: track all child item so we can do proper focus resolution

b2d74d4

Westbrook force-pushed the main branch from 7cfc3bd to 1ffdbe1 Compare June 7, 2021 12:39

adixon-adobe added 2 commits June 7, 2021 08:44

fix: picker tests by changing added event to connectedCallback

6593bfe

chore: some small review tweaks

ad92af1

Westbrook changed the base branch from main to next June 12, 2021 00:54

Westbrook closed this Jun 14, 2021

Westbrook deleted the adixon-1189-menu-prep branch June 24, 2021 23:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adixon 1189 menu prep #1475

Adixon 1189 menu prep #1475

adixon-adobe commented May 22, 2021 •

edited by Westbrook

Loading

adixon-adobe commented May 25, 2021

Westbrook left a comment

Westbrook May 25, 2021

adixon-adobe Jun 7, 2021

Westbrook May 25, 2021

adixon-adobe Jun 7, 2021

Westbrook Jun 7, 2021

adixon-adobe commented Jun 1, 2021

adixon-adobe Jun 1, 2021

Westbrook Jun 2, 2021

adixon-adobe Jun 2, 2021

adixon-adobe Jun 2, 2021

Westbrook Jun 2, 2021

Westbrook Jun 2, 2021

adixon-adobe Jun 3, 2021

Westbrook Jun 3, 2021

Westbrook Jun 2, 2021

Westbrook Jun 2, 2021

adixon-adobe Jun 3, 2021

adixon-adobe Jun 3, 2021

Westbrook Jun 2, 2021

adixon-adobe Jun 2, 2021

adixon-adobe Jun 3, 2021

adixon-adobe Jun 3, 2021

adixon-adobe Jun 3, 2021

adixon-adobe Jun 3, 2021

adixon-adobe Jun 3, 2021

adixon-adobe Jun 3, 2021

Westbrook Jun 3, 2021

adixon-adobe Jun 3, 2021

Westbrook Jun 3, 2021

adixon-adobe Jun 4, 2021

adixon-adobe commented Jun 7, 2021

Westbrook commented Jun 14, 2021

		let selected: string[] = [];
		let selectedItems: MenuItem[] = [];

Adixon 1189 menu prep #1475

Adixon 1189 menu prep #1475

Conversation

adixon-adobe commented May 22, 2021 • edited by Westbrook Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

adixon-adobe commented May 25, 2021

Westbrook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adixon-adobe commented Jun 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adixon-adobe commented Jun 7, 2021

Westbrook commented Jun 14, 2021

adixon-adobe commented May 22, 2021 •

edited by Westbrook

Loading