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

Movers: Add bigger visible focus rectangle. #23760

Merged
merged 2 commits into from
Aug 10, 2020

Conversation

jasmussen
Copy link
Contributor

This PR makes the focus rectangle the same size as the button itself. This is a purely visual change.

This addresses feedback from #21935 (comment).

Before:

focus-before

After:

focus-after

focus-after-hoz

@jasmussen jasmussen added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Jul 7, 2020
@jasmussen jasmussen self-assigned this Jul 7, 2020
@jasmussen jasmussen mentioned this pull request Jul 7, 2020
@github-actions
Copy link

github-actions bot commented Jul 7, 2020

Size Change: +14 B (0%)

Total Size: 1.15 MB

Filename Size Change
build/block-editor/style-rtl.css 10.6 kB +8 B (0%)
build/block-editor/style.css 10.6 kB +6 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.67 kB 0 B
build/api-fetch/index.js 3.44 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 7.97 kB 0 B
build/block-directory/style-rtl.css 953 B 0 B
build/block-directory/style.css 952 B 0 B
build/block-editor/index.js 125 kB 0 B
build/block-library/editor-rtl.css 7.59 kB 0 B
build/block-library/editor.css 7.59 kB 0 B
build/block-library/index.js 132 kB 0 B
build/block-library/style-rtl.css 7.76 kB 0 B
build/block-library/style.css 7.77 kB 0 B
build/block-library/theme-rtl.css 729 B 0 B
build/block-library/theme.css 730 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.2 kB 0 B
build/components/index.js 200 kB 0 B
build/components/style-rtl.css 15.7 kB 0 B
build/components/style.css 15.7 kB 0 B
build/compose/index.js 9.68 kB 0 B
build/core-data/index.js 11.8 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.45 kB 0 B
build/date/index.js 5.38 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.23 kB 0 B
build/edit-navigation/index.js 10.9 kB 0 B
build/edit-navigation/style-rtl.css 1.08 kB 0 B
build/edit-navigation/style.css 1.08 kB 0 B
build/edit-post/index.js 304 kB 0 B
build/edit-post/style-rtl.css 5.59 kB 0 B
build/edit-post/style.css 5.59 kB 0 B
build/edit-site/index.js 17 kB 0 B
build/edit-site/style-rtl.css 3.06 kB 0 B
build/edit-site/style.css 3.06 kB 0 B
build/edit-widgets/index.js 9.38 kB 0 B
build/edit-widgets/style-rtl.css 2.45 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 537 B 0 B
build/editor/editor-styles.css 539 B 0 B
build/editor/index.js 45.2 kB 0 B
build/editor/style-rtl.css 3.8 kB 0 B
build/editor/style.css 3.79 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.72 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.11 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.33 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.41 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 13.9 kB 0 B
build/server-side-render/index.js 2.71 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@talldan
Copy link
Contributor

talldan commented Jul 8, 2020

@jasmussen I haven't tested, but this seems like it might partially address an issue I noticed in #23667.

@ZebulanStanphill
Copy link
Member

Hmmm... I know this is more accurate to the actual clickable area, but it's also a lot uglier. It's also inconsistent with all the other toolbar buttons. I'm thinking perhaps we should just bite the bullet and do this:

image

That would both make the clickable areas larger while also solving the issue of users thinking that the chevrons are part of the same button as the block icon. If we then increased the focus rectangles in that design, it wouldn't look as bad (though it would still be inconsistent).

@ZebulanStanphill
Copy link
Member

Also, technically the movers should be separated from the switcher anyway since they're technically in separate toolbar groups in the actual markup. Toolbar groups are supposed to be separated with a dividing line; that's how it works in the rest of the block toolbar.

@jasmussen
Copy link
Contributor Author

Hmmm... I know this is more accurate to the actual clickable area, but it's also a lot uglier.

Yep, very much agreed. But it's often the first reaction when reviewing these, judging the hit area by the focus rectangle moreso than trying them. This would help with that.

It's also inconsistent with all the other toolbar buttons. I'm thinking perhaps we should just bite the bullet and do this:

I think a version of that may be the next thing to try, yes, at the very least an iteration of the design towards that. I would love for us to consider the borders a little bit, because it very quickly gets heavy — I don't have a solution, but I know there is one if we look for it.

@afercia
Copy link
Contributor

afercia commented Jul 14, 2020

Thanks for opening this PR. We (accessibility team) really hope this is solved soon and cherry picked for 5.5 as it's part of a broader issue that in our opinion is an accessibility regression in 5.5 as reported on #21935.

Id say that at least the border to separate the movers should go into WordPress 5.5. @jasmussen any news?

@ZebulanStanphill
Copy link
Member

@afercia

Id say that at least the border to separate the movers should go into WordPress 5.5.

I've created #23971 to make this change. It is now ready for review. 🙂

@jasmussen
Copy link
Contributor Author

@ZebulanStanphill should I move forward with rebasing and refreshing this one, or would you like to take it from here?

@ZebulanStanphill
Copy link
Member

I think it all kind of depends on what happens with #23971. (Which I'm about to rebase.) A decision needs to be made on whether or not we go that direction or not. The accessibility team seems to really want it (and would have liked to see it in WP 5.5, though it's most likely too late for that), but I don't think I have the authority to merge it yet without design team approval.

If that PR is merged, then this PR becomes less necessary, but it would also not look as awkward anymore.

@joedolson
Copy link
Contributor

The accessibility team would definitely like to see this happen in WP 5.5; it would help a lot in mitigating some of the challenges with this part of the control.

I do still think that increasing the visible focus rectangle on this is helpful - I have personally found that with the current design, I routinely click (using a mouse) on the wrong arrow when trying to move up, and end up moving blocks down instead of up.

@jasmussen jasmussen force-pushed the update/bigger-mover-focus branch from 60a13ce to f951e6a Compare August 5, 2020 07:08
@jasmussen
Copy link
Contributor Author

Thanks Joe. There was a bug with some overlapping that you may have encountered (fixed in #24349). I'll refresh this and try to address Zeb's feedback and wrap this one.

@jasmussen
Copy link
Contributor Author

I refreshed the PR, and made the focus style sit inside the frame, balancing it a bit:

Screenshot 2020-08-05 at 09 25 36

@jasmussen jasmussen force-pushed the update/bigger-mover-focus branch from a3eeef5 to 2221bfe Compare August 7, 2020 14:25
@jasmussen
Copy link
Contributor Author

Refreshed this one:

Screenshot 2020-08-07 at 16 25 36

Joen A and others added 2 commits August 10, 2020 08:30
This PR makes the focus rectangle the same size as the button itself.
@jasmussen jasmussen force-pushed the update/bigger-mover-focus branch from 2221bfe to b5b7924 Compare August 10, 2020 06:36
@youknowriad youknowriad added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Aug 10, 2020
@youknowriad youknowriad merged commit fd54e47 into master Aug 10, 2020
@youknowriad youknowriad deleted the update/bigger-mover-focus branch August 10, 2020 07:58
@github-actions github-actions bot added this to the Gutenberg 8.8 milestone Aug 10, 2020
@youknowriad youknowriad removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants