Closed Bug 1525372 Opened 2 years ago Closed 2 years ago

background-clip: text is not properly invalidated

Categories

(Core :: Web Painting, defect, P3)

defect

Tracking

()

VERIFIED FIXED
mozilla68
Tracking Status
firefox-esr60 --- unaffected
firefox66 --- wontfix
firefox67 --- verified
firefox68 --- verified

People

(Reporter: jrmuizel, Assigned: emilio)

References

Details

Attachments

(4 files, 1 obsolete file)

Attached file Reduced test case

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

Flags: needinfo?(matt.woodrow)
Blocks: 1524801

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

Priority: -- → P3

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)

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

Flags: needinfo?(matt.woodrow)

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

A bit simpler. We also fail to invalidate.

And no font loads around.

Attachment #9042186 - Attachment mime type: text/plain → text/html
Assignee: nobody → emilio
Summary: background-clip: text is not properly painted on font-load → background-clip: text is not properly invalidated

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's sibling 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/

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)

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. :/

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
Status: NEW → RESOLVED
Closed: 2 years ago
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

Is it worth uplifting this for WebRender's sake?

Flags: needinfo?(matt.woodrow)

Comment on attachment 9044542 [details]
Bug 1525372 - Add background-clip:text rendering observer so that we get notified of changes to the clipped contents. r?jwatt

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: -
  • User impact if declined: Invalidation has always been broken for background-clip:text, but usually renders correctly due to a coincidence. WebRender doesn't have the same code, so hits the bug more frequently.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Very simple patch, makes use of an existing mechanism to notify when areas need to be repainted.
  • String changes made/needed:
  • Do you want to request approval of these patches as well?: on
Flags: needinfo?(matt.woodrow)
Attachment #9044542 - Flags: approval-mozilla-beta?
Attachment #9042203 - Attachment is obsolete: true

Comment on attachment 9044542 [details]
Bug 1525372 - Add background-clip:text rendering observer so that we get notified of changes to the clipped contents. r?jwatt

Fix for a painting correctness issue which will become more visible with WebRender, approved for 67 beta 5, thanks.

Attachment #9044542 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

Hi, I have retested this issue and I was able to reproduce it in older versions of Firefox (the reduced test cases helped) and I can no longer reproduce the issue in Firefox Beta 0b5 or Nightly 68.0a1 (2019-03-26). I will mark this issue accordingly.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Whiteboard: [qa-triaged]
See Also: → 1543117
Regressions: 1556441
You need to log in before you can comment on or make changes to this bug.