Improve Safari text-selection style. #43313
Conversation
jasmussen
added
[Type] Bug
| } | ||
|
|
||
| // Re-enable it on Paragraphs. | ||
| .block-editor-rich-text__editable { |
There was a problem hiding this comment.
Could we generalise this to [contenteditable]?
There was a problem hiding this comment.
Very probably! And feel free to push directly to this branch, or make an entirely new one.
There was a problem hiding this comment.
No, you got this :) this branch is good
|
|
||
| // Hide the select style pseudo element as it interferes with the style. | ||
| &.is-partially-selected::after { | ||
| display: none; |
There was a problem hiding this comment.
What is this hiding? Isn't it now always hidden with this rule?
There was a problem hiding this comment.
There's a pseudo element on every block present to fade in to the blue focus/select style. This:
By hiding it entirely, we get a pure text selection, only the text inside the two paragraphs:
If we don't hide it, you can see a small 1px selection style, that's the "after" element that gets selected:
There was a problem hiding this comment.
Ah yes, but doesn't this line remove it entirely? Or is there a rule somewhere that enables it under the right circumstances?
There was a problem hiding this comment.
It hides it while is-partially-selected is applied. I tried also to just add user-select: none; to the pseudo element, but I don't think that works because the pseudo element is technically a "child" of the paragraph. I'm not sure how to do this differently 🤔
|
Size Change: +137 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
|
|
||
|
|
||
| // Precise partial text selections. | ||
| // @todo: For now, proof of concept. But code needs to be moved to the most appropriate place. |
There was a problem hiding this comment.
I think this belongs in block-editor/block-list/style.css
| // Precise partial text selections. | ||
| // @todo: For now, proof of concept. But code needs to be moved to the most appropriate place. | ||
| // Furthermore, this code right now is likely going to affect numerous nested blocks, so | ||
| // it will be important to scope it right! |
There was a problem hiding this comment.
What blocks will be negatively affected? I can't think of any, so maybe no need to scope?
There was a problem hiding this comment.
It's important to test. This rule is the one we should be sure about:
.block-editor-block-list__layout {
user-select: none;
}
Basically it says, anything that's inside .block-editor-block-list__layout, you can't select, unless we explicitly re-enable it using user-select: text;, or even user-select: auto;
|
We should test this in all browsers, but it seems to work well also in Chrome and Firefox. |
|
I'll ping a bunch of folks for opinions. Essentially I'm pretty confident in that using |
jasmussen
requested review from
a team,
jorgefilipecosta and
ntsekouras
and removed request for
a team
|
It's better, but perhaps not perfect? Here's what I'm seeing in Safari:
It seems to be related to line-height. |
|
I've addressed the feedback and moved the CSS around a bit. One of my concerns remains, and is showing up in a few places. Notably, text selection of contents inside placeholders was also disabled. I re-enabled that. But it's also disabled here: That's a post-list, and the excerpts aren't selectable. But wouldn't we still want a user to select text there even if it isn't editable? Or is the fact that it isn't editable and therefore unselectable a good thing? 🤔 |
jasmussen
force-pushed
the
try/better-safari-text-selection
branch
from
a4fe161 to
1ea63d4
Compare
|
The test failure looks real. I'd appreciate help here 🤔 |
|
I'd be inclined to say that un-editable text being un-selectable is probably a good heuristic. |
|
@ntsekouras I would love your input on this one (and your help in case you can figure out the failing test) 🙏 |
| // To do this, we disable text selection on the main container, then re-enable it only on the | ||
| // elements that actually get selected. | ||
| // To keep in mind: user-select is currently inherited to all nodes inside. | ||
| user-select: none; |
There was a problem hiding this comment.
Have you made any experiments with auto value(ref)? I haven't thought much about it, but this effects everything besides the exceptions below. For example you can't select any placeholder text etc.. - maybe that's better though..
There was a problem hiding this comment.
Thank you for looking 🙏
auto is default, and as it turns out, is the cause of the glitchy selection we've been seeing in Safari. For a partial selection between two paragraphs, the following selection is accurate to what you are actually selecting:
The above works in this branch because we disallow everything but the paragraph text to be selected. It seems Safari's interpretation of selection in browsers is to capture everything, every div, every pseudo element. And so if we don't disable user select, we get the trunk behavior, which is this:
So in order to accurately depict to the user what's selected (and what they can copy if they press ⌘C), we have to disable user-select on everything else. The fact that this affects everything down the stack is the main thing to test and write exceptions for, and I'm not sure if there's a better way.
The downside I could see here it would be to a flow of selecting some blocks in Query Loop(not content editable) and pressing copy where it will first select the blocks(if they cannot be merged) and then you can actually copy them. This cannot be done here. |
Excellent point. I tried enabling selection also on blocks, and it seems to thread the needle here, so it still has a correct partial selection indication in Safari. What do you think? |
|
|
||
| // Hide the select style pseudo element as it interferes with the style. | ||
| &.is-partially-selected::after { | ||
| display: none; |
There was a problem hiding this comment.
This creates a regression when we have multiselected blocks and hover the block toolbar. This is in trunk:
Screen.Recording.2022-08-22.at.2.28.30.PM.mov
|
I took a look at the test and what happens is that with this change we can't select anything between selected blocks so if we click between them, the selection is not cleared. If that's okay, we can update the test to click somewhere else, but I have no strong opinions on which is a better experience.. |
Hmm. That's a lot of little issues popping up. To be clear, ensuring a correct text selection is critical for 6.1 (though it's in bug-fix territory so we have a little time still). But there might be a better approach to it, than this CSS-only version, which was initially meant as a proof of concept. The key to managing what shows up as selected is to judiciously apply the |
| // Re-enable it on editable items and a few others. | ||
| .block-editor-block-list__block, | ||
| [contenteditable] { | ||
| user-select: text; |
There was a problem hiding this comment.
What is the point of adding [contenteditable] if it's already enabled on the block.
That actually seems like a god solution: disable text selection for block lists and re-enable it for blocks.
There was a problem hiding this comment.
Good call, I removed contenteditable and just targetting blocks. I had to move the code a bit.
There was a problem hiding this comment.
Which components are in a list of blocks that is not inside a block?
There was a problem hiding this comment.
The components reference is meant to target content inside placeholders.
There was a problem hiding this comment.
Aren't these inside .block-editor-block-list__block? I don't get this extra rule, sorry.
There was a problem hiding this comment.
@jasmussen I'm still not sure what this rule is needed btw.
There was a problem hiding this comment.
You're referring to [class^="components-"]? It's needed to enable text selection inside placeholders like Cover. It's not that you need to select text there that often, but right now the multi-select still selects that text, and without a visible selection marker, it becomes confusing why the multi-select isn't invoking. Possibly related to this: #42983 (comment)
| // This is a hack to only disable user-select when blocks are selected inside. | ||
| // @todo: We shouldn't be targetting the label, this is only a POC. Should be replaced with a class. | ||
| //user-select: none; | ||
| [aria-label="Multiple selected blocks"] & { |
There was a problem hiding this comment.
This is obviously in need of a change.
But what I did here was to scope the disabling of user-select: none; to only ever apply when a block is selected. I don't know if it's needed — if it is, we should assign a classname on this element instead — but it does seem safer. @ellatrix what do you think?
|
|
||
| // Hide the select style pseudo element as it interferes with the style. | ||
| &.is-partially-selected::after { | ||
| height: 0; |
There was a problem hiding this comment.
This is a good change which I think fixes the issues reported by @ntsekouras. Specifically instead of removing the pseudo element, it's made invisible. In practice for me that means I can hover the drag handle to highlight the blocks again, and click between them to de-select.
|
Hooray, tests are green. That doesn't mean we can merge, but it means we can now figure out if #43313 (comment) is needed, and then either address the discussion point or go without it. |
jasmussen
force-pushed
the
try/better-safari-text-selection
branch
from
3bc62d7 to
33e0631
Compare
|
I have removed the hack I had in place, because I think we should land a first version of this PR as is. The hack demonstrates that if this version which deselects user-select in broad strokes need further scoping, there's a clear path to doing that as an easy bugfix. But we can try first without it. |
|
To connect dots, I noticed this issue as I was tinkering with this branch. |
|
@jasmussen I took a liberty of updating your branch with some code that adds the class name when needed: 7aa51cc Feel free to discard it or use it as needed. |
|
Oh nice! Thank you 🙏 I'll let this one sit over the weekend for any input, and then I can add the scoping-to-multi selected code back then. |
jasmussen
force-pushed
the
try/better-safari-text-selection
branch
from
fee8037 to
e5cb9b9
Compare
|
Thank you for the help, and for the green check! This is an important one, so when the checks pass, I'll land it. I added back the scoping for the user-select rule per Adam's help, but I'll be here to work on any followups we might encounter. |
What?
Currently a proof of concept, this PR means to improve the clarity of the partial selection style in Safari and other browsers, where it currently selects not just text, but divs and pseudo elements, resulting in a very broken looking experience:
By applying
user-select: none;to elements that are not meant to be shown as selected, and by enabling it again usinguser-select: text;, we are able to ensure that the visible selection style matches what you have actually selected:Why?
This is crucially important for the experience, because the current experience mis-represents what you have selected.
How?
This proof of concept simply places rules that makes this work for partial text selections in the Paragraph block CSS. This is not where I expect the code to live, but it shows how the issue can be fixed.
We'll want to find out how to best apply these properties so that the issue is fixed, but we don't disable text selection for other parts. At the moment (this appears to be a bug), major browsers treat the property as one to be inherited by default, which means we need to unset the property again for any child elements.
One idea as alluded to in the comments, is to apply the properties during the selection, and manage them that way.
Testing Instructions