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

Refactor the Settings button CSS. #14475

Merged
merged 2 commits into from
Apr 10, 2019

Conversation

afercia
Copy link
Contributor

@afercia afercia commented Mar 16, 2019

Fixes #14474.

This PR seeks to solve a Firefox / NVDA bug where changes to the Settings button aria-expanded state are not announced because of the CSS pseudo-element used for styling. For more details, please see the related issue #14474.

I've opted to just remove the CSS pseudo element and re-implement the styling by other means.

Worth nothing the .is-toggled:hover state didn't work as intended, not even on master. I had to use !important to override a pile of :not() selectors used in the icon-button. Adding one more :not() there didn't seem to me the best option.

The CSS should replicate the current styling. Do feel free to change it if desired, but please don't use a CSS pseudo-element 🙂

/Cc @jasmussen

Screenshot (for comparison, see the GIF on the issue):

Screenshot (190)

@afercia afercia added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Mar 16, 2019
@afercia afercia requested a review from chrisvanpatten as a code owner March 16, 2019 12:15
@jasmussen
Copy link
Contributor

Nice.

Tab, hover, focus, look good.

There's a small subtle change with the selected state that we should fix. The radius is off, and there's some shimminess around the corners. I've zoomed in these so it's visible.

Before:

before

After:

after

Two ways to fix this.

  1. Remove the inset box shadow you added, and add padding: 7px to the toggled button instead. This keeps it 34px, which makes it more closely match the preview/publish buttons heights, which is why we initially made the toggled button 34px tall.

  2. Remove the inset box shadow you added, and accept that it is now 36px like the other icon buttons in the menu. We might want to compensate by making the preview and publish buttons taller, to match.

To accomlpish 2, you'd need to find the spot in the code that compiles into this:

.edit-post-header .components-button.editor-post-save-draft, .edit-post-header .components-button.editor-post-switch-to-draft, .edit-post-header .components-button.editor-post-preview, .edit-post-header .components-button.editor-post-publish-button, .edit-post-header .components-button.editor-post-publish-panel__toggle {
    margin: 2px;
    height: 33px;
    line-height: 32px;
    font-size: 13px;
}

And change the height to 36, and line-height to 35. That would make it look like this:

Screenshot 2019-03-20 at 09 50 05

But that's sort of a big change. Since we mean to revisit buttons and their focus states across WordPress, perhaps it's best to go with option 1 for now?

@afercia
Copy link
Contributor Author

afercia commented Mar 21, 2019

Curious that a solid box-shadow doesn't fully cover the background. Yep I'd go with padding: 7px and margin 1px.

@afercia
Copy link
Contributor Author

afercia commented Mar 21, 2019

Done.

@jasmussen
Copy link
Contributor

A bit busy today so can't review, but putting it in a milestone because the changes look good at a code glance.

@jasmussen jasmussen added this to the 5.4 (Gutenberg) milestone Mar 21, 2019
@gziolo gziolo requested review from kjellr, mapk and jasmussen April 10, 2019 08:17
Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

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

Thanks, @afercia! These changes look good. It's unfortunate to have to use an !important override all those button :not()'s, but I think it's more or less fine to do it in this specific case.

I noticed that the height of this button — and all the other toolbar buttons — are different from the height of the text buttons, but seems like that's always been the case, so we can think about that separately:

Screen Shot 2019-04-10 at 2 00 50 PM

@afercia afercia merged commit c939517 into master Apr 10, 2019
@youknowriad youknowriad deleted the update/settings-button-firefox-nvda-aria-expanded branch April 12, 2019 10:58
@youknowriad youknowriad added the [Type] Bug An existing feature does not function as intended label Apr 12, 2019
mchowning pushed a commit to mchowning/gutenberg that referenced this pull request Apr 15, 2019
* Refactor the Settings button CSS.

* Remove box-shadow use padding and margin.
aduth pushed a commit that referenced this pull request Apr 16, 2019
* Refactor the Settings button CSS.

* Remove box-shadow use padding and margin.
aduth pushed a commit that referenced this pull request Apr 16, 2019
* Refactor the Settings button CSS.

* Remove box-shadow use padding and margin.
This was referenced Apr 17, 2019
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). [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Settings button: Firefox and NVDA don't announce the aria-expanded state
5 participants