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

Audio Block: Styles for focus outline #27335

Closed

Conversation

MaggieCabrera
Copy link
Contributor

@MaggieCabrera MaggieCabrera commented Nov 27, 2020

Description

This PR intends to bring this fix from TT1 to Gutenberg. I am adding an outline to the audio block when it's focused.

How has this been tested?

Tested in TwentyTwentyOne on Safari, Chrome, and FF (mac)

Screenshots

Firefox:

Screenshot 2020-11-30 at 09 36 17

Chrome:

Screenshot 2020-11-30 at 09 36 59

Safari:

Screenshot 2020-11-30 at 09 36 48

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.

@carolinan
Copy link
Contributor

I agree that the focus style can be improved to look more identical in different browsers, but I think
this thick focus style may not be neutral enough to have as a default style.

@MaggieCabrera
Copy link
Contributor Author

I agree that the focus style can be improved to look more identical in different browsers, but I think
this thick focus style may not be neutral enough to have as a default style.

Thanks for your input, I was unsure about the best look for this myself, I was thinking maybe reduce it to 1px and make it dotted? Any feedback is appreciated

@carolinan
Copy link
Contributor

Yes I think that would be a more neutral style and an improvement from the current.

@kjellr
Copy link
Contributor

kjellr commented Nov 30, 2020

Do we do something similar for any other front-end focus states? If so, we should borrow whatever focus style is used elsewhere.

In general, I agree that it's an improvement, but I wonder if we're solving something that the browsers really just need to fix themselves. 🤔

@jasmussen
Copy link
Contributor

jasmussen commented Dec 1, 2020

I would actually agree with Kjell — this is not something we should fix in the block editor, and is prime real estate for the theme itself to fix.

Generally the less we put in style.scss, the better — this enables themes like TT1 to have an opinionated and consistent focus style that is the same across all blocks.

The fact that the focus style looks so broken in the browser by default is unfortunate and a little embarrassing on part of the browser vendors, but it should still be fixed directly in the browsers.

But thank you so much for the PR, though! 😅

@MaggieCabrera
Copy link
Contributor Author

I would actually agree with Kjell — this is not something we should fix in the block editor, and is prime real estate for the theme itself to fix.

Generally the less we put in style.scss, the better — this enables themes like TT1 to have an opinionated and consistent focus style that is the same across all blocks.

Thank you so much for your feedback @kjellr and @jasmussen

I agree that the theme is the one responsible for how this focus style but I made this PR thinking about how solving the original issue should actually be a part of the block, so that we don't have to repeat this code across every single theme, and just leave the themer the liberty to decide how they want it to look (once the initial issue is resolved). As theme developers, we find ourselves "fixing" things time and time again and repeating code unnecessarily and that was the intention behind it.

The whole idea behind this is to have a default state that will be accessible and work fine for every theme even if the theme doesn't decide to change its appearance, instead of passing the "problem" to the theme developer.

@jasmussen
Copy link
Contributor

but I made this PR thinking about how solving the original issue should actually be a part of the block, so that we don't have to repeat this code across every single theme, and just leave the themer the liberty to decide how they want it to look (once the initial issue is resolved). As theme developers, we find ourselves "fixing" things time and time again and repeating code unnecessarily and that was the intention behind it.

I do agree and this is noble. However by adding an explicitly designed focus style, we are essentially providing CSS that the theme developer should override, ultimately creating CSS that ideally goes unused.

This chrome focus style is really bad:

Screenshot 2020-12-01 at 12 23 47

But it is the default focus style, and it's a browser issue, and they are actually reasonably new, and probably subject to iteration.

By not specifying a focus style for this block, a theme developer can do the following:

:focus {
    outline: 2px dotted;
}

which would result in a better looking focus style:

Screenshot 2020-12-01 at 12 26 41

Whereas if we started to add focus styles to the built-in block styles, theme developers would have to add more specificity to override it:

:focus,
.wp-block-audio audio:focus {
    outline: 2px dotted;
}

The whole idea behind this is to have a default state that will be accessible and work fine for every theme even if the theme doesn't decide to change its appearance, instead of passing the "problem" to the theme developer.

Absolutely. This could potentially be a focus area for the global styles, so that there was a single central place to define focus styles. CC: @nosolosw

@MaggieCabrera
Copy link
Contributor Author

Absolutely. This could potentially be a focus area for the global styles, so that there was a single central place to define focus styles. CC: @nosolosw

YES. I didn't know about this, and there's some other styles like this in TT1 that might be interesting to add to such a place. Please let me know if I can help in any way shape or form.

Thanks again for your input, very appreciated

@oandregal
Copy link
Member

FWIW, there's an issue to discuss improving how states are managed by blocks. I've added it to the backlog, so we don't lose track of it.

@carolinan
Copy link
Contributor

I believe that this can be closed?

@MaggieCabrera
Copy link
Contributor Author

I believe that this can be closed?

Yep, thank you all for your feedback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Audio Affects the Audio Block [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.

6 participants