1516361 - Selection in the titles in 35c3 wiki is broken (background-clip: text)

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P4)

Description

[Emilio Cobos Álvarez (:emilio)]

Attachment: Reduced test-case

STR:

 * Go to https://events.ccc.de/congress/2018/wiki/index.php
 * Try to select some text in a heading.

Expected:

 * It works

Actual:

 * It doesn't. Though if you zoom-in or such it suddenly does. But it never gets invalidated properly.

Furthermore there seems to be very fun stuff going on if you tweak borders dynamically and such, it's really broken.

Seems to be related to background-clip: text, see the reduced test-case attached.

Comment 1

[Darkspirit]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #0)
>  * Go to https://events.ccc.de/congress/2018/wiki/index.php

> fun

When opening that page by clicking on the link the title box is empty until I hit F5. Fine with WR disabled.

Comment 2

[Darkspirit]

Regression date would be before 2018-01-15.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #1)
> When opening that page by clicking on the link the title box is empty until
> I hit F5. Fine with WR disabled.

Yeah, it's probably the same issue. They're using a web font, so it's supposed to not render until it loads.

We fall back to a blob when using background-clip: text, and looks like we're not invalidating the fallback blob at all in either case (when you select text, or when the font loads). I guess the tricky part is that in both cases the text frame is invalidated, but what we need to repaint is the parent element's background display item (I suspect?)... Maybe Jeff, Matt or Markus know how this is supposed to work in non-WR?

I'm happy to take a look when I'm back from PTO in any case.

Comment 4

[Matt Woodrow (:mattwoodrow)]

So it looks like the background color display item paints both the clip background color, as well as the selection. These all end up in a single blob, and we don't invalidate it when the selection changes.

We do also create a text display item for painting the selection [1] though, it just doesn't draw anything due to ShouldDrawSelection [2] returning false when the current text is being used for background-clip:text.

Even though the text item for selection doesn't actually draw any pixels, it reports that it does, and participates in invalidation as if it did. Changes to the selection result in invalidations to this item, which invalidates the right pixels in the Layer (assuming the two items are in the same Layer).

I haven't reproduced the broken rendering with font loading yet, but I can't see how that would work even in non-WR.

[1] https://searchfox.org/mozilla-central/rev/330daedbeac2bba296d663668e0e0cf248bc6823/layout/generic/nsTextFrame.cpp#5140
[2] https://searchfox.org/mozilla-central/rev/330daedbeac2bba296d663668e0e0cf248bc6823/layout/generic/nsTextFrame.cpp#6867

Comment 5

[Matt Woodrow (:mattwoodrow)]

It looks like the reason the background color display item paints the selection (despite it not being related otherwise) is so that the selection color appears behind the clipped background (which is the ordering for normal selected text).

Webkit/blink don't appear to do this, and just paint the selection on top of the text-clipped-background (add -webkit-background-clip:text to the testcase).

The relevant spec [1] doesn't mention anything about selections, and I can't find any motivation in the original bug as to why we wanted to do it in this order (bug 759568, and bug 1269971).

Jonathan, is there any reason we shouldn't simplify our code and just draw the selection on top of background-clip:text?

[1] https://drafts.csswg.org/css-backgrounds-4/#background-clip

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Also it sounds like that's the reason bug 1516362 happens for example (or a way to fix it), so we could fix those at the same time if I'm understanding correctly.

Comment 7

[Alice0775 White]

I can reproduce the issue using attachment 9033247 [details] with/without WebRender on Nightly66.0a1 windows10.

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=258f53f98c75798678b1e96f8059b5d950dd0fe8&tochange=4014fede870e2b2adef58375d42e3af7c0f301a5

Comment 8

[Jonathan Kew [:jfkthame]]

Seems like this really should be clarified in the spec somehow. https://drafts.csswg.org/css-backgrounds-4/#background-clip doesn't even mention anything about background-clip:text affecting the normal painting of the text, such that it becomes transparent (maybe that's described somewhere else?), let alone the interaction with painting of selections.

From an author's point of view, I'd assume the usual use-case for background-clip:text is to apply a special "fill" such as a gradient or image to some text, instead of a solid color. The "background" isn't really being thought of as a background in this context; it's being used as a fill for the (foreground) text. As such, it makes sense to me that the selection would be painted behind this special-filled text, just as it paints behind normal solid-filled text.

If CSS were to add some more direct way to specify a complex fill (gradient or image) as a "foreground color" for text (e.g. an enhanced version of the color property), then authors could use this to change the text fill without affecting the painting order. Then I'd agree with the argument that background-clip:text should paint behind the selection (because it's a background, it just happens to be clipped). But if CSS isn't going to do that, and the long-term position is that background-clip:text is THE way to achieve the effect of "color: <gradient>", then I feel that such text should paint in front of the selection.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jonathan Kew (:jfkthame) from comment #8)

Seems like this really should be clarified in the spec somehow. https://drafts.csswg.org/css-backgrounds-4/#background-clip doesn't even mention anything about background-clip:text affecting the normal painting of the text, such that it becomes transparent (maybe that's described somewhere else?), let alone the interaction with painting of selections.

Well, it doesn't, right? I needed to use color: transparent in order to let the background show up.

From an author's point of view, I'd assume the usual use-case for background-clip:text is to apply a special "fill" such as a gradient or image to some text, instead of a solid color. The "background" isn't really being thought of as a background in this context; it's being used as a fill for the (foreground) text. As such, it makes sense to me that the selection would be painted behind this special-filled text, just as it paints behind normal solid-filled text.

If CSS were to add some more direct way to specify a complex fill (gradient or image) as a "foreground color" for text (e.g. an enhanced version of the color property), then authors could use this to change the text fill without affecting the painting order. Then I'd agree with the argument that background-clip:text should paint behind the selection (because it's a background, it just happens to be clipped). But if CSS isn't going to do that, and the long-term position is that background-clip:text is THE way to achieve the effect of "color: <gradient>", then I feel that such text should paint in front of the selection.

I disagree, at least looks inconsistent given we still paint the text on top of the background regardless. Given we paint the text on top unless the author specifies color: transparent, and that (and painting the selection on top) is what other browsers do as well, I think we should paint the selection on top.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1516361 - Remove code for painting text selection behind the background when background-clip:text is set. r?jfkthame,emilio

Comment 11

[Jonathan Kew [:jfkthame]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #9)

Well, it doesn't, right? I needed to use color: transparent in order to let the background show up.

Oh, right - sorry, I was forgetting that this has to be explicitly set by the author. That does make it more reasonable to go this way.

(Personally, I still think our current behavior of painting selection behind the background-filled text gives a better-looking result for examples such as those linked from https://bugzilla.mozilla.org/show_bug.cgi?id=1269971#c11, because I think the user's perception of the effect is that (foreground) text is being filled with an image/pattern/gradient, not that they're viewing a text-shaped piece of background. But given that the spec doesn't actually let the author specify "fill foreground text with...", and nobody else implements selection-painting this way, I guess it makes sense for us to simplify things here and match other browsers.)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/43a795c02325
Remove code for painting text selection behind the background when background-clip:text is set. r=jfkthame,emilio

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/43a795c02325

Comment 14

[Darkspirit]

(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #1)

When opening that page by clicking on the link the title box is empty until I hit F5. Fine with WR disabled.

Win10/GTX1060 + Debian Testing/Macbook Pro
This is not fixed.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Can you file a separate bug for that?