Closed
Bug 1351359
Opened 8 years ago
Closed 8 years ago
[css-grid] Make nsGridContainerFrame call ReflowInput::SetBResize(true) because of auto-block-size swapping between measuring reflows and regular reflows
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: dbaron, Assigned: dbaron)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
8.95 KB,
patch
|
MatsPalmgren_bugz
:
review+
gchang
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
This fixes the failure of
layout/reftests/css-grid/grid-min-max-content-sizing-002.html with the
primary patch in bug 1308876 (which causes a child whose parent is dirty
to pick up the dirty bit from the parent only the first reflow of the
child if the parent reflows the child multiple times). A simplified
testcase for that failure is
https://bugzilla.mozilla.org/attachment.cgi?id=8849771 .
The failure was caused by an error in height calculation of the first
<x> in the test. The div that is the parent of that x has a definite
height (presumably due to rules in grid), and the x has a specified
height. The div gets three reflows: two measuring reflows (from
MinContentContribution and then from MaxContentContribution) and then a
final reflow from nsGridContainerFrame::ReflowInFlowChild. Prior to the
primary patch in this bug, the div was marked dirty on all three
reflows, but with it it is marked dirty only on the first. This means
that, without the block-resize flag, the div optimizes away the reflow
of its children, since ShouldReflowAllKids returns false because
IsBResize() is false, even though NS_FRAME_CONTAINS_RELATIVE_BSIZE is
correctly set.
In order to fix this, we need to make sure the BResize flag on the
reflow state in at least some cases (see the comments in the patch for
when, and for how the cases could be optimized in the future).
Note that:
* when the dirty bit is set on the grid container, the new behavior
(with the combination of the patches) is strictly more efficient than
the old, since we will sometimes do non-dirty reflows on the grid
items (with the b-resize flag)
* when the dirty bit is *not* set on the grid container, the new
behavior is less efficient than the old, since we will set the
b-resize flag when we did not do so before. However, this slowdown
almost certainly fixes existing bugs.
Given that I was able to construct a reftest that triggers the failure
without the changes from bug 1308876, I've moved this to a separate bug.
Without the patch, grid-measuring-reflow-resize-dynamic-001.html fails,
but grid-measuring-reflow-resize-static-001.html passes. With the patch
both tests pass. (And without the patch, doing a text zoom on the
dynamic test fixes the layout error.)
MozReview-Commit-ID: JQOdVTQIkU0
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8852058 -
Flags: review?(mats)
Assignee | ||
Comment 2•8 years ago
|
||
Here's a try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d9894c92eb0de715d503f953be301a26953ae54&group_state=expanded
and here's a try push with only the tests and not the patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d48c58bfbbb82efcaa6b6790d3fba21db6a30126&group_state=expanded
to check that the test fails as expected without the patch.
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to David Baron :dbaron: ⌚️UTC-4 from comment #0)
> Given that I was able to construct a reftest that triggers the failure
> without the changes from bug 1308876, I've moved this to a separate bug.
> Without the patch, grid-measuring-reflow-resize-dynamic-001.html fails,
> but grid-measuring-reflow-resize-static-001.html passes.
I *believe* that the patch from bug 1308876 would cause static-001 to fail as well, without this patch. That's essentially the regression that I detected with the existing test.
Assignee | ||
Comment 4•8 years ago
|
||
The try runs from comment 2 produced the expected results. Things passed with the patched. But with only the tests and not the patch, layout/reftests/css-grid/grid-measuring-reflow-resize-dynamic-001.html == layout/reftests/css-grid/grid-measuring-reflow-resize-001-ref.html failed (in R5, Ru5, R5-e10s, and Ru5-e10s).
Comment 5•8 years ago
|
||
Comment on attachment 8852058 [details] [diff] [review]
Make nsGridContainerFrame call ReflowInput::SetBResize(true) because of auto-block-size swapping between measuring reflows and regular reflows
Thanks for fixing this. r=mats
Attachment #8852058 -
Flags: review?(mats) → review+
Assignee | ||
Comment 6•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed0b77acc612e03c89aa77db3c53ba0a96743a51
Bug 1351359 - Make nsGridContainerFrame call ReflowInput::SetBResize(true) because of auto-block-size swapping between measuring reflows and regular reflows. r=mats
Comment 7•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 8•8 years ago
|
||
Comment on attachment 8852058 [details] [diff] [review]
Make nsGridContainerFrame call ReflowInput::SetBResize(true) because of auto-block-size swapping between measuring reflows and regular reflows
Approval Request Comment
[Feature/Bug causing the regression]: CSS Grid
[User impact if declined]: broken grid layout in some cases
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:no
[Why is the change risky/not risky?]:the changes are simple and clearly only affects grid layout
[String changes made/needed]:none
I have a couple of other Grid layout bug fixes in the pipeline that
I intend to uplift to v54 and it seems it would be less risky to do
those changes if we also uplift this change first.
Attachment #8852058 -
Flags: approval-mozilla-aurora?
Updated•8 years ago
|
Blocks: css-grid
Summary: Make nsGridContainerFrame call ReflowInput::SetBResize(true) because of auto-block-size swapping between measuring reflows and regular reflows → [css-grid] Make nsGridContainerFrame call ReflowInput::SetBResize(true) because of auto-block-size swapping between measuring reflows and regular reflows
Updated•8 years ago
|
status-firefox54:
--- → affected
Comment 9•8 years ago
|
||
Comment on attachment 8852058 [details] [diff] [review]
Make nsGridContainerFrame call ReflowInput::SetBResize(true) because of auto-block-size swapping between measuring reflows and regular reflows
Fix a broken css grid layout. Aurora54+.
Attachment #8852058 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 10•8 years ago
|
||
bugherder uplift |
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•