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] Add focus traps to modals and nav drawer #3018

Merged
merged 19 commits into from
Nov 21, 2021
Merged

[A11Y] Add focus traps to modals and nav drawer #3018

merged 19 commits into from
Nov 21, 2021

Conversation

davwheat
Copy link
Member

@davwheat davwheat commented Aug 12, 2021 •
edited
Loading

Fixes #2663
Fixes #2665

Please merge #3007 before this PR!

Changes proposed in this pull request:

  • Add focus trap to modals
  • Add focus trap to drawer on phone viewports
  • Auto close drawer when leaving phone viewport
  • Replace deprecated JQuer ($.click() -> $.on('click'))
  • Use requestAnimationFrame instead of setTimeout

Reviewers should focus on:

Screenshot

Confirmed

  • Frontend changes: tested on a local Flarum installation.

@davwheat davwheat added the type/accessibility Issues relating to accessibility (keyboard navigation, screenreaders, text contrast, etc.) label Aug 12, 2021
@davwheat davwheat self-assigned this Aug 12, 2021
@davwheat davwheat added this to the 1.1 milestone Aug 12, 2021
askvortsov1
askvortsov1 requested changes Aug 14, 2021
View reviewed changes
js/package.json Outdated Show resolved Hide resolved
js/src/common/components/ModalManager.js Outdated Show resolved Hide resolved
js/src/common/components/ModalManager.js Outdated Show resolved Hide resolved
js/src/common/components/ModalManager.js Outdated Show resolved Hide resolved
js/src/common/components/ModalManager.js Outdated Show resolved Hide resolved
js/src/common/utils/Drawer.js Outdated Show resolved Hide resolved
js/src/common/utils/Drawer.js Outdated Show resolved Hide resolved
js/src/common/utils/Drawer.js Outdated Show resolved Hide resolved
js/src/common/utils/Drawer.js Outdated Show resolved Hide resolved
js/src/common/utils/Drawer.js Outdated Show resolved Hide resolved
dsevillamartin
dsevillamartin approved these changes Sep 10, 2021
View reviewed changes
Copy link
Member

@dsevillamartin dsevillamartin left a comment

Choose a reason for hiding this comment

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

Not tested locally, but code looks good aside from a few comments.

js/src/common/utils/Drawer.js Outdated Show resolved Hide resolved
js/src/common/components/ModalManager.js Outdated Show resolved Hide resolved
js/src/forum/utils/KeyboardNavigatable.ts Show resolved Hide resolved
dsevillamartin
dsevillamartin requested changes Sep 10, 2021
View reviewed changes
Copy link
Member

@dsevillamartin dsevillamartin left a comment

Choose a reason for hiding this comment

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

I didn't mean to approve.

@davwheat davwheat force-pushed the dw/2663-2665-add-focus-trap branch 3 times, most recently from 2790899 to 4f19eab Compare September 10, 2021 22:05
@davwheat
Copy link
Member Author

YOU WILL GET CO-AUTHORSHIP OF THAT COMMIT IF IT IS THE LAST THING I DO

@askvortsov1 askvortsov1 modified the milestones: 1.1, 1.2 Sep 28, 2021
@davwheat
Copy link
Member Author

Rebase in progress...

@datitisev you still have "requested changes" marked on this PR -- I think I addressed your comment about a breaking change, but I'll re-request your review and you can either approve, dismiss the request, or request other changes.

@davwheat davwheat requested review from dsevillamartin and removed request for dsevillamartin November 19, 2021 13:19
@dsevillamartin dsevillamartin removed their request for review November 21, 2021 19:34
@davwheat davwheat merged commit eaf1b86 into master Nov 21, 2021
@davwheat davwheat deleted the dw/2663-2665-add-focus-trap branch November 21, 2021 19:44
@SychO9 SychO9 removed this from the 1.2 milestone Dec 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SychO9 SychO9 SychO9 approved these changes

@askvortsov1 askvortsov1 askvortsov1 approved these changes

@dsevillamartin dsevillamartin dsevillamartin approved these changes

Assignees

@davwheat davwheat

Labels
type/accessibility Issues relating to accessibility (keyboard navigation, screenreaders, text contrast, etc.)
Projects
None yet
Milestone
No milestone
4 participants
@davwheat @dsevillamartin @SychO9 @askvortsov1