Skip to content

Conversation

@devongovett
Copy link
Member

This splits Menu into two components: Menu (as used with MenuTrigger), and ListBox (used inside Picker, ComboBox, etc.). The components have slightly different usecases and accessibility requirements, so this makes sense. The differences between them are:

  • Menu is not virtualized using CollectionView whereas ListBox is. Menus have the requirement of automatically resizing to fit their content, which is not possible with virtualization. When used with Pickers and ComboBoxes, ListBoxes have a fixed width. In general, we expect the length of most Menus to be relatively short (e.g. a fixed list of actions), whereas Pickers and ComboBoxes may contain many items.
  • Menu uses role=menu with role=menuitem, role=menuitemradio, or role=menuitemcheckbox for its items whereas ListBox uses role=listbox with role=option for its items.
  • ListBox uses aria-multiselectable whereas Menu does not.
  • Menu items use aria-checked whereas ListBox items use aria-selected
  • Menus have onClose behavior with different enter and space key behavior.
  • According to Spectrum, Menu items can have keyboard shortcuts and other information/controls on the right side of each item, whereas ListBox items cannot.

The DOM structure changed for sections to be heirarchical, using a role="group" with an aria-labelledby pointing at the heading. This is implemented in Menu so far, but ListBox is pending changes to collection view to support this.

A useSelectableList hook has been added that wraps useSelectableCollection and adds a default keyboard handler for non-virtualized lists. It uses the DOM to get layout information (e.g. for implementing page up/page down support). When non-virtualized, a data-key attribute is added to each item, so that DOM nodes can be found by their key.

In addition, keyboard navigation now skips disabled items. aria-posinset and aria-setsize have been added to items when virtualized.

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

Comment on lines +75 to +79
slots={{
text: styles['spectrum-Menu-itemLabel'],
icon: styles['spectrum-Menu-icon'],
description: styles['spectrum-Menu-description']
}}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed this in Listbox:
image

I think you just need to add

            end: styles['spectrum-Menu-end'],
            keyboard: styles['spectrum-Menu-keyboard']

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's a bug in the story. The end and keyboard slots were removed from listbox intentionally after talking to design. Will be fixed in my next PR.

LFDanLu
LFDanLu previously requested changes Mar 6, 2020
Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the one change mentioned above, looks fine to me

@adobe-bot
Copy link

Build successful! 🎉

@dannify dannify dismissed LFDanLu’s stale review March 7, 2020 00:02

will be followed up in next pr

@dannify dannify merged commit 8962237 into master Mar 7, 2020
@dannify dannify deleted the menu-listbox branch March 7, 2020 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants