Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

A11y popup menu #874

Merged
merged 9 commits into from
Mar 22, 2024
Merged

A11y popup menu #874

merged 9 commits into from
Mar 22, 2024

Conversation

nikku
Copy link
Member

@nikku nikku commented Mar 21, 2024 •
edited
Loading

Firefox

capture 5fPyRh_optimized

Chrome

capture 5w5PjG_optimized

This PR improves the accessibility of the popup menu:

  • Ensures all items are keyboard accessible
  • Ensures that existing TAB and arrow key navigation continues to work
  • Ensures that SPACE and ENTER can be used as expected to toggle features
  • Ensure that outside click is no longer trapped
  • Use browser native outline for keyboard focus state

Additional notes:

  • I decided that tabbing through the whole component (all entries) shall be possible
  • I decided against making this a partial select/fake drop down from aria perspective, as it does not work in scenarios without search

Can be tried out using the following command on your local machine:

npx @bpmn-io/sr bpmn-io/bpmn-js -l bpmn-io/diagram-js#a11y-popup-menu

Closes #871

nikku added 6 commits March 20, 2024 13:07
@nikku
chore: remove unused styles
8cf8544
@nikku
fix(popup-menu): make header entries keyboard accessible
32c9828
@nikku
fix(popup-menu): make entries keyboard accessible
17d1fba
@nikku
fix(popup-menu): do not prevent click outside of popup menu
64b5152 
  Ensures we don't capture a canvas click via the backdrop,
  but instead register click outside as an external (global)
  handler.

chore(popup-menu): remove backdrop

  * Drop backdrop
  * Move exit and focus handling to PopupMenuWrapper
@nikku
fix(popup-menu): do not trap TAB
bb9340e
@nikku
chore(popup-menu): remove unused dependency
bcafad0
@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Mar 21, 2024
@nikku
Copy link
Member Author

nikku commented Mar 21, 2024

To be considered: Do not browser focus existing element, but only do so on tab navigation:

capture r7pQV7_optimized

@nikku nikku force-pushed the a11y-popup-menu branch from e391076 to f183011 Compare March 21, 2024 11:03
@nikku
Copy link
Member Author

nikku commented Mar 21, 2024

As per discussion discarded the automatic keyboard focus:

  • Ensure that resonable item is intially focused

capture 4dT3A2_optimized

nikku added 2 commits March 21, 2024 14:26
@nikku
fix(keyboard): do not block browser default behavior on buttons
cbc8361 
Ensure we do not react to `SPACE` and `ENTER` on buttons to shadow stock keyboard interaction / block browser default behavior.
@nikku
fix(popup-menu): use semantic button for actionable header entries
24e421e
@nikku nikku force-pushed the a11y-popup-menu branch from f183011 to 3daa536 Compare March 21, 2024 22:08
@nikku
fix(popup-menu): prevent outline cut-off
a574bf0 
Add a marginal margin to ensure outline is properly displayed.
@nikku nikku force-pushed the a11y-popup-menu branch from 3daa536 to a574bf0 Compare March 22, 2024 07:20
@nikku nikku marked this pull request as ready for review March 22, 2024 08:05
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Mar 22, 2024
@nikku
Copy link
Member Author

nikku commented Mar 22, 2024

Updated with latest screenshots, preventing outline cut off via a574bf0. Now ready for review.

@nikku nikku requested a review from barmac March 22, 2024 08:13
@barmac
Copy link
Member

barmac commented Mar 22, 2024

It looks good on Chrome, but there are some problems on Safari:

Screen.Recording.2024-03-22.at.09.18.56.mov
  • The outline is cut-off.
  • I cannot focus headers buttons with keyboard.
  • The menu is blurred when zoomed in o.O (out of scope).

@barmac
Copy link
Member

barmac commented Mar 22, 2024

This is Safari 17.0

@barmac
Copy link
Member

barmac commented Mar 22, 2024

On Firefox, the outline is also cut off, but at least I can focus the header buttons, and the menu is never blurred:

image

@nikku
Copy link
Member Author

nikku commented Mar 22, 2024

So we have to decide if we invent a completely new outline, standardized across all browsers.

I'd keep that as a follow-up activity.

@barmac
Copy link
Member

barmac commented Mar 22, 2024

I'm OK with outline cut-off but it would be great if the focus worked correctly. I will check in the dev tools what is happening there.

@barmac
Copy link
Member

barmac commented Mar 22, 2024

So via TAB I cannot reach the header modules, and after I TAB out, it focusses the body.

barmac
barmac reviewed Mar 22, 2024
View reviewed changes
lib/features/popup-menu/PopupMenuItem.js
Comment on lines +36 to +43
tabIndex="0"
onClick=${ onAction }
onFocus=${ onMouseEnter }
onBlur=${ onMouseLeave }
onMouseEnter=${ onMouseEnter }
onMouseLeave=${ onMouseLeave }
onDragStart=${ (event) => onAction(event, entry, 'dragstart') }
aria-role="button"
Copy link
Member

Choose a reason for hiding this comment

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

If the items serve as buttons now, why don't we make them actual buttons? Spans make sense if we want to keep the select-option-like behavior but we decided against this as I understand.

barmac
barmac reviewed Mar 22, 2024
View reviewed changes
lib/features/popup-menu/PopupMenuComponent.js

const inputEl = popupEl.querySelector('input');

(inputEl || popupEl).focus();

This comment was marked as resolved.

Sign in to view

Copy link
Member Author

Choose a reason for hiding this comment

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

autofocus is unfortunately regarded as an anti-pattern.

@barmac
Copy link
Member

barmac commented Mar 22, 2024

OK it might be that I don't know my tool. I can tab to the header buttons via Option+TAB: https://stackoverflow.com/questions/1848390/safari-ignoring-tabindex

barmac
barmac approved these changes Mar 22, 2024
View reviewed changes
Copy link
Member

@barmac barmac left a comment

Choose a reason for hiding this comment

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

LGTM

@nikku nikku merged commit aaaeeff into develop Mar 22, 2024
10 of 12 checks passed
@nikku nikku deleted the a11y-popup-menu branch March 22, 2024 09:24
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Mar 22, 2024
@nikku nikku mentioned this pull request Mar 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@barmac barmac barmac approved these changes

Assignees

@nikku nikku

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make popup menu WCGA 2.1 compliant
2 participants
@nikku @barmac