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.

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

Fix accessibility issues in modal component #1960

Merged
merged 4 commits into from
Jul 7, 2023

Conversation

tuukkao
Copy link
Contributor

@tuukkao tuukkao commented Jul 2, 2023

  • Fix modal aria role
  • Trap focusing with tab / shift+tab inside the modal
  • Restore keyboard focus when closing modal
  • Automatically move keyboard focus to first focusable element unless specified otherwise when opening modal
  • Keyboard shortcut help modal: move keyboard focus to modal title
  • Keyboard shortcut help modal: change close control from link to button

Fixes #1959.

Do you follow the guidelines?

* Fix modal aria role
* Trap focusing with tab / shift+tab inside the modal
* Restore keyboard focus when closing modal
* Automatically move keyboard focus to first focusable element unless specified otherwise
* Keyboard shortcut help modal: move keyboard focus to modal title
* Keyboard shortcut help modal: change close control from link to button
@fguillot
Copy link
Member

fguillot commented Jul 3, 2023

There are some issues detected by the JavaScript linter:

ui/static/js/modal_handler.js: line 56, col 10, Missing semicolon.

@fguillot
Copy link
Member

fguillot commented Jul 4, 2023

It would be nice to keep the same styling as before for the closing button.

Your change with the dark theme:

image

Your change with the light theme:

image

Actual styling:

image

@tuukkao
Copy link
Contributor Author

tuukkao commented Jul 5, 2023

Could you help me out with this a bit? I'm a blind screen reader user and my understanding of CSS is rudimentary at best.

The only style difference between this change and main that I could think of are the a:focus styles which (I believe) used to apply before but are now missing. Is that the issue?

@fguillot
Copy link
Member

fguillot commented Jul 7, 2023

Could you help me out with this a bit? I'm a blind screen reader user and my understanding of CSS is rudimentary at best.

The only style difference between this change and main that I could think of are the a:focus styles which (I believe) used to apply before but are now missing. Is that the issue?

This change should address the styling issue: c7a409c

@fguillot fguillot merged commit f286c3c into miniflux:main Jul 7, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Keyboard shortcut modal lacks modal dialog semantics
2 participants