2023621 - Transformed content not initially rendered

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jake Archibald [:jakea]]

https://random-stuff.jakearchibald.com/bug-repros/low-res-transform/

Initially, this page renders a grey square. When you hit "invalidate" styles, it sets an element to display:none, waits a frame, then reverts to display:block. The element then appears.

The element should have been there all along.

Comment 1

[Daniel Holbert [:dholbert]]

Regression range:

INFO: Last good revision: 0bf6bf2b89215ce4dfa2eec4b0f33414c25fb677 (2024-05-16)
INFO: First bad revision: f35859c2fd56665ad31bcc859c08cca0d52ee800 (2024-05-17)
INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0bf6bf2b89215ce4dfa2eec4b0f33414c25fb677&tochange=f35859c2fd56665ad31bcc859c08cca0d52ee800

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (simplified version of Jake's original testcase)

Here's a reduced testcase.

As noted in a code-comment within the testcase: the lime element doesn't render with the included styles, particularly transform: translateX(100px) translateY(140px) translateZ(550px); -- but if I reducuce translateY to 137px, then it does render properly.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: reference case 1 (positioning slightly different, which avoids the issue)

Here's a not-quite-reference-case where I've reduced the translateY value by a few pixels. On my machine at least, this makes us render the element that would otherwise be missing.

Comment 4

[Daniel Holbert [:dholbert]]

Given that this is using 3d transformations (perspective and translateZ) with specific dependency on the exact pixel values, it's likely that this is a bug in WebRender (or somewhere in painting) rather than in layout. (I'm guessing that some of our painting code is incorrectly assuming that the shape is fully occluded/clipped and hence doesn't need to be drawn.)

The only graphics-related change that I'm seeing in the regression range is bug 1750348 (which does mention invalidation and scaling, so it's believable that there's a connection). So for now I'm assuming this was a regression from that bug.

--> Reclassifying and marking as regression from bug 1750348.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1750348

:yannis, since you are the author of the regressor, bug 1750348, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 6

[Yannis Juglaret [:yannis]]

Thanks :dholbert for the ping. I disagree about the regressor. I suppose that you used the test case from comment 0 for testing the regression, but with the simplified test case from comment 2, I can reproduce the issue in e.g. Firefox 115.0.3. This suggests that the real underlying issue is older than the regression range, and most likely something in the regression range just changed the ways in which the real bug can manifest. Nonetheless, your reduced test case was super helpful as a starting point and I have a patch to offer.

Comment 7

[Yannis Juglaret [:yannis]]

Attachment: 115.33.0esr.png

Test case from comment 2 reproduces the issue in ESR115 115.33.0esr (tested locally on Windows). As for the original test from comment 0, on ESR115 we have a different problem with the image being displayed way too large, see attachment. Not sure if we want to do something about that.

Comment 8

[Yannis Juglaret [:yannis]]

Reflecting :dmeehan's choice for esr140 on esr115.

Comment 9

[Yannis Juglaret [:yannis]]

Attachment: WIP: Bug 2023621 - Recompute perspective children overflow during reflow and update line overflow areas. r?emilio

During reflow, children may be reflowed before the perspective frame is
sized, so their overflow is computed with the wrong perspective-origin.
Previously, RecomputePerspectiveChildrenOverflow was only called when the
frame's size changed, but this missed the common case where mRect was
already set to the final size before FinishAndStoreOverflow ran.

Fix this by gating on NS_FRAME_IN_REFLOW instead of sizeChanged. This
ensures we always recompute during reflow (when children may be stale)
while avoiding redundant work from OverflowChangedTracker. Non-reflow
callers (UpdateOverflow, OverflowChangedTracker::Flush) always pass
aNewSize == mRect.Size(), so sizeChanged was always false for them and
perspective recomputation was never triggered outside reflow anyway.

Additionally, after recomputing children's overflow, update cached line
overflow areas on block frames. Without this, the display list builder
uses stale line overflow to skip lines, causing perspective-transformed
children to not be painted.

RecomputePerspectiveChildrenOverflow now returns bool to propagate
whether any child overflow changed through recursive calls into
wrapper/anonymous frames, ensuring the outer caller's UnionChildOverflow
is triggered when needed.

Skip the overflow area recollection for scroll containers to avoid
reflow-during-reflow assertions; they manage overflow through scrolling
and already call RecomputePerspectiveChildrenOverflow separately in
ScrollToImpl.

Comment 10

[Yannis Juglaret [:yannis]]

Attachment: bug-2023621-blogpost.html

Please find attached an AI-generated blogpost that explains the issue and the WIP patch, with JavaScript visualizations to ease understanding. However, I'm giving back the assignee token because this issue is way outside my comfort zone and even with the blogpost available I cannot claim that I understand the patch. Feel free to use the WIP patch if it helps, or to start fresh otherwise.

Comment 11

[Emilio Cobos Álvarez [:emilio]]

Sorry, I hadn't seen the phab ping... So the patch looks roughly right (the issue being that the line overflow areas don't get updated for perspective if we're clipping).

However, I think we should probably take a less invasive patch... Calling UnionChildOverflow during reflow again is pretty sketchy...

I don't think we use line overflow areas for any other post-reflow effect than this display list optimization, so maybe something like just omitting ShouldDescendInto with perspective is simpler.

Comment 12

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 2023621 - Fix line scrollable frame overflow with perspective. r=tnikkel,#layout

The optimization where we avoid UpdateOverflow if clipped on both axes
is not sound, because it has side effects like updating nsBlockFrame's
line overflow.

Also, make the recursive path update overflows of anonymous boxes like
the scrolled frame.

Comment 13

[Yannis Juglaret [:yannis]]

Thanks :emilio!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/7619a268a589
https://hg.mozilla.org/integration/autoland/rev/a7ae9ac3a9b5
Fix line scrollable frame overflow with perspective. r=layout-reviewers,dshin

Comment 15

[amarc]

https://hg.mozilla.org/mozilla-central/rev/a7ae9ac3a9b5

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/58761 for changes under testing/web-platform/tests

Comment 17

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:emilio, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox150 to wontfix.

For more information, please visit BugBot documentation.

Comment 19

[Emilio Cobos Álvarez [:emilio]]

Very old issue, can probably ride the trains.