background-clip: text is not properly invalidated
Categories
(Core :: Web Painting, defect, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox-esr60 | --- | unaffected |
| firefox66 | --- | wontfix |
| firefox67 | --- | verified |
| firefox68 | --- | verified |
People
(Reporter: jrmuizel, Assigned: emilio)
References
Details
Attachments
(4 files, 1 obsolete file)
This is a reduced test case from bug 1524801 - https://events.ccc.de/congress/2018/wiki/index.php
|
Reporter | |
Comment 1•6 years ago
|
||
"Main Page" is not painted on load. You need to switch tabs and switch back for it to show up.
|
|
Reporter | |
Updated•6 years ago
|
|
||
Comment 2•6 years ago
|
||
I ran mozregression and this has been broken (at least) since FF 50.
|
||
Comment 4•6 years ago
|
||
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.
|
|
Reporter | |
Comment 5•6 years ago
|
||
What mechanism should turn the requested Reflow into a frame invalidation?
|
Assignee | |
Comment 6•6 years 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 | |
Updated•6 years ago
|
|
|
Assignee | |
Updated•6 years ago
|
|
|
Assignee | |
Comment 9•6 years ago
|
||
|
|
||
Comment 11•6 years ago
•
|
||
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?
|
|
||
Comment 12•6 years ago
|
||
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•6 years 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.
|
|
||
Comment 14•6 years ago
|
||
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.
|
|
||
Comment 15•6 years ago
|
||
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?
|
|
||
Comment 16•6 years ago
|
||
|
||
Comment 19•6 years ago
|
||
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?
|
|
||
Comment 20•6 years ago
|
||
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.
|
||
Comment 22•6 years 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•6 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/d0f776caeac8
https://hg.mozilla.org/mozilla-central/rev/17b9bc032bc9
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16003 for changes under testing/web-platform/tests
Upstream PR merged
|
|
Reporter | |
Comment 26•6 years ago
|
||
Is it worth uplifting this for WebRender's sake?
|
|
||
Comment 27•6 years ago
|
||
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
|
||
Updated•6 years ago
|
|
||
Updated•6 years ago
|
|
|
||
Comment 28•6 years ago
|
||
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.
|
|
||
Comment 29•6 years ago
|
||
| bugherder uplift | ||
https://hg.mozilla.org/releases/mozilla-beta/rev/e4ee5e511a6f
https://hg.mozilla.org/releases/mozilla-beta/rev/77a1f36d6cad
|
||
Updated•6 years ago
|
|
||
Updated•6 years ago
|
|
||
Comment 30•6 years ago
|
||
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.
|
|
||
Updated•6 years ago
|
Description
•