Skip to content

Fix: Move the "View Posts..." anchor out of the role=toolbar section in the header - #15693

Merged
gziolo merged 1 commit into
masterfrom
fix/header-toolbar-full-screen
May 20, 2019
Merged

Fix: Move the "View Posts..." anchor out of the role=toolbar section in the header#15693
gziolo merged 1 commit into
masterfrom
fix/header-toolbar-full-screen

Conversation

@gziolo

@gziolo gziolo commented May 17, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Description

Partially fixes #15331.

Screenshot from full report:

image

Keyboard-only users who enter the Editor Top Bar must Tab from button to
button. Although this is manageable when there's a small number of
buttons, the group of buttons is given the role of toolbar, whose
purpose (besides grouping related controls) is to lessen the number of
Tab stops keyboard users must make to navigate past widgets.

This PR follows the recommendation:

Move the "View Posts..." anchor out of the role=toolbar section,
so the toolbar only has <button> children.

Testing

Enabled Fullscreen Mode through More Menu item. Ensure that the header looks like before and View Posts anchor is outside of the toolbar.

Screenshots

Screen Shot 2019-05-17 at 09 56 37
Screen Shot 2019-05-17 at 09 57 03

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.

@gziolo gziolo added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Feedback Needs general design feedback. [a11y] Keyboard & Focus labels May 17, 2019
@gziolo
gziolo requested review from jasmussen and kjellr May 17, 2019 07:58
@gziolo gziolo self-assigned this May 17, 2019
gziolo
gziolo commented May 17, 2019
View reviewed changes
Comment thread packages/edit-post/src/components/header/index.js Outdated
tabIndex="-1"
>
<HeaderToolbar />
<div>

@gziolo gziolo May 17, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@kjellr or @jasmussen - I don't know if this is the best idea to wrap them with div but I took as little steps as possible to make it look close to what we had before. I would love to hear your recommendations because I also noticed that the border is slightly broken now:

Screen Shot 2019-05-17 at 10 02 08

@jasmussen jasmussen May 17, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yep was about to note the same. There's a height difference to the toolbar. Even outside of fullscreen. This branch:

Screenshot 2019-05-17 at 10 58 51

Master:

Screenshot 2019-05-17 at 10 59 47

Let me take a look.

@jasmussen jasmussen May 17, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like if this particular div is assigned display: flex;, and .edit-post-fullscreen-mode-close__toolbar is also assigned display: flex; (instead of inline-block as it is now), both issues are fixed:

Screenshot 2019-05-17 at 11 03 19

Screenshot 2019-05-17 at 11 03 38

I could use a sanity check by Kjell though.

@kjellr kjellr May 17, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like if this particular div is assigned display: flex;, and .edit-post-fullscreen-mode-close__toolbar is also assigned display: flex; (instead of inline-block as it is now), both issues are fixed:

Yep, that fixes it for me too. 👍

@gziolo
gziolo force-pushed the fix/header-toolbar-full-screen branch from 09d4dc9 to 7511aee Compare May 17, 2019 15:01
@gziolo

gziolo commented May 17, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

I applied changes as recommended by @jasmussen and @kjellr, many thanks for your help 🙇

It seems like UI wise everything looks exactly the same now:

Branch:

Screen Shot 2019-05-17 at 16 55 23
Screen Shot 2019-05-17 at 16 55 38

Master:

Screen Shot 2019-05-17 at 17 00 04
Screen Shot 2019-05-17 at 16 59 53

Well, there might be a slight difference in the left margin of the inserter. It's hard to confirm 😅

@jasmussen

jasmussen commented May 17, 2019

Copy link
Copy Markdown
Contributor

Nice work. Be sure to test all mobile breakpoints, and Firefox and Safari. Otherwise, 👍👍

@gziolo

gziolo commented May 17, 2019

Copy link
Copy Markdown
Member Author

It seems to work the same on Firefox, Safar. At mobile breakpoints, there is no Fullscreen Mode so nothing to test there, well I did it, it just works :)

One more check and I think it confirms it works the same as in master.

Branch:

Screen Shot 2019-05-17 at 17 32 18

Master:

Screen Shot 2019-05-17 at 17 45 01

@gziolo gziolo added General Interface Parts of the UI which don't fall neatly under other labels. Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code and removed Needs Design Feedback Needs general design feedback. labels May 17, 2019
kjellr
kjellr approved these changes May 17, 2019
View reviewed changes

@kjellr kjellr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tested in Safari, Chrome, + FF, across a wide range of screen widths. Looks great to me!

@gziolo
gziolo merged commit c5f95a2 into master May 20, 2019
@gziolo
gziolo deleted the fix/header-toolbar-full-screen branch May 20, 2019 06:55
@gziolo gziolo added this to the 5.8 (Gutenberg) milestone May 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasmussen jasmussen Awaiting requested review from jasmussen
@noisysocks noisysocks Awaiting requested review from noisysocks
@talldan talldan Awaiting requested review from talldan
@youknowriad youknowriad Awaiting requested review from youknowriad
1 more reviewer
@kjellr kjellr kjellr approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@gziolo gziolo

Labels

[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). General Interface Parts of the UI which don't fall neatly under other labels. Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code

Projects

None yet

Milestone

5.8 (Gutenberg)

Development

Successfully merging this pull request may close these issues.

Toolbar elements are tabbable

3 participants

@gziolo @jasmussen @kjellr