Try: Make focus width a CSS variable. #27968
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jasmussen
requested review from
adamziel,
ajitbohra,
chrisvanpatten,
draganescu,
ellatrix,
kevin940726,
noisysocks,
talldan and
tellthemachines
as code owners
This was referenced Jan 4, 2021
|
Size Change: +14.7 kB (+1%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
|
Great idea! Porting your question from the other thread to this one:
I can't reproduce on a 2x screen either so this solution is 🎯 I was excited to see this approach because it should fix the rounding inconsistencies that are also present in the general focus styles (which I hadn't brought up in #27925 just to keep the focus on the block highlight/selection indicators). To demonstrate: Before decimal-focus-width-1x.mp4After focus-width-1x.mp4Much better! Though it can be seen there are the faintest of rounding differences still present. Back to the topic of block outlining. I've tested this PR only on a 1x screen. It is a great improvement there. It may be too micro to worry about but there can still be very slight rounding inconsistencies: So even with an integer spread-radius (2px instead of 1.5px) the box-shadow is still prone to very minor rounding differences. Because I was pretty sure that in #27926 all rounding problems were gone, I experimented and it seems the difference is the inset box-shadow is less susceptible to rounding differences. Here's a screen recording to demonstrate: inset-vs-outset.mp4That may not be easy to follow without some explanation. Line-height changes are used to emulate the type of variance that will exist between themes. First, line-height is changed slightly to exaggerate the rounding fuzz. Then the styles to inset the box-shadow (and inversely compensate the position/radius) are uncommented. The rounding fuzz vanishes and stays gone no matter the changes to line-height. Then the styles are switched again just to reiterate. It may help to pause and jog between some frames. While I don't think the inset box-shadow styling would be problematic I've not really tested it and since the rounding issues are already mostly gone (and only present in Firefox on 1x screens) I'm not pushing to try it. To sum up: LGTM. |
|
Awesome, thanks so much.
Yep, I think this is worth keeping in mind as we explore further improvements. For example we could keep your initial ticket open, with the excellent detective work you just did in your comment above, to explore moving towards using only an inset shadow to enhance things further. Especially if shipping this fix in the mean time doesn't block that from happening. We'll have improvements to the somewhat embarrassing current issue, that don't preclude further enhancements. Given your LGTM (thanks again for the review), I'll request a couple of reviews to make sure the rules I added are in the right CSS file, and then we can ship this! |
jasmussen
commented
Jan 5, 2021
| @@ -454,6 +454,13 @@ | |||
| // Darker shades. | |||
| --wp-admin-theme-color-darker-10: #{darken($color-primary, 5%)}; | |||
| --wp-admin-theme-color-darker-20: #{darken($color-primary, 10%)}; | |||
|
|
|||
| // Focus style width. | |||
There was a problem hiding this comment.
@gziolo or @aristath if either of you have time, is this the right place to register this rule? I placed it here because then it's in context of the other CSS variables registered. But the side-effect is that the rule is duplicated multiple times in the DOM (something to look at separately for the color, perhaps, but perhaps best to not exacerbate with this PR). Is there a more appropriate place to store this rule? The base block-list CSS perhaps? Thank you for looking!
There was a problem hiding this comment.
Can I get approval? :)
gziolo
approved these changes
Jan 5, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Alternative to #27926, and also aims to fix #27925. CC and props: @stokesman
The most urgent issue is that on 1x normal resolution screens, the 1.5px focus style rounds badly, causing the issues outlined in #27925.
This PR adds the following base style:
In other words, it makes the thickness of the focus style a CSS variable. This CSS variable is then overridden on high resolution screens. In my testing (Chrome, Safari, Firefox), it works as advertised:
2x screen:
1x Firefox (note, emulated):
As far as I can tell, this solves the rounding issue, but lets us keep the more balanced but still thicker focus style for high resolution screens.
In addition to this, the move to making the focus style a CSS variable means that users can override these easily, whether through plugins or even a future option in settings, should you want a thicker focus style:
This needs good testing. In addition, the placement of these rules in the mixin means the CSS variables are duplicated many times. This appears to be an existing issue, but I'm happy to move the initial definition somewhere else, for example to the block-list stylesheet.