1543773 - non-webrender doesn't expand the nsTextFrame's bounds to include ::selection shadows (possibly fine)

Status: NEW

(Core :: Graphics: Text, defect, P5)

Description

[Aria Beingessner [:Gankra]]

Follow up bug for an issue I discovered when working on Bug 1529992, but wasn't immediately clear to me how to handle. I will be checking in layout/reftests/bugs/1529992-2.html as fails-if(webrender), which demonstrates the problem.

text-shadows that are applied in the ::selection or ::inactive-selection pseudo-selector should be clipped to the bounds of the display item's selection, but this doesn't currently happen with webrender. Not yet sure where exactly this clipping is applied, and why we're missing it (I expect it's being attached to the PushShadow as a rect, which WR currently ignores).

It's a pretty niche issue, and when done correctly it creates ugly results (cut off shadows), so I don't think fixing this should be a very high priority. That said, it's a regression from non-wr, and chrome/safari also implement it (although safari has really bad invalidation bugs with selection shadows).

Comment 1

[Aria Beingessner [:Gankra]]

TODO: also look into why layout/reftests/text-shadow/blur-opacity.html isn't quite right.

Comment 2

[Aria Beingessner [:Gankra]]

Hey jfkthame, any idea what actually ends up clipping ::selection shadows differently from normal shadows in vanilla gecko? I've been combing over nsTextFrame.cpp and can't for the life of my find it. (I also think it might be super buggy...)

Comment 3

[Aria Beingessner [:Gankra]]

Attachment: 1529992-2.html

Attached: test-case demonstrating Firefox misbehaving with ::selection shadow clipping when there's "normal" shadows.

(also with notes on how all the other browsers act)

Comment 4

[Aria Beingessner [:Gankra]]

Oh. I think gecko only manages to clip selection shadows because nsTextFrame::UnionAdditionalOverflow (really nsLayoutUtils::GetTextShadowRectsUnion) doesn't include selection shadows. This becomes the visual rect, which we clip to? This seems to be corroborated by my ability to clip selection shadows in the presence of non-selection shadows by just making the non-selections shadows less extreme (thus minimizing their effect on the visual rect).

I'm not even sure if the current behaviour is designed or just a happy accident (due to selection shadows being largely bolted on in nsTextFrame's code).

Comment 5

[Jonathan Kew [:jfkthame]]

It sounds like you've answered your own question here, while I was asleep. :)

FWIW, it's not immediately clear to me why it would be desirable to clip selection shadows; the result seems pretty ugly, and as a user I think I'd feel it was broken. But maybe there was a good reason to spec it that way. (I'm not sure offhand where this is defined...)

Comment 6

[Aria Beingessner [:Gankra]]

Attachment: selection-shadows-demo.html

I remember it being more clearly written out, but aiui it falls out of the description of each element (re-)painting its portion of the highlight: https://drafts.csswg.org/css-pseudo-4/#highlight-painting

I have now better analyzed the situation, and updated my demo/notes. Firefox seems to be the odd one out (safari is just very buggy).

There are two issues:

• Firefox only constrains ::selection shadows to the bounds of the nsTextFrame, which produces hacky/bad results. Notably because non-::selection shadows can be used to expand this area, and because it lets ::selection shadows escape the selection as long as there's text from the same frame there.

• Firefox considers ::selection to be a restyling of the existing text, while Chrome/Safari consider the ::selection be a new element. Or at least, that's how they behave with-respect to keeping non-::selection shadows around (instead of overwriting them with cascading).

Comment 7

[Aria Beingessner [:Gankra]]

A more subtle description of chrome's behaviour: if a character is selected, its non-::selection shadows are clipped out of the highlight, and its ::selection shadows are clipped to the highlight. Notably, unselected characters' shadows will still display inside the highlight area, because they haven't been "told" to be clipped out. You can see this if you select the "e" in either of the bottom cases in my demo -- the purple "h" is visible under the selection (because it's not selected), but the red "e" has been cleanly clipped out (otherwise you would have a purplish mess where the red and blur mix).

Also selecting "e" demonstrates a humorous artifact of how they break up drawing the text: the purple "e" shadow suddenly pops above the real text!

Comment 8

[Aria Beingessner [:Gankra]]

Hey emilio, do you understand this spec and/or what we should be doing here? Should we just be trying to be as similar to chrome as possible in this case? (Comment 6 has the best summary/demo)

Comment 9

[Emilio Cobos Álvarez [:emilio]]

So, if I understand, current WebRender behavior is that we don't clip the selection shadow, but it doesn't add to the overflow rect right?

So if they're at the bottom of a scroller, for example, you won't be able to scroll all the way to see them. Is that right?

I think, per the discussion in https://github.com/w3c/csswg-drafts/issues/2362, that the CSSWG is fine with stuff affecting ink overflow but not scrollable overflow. I'm with Jonathan that clipping the Shadow is quite ugly. Do engines clip text decorations too?

Given the world clip doesn't appear at all in https://drafts.csswg.org/css-pseudo-4/, I'd say you should open a spec issue to get a clarification from the working group at https://github.com/w3c/csswg-drafts/issues/new.

But unless we find broken sites because of this, the WebRender behavior looks preferable to me.

Comment 10

[Jonathan Kew [:jfkthame]]

Just to add a thought here:

(In reply to Alexis Beingessner [:Gankro] from comment #6)

• Firefox considers ::selection to be a restyling of the existing text, while Chrome/Safari consider the ::selection be a new element. Or at least, that's how they behave with-respect to keeping non-::selection shadows around (instead of overwriting them with cascading).

Trying to think as an author and/or a reader, it seems much more natural to me that ::selection is treated as a restyling of the existing text than being a new element that somehow appears over the original text.

When I'm looking at a block of text, and I drag the mouse through some of it to select it, I don't think about instantiating a new "element" that represents the selected text, independent of (and floating above) the text that was already present. I'm simply indicating a particular range of the existing text, and I expect the browser to provide visual feedback of the exact range that is selected by painting it with different styling than its unselected surroundings.

Usually that's automatic, the browser has some built-in behavior that provides such feedback by painting the selected text with a different background color (and foreground color, on some OSes). If as a page author I want to customize the painting of the selected text, I can use ::selection to target it and provide styles that will override the browser's default behavior.

But nowhere in my mental model do I see a new transient element appearing that represents the selected text and gets painted on top of the original content with different styling.

To my mind, the main bug I see is the fact that we do clip the painting of ::selection shadows when WR is not enabled; I think we should be painting them in full.

Comment 11

[Aria Beingessner [:Gankra]]

Filed https://github.com/w3c/csswg-drafts/issues/3932

Comment 12

[Aria Beingessner [:Gankra]]

Ok everyone involved seems to think webrender is doing The Best so this is really more of a vanilla firefox bug. Since it's relatively minor and fixed by webrender, I don't consider it that important to prioritize. Also I wouldn't want anyone to put effort into fixing this until the csswg definitively concludes that webrender's behaviour is the desired one (it's possible the chrome/safari teams believe their solution is better...)