1735931 - {inc} inconsistent height for percent-sized item in grid area, with `layout.css.grid-item-baxis-measurement.enabled` enabled

Status: RESOLVED DUPLICATE of bug 1473789

(Core :: Layout: Grid, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STR:

1. Ensure you have about:config pref layout.css.grid-item-baxis-measurement.enabled set to true
2. Load attached testcase.
3. Open web console (ctrl shift k, or cmd shift k)
4. Type "simpleRecreate()"

EXPECTED RESULTS:
The same value should be logged for three.height

ACTUAL RESULTS:
Two different values are logged. For me, I get:

After style changes, before reload
#three.height 819.2999877929688
After reload
#three.height 890.5499877929688

The first measurement here is just coming from an incremental-layout codepath (with a dynamic tweak to padding), whereas the second measurement is using the same final DOM and styling laid out in one shot (no dynamic restyles).

This testcase was generated by the Layout Quick Check fuzzer (with minor edits/simplification from me); see bug 1724999.

Comment 1

[Daniel Holbert [:dholbert]]

As in bug 1735376 comment 3: this is only a regression in Nightly. This is caused by bug bug 1591366's optimization, which is pref-controlled and is preffed off for all non-Nightly release channels for the time being, until we take action in bug 1732082 to let it ride the trains.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

Here's another testcase that was independently generated by the LQC fuzzer, but I suspect it's the same root bug (since it's about an element with vertical-lr inside of a grid item).

For this one, simpleRecreate() gives me the following output (indicating that two elements have changed height from 972 to 968 despite the DOM/styling not changing):

simpleRecreate()
Dimensions after style changes, before reload
#two 972
#four 972

Dimensions after reload
#two 968
#four 968

Comment 3

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

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

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 3

Here's one more testcase, generated by LQC with light edits by me, which I suspect is a variant of this same issue, since it's also got a writing-mode dependency.

(I'm not entirely sure it's precisely the same hence, I'm attaching it here, and we can spin off separate bugs as-needed for any testcases that remain broken when we're ready to land a patch here.)

As with the others, the STR are to run simpleRecreate() in your console and note the discrepancy in logged values -- e.g. here's what I see:

Height after style changes, before reload
#three 1952
Height after reload
#three 968

If I disable the optimization pref, then I get 968 for both measurements.

Comment 5

[Sean Feng [:sefeng]]

Attachment: Bug 1735931 - Don't use cached measurement of grid items when descendants have relative BSize r=dholbert

It seems that in some cases we shouldn't use the cached measurement when
having descendants which have percentage block-size that depends on the
ancestor of the grid items because the descendants expect an reflow with
indefinite container BSize so that the percentages shouldn't be
resolvable. However, if we decide to use the cached measurement,
we end up reflowing the descendants with a definite container BSize.

Comment 6

[Daniel Holbert [:dholbert]]

So, bad-news & good-news:

I poked at this a bit more, and I think this is like bug 1735376, in that it's actually a case of a preexisting bug that's rearing its head in a new way, where our layout is accidentally/unintentionally stateful. (Specifically, the preexisting bug here is bug 1473789 or something similar.) It's a situation where we get the wrong result (the frame lies about its content-size) if we reflow it once, but we end up getting the right result if we reflow it again. Or something to that effect. So, the grid optimization (which reduces the number of times we reflow a frame) ends up causing an observable change, in particular specially-crafted testcases.

The attached patch did indeed avoid the issue for the attached testcases, but only by turning off the optimization for particular classes of percent-height content (i.e. it's equivalent to preffing off the optimization), which are classes of content that we shouldn't actually have to turn off the optimization for. (Percent-height stuff does indeed require special handling because we need to consider whether the percent basis is definite or not; but in this case where we're doing content measurements, I think the percent basis will always be indefinite.)

In any case: during some investigation today, I came up with variants of the attached testcases where we end up producing wrong/inconsistent behavior, even with the grid optimization turned off, which demonstrates that the root issue is unrelated to that optimization, and where the issue would not be fixed by the attached patch.

So: sorry for this turning out to be a bit of a goose-chase, but the good news is we don't actually need to do anything here, at least not specific to the grid optimization. Fortunately, combinations of orthogonal writing-modes are not super-common on the web.

For now, I'm going to dupe this against bug 1473789.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #6)

In any case: during some investigation today, I came up with variants of the attached testcases where we end up producing wrong/inconsistent behavior, even with the grid optimization turned off, which demonstrates that the root issue is unrelated to that optimization, and where the issue would not be fixed by the attached patch

Sorry, actually my new testcase variants do produce the correct behavior with the grid optimization turned off. I meant to say they produce the wrong behavior (in Nightly) in both the incremental-layout path ("height after style changes") as well as the rebuild-DOM-from-scratch layout path ("height after reload"); and then only with a further layout tweak do they end up producing the correct result.

I think this is still bug 1473789 at the core, though (where reducing the number of redundant reflows is un-papering-over the fact that the initial reflow produces the wrong results in the attached testcase).

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: testcase 4

STR with this testcase:
(1) Load testcase, watch the dynamic tweak (after 300ms).
(2) Click the button.
(3) Check your web console.

ACTUAL RESULTS:
After step 1 (after the automatic dynamic tweak), the rendering has a red strip above the tall green area.
After step 2 (when you click the button), that red strip goes away.
In step 3, your web console shows a different final logged result (436, 436, 20)

EXPECTED RESULTS:
After the automatic dynamic tweak, there should be no red strip.
Step 2 should have no effect on the layout of the page (since it just makes & undoes a temporary change)
And it should have no effect on the logged getBoundingClientRect().height value -- console.log should show the same value (20) repeated 3 times.

Nightly (with the layout.css.grid-item-baxis-measurement.enabled optimization default-enabled) gives ACTUAL RESULTS; if I turn off the pref, I get EXPECTED RESULTS. But as discussed above, I think this is just because the optimization is uncovering an instance of bug 1473789, where we happen to be caching bad data from our first reflow with an orthogonal writing mode.

Comment 9

[Daniel Holbert [:dholbert]]

Note: we will need something similar to this patch, if & when we add an optimization to skip a grid item's "final" reflow if we already reflowed to measure content and that reflow left us at the correct size. That's the point at which we would need to consider skipping the hypothetical optimization (and re-laying-out the contents), even if the size didn't change, due to the fact that the size is now considered definite & will impact percent-resolution on the descendants. That's where we currently have to take this into consideration, in nsFlexContainerFrame.

But right now, we don't have that particular optimization in grid.