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

Accessibility: Show open button when the sidebar is closed and tabbingg out of the content #19726

Merged
merged 1 commit into from Feb 4, 2020

Conversation

@gziolo
Copy link
Member

gziolo commented Jan 17, 2020

Description

When the Document/Block sidebar is closed then there is this empty Sidebar region rendered which causes issues when navigating between landmarks. ctrl + backtick gets stuck when it reaches the empty sidebar.

I see 4 possible options:

  1. The simplest approach would be to remove display: none applied which will result in rendering the blue line on the left side (for LTR languages).
  2. Another approach would be to not render the region at all in such case - it also fixes the issue.
  3. I tried an alternative which is similar to what we do for the Publish panel. It renders a button which allows opening the sidebar. It fits perfectly fine with the Edit mode flow where user tabs out of the content. I would need some help from designers if we opt for this one – I tried really hard but I couldn't sort it out fully myself.
  4. It's a mix of (2) and (3) but the button is rendered after the content but inside the region.

This issue was also discussed on the WordPress Slack in #accessibility channel (link requires registration):
https://wordpress.slack.com/archives/C02RP4X03/p1579012942211300

Screenshots

Before

landmarks

After

landmarks

In the case of mobile viewports, this button is not supported. Instead of the sidebar landmark is not displayed if it doesn't have content inside.

landmarks-mobile

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • 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. .
@MarcoZehe

This comment has been minimized.

Copy link
Contributor

MarcoZehe commented Jan 17, 2020

I give my thumbs up for the feature. Since this settings panel can be toggled, it makes sense to have a button in there that opens it when it is closed.
I'll leave the actual code review to one of the others. To me it looks OK, but I don't know enough about the codebase to give a confident real review.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 17, 2020

At a quick glance, this seems consistent with other efforts, and the benefits seem well endorsed, so let's do it. I wonder if we can take this opportunity to polish the styles ever so slightly — I think we can go a long way with margins, paddings and button visuals, to make the buttons that appear clearer even if you're not an experienced screen-reader user.

@enriquesanchez

This comment has been minimized.

Copy link
Contributor

enriquesanchez commented Jan 21, 2020

hey @gziolo!

I think I'm a bit confused and hope you don't mind me asking this if its too obvious, but why do we have two 'Open sidebar panel' buttons one after the other when tabbing through?

2020-01-21 17-04-36 2020-01-21 17_05_59

Alt: The previous GIF shows the editor UI being tabbed through.

I get the first one that appears in the bottom right corner, but I'm not understanding why we show the white empty bar at the right with the same button again.

@gziolo gziolo added this to In progress in WordPress 5.4 Must Have Jan 24, 2020
@gziolo gziolo removed this from In progress in WordPress 5.4 Must Have Jan 24, 2020
…g out of the content
@gziolo gziolo force-pushed the update/landmark-sidebar-button branch from 6440511 to c2e8361 Feb 3, 2020
@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented Feb 3, 2020

I improved the flow based on the feedback received.

I give my thumbs up for the feature. Since this settings panel can be toggled, it makes sense to have a button in there that opens it when it is closed.

This is how it works in the final version.

I think I'm a bit confused and hope you don't mind me asking this if its too obvious, but why do we have two 'Open sidebar panel' buttons one after the other when tabbing through?

We no longer have two of them. It was buggy because changing styles were very challenging, at least for me :)

This is how the actual approach works. I decided to display 2 versions of the button depending on the context. Open document settings when no block is selected or Open block settings when a block or blocks are selected.

landmarks

In the case of mobile viewports, this button is not supported. Instead of the sidebar landmark is not displayed if it doesn't have content inside.

landmarks-mobile

Let me know what do you think. I would appreciate some help with polishing CSS styles. It feels like there should be an easier approach. I don't know, I'm not an expert. 🤷‍♂

@gziolo gziolo marked this pull request as ready for review Feb 3, 2020
@gziolo gziolo requested a review from talldan as a code owner Feb 3, 2020
@gziolo gziolo added this to Needs Review in WordPress 5.4 Must Have Feb 3, 2020
Copy link
Contributor

jasmussen left a comment

From a user experience point of view, this is good. CSS looks good too. This is what I see:

focus

It could still use a sanity check on the JS bits, but 👍 👍 from me. Nice work.

@gziolo gziolo merged commit 2193ce9 into master Feb 4, 2020
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
WordPress 5.4 Must Have automation moved this from Needs Review to Done Feb 4, 2020
@gziolo gziolo deleted the update/landmark-sidebar-button branch Feb 4, 2020
@github-actions github-actions bot added this to the Gutenberg 7.5 milestone Feb 4, 2020
@enriquesanchez

This comment has been minimized.

Copy link
Contributor

enriquesanchez commented Feb 6, 2020

This feels good! 👍 Tested with VoiceOver and had no issues.
Also, I agree with Joen, I don't see any issues with the CSS.

chipsnyder added a commit that referenced this pull request Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.