Enhance block outlines and selection interactions #60757
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
richtabor
added
[Type] Enhancement
|
Size Change: -186 B (-0.01%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
t-hamano
reviewed
Apr 16, 2024
There was a problem hiding this comment.
The outline approach seems simple and makes sense to me. box-shadow borders are not drawn in Windows high-contrast mode, so there is a hack to add a transparent outline, but this itself becomes unnecessary.
This PR also appears to work correctly in Windows high contrast mode.
One suggestion is to widen the offset by 1px. This is to make the text easier to read by leaving a small spacing between the outline and the text when a block with text is focused.
When a paragraph block is focused in trunk:
When a paragraph block is focused in this PR:
I believe this offset is intentional, but what do you think about spreading it out a bit more?
Yes, but I'm just negating the space the outline occupies essentially, so I can't offset everything—or it'd offset off the page, or off of an element with a filled surface, like an image. But if I target content/text, it's easy to end up with this scenario, because the outlines do not line up equally anymore:
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
richtabor
requested review from
a team
and removed request for
ajitbohra and
ellatrix
|
Overall, happy to try this. Trunk:
This branch:
The main difference is that the hover and selection style are the same (that feels a clear win) and the corner radius is removed. A side benefit is that instead of using both before and after pseudo selectors, this one only uses after. There's an issue with selecting linked images, that needs to be fixed first, however:
|
|
Wow, weren't there limitations to what outlines could do? Could we stop using the pseudo element now? Or am I confusing two different things? |
|
Found the old issue for this: #22834 |
Potentially we can, and this PR could be a step on the way. I don't know that outline supported 1.5px stroke thickness back in the day, it certainly didn't support border radius. |
Should this apply to any block containing text? It's a bit inconsistent at the moment; blocks like site title, tagline, page link, button, list item, code, quote, are not captured. |
Looks like this is generated within the editor and front-end, as such: @tellthemachines @aaronrobertshaw — I know you're both really familiar with generated global styles. Do you have an idea where this is coming from/the related issue? I couldn't find it. I'm curious if we should be generating this. Why not allow the browser to handle default focus states? |
richtabor
force-pushed
the
enhance/outline-selection
branch
from
4b3da34 to
11f09d6
Compare
Thanks for the ping @richtabor 👍 I'm still catching up on the issue but it looks like the styles in question are coming from TwentyTwentyFour's theme.json.
Unfortunately, I don't have any context around what went into the decision to add these styles. |
You're right! Good find. I'll dig in a bit. Much appreciated. |
This style was added to the Twenty Twenty-Four theme to improve accessibility. However, unintended problems have occurred, and improvements are being made in the core. See below for details. |
|
@jasmussen considering this is added by the theme, do you think we can merge as-is and fix TT4 to not auto-apply these styles? |
|
Yes, that should not block this, then. Just want to make sure it's not an accidental regression from elsewhere. Though that does seems something to fix in TT4. |
richtabor
force-pushed
the
enhance/outline-selection
branch
from
11f09d6 to
42014e7
Compare
There's not one way to target all text, as |
|
Honestly I can go either way on the paragraph / heading outlines. When writing long-form content I'd probably prefer them to be hidden, but otherwise they can be helpful guides for applying borders, backgrounds, composing layouts, etc. It's not something that should hold up the other benefits here, and can be easily reverted if necessary. |
scruffian
force-pushed
the
enhance/outline-selection
branch
from
6803d3f to
bc01b63
Compare
scruffian
approved these changes
May 8, 2024
Let's give it a shot and feel it out. Thanks! |
I have submited #61470 to fix this issue. The fundamental problem was that the |
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.
What?
Originally explored as part of #60286, cleans up the site editor's hover and selection outlines by:
Testing Instructions
Screenshots or screencast