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

Desktop: Improve focus handling for notebook edit, share, and sync dialogs #10779

Conversation

personalizedrefrigerator
Copy link
Collaborator

Summary

This pull request switches many dialogs to the HTML <dialog> element, which:

  • Auto-focuses the first focusable item in the dialog (this can be customized by adding the autofocus attribute to a different element).
  • Traps focus in the dialog. This means that repeatedly pressing Tab no longer causes focus to leave the dialog.
    • This is helpful when using a screen reader.
  • Marks the dialog as a dialog for screen readers.
  • Makes the dialog modal, showing it above other elements.

Testing plan

  1. Right-click on a notebook and select "edit".
  2. Click "select emoji" and choose an Emoji using the Emoji picker.
  3. Hold tab, verify that the focus cycles through the dialog without leaving it.
  4. Close the dialog by clicking "OK".
  5. Open the share dialog for the current notebook.
  6. Share the notebook with another user.
  7. Close the dialog.
  8. Open the publish dialog for the current note.
  9. Close the dialog by pressing esc.
  10. Enable the web clipper.
  11. Grant access to the web clipper by pressing Enter.
    • Similar issue: Enter always accepts the same button and closes the dialog, regardless of keyboard focus (unless in a textarea). (Code that causes the issue).

This has been tested successfully on Ubuntu 24.04.

…zard dialogs

This commit switches many dialogs to the semantic `<dialog>` element,
which:
- Traps focus in the dialog. This means that repeatedly pressing `Tab`
  no longer causes focus to leave the dialog.
  - This is useful when using a screen reader.
- Autofocuses the first focusable item in the dialog (can be customized
  with the `autofocus` attribute).
- Marks the dialog as a dialog for screen readers.
Comment on lines +1 to +2

@use './dialog-modal-layer.scss';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

RSCSS specifies that one top-level rule should be in each file. I've created a new styles/ folder to satisfy this rule. Note, however, that a style/ folder already exists, and contains styled component logic. As such, it might make sense to move styles/dialog-modal-layer.scss to a different location.

Additionally, RSCSS states that glob matching should be used. However, Dart SCSS does not seem to support globs in @use or @import.

@personalizedrefrigerator personalizedrefrigerator force-pushed the pr/desktop/improve-dialog-accessibility branch 2 times, most recently from 6238498 to 0df63ff Compare July 24, 2024 00:48
@laurent22 laurent22 merged commit dd5240d into laurent22:dev Jul 26, 2024
10 checks passed
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.

2 participants