feat(web): keyboard accessible context menus #10017

ben-basten · 2024-06-07T03:19:13Z

Description

Enable arrow key navigation of dropdown menus. Accessibility features are based on this WCAG "actions menu" pattern:

https://www.w3.org/WAI/ARIA/apg/patterns/menu-button/examples/menu-button-actions-active-descendant/

This also simplifies implementing a ContextMenu dropdown, because the implementing component no longer needs to keep track of the dropdown visibility state or position. All of that logic is handled internally within the component.

Also in this change:

rename AssetSelectContextMenu to ButtonContextMenu, to make the naming more accurately reflect what it does
albums list dropdown menu now uses the i18n translations
darken the active option color in the context menu to make it easier to see
context menu is scrollable on short screen sizes
maintain context menu position on screen resize
the right click context menu on the albums list page supports arrow key navigation

How Has This Been Tested?

VoiceOver screen reader testing
- works with Firefox, Chrome, and Safari on MacOS
- example test scenarios: navigating with arrow keys, hovering over items with a mouse, clicking options with enter/spacebar/mouse, clicking away to dismiss, using the tab key to dismiss, using the escape key to dismiss
NVDA testing on Windows
- I do not have a Windows environment, so if anyone has the tools to try this it would be very helpful :)

Verified that options in affected context menus behave as expected:

Screenshots

Example dropdown menu, with the darker selected color.

Expand for screenshot

…/accessible-menu-v2

- configurable title - focus management: tab keys, clicks, closing the menu - automatically closing when an option is selected

…/accessible-menu-v2

- adjust the aria attributes - intuitive escape key propagation - extract context into its own file

…/accessible-menu-v2

- export selectedColor to prevent unexpected styling - better context function naming

web/src/lib/actions/list-navigation.ts

jrasm91

It is a lot of changes. It seems like the list enhancements could have been separated out into a different change set.

web/src/lib/components/elements/buttons/circle-icon-button.svelte

jrasm91 · 2024-06-13T12:31:03Z

web/src/lib/components/elements/buttons/circle-icon-button.svelte

+  export let ariaHasPopup: boolean | undefined = undefined;
+  export let ariaExpanded: boolean | undefined = undefined;
+  export let ariaControls: string | undefined = undefined;


I think there is an export HTML*Type field you can use to get the actual type for these properties.

In my opinion, I find the syntax for automatically adding the HTML props a little confusing/cumbersome, especially because this CircleIconButton component has so many props. Here's a similar example:

immich/web/src/lib/components/shared-components/immich-logo.svelte

Lines 13 to 19 in 08426e1

interface $$Props extends HTMLImgAttributes {

noText?: boolean;

draggable?: boolean;

}

export let noText = false;

export let draggable = false;

I'm wondering if this would be an easier change to do once we have the new Svelte 5 prop typing? Happy to add the automatic HTML props now if that's preferred.

https://svelte-5-preview.vercel.app/docs/runes#$props

web/src/lib/components/faces-page/people-card.svelte

web/src/lib/components/photos-page/actions/asset-job-actions.svelte

web/src/lib/actions/list-navigation.ts

ben-basten · 2024-06-13T21:19:59Z

It is a lot of changes. It seems like the list enhancements could have been separated out into a different change set.

Thank you for the time and feedback, @jrasm91!

I agree, the list changes expanded the scope of the PR. I'll revert those and put them into their own Svelte action, which I think would still be helpful to prevent duplicated code between the ButtonContextMenu and RightClickContextMenu.

…/accessible-menu-v2

…/accessible-menu-v2, fix merge conflicts

- better prop naming for ButtonContextMenu

- also: minor cleanup of the context-menu-navigation Svelte action

…/accessible-menu-v2

ben-basten · 2024-06-17T15:20:06Z

This is ready for review! I've responded to open feedback and closed the remaining tasks on my list.

…/accessible-menu-v2

…ble-menu-v2

alextran1502

Good work! Thanks Ben!

waclaw66 · 2024-06-25T12:25:17Z

@ben-basten this PR introduces a bug that makes three dot button still visible after context menu is closed.

2024-06-25.14.22.24.mp4

ben-basten · 2024-06-25T13:33:22Z

@waclaw66 This is working as designed - focus is on the 3 dot button after the context menu closes, so it stays visible.

New in this PR is that focus returns to the button that triggered the context menu when it is closed. This allows keyboard users to regain their previous focus position after interacting with a context menu. The same behavior occurs when a mouse user clicks away from the menu.

Test scenario to check this:

Click the 3 dots to open the album context menu
Click away to close the menu
Press the enter key or spacebar. The context menu should open again, because the button has focus.

I thought this made sense since buttons/links on the screen aren't interactive when clicking away from the context menu. An option we have is to consider disabling this refocus behavior for clicking away.

waclaw66 · 2024-06-25T16:33:49Z

@ben-basten That make sense, thanks for detailed explanation as usual.

waclaw66 · 2024-06-25T16:42:09Z

I thought this made sense since buttons/links on the screen aren't interactive when clicking away from the context menu. An option we have is to consider disabling this refocus behavior for clicking away.

Loosing focus and forwarding click event should be consolidated though. Some context menus (asset viewer, face) allow to click outside and forward click event to the target control (e.g. left panel), album menu doesn't allow that. Please check a compare the behaviour.

ben-basten · 2024-06-25T21:01:12Z

Loosing focus and forwarding click event should be consolidated though. Some context menus (asset viewer, face) allow to click outside and forward click event to the target control (e.g. left panel), album menu doesn't allow that. Please check a compare the behaviour.

Yeah, the albums page is unique because it has a context menu that appears when you right click on the album cover. When I right click on a website on my Mac, the default MacOS context menu doesn't let me click anything behind it until the context menu is dismissed. So, it makes sense to imitate that behavior with our custom right click context menu.

However, this behavior doesn't make as much sense when the 3 dots button on the top right of the album cover is clicked, because we already have an established pattern for how that should work everywhere else.

The fix here would be to move the 3 dot menu from using the RightClickContextMenu component to the ButtonContextMenu component. I determined this change to be out of scope in this PR because this is how it was previously built, but I agree that it would be a better experience for that button to have the same behavior as other menus. For example, a benefit of migrating would include automatically selecting the first item in the menu when it is opened using the enter key or space bar.

Feel free to pitch out a PR for this, I'm not sure when I'll have time to make this change yet. This can certainly be improved, thanks for calling it out!

ben-basten added 7 commits June 2, 2024 14:09

feat(web,a11y): context menu keyboard navigation

f11f579

Merge branch 'main' of https://github.com/immich-app/immich into feat…

124e401

…/accessible-menu-v2

wip: all context menus visible

081a82a

Merge branch 'main' of https://github.com/immich-app/immich into feat…

059f923

…/accessible-menu-v2

wip: more migrations to the ButtonContextMenu, usability improvements

8b9a1e5

Merge branch 'main' of https://github.com/immich-app/immich into feat…

5c9d06c

…/accessible-menu-v2

wip: migrate Administration, PeopleCard

7c30a19

ben-basten added 🖥️web accessibility labels Jun 7, 2024

ben-basten changed the title ~~feat(web,a11y): keyboard accessible dropdown menus~~ feat(web): keyboard accessible dropdown menus Jun 7, 2024

wip: refocus the button on click, docs

276a446

ben-basten force-pushed the feat/accessible-menu-v2 branch from 14bd691 to 276a446 Compare June 9, 2024 22:49

ben-basten added 2 commits June 9, 2024 18:49

Merge branch 'main' of https://github.com/immich-app/immich into feat…

675110c

…/accessible-menu-v2

fix: more intuitive RightClickContextMenu

a755acc

- configurable title - focus management: tab keys, clicks, closing the menu - automatically closing when an option is selected

ben-basten force-pushed the feat/accessible-menu-v2 branch from 7365b45 to a755acc Compare June 10, 2024 20:32

Merge branch 'main' of https://github.com/immich-app/immich into feat…

84fdc9e

…/accessible-menu-v2

ben-basten force-pushed the feat/accessible-menu-v2 branch from e22868e to 13a4598 Compare June 11, 2024 23:50

fix: refining the little details

04b06d2

- adjust the aria attributes - intuitive escape key propagation - extract context into its own file

ben-basten force-pushed the feat/accessible-menu-v2 branch from 13a4598 to 04b06d2 Compare June 12, 2024 01:42

ben-basten added 2 commits June 12, 2024 09:08

fix: dropdown options not clickable in a <Portal>

57fb25d

Merge branch 'main' of https://github.com/immich-app/immich into feat…

29d5f3e

…/accessible-menu-v2

ben-basten force-pushed the feat/accessible-menu-v2 branch from 2bc383a to 998376e Compare June 12, 2024 21:25

wip: small fixes

ac51083

- export selectedColor to prevent unexpected styling - better context function naming

ben-basten force-pushed the feat/accessible-menu-v2 branch from 998376e to ac51083 Compare June 12, 2024 21:40

ben-basten commented Jun 12, 2024

View reviewed changes

web/src/lib/actions/list-navigation.ts Outdated Show resolved Hide resolved

bo0tzz requested review from jrasm91 and alextran1502 June 13, 2024 07:52

jrasm91 reviewed Jun 13, 2024

View reviewed changes

Merge branch 'main' of https://github.com/immich-app/immich into feat…

6ef0d7b

…/accessible-menu-v2

Merge branch 'main' of https://github.com/immich-app/immich into feat…

c93a7aa

…/accessible-menu-v2, fix merge conflicts

ben-basten force-pushed the feat/accessible-menu-v2 branch from c44ea24 to ca527af Compare June 15, 2024 14:10

fix: use Svelte store instead to handle selecting menu options

bbf8ab5

- better prop naming for ButtonContextMenu

ben-basten force-pushed the feat/accessible-menu-v2 branch from ca527af to bbf8ab5 Compare June 15, 2024 16:19

feat: hovering the mouse can change the active element

08426e1

ben-basten changed the title ~~feat(web): keyboard accessible dropdown menus~~ feat(web): keyboard accessible context menus Jun 15, 2024

fix: remove Portal, more predictable open/close behavior

03d1263

ben-basten force-pushed the feat/accessible-menu-v2 branch from 319ace8 to 03d1263 Compare June 16, 2024 00:58

feat: make selected item visible using a scroll

fbc373a

- also: minor cleanup of the context-menu-navigation Svelte action

ben-basten force-pushed the feat/accessible-menu-v2 branch from 1404a77 to fbc373a Compare June 16, 2024 13:01

ben-basten added 5 commits June 16, 2024 09:13

feat: maintain context menu position on resize

ede2f0d

Merge branch 'main' of https://github.com/immich-app/immich into feat…

057b3df

…/accessible-menu-v2

fix: use the whole padding class as better tailwind convention

5da00aa

fix: options not announcing with screen reader for ButtonContextMenu

749f0be

fix: screen reader announcing right click context menu options

76ae57a

ben-basten force-pushed the feat/accessible-menu-v2 branch from ad9cfad to 76ae57a Compare June 16, 2024 16:10

ben-basten marked this pull request as ready for review June 16, 2024 16:13

fix: handle focus out scenario

3b70ae5

Merge branch 'main' of https://github.com/immich-app/immich into feat…

96da36a

…/accessible-menu-v2

alextran1502 self-assigned this Jun 17, 2024

Merge branch 'main' of github.com:immich-app/immich into feat/accessi…

1f7f457

…ble-menu-v2

alextran1502 approved these changes Jun 18, 2024

View reviewed changes

alextran1502 merged commit b71aa44 into main Jun 18, 2024
22 checks passed

alextran1502 deleted the feat/accessible-menu-v2 branch June 18, 2024 03:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(web): keyboard accessible context menus #10017

feat(web): keyboard accessible context menus #10017

ben-basten commented Jun 7, 2024 •

edited

Loading

jrasm91 left a comment

jrasm91 Jun 13, 2024

ben-basten Jun 15, 2024

ben-basten commented Jun 13, 2024

ben-basten commented Jun 17, 2024

alextran1502 left a comment

waclaw66 commented Jun 25, 2024 •

edited

Loading

ben-basten commented Jun 25, 2024

waclaw66 commented Jun 25, 2024 •

edited

Loading

waclaw66 commented Jun 25, 2024

ben-basten commented Jun 25, 2024

	interface $$Props extends HTMLImgAttributes {
	noText?: boolean;
	draggable?: boolean;
	}

	export let noText = false;
	export let draggable = false;

feat(web): keyboard accessible context menus #10017

feat(web): keyboard accessible context menus #10017

Conversation

ben-basten commented Jun 7, 2024 • edited Loading

Description

How Has This Been Tested?

Screenshots

jrasm91 left a comment

Choose a reason for hiding this comment

jrasm91 Jun 13, 2024

Choose a reason for hiding this comment

ben-basten Jun 15, 2024

Choose a reason for hiding this comment

ben-basten commented Jun 13, 2024

ben-basten commented Jun 17, 2024

alextran1502 left a comment

Choose a reason for hiding this comment

waclaw66 commented Jun 25, 2024 • edited Loading

ben-basten commented Jun 25, 2024

waclaw66 commented Jun 25, 2024 • edited Loading

waclaw66 commented Jun 25, 2024

ben-basten commented Jun 25, 2024

ben-basten commented Jun 7, 2024 •

edited

Loading

waclaw66 commented Jun 25, 2024 •

edited

Loading

waclaw66 commented Jun 25, 2024 •

edited

Loading