Bug 224734 – Selected image overlay text should never be visible

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Wenson Hsieh]

<rdar://problem/76806399>

[Wenson Hsieh]

Created attachment 426383 [details]
For EWS

[Darin Adler]

Comment on attachment 426383 [details]
For EWS

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

> Source/WebCore/ChangeLog:29
> +        Make an additional adjustment when computing selection pseudo styles for UA shadow root content. Currently, for
> +        elements inside a UA shadow root, we always immediately ascend to the shadow host; this means that `::selection`
> +        pseudo selectors currently don't work in UA stylesheets, since they're skipped when resolving styles, upon
> +        painting selected text.
> +
> +        To fix this, we avoid ascending to the shadow host's renderer in the case where we have a selection pseudo
> +        style. Since image overlays are the first (and only) case where UA styles include `::selection`, this has no
> +        effect on any other existing UA styles.

This seems like a hack. We are saying "selection" but we really mean "the image overlay style".

What’s the rationale for always ascending to the shadow host in the first place? What does that help with? If I understood that then perhaps I would understand more deeply why it’s safe to make a special case for ::selection. Rather than "it's the only case of this right now", which sounds fragile to me.

[Wenson Hsieh]

Comment on attachment 426383 [details]
For EWS

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

>> Source/WebCore/ChangeLog:29
>> +        effect on any other existing UA styles.
> 
> This seems like a hack. We are saying "selection" but we really mean "the image overlay style".
> 
> What’s the rationale for always ascending to the shadow host in the first place? What does that help with? If I understood that then perhaps I would understand more deeply why it’s safe to make a special case for ::selection. Rather than "it's the only case of this right now", which sounds fragile to me.

Interesting! I'm not at all familiar with this logic, but the way I interpreted this was that we made a special case for `::selection` to eagerly bail out of the UA shadow root (and that this was essentially making an exception to avoid this special case).

After a bit of digging, it looks like this behavior was initially added to address https://bugs.webkit.org/show_bug.cgi?id=38943.

[Wenson Hsieh]

Created attachment 426384 [details]
Maybe slightly cleaner?

[Darin Adler]

Comment on attachment 426384 [details]
Maybe slightly cleaner?

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

> Source/WebCore/rendering/RenderElement.cpp:1533
> +    if (auto selectionStyle = getUncachedPseudoStyle({ PseudoId::Selection }))
> +        return selectionStyle;

OK, does seem a little better. If worded clearly, a brief "why" comment here might make it even better.

[Wenson Hsieh]

(In reply to Darin Adler from comment #5)
> Comment on attachment 426384 [details]
> Maybe slightly cleaner?
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426384&action=review
> 
> > Source/WebCore/rendering/RenderElement.cpp:1533
> > +    if (auto selectionStyle = getUncachedPseudoStyle({ PseudoId::Selection }))
> > +        return selectionStyle;
> 
> OK, does seem a little better. If worded clearly, a brief "why" comment here
> might make it even better.

Added a comment explaining the intent behind this logic. Thanks for the review!

[Wenson Hsieh]

Created attachment 426389 [details]
For landing

[EWS]

Committed r276236 (236718@main): <https://commits.webkit.org/236718@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426389 [details].