background-clip: text is not properly invalidated

RESOLVED FIXED in Firefox 68

Status

()

P3
normal
RESOLVED FIXED
2 months ago
8 hours ago

People

(Reporter: jrmuizel, Assigned: emilio)

Tracking

(Blocks: 1 bug)

unspecified
mozilla68
Points:
---

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(5 attachments)

(Reporter)

Description

2 months ago
(Reporter)

Comment 1

2 months ago
Posted file Reduced test case

"Main Page" is not painted on load. You need to switch tabs and switch back for it to show up.

(Reporter)

Updated

2 months ago
Flags: needinfo?(matt.woodrow)
(Reporter)

Updated

2 months ago
Blocks: 1524801

I ran mozregression and this has been broken (at least) since FF 50.

Priority: -- → P3
(Reporter)

Comment 3

2 months ago

I wouldn't be surprised if it never worked.

I can't see any mention of invalidation in the initial implementation bug, or any code that would handle it.

With non-WebRender we sometimes still get a display item for the text (text color isn't transparent, text has decorations, text is in SVG, shadows, selection), and invalidations to that should usually invalidate the right Layer pixels.

Flags: needinfo?(matt.woodrow)
(Reporter)

Comment 5

a month ago

What mechanism should turn the requested Reflow into a frame invalidation?

Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 6

a month ago

I assume nsFontFaceUtils::MarkDirtyForFontChange should also check for background-clip: text and invalidate the background if so. I can do that if you want.

(Assignee)

Comment 7

a month ago

A bit simpler. We also fail to invalidate.

(Assignee)

Comment 8

a month ago

And no font loads around.

(Assignee)

Updated

a month ago
Attachment #9042186 - Attachment mime type: text/plain → text/html
(Assignee)

Updated

a month ago
Assignee: nobody → emilio
Summary: background-clip: text is not properly painted on font-load → background-clip: text is not properly invalidated
(Reporter)

Comment 10

a month ago

Review ping

Sorry, I mentioned this on irc to emilio but didn't document it here.

We can probably take this patch, since it's strictly better than what we have, but it still has bugs where we don't invalidate.

The current invalidation model is that explicit invalidations are marked on the frame, and reflow position/size changes are detected by DLBI geometry comparison. This patches handles the former, but not the latter.

This could happen if an ancestor-size changes size, which causes the ancestor of the text to be reflowed into a new position. This changes the resulting position of the text within the mask, and we need to invalidate, but nothing on either the text frame or background frame changed, and we wouldn't detect it.

I'm not sure if there's a great solution to fix this, but I'll think about this more.

Is there an urgent rush to fix this, the bug has existed for a while?

Flags: needinfo?(matt.woodrow)

I had a bit of an play around with this, and it looks like we try a lot harder to find and paint text children than WebKit/blink do.

See https://jsfiddle.net/jqsp4f6n/7/

It appears that wrapping the text in any property that creates a (pseudo-)stacking context results in the text not being found for clipping. We don't do this, and will happily create a clip mask that includes positioned and transformed text (though we do ignore filters).

We also clip to things that aren't text - https://jsfiddle.net/jqsp4f6n/8/

(Reporter)

Comment 13

a month ago

I suspect that moving towards the WebKit/blink behaviour is what we'll need to do for webcompat and hopefully it can give us a simpler implementation.

I wonder if we could implement this better by ensuring that we always create nsDisplayText display items if they're participating in background-clip:text (mark the state on the DL builder when we descend through the frame with the property, clear on exit and when descending through a SC or positioned frame), even if the text colour is transparent (we already do this for other reasons including the presence of decorations/selections).

I think we could then render the background-clip:text background item by looking forward in the display list for nsDisplayText items that are descendants (again, we could flag these for quick lookup) and calling Paint on them with our mask surface. We'd skip over any container items, and match WebKit/blink's behaviour.

We'd probably also try to render the actual text, but it's transparent, so should quickly be a no-op.

That would then mean the display items exist within FLB's DLBI tracking, such that any changes would result in an invalidation to their Layer. I think that should usually be the same Layer as the clipped background, but we could do better and figure out that the background item is dependent on the text, and forward the invalidation there too.

WebRender also tracks the equivalent to DLBI tracking data, so it seems like we should be able to opt-in to doing that for text items that contribute to a clipped background item, and make sure we check all dependencies.

The invalidation side is very hand-wavy right now, I'll think through it some more.

Actually, it might be easier to just use SVG rendering observers. This isn't really SVG, but we use them for -moz-element too.

SVG rendering observers allow one element to watch another, and get notified when anything in the observed element's frame subtree changes. We fire these for anything that schedules a paint, including reflowing any frame in the subtree.

I think we could use SVGMozElementObserver (or something similar) to let frames with background-clip:text observe themselves, and then we'd get notified when any of our descendants change.

It's a lot less granular (just a bool notification), and fires when anything changes, not just things that affect the text clip mask, but that's probably ok. We do the same for filters/masks, and over-invalidation hasn't been a big issues.

Does that sounds reasonable to you jwatt?

Flags: needinfo?(jwatt)
(Reporter)

Comment 18

25 days ago

jwatt review ping

Having SVG observers observe reflow is something I was hoping to get rid of in bug 1496065. Marking frames as needing invalidation at reflow time (when we may have finished reflowing/updating overflow for those frames) is broken and has caused annoying bugs in the past.

Comment 14 sounds reasonable to me as an alternative approach. What are your current thoughts on that approach, Matt?

Flags: needinfo?(jwatt) → needinfo?(matt.woodrow)

It's mainly just a bunch of work, since it's effectively rewriting the whole background-clip:text implementation from scratch.

The actual invalidation changes required seem somewhat tricky. All the information we need would be contained within the display list, but we'd have to setup dependencies so that the invalidation of the background could check the invalidation state of the text item.

FLB at least doesn't guarantee any sort of ordering for computing invalidation of items (since it's iterating a hashtable), so whatever solution we do can't rely on that.

It seems like a lot of complexity (mainly managing the dependencies and lifetimes around that) and new code for a single use case.

Flags: needinfo?(matt.woodrow)

Okay, let's go with this for now. :/

Comment 22

4 days ago
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d0f776caeac8
Add background-clip:text rendering observer so that we get notified of changes to the clipped contents. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/17b9bc032bc9
Add WPT reftest for moving background-clip:text text contents without invalidating the text frame. r=emilio

Comment 23

3 days ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 3 days ago
status-firefox68: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16003 for changes under testing/web-platform/tests
You need to log in before you can comment on or make changes to this bug.