Bug 209822 - Cannot style ::selection for a flex container
Summary: Cannot style ::selection for a flex container
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: Safari 13
Hardware: Macintosh macOS 10.15
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-31 10:52 PDT by adam
Modified: 2020-05-22 09:41 PDT (History)
13 users (show)

See Also:


Attachments
Patch (34.76 KB, patch)
2020-04-12 21:42 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (34.76 KB, patch)
2020-04-12 22:47 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (34.79 KB, patch)
2020-05-20 22:48 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (34.74 KB, patch)
2020-05-21 10:54 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description adam 2020-03-31 10:52:07 PDT
See the repro here: https://jsfiddle.net/gs791rx4/8/

Selecting "some text" will show a red backgrounded selection on Chrome but a default selection background on Safari.
Comment 1 Alexey Proskuryakov 2020-03-31 11:39:10 PDT
<style>
.example {
  display: flex;
}

.example::selection {
  background: red !important;
}
</style>
<div class="example">
  some text
</div>
Comment 2 Radar WebKit Bug Importer 2020-03-31 11:39:19 PDT
<rdar://problem/61117620>
Comment 3 Tyler Wilcock 2020-04-05 20:43:27 PDT
Did a little digging into this one.  From what I can tell, this is happening because the TextRender's parent is sometimes set as anonymous when it maybe shouldn't be.

https://github.com/WebKit/webkit/blob/master/Source/WebCore/rendering/TextPaintStyle.cpp#L161

calls here:

https://github.com/WebKit/webkit/blob/master/Source/WebCore/rendering/RenderText.h#L303#L306

eventually calling here, and shortcircuiting `nullptr` in the `isAnonymous()` check:

https://github.com/WebKit/webkit/blob/master/Source/WebCore/rendering/RenderElement.cpp#L1407#L1410

Also of note, this behavior is not specific to `display: flex`.  See this repro with `display: block` and the introduction of a sibling div.  Again, "some text" should be highlighted red, assuming Chrome and Firefox are correct and our behavior is not.  Removing the sibling div results in the correct behavior.

https://jsfiddle.net/dwuzsmft/

Going to do some more investigation.
Comment 5 Tyler Wilcock 2020-04-12 21:42:01 PDT
Created attachment 396256 [details]
Patch
Comment 6 Tyler Wilcock 2020-04-12 22:47:57 PDT
Created attachment 396257 [details]
Patch
Comment 7 Tyler Wilcock 2020-04-13 21:53:25 PDT
I think this should do it.  The TLDR is that RenderText unconditionally checks its direct parent for pseudostyles, but the direct parent of text nodes/runs is often anonymous, and pseudostyles are associated with elements of the DOM (i.e., non-anonymous).  My proposed patch changes RenderText look for pseudostyles by ascending the tree until a non-anonymous renderer is found.  Should be ready for review.

I added two test cases.  I'm running the GTK port of WebKit, so test expectations were added for LayoutTests/platform/gtk.  According to this (https://trac.webkit.org/wiki/CreatingLayoutTests) wiki article, I also need to add test expectations for other platforms.

What is the best way to do this?  I'm not seeing anything on the Builders page that shows newly generated test expectations as the previous article suggests, but perhaps I'm missing it.
Comment 8 Tyler Wilcock 2020-05-20 22:48:58 PDT
Created attachment 399934 [details]
Patch
Comment 9 Tyler Wilcock 2020-05-21 07:42:52 PDT
Rebased onto latest master -- tests are still passing.
Comment 10 Antti Koivisto 2020-05-21 08:42:23 PDT
Comment on attachment 399934 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399934&action=review

> Source/WebCore/rendering/RenderObject.cpp:170
> +    RenderElement* ancestor = parent();

Could use auto*

> Source/WebCore/rendering/RenderText.h:287
> +    if (RenderElement* ancestor = firstNonAnonymousAncestor())

Here too

> Source/WebCore/rendering/RenderText.h:294
> +    if (RenderElement* ancestor = firstNonAnonymousAncestor())

etc
Comment 11 Tyler Wilcock 2020-05-21 10:54:45 PDT
Created attachment 399968 [details]
Patch
Comment 12 Tyler Wilcock 2020-05-21 15:29:46 PDT
Thanks for the review, Antti.  I applied your suggestions in this latest patch and tests are again green.  I think my addition of a new patch nullified your r+ -- could you take another look, please?
Comment 13 EWS 2020-05-22 01:34:35 PDT
Committed r262049: <https://trac.webkit.org/changeset/262049>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399968 [details].
Comment 14 Ryan Haddad 2020-05-22 09:41:24 PDT
Added test baselines for macOS and iOS in r262059