Text Selection in Safari: Try new fix for recent version #44148
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
added
[Type] Bug
|
Size Change: +56 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
|
👋 Safari is still being difficult with me. I'm on 17614.1.25.9.10 too, with the TT2 theme, and this is what I get: 44148-safari-text-selection.mov
|
|
Thank you for testing. One issue that would be excellent to fix, but is beyond the scope to fix, is that when your cursor is between two blocks, the selection is treated as a multi-selection (invoking the blue borders) even if it should be counted as a partial selection. If we can fix that so multi selection truly only invokes if a non-text block is selected, that would fix the issue right up until what happens at the 6 second mark, which I'll try my best to reproduce and fix now 😅 |
|
While I'm fixing this branch, can you take a quick look at #44150 which is a work in progress refactor? |
|
Alright @mcsf, I think I've fixed the issue you saw. It was quite hard to reproduce, it helps when the last block you select is the last block on the page it seems, and the state you managed to get it in was one of multi-selection, but not partial selection. I'll get back to this in a minute, but for now, I think I've fixed the issue you were seeing, can you please test again?
One thing you'll observe here is the one I mentioned: when the cursor is between two blocks, the blue borders appear. Here's what happens, technically:
When you are making a partial selection, |
|
Working on this, noting here some observations on how Safari is drawing selection markers:
|
|
I extracted the flickering issue into a separate issue: #44153. |
jasmussen
force-pushed
the
try/safari-selection-fixes
branch
from
71326f8 to
849de8b
Compare
|
Thanks for the excellent follow-up! Unfortunately I am still getting the wide background selection. I find it interesting that you found it hard to reproduce, because in my setup/post the result is guaranteed. I don't know what it is, but I'll share my whole content, in case that helps:
|
|
Indeed, I can reproduce with that particular test content, which blows my mind 😅 — thank you, I'll dive in and see what's up! |
jasmussen
force-pushed
the
try/safari-selection-fixes
branch
from
849de8b to
e3cd560
Compare
|
Alright, I think I have a pretty solid fix:
Some new revelations:
I think this PR is pretty solid, if it fixes the issue for you as well Miguel, I think we should land it. But an important note, the flickering seen when hovering between two blocks is entirely fixed by #44154. So we should land that one too! |
|
Also of note: the issue of the pseudo element sub-pixel glitches mentioned can potentially be fixed more or less entirely by #44150, which at the moment tries to refactor that pseudo element away entirely. |
|
Edit, posted in the wrong place, meant to post on #44150. |
priethor
added
the
Backport to WP 6.7 Beta/RC
draganescu
approved these changes
Sep 16, 2022
|
Seems fine to me. I'll look into the other issues. |
|
Thanks, I'll land this and rebase the others! |
ockham
pushed a commit
that referenced
this pull request
Sep 19, 2022
* Text Selection in Safari: Try new fix for recent version * Fix issue. * Better fix.
|
I just cherry-picked this PR to the wp/6.1 branch to get it included in the next release: 6082384 |
ockham
removed
the
Backport to WP 6.7 Beta/RC
|
@jasmussen: this looks great now! Thank you to everyone for the fixes and reviews. |
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?
Followup to #43313. A very recent update to the Safari browser has reintroduced the original issue of full-width selection indicators appearing between blocks. This PR dives in and applies a new fix.
In trunk, selecting text looks like this:
With this PR applied, the selection looks better and more correct in terms of what you've actually selected:
How?
It seems the Safari selection indicator has become sensitive to the presence of
position: relative;for the purposes of drawing the indicator. For context, that property is set so that we can draw the blue focus rectangle around the selected block in a pseudo element.But if set initially, Safari will draw an incorrect selection indicator. It is unfortunately not enough to apply
position: static;orunsetor anything like that, the property must simply not be set at all. This is a little curious, and if you have any insights as to why this might be, I'd appreciate it.Nevertheless, setting the value contextually as this PR does, appears to still allow us to paint the pseudo elements as we need to, without breaking the selection.
Testing Instructions
The selection marker should look correct.