1782195 - {inc} Grid with only one image exports broken baseline on incremental reflow

Status: NEW

(Core :: Layout: Grid, defect, P3)

Description

[Ator [:ator]]

Attachment: minimal-reproduction.html

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Firefox/102.0

Steps to reproduce:

1. Create a specific HTML+CSS element layout (attached minimally as HTML). Replace the <img> [src] value with a valid image path.
2. Open the Developer Tools; if already open, close them first. Do not hover over the DOM tree in the Inspector.
3. a) For an arbitrary separate element affecting the layout, set its display to anything apart from "none", then to "none" leaving time for Firefox to recalculate positions.
   b) Use the console to select the element (without triggering Firefox to highlight its position on the page, so write this code e.g. in a comment initially or paste in) to get the client Rect of its computed style.
4. a) Hover over the element in the Inspector DOM tree.
   b) Repeat step (3b).

Actual results:

On step (3a), the element disappeared. If more elements are used, it may be observed that they do not move to fill in the visual horizontal gap, but act as if the element did not exist vertically.
On step (3b), its client Rect had unexpected invalid values. In my case:
{
"x": 0,
"y": 17895698,
"width": 20,
"height": -8947863,
"top": 8947835,
"right": 20,
"bottom": 17895698,
"left": 0
}
On step (4a), the element reappeared (and any associated layout differences were reversed).
On step (4b), its client Rect had expected values. In my case:
{
"x": 0,
"y": 0,
"width": 20,
"height": 20,
"top": 0,
"right": 20,
"bottom": 20,
"left": 0
}

Expected results:

The observed results of steps 3 (bug) and 4 (normal) should not have differed. Firefox has no discernable reason to change the layout or hide the affected element in this case.

Note that this is about as minimal as I can make the test. Almost any change will invalidate it, and all CSS is there for a specific reason. However, I believe any element except <img> can be changed to a different element type without affecting the test. If [width] is also set explicitly for <img> the test yields normal results.

Comment 1

[Ator [:ator]]

Reproduction code for your convenience:

(e => {
	e.style.display = "block";
	setTimeout(() => {
		e.style.display = "none";
	}, 100);
})(document.children[0].children[1]);

Comment 2

[BugBot [:suhaib / :marco]]

The Bugbug bot thinks this bug should belong to the 'DevTools::Inspector' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 3

[BugBot [:suhaib / :marco]]

The severity field is not set for this bug.
:jdescottes, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 4

[Julian Descottes [:jdescottes]]

Moving back to triage

Comment 5

[Julian Descottes [:jdescottes]]

I can't reproduce this anymore.

Comment 6

[Ator [:ator]]

:jdescottes, thanks for testing. While I can't reproduce it anymore using the steps I described, I found two easier ways:

• Resize the window (make sure the width changes; height doesn't affect it.)
• Keep increasing the body's margin-block, or margin-top, or margin-bottom. As soon as vertical overflow would occur (i.e. the scrollbar would appear), the bug happens.
  • Alternatively, keep increasing the body's padding-block (etc) similarly. Note that this has a slightly different effect in that the bug goes away if you increase the padding again after it occurs.
  • Alternatively, keep increasing the height of the devtools. This has the same effect as increasing the margin.

Result: The element disappears, gaining those weird values (if you use element.getClientRects()), and the page becomes incredibly tall - to the point that using the scrollbar causes a flashing effect at the bottom of the screen.

To put it back to normal, you can change the padding values or set the body's display to none and back again.

Comment 7

[Julian Descottes [:jdescottes]]

Indeed! Thanks a lot for the reduced STRs.

New STRs:

• open https://bug1782195.bmoattachments.org/attachment.cgi?id=9287623
• open devtools (docked to the bottom preferably, separate window doesn't trigger the issue)
• resize the width of the browser window

At some point the "broken image" element disappears and a very tall scrollbar appears

Comment 8

[Julian Descottes [:jdescottes]]

It also doesn't seem to repro if devtools are started on the inspector.

Comment 9

[Julian Descottes [:jdescottes]]

After trying to reproduce in triage, this doesn't even require devtools. Moving to Core.

STRs without devtools:

• open https://bug1782195.bmoattachments.org/attachment.cgi?id=9287623
• resize the window both vertically and horizontally (might have to do it a few times)

The scrollbar should then appear and the image element is gone.

Comment 10

[Emilio Cobos Álvarez [:emilio]]

Attachment: Test-case with a real image

(So it's easier to bisect on older builds that don't show the broken icon in this case)

Comment 11

[Emilio Cobos Álvarez [:emilio]]

Huh, really old bug. mozregression says: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=567894f026558e6dada617a3998f29aed06ac7d8&tochange=5b0afeaeebdd3a60e1885912cda2e48a9233be52

Most likely bug 1312379... It seems we're probably exporting a bogus baseline from the grid container, when doing an incremental reflow, somehow?

Tiaan, you've looked at a lot of grid baseline-related stuff, maybe you have a good idea of what's going on?

Comment 12

[BugBot [:suhaib / :marco]]

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

Comment 13

[BugBot [:suhaib / :marco]]

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

Comment 14

[Daniel Holbert [:dholbert]]

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

It seems we're probably exporting a bogus baseline from the grid container, when doing an incremental reflow, somehow?

Most likely, what's happening is:

• When you resize the window, we're calling nsGridContainerFrame::MarkIntrinsicISizesDirty which resets our cached baseline values to NS_INTRINSIC_ISIZE_UNKNOWN, here:
  https://searchfox.org/mozilla-central/rev/b63f8c50a3398aff80d2a5f185a90dcdba47f419/layout/generic/nsGridContainerFrame.cpp#10342,10346
• Then we do an incremental reflow, and we call one of the baseline getters before we manage to reflow the grid again (possibly because we're able to recompute the grid's marked-as-dirty intrinsic sizes -- and aside from those, the inline-block doesn't think the grid needs to be reflowed and so doesn't call its reflow method)
• ...and so that baseline getter returns the bogus NS_INTRINSIC_ISIZE_UNKNOWN value which is nscoord_MIN, a giant negative number.

To fix this, we probably need to ensure that the grid always gets a reflow after MarkIntrinsicISizesDirty has been called, in order to recompute the baseline offsets; or, we need to let our baseline-offset getters have a fallback codepath that's able to dynamically compute the baseline by walking the persistent data that we built up in the last reflow.

Comment 15

[Daniel Holbert [:dholbert]]

Arguably we could also paper-over this somewhat by modifying this function to check the GetBBaseline return-value and return Nothing{} if it happens to be NS_INTRINSIC_ISIZE_UNKNOWN:
https://searchfox.org/mozilla-central/rev/b63f8c50a3398aff80d2a5f185a90dcdba47f419/layout/generic/nsGridContainerFrame.h#130-137

Maybe<nscoord> GetNaturalBaselineBOffset(
    mozilla::WritingMode aWM, BaselineSharingGroup aBaselineGroup,
    BaselineExportContext) const override {
  if (StyleDisplay()->IsContainLayout() ||
      HasAnyStateBits(NS_STATE_GRID_SYNTHESIZE_BASELINE)) {
    return Nothing{};
  }
  return mozilla::Some(GetBBaseline(aBaselineGroup));

(If we start returning Nothing for this edge case, it wouldn't be entirely correct there -- it indicates "there is no natural baseline", when in fact the reality is that there is a baseline and we don't know what it is. But at least the "there-is-no-baseline" signal will prompt the parent to synthesize one from the grid's border-box in most cases, which is a more graceful failure-case than using a giant negative number.)