Reuse the menu pane component for dropdown select options #17271

tidy-dev · 2023-08-23T01:34:58Z

Supersedes #17231.

xref. https://github.com/github/accessibility-audits/issues/4936

Description

This PR aims to make the dropdown select button meet the accessibility criteria described in the W3C Action Menu Button Example

I first start to simply change it to use ul and a li and knew that a lot of this logic was already done in our windows app menu components and so I started stealing bits of logic till I determined that I could probably just reuse the MenuPane component. That is what this PR does and voila I got the vast majority of keyboard we needed from that.

That is:

Enter key "clicks" a selected item.
Up/Down arrows navigate the list items
Home and End keys move to the first and last list items

There is a on pane keydown handler to allowed me to easily add the "Escape" to close the menu and set focus to the dropdown button.

Things I have added to the dropdown select component:

The dropdown button aria attributes that Sergio had added in Improve accessibility of dropdown select button #17231
The dropdown button now has some more keyboard functionality
- Down Arrow, Space, and Enter open menu to first menu item
- Up Arrow opens menu to last menu item
- Escape closes the dropdown if open

Things added to the menu pane and menu list item

Ability to navigate the list by hitting the first character of a menu item
Ability to render a label via a callback for JSX.Element rendering (as opposed to just a string)
MenuItems of type Checkbox use the role "menuitemradio".

Other thoughts:

Now the IDropdownSelectButtonOption feels like another abstraction that is being translated into a MenuItem.. maybe I should refactor one step more and just make it so the options are MenuItem, or make it extend the MenuItem. But, not sure if it is worth changing more stuff..
- Edit: Have since updated such that that option does use same properties just for ease of reading the code, but extending the menu item is a little more messy as the three half a 4-5 unused properties by this implementation. Thus, translating is less trouble then everywhere we use defining useless properties or updating the current app window such that those properties are optional (really trying not to impact current windows system menus).
Now both macOS and Windows will use these components (previously only used by windows app menu), thus broadening use case. (not necessarily a bad thing.. just of note) .. Could have tried to pull out just the reused bits into more reusable components or methods.. but seemed like a lot of effort for not a lot of gain as there was quite a bit of overlap.

Screenshots

This video goes side by side down through the criteria in W3C Action Menu Button Example and demonstrates it with a key caster on.

Screen.Recording.2023-08-23.at.4.51.55.PM.mov

Things not highlighted in the video:

the button component losing focus (tab or mouse) will close the menu
the dropdown button has an aria-label - the criteria says aria-labelledby but there is no element with a label to apply here
the criteria mentions apply the role of menuitem to the items. We use menuitemradio as one the menu items is marked as checked. It has the aria-checked applied to it.

Release notes

Notes: [Improved] Improve accessibility of dropdown select buttons

app/src/ui/app-menu/menu-list-item.tsx

app/src/ui/app-menu/menu-pane.tsx

tidy-dev · 2023-08-23T01:37:44Z

app/src/ui/dropdown-select-button.tsx

@@ -20,16 +23,19 @@ interface IDropdownSelectButtonProps<T extends string> {
  readonly options: ReadonlyArray<IDropdownSelectButtonOption<T>>

  /** The selection option value */
-  readonly selectedValue?: T
+  readonly checkedOption?: T


Since the menu items uses the term "selected" to denote the "Highlighted" or "Focused" value, I changed this to "checked" to make that distinction clearer.

app/src/ui/dropdown-select-button.tsx

app/styles/ui/_dropdown-select-button.scss

tidy-dev · 2023-08-23T21:06:39Z

When the menu closes, as important for accessibility, it returns focus to the dropdown button. This is how it is programmed and works for the suggested next actions and the merge options in the compare branch feature. However, for the merge, squash merge, and rebase dialog, the dialog is actually re-rendered when switching between rebase and one of the other two options. Thus, the focus is reset to the filter. This will take a little rework of the multi commit operation handling of that first step of those operations to fix. I will address in another PR.

Expected behavior:

Screen.Recording.2023-08-23.at.5.06.59.PM.mov

Bug:

Screen.Recording.2023-08-23.at.5.08.20.PM.mov

This reverts commit 2b2596a.

tidy-dev · 2023-08-23T21:16:55Z

app/src/ui/app-menu/menu-list-item.tsx

@@ -161,6 +178,13 @@ export class MenuListItem extends React.Component<IMenuListItemProps, {}> {
      selected: this.props.selected,
    })

+    const role = this.props.hasNoRole
+      ? undefined
+      : type === 'checkbox'


So.. this should probably be the radio one... but it uses the wrong icon. :/ May circle back and allow for a custom icon.

sergiou87

Amazing refactor!! Great work!!! 👏

It looks good to me, only found a few nits, and testing worked as expected, I couldn't detect regressions on menus.

I will approve the PR in case you decide to not to accept my suggestions or to do so in a different PR.

app/src/ui/app-menu/menu-list-item.tsx

app/src/ui/lib/button.tsx

app/src/ui/suggested-actions/dropdown-suggested-action.tsx

app/src/ui/dropdown-select-button.tsx

Co-authored-by: Sergio Padrino <sergio.padrino@gmail.com>

tidy-dev added 10 commits August 22, 2023 14:09

Reuse Menu Pane component

83b38b6

Add mouse clear event

8450d2a

Dropdown button aria attributes

e078909

Don't close dialog on escape

0851f9b

Make label render function

9aa0a37

Remove uneeded undefined and default value

c2a11f4

Add navigation via a-z characters

178912e

Add dropdown button event handling

fb788e1

Linter

8cbb3ee

Focus dropdown button on close

52a3fd0

tidy-dev commented Aug 23, 2023

View reviewed changes

app/src/ui/app-menu/menu-list-item.tsx Show resolved Hide resolved

tidy-dev commented Aug 23, 2023

View reviewed changes

app/src/ui/app-menu/menu-pane.tsx Show resolved Hide resolved

tidy-dev commented Aug 23, 2023

View reviewed changes

app/src/ui/dropdown-select-button.tsx Outdated Show resolved Hide resolved

tidy-dev commented Aug 23, 2023

View reviewed changes

app/styles/ui/_dropdown-select-button.scss Show resolved Hide resolved

tidy-dev marked this pull request as draft August 23, 2023 01:43

tidy-dev added 5 commits August 23, 2023 10:14

Remove Generic typing so id can be a string like menus

5f8bd13

Linter

a607ad5

Tidying

81423d2

Close menu when select button loses focus

6712a11

Making the alignment and border separation happier

ef22059

tidy-dev marked this pull request as ready for review August 23, 2023 18:09

tidy-dev changed the title ~~See if we can reuse the menu pane component for dropdown select options~~ Reuse the menu pane component for dropdown select options Aug 23, 2023

tidy-dev requested a review from sergiou87 August 23, 2023 21:02

tidy-dev added 2 commits August 23, 2023 17:14

Use Radio not checkbox..

2b2596a

Revert "Use Radio not checkbox.."

229de1e

This reverts commit 2b2596a.

tidy-dev commented Aug 23, 2023

View reviewed changes

sergiou87 self-assigned this Aug 24, 2023

sergiou87 previously approved these changes Aug 25, 2023

View reviewed changes

sergiou87 mentioned this pull request Aug 25, 2023

Improve accessibility of dropdown select button #17231

Closed

tidy-dev dismissed sergiou87’s stale review via 66d14ee August 25, 2023 12:07

tidy-dev and others added 5 commits August 25, 2023 08:07

fix copy/pasta docs

66d14ee

Co-authored-by: Sergio Padrino <sergio.padrino@gmail.com>

Update app/src/ui/suggested-actions/dropdown-suggested-action.tsx

d6d1f25

Co-authored-by: Sergio Padrino <sergio.padrino@gmail.com>

Update app/src/ui/suggested-actions/dropdown-suggested-action.tsx

26d7ca2

Co-authored-by: Sergio Padrino <sergio.padrino@gmail.com>

Update app/src/ui/dropdown-select-button.tsx

9280f9d

Co-authored-by: Sergio Padrino <sergio.padrino@gmail.com>

Linter

6cd67e7

tidy-dev enabled auto-merge August 25, 2023 12:25

sergiou87 approved these changes Aug 25, 2023

View reviewed changes

tidy-dev merged commit eb957ba into development Aug 25, 2023
7 checks passed

tidy-dev deleted the see-if-we-can-reuse-the-menu-pane-component branch August 25, 2023 12:44

tidy-dev added the accessibility Issues related to accessibility improvements label Nov 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reuse the menu pane component for dropdown select options #17271

Reuse the menu pane component for dropdown select options #17271

tidy-dev commented Aug 23, 2023 •

edited

Loading

tidy-dev Aug 23, 2023

tidy-dev commented Aug 23, 2023 •

edited

Loading

tidy-dev Aug 23, 2023

sergiou87 left a comment

Reuse the menu pane component for dropdown select options #17271

Reuse the menu pane component for dropdown select options #17271

Conversation

tidy-dev commented Aug 23, 2023 • edited Loading

Description

Screenshots

Release notes

tidy-dev Aug 23, 2023

Choose a reason for hiding this comment

tidy-dev commented Aug 23, 2023 • edited Loading

tidy-dev Aug 23, 2023

Choose a reason for hiding this comment

sergiou87 left a comment

Choose a reason for hiding this comment

tidy-dev commented Aug 23, 2023 •

edited

Loading

tidy-dev commented Aug 23, 2023 •

edited

Loading