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

Editor: when Toggle navigation receives state false, focus #37265

Merged
merged 3 commits into from Dec 17, 2021
Merged

Editor: when Toggle navigation receives state false, focus #37265

merged 3 commits into from Dec 17, 2021

Conversation

alexstine
Copy link
Contributor

@alexstine alexstine commented Dec 9, 2021 •
edited

Description

In the Site Editor, Toggle navigation button will have focus placed on the trigger when open state changes to false. This is so when screen reader/keyboard users select an item from the menu, they are dropped back at a global point "Toggle navigation" which is the first element on the page.

Follow up to: #36597

How has this been tested?

I tested in Firefox with NVDA screen reader on Windows 10.

To test:

  1. Activate a block based theme such as TT1 Blocks or Twenty-Twenty Two.
  2. Open the Site Editor. Appearance > Editor. (Beta)
  3. Select Toggle navigation.
  4. Select one of the navigation items.
  5. Notice how the sidebar closes and focus is placed back on Toggle navigation button.

Screenshots

Types of changes

Bug fix.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@alexstine alexstine self-assigned this Dec 9, 2021
@alexstine alexstine added [a11y] Keyboard & Focus [Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") [Package] Edit Site /packages/edit-site [Type] Enhancement A suggestion for improvement. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). labels Dec 9, 2021
@alexstine alexstine mentioned this pull request Dec 9, 2021
Closed
38 tasks
@noisysocks noisysocks added [Type] Bug An existing feature does not function as intended and removed [Type] Enhancement A suggestion for improvement. labels Dec 10, 2021
@noisysocks noisysocks added this to 👀 Needs review in WordPress 5.9 Must-Haves via automation Dec 13, 2021
@noisysocks noisysocks added the Backport to WP Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Dec 13, 2021
@kevin940726 kevin940726 mentioned this pull request Dec 13, 2021
Closed
7 tasks
kevin940726
kevin940726 reviewed Dec 13, 2021
View reviewed changes
packages/edit-site/src/components/navigation-sidebar/navigation-toggle/index.js Outdated
Comment on lines 46 to 51
useEffect( () => {
// Focus the trigger button if close
if ( ! isNavigationOpen ) {
navigationToggleRef.current.focus();
}
}, [ isNavigationOpen ] );
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for making this PR! However, I don't think this is the right solution. We don't want to focus the button whenever it gets closed, we only want to focus it whenever route changes. This could have some unwanted side-effects (for instance, if we want to close this one to open another sidebar, the focus could be moved to this button). I opened #37314 for an alternative approach which I believe could be more robust, but also requires a lot of changes 😅 .

@alexstine
Copy link
Contributor Author

@kevin940726 Think you'll be able to land your PR for 5.9 beta? Concept seems simple enough but that is a ton of changed files. I know my solution is subjective at best but do you think it's worth keeping this open in case your PR doesn't make beta in time? One way or another, this issue with focus has to be managed. Your way sounds a lot more full proof though so if that lands, let's go for it.

@kevin940726
Copy link
Member

@alexstine I 100% agree. It's essentially what I wrote in the PR description too!

kevin940726
kevin940726 approved these changes Dec 14, 2021
View reviewed changes
Copy link
Member

@kevin940726 kevin940726 left a comment

Choose a reason for hiding this comment

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

I think we can go with this approach for 5.9, and slowly move to #37314 once it has received thorough tests.

We should probably add a TODO comment in this PR though and point to the relevant discussion (maybe just point to #37314).

WDYT?

@alexstine
Copy link
Contributor Author

@kevin940726 Sounds good.

@kevin940726
Copy link
Member

There's a conflict in the file, let's rebase and fix it. And maybe add the // TODO comment :)

@alexstine @Mamaduka
Add comment that points to a future PR
26ae22f 
Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>
@alexstine
Copy link
Contributor Author

@kevin940726 Committed @Mamaduka changes. Hopefully this should be good to go now. 👍 Thanks to you both for working on this.

@tellthemachines tellthemachines merged commit 8b2b37e into WordPress:trunk Dec 17, 2021
WordPress 5.9 Must-Haves automation moved this from 👀 Needs review to ✅ Done Dec 17, 2021
@github-actions github-actions bot added this to the Gutenberg 12.3 milestone Dec 17, 2021
@tellthemachines tellthemachines removed the Backport to WP Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Dec 21, 2021
tellthemachines pushed a commit that referenced this pull request Dec 21, 2021
@alexstine @Mamaduka @tellthemachines
Editor: when Toggle navigation receives state false, focus (#37265)
0e76b33 
* Site Editor: when Toggle navigation receives state false, focus the trigger for screen readers.

* Add comment that points to a future PR

Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>

Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>
@jsnajdr jsnajdr mentioned this pull request Nov 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mamaduka Mamaduka Mamaduka left review comments

@kevin940726 kevin940726 kevin940726 approved these changes

@noisysocks noisysocks Awaiting requested review from noisysocks

@youknowriad youknowriad Awaiting requested review from youknowriad

@talldan talldan Awaiting requested review from talldan

Assignees

@alexstine alexstine

Labels
[Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Edit Site /packages/edit-site [Type] Bug An existing feature does not function as intended
Projects
No open projects
WordPress 5.9 Must-Haves
✅ Done
Milestone
Gutenberg 12.3
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@alexstine @kevin940726 @Mamaduka @noisysocks @tellthemachines