Closed
Bug 1278455
Opened 8 years ago
Closed 8 years ago
[css-grid][css-flexbox] "ASSERTION: Reference frame mismatch" with display:grid/flex
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: jruderman, Assigned: tnikkel)
References
(Blocks 1 open bug)
Details
(Keywords: assertion, testcase)
Attachments
(4 files, 1 obsolete file)
With: user_pref("layout.event-regions.enabled", true); ###!!! ASSERTION: Reference frame mismatch: 'aBuilder->FindReferenceFrameFor(aFrame) == aBuilder->FindReferenceFrameFor(mFrame)', file layout/base/nsDisplayList.cpp, line 3578
Reporter | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
Also reproduces in e10s mode (which turns on that pref by default).
Assignee | ||
Comment 3•8 years ago
|
||
There's actually a rendering difference between e10s and non-e10s with this testcase. The Z appears further down.
Assignee | ||
Comment 4•8 years ago
|
||
The absolute frame is a child of the canvas frame, which is outside of the transformed subtree (rooted at the GridContainer frame). Mats, do you know what is going on here?
Flags: needinfo?(mats)
Comment 5•8 years ago
|
||
It also occurs for display:flex, so maybe it has something to do with this: https://hg.mozilla.org/mozilla-central/annotate/ec20b463c04f/layout/generic/nsGridContainerFrame.cpp#l2212 https://hg.mozilla.org/mozilla-central/annotate/ec20b463c04f/layout/generic/nsFlexContainerFrame.cpp#l2134 Does adding "|| !aFrame->GetParent()" to the condition there fix it?
Flags: needinfo?(mats)
Summary: "ASSERTION: Reference frame mismatch" with display:grid → "ASSERTION: Reference frame mismatch" with display:grid/flex
Assignee | ||
Comment 6•8 years ago
|
||
I think the abs pos child should have a parent inside the transformed subtree (since transforms induce an abs pos containing block). So I think we are going wrong before we even get to painting.
Comment 7•8 years ago
|
||
OK. I'm not aware of any other exceptions for grid/flex off the top of my head.
Assignee | ||
Comment 8•8 years ago
|
||
I looked into this very shallow-ly. It seems we are missing a call to PushAbsoluteContainingBlock when the root element is grid or flex. ConstructBlock and ConstructTable do that. This gives me the expected frame tree.
Attachment #8761419 -
Flags: feedback?(dholbert)
Assignee | ||
Comment 9•8 years ago
|
||
FWIW green on try https://treeherder.mozilla.org/#/jobs?repo=try&revision=78fa1be59cfe&group_state=expanded
Comment 10•8 years ago
|
||
Comment on attachment 8761419 [details] [diff] [review] pushabsblock Review of attachment 8761419 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, I think this probably makes sense. ::: layout/base/nsCSSFrameConstructor.cpp @@ +2568,5 @@ > + > + nsFrameConstructorSaveState absoluteSaveState; > + newFrame->AddStateBits(NS_FRAME_CAN_HAVE_ABSPOS_CHILDREN); > + if (display->IsAbsPosContainingBlock(newFrame)) { > + state.PushAbsoluteContainingBlock(contentFrame, newFrame, absoluteSaveState); (Extreme nit: this line is too long; wrap after "newFrame," probably) @@ +2581,5 @@ > + > + nsFrameConstructorSaveState absoluteSaveState; > + newFrame->AddStateBits(NS_FRAME_CAN_HAVE_ABSPOS_CHILDREN); > + if (display->IsAbsPosContainingBlock(newFrame)) { > + state.PushAbsoluteContainingBlock(contentFrame, newFrame, absoluteSaveState); (Same here)
Attachment #8761419 -
Flags: feedback?(dholbert) → feedback+
Comment 11•8 years ago
|
||
Comment on attachment 8761419 [details] [diff] [review] pushabsblock Hmm, does this actually fix the bug? It seems odd because the nsFrameConstructorSaveState actually goes out of scope before we process the children for grid/flexbox, which happens in the "if (processChildren)" block further down. ~nsFrameConstructorSaveState restores the old state. Shouldn't this go inside the processChildren block instead, so that it's active during ProcessChildren() ?
Flags: needinfo?(tnikkel)
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #11) > Hmm, does this actually fix the bug? > It seems odd because the nsFrameConstructorSaveState actually goes out > of scope before we process the children for grid/flexbox, which happens > in the "if (processChildren)" block further down. > ~nsFrameConstructorSaveState restores the old state. > > Shouldn't this go inside the processChildren block instead, so that it's > active during ProcessChildren() ? I think you're right. But I did verify that it does in fact fix the frametree. The reason is that we don't seem to process the absolutely positioned div in the ProcessChildren call in ConstructDocElementFrame. We process it later during a flush. This does a ContentAppended call to create the frame, and it calls nsCSSFrameConstructor::GetAbsoluteContainingBlock to find the appropriate abs pos frame for the nsFrameConstructorState it creates. The gridcontainer frame has MarkAsAbsoluteContainingBlock called on it by the PushAbsoluteContainingBlock call I added. Without that call it isn't marked as an absolute containing block so nsCSSFrameConstructor::GetAbsoluteContainingBlock skips over it.
Flags: needinfo?(tnikkel)
Assignee | ||
Comment 13•8 years ago
|
||
Attachment #8761419 -
Attachment is obsolete: true
Attachment #8761889 -
Flags: review?(dholbert)
Comment 14•8 years ago
|
||
Comment on attachment 8761889 [details] [diff] [review] pushabsblock Review of attachment 8761889 [details] [diff] [review]: ----------------------------------------------------------------- r=me, just a few nits: ::: layout/base/nsCSSFrameConstructor.cpp @@ +2495,5 @@ > > // Make sure to start any background image loads for the root element now. > styleContext->StartBackgroundImageLoads(); > > + nsFrameConstructorSaveState parentAbsoluteSaveState; Maybe s/parent/root/? (or "rootFrame"?) I'm not sure what nesting-level "parent" is referring to here. I think it's meant to refer to the root frame? @@ +2570,5 @@ > + > + newFrame->AddStateBits(NS_FRAME_CAN_HAVE_ABSPOS_CHILDREN); > + if (display->IsAbsPosContainingBlock(newFrame)) { > + state.PushAbsoluteContainingBlock( > + contentFrame, newFrame, absoluteSaveState); Stylistic nit: this arg-indentation style is a bit unconventional. It seems like the least-bad thing when it's necessary to avoid super-long lines or excessive linewrapping, but we don't seem to be in any danger of those here. So I think this should just be unwrapped to: state.PushAbsoluteContainingBlock(contentFrame, newFrame, absoluteSaveState); @@ +2583,5 @@ > + > + newFrame->AddStateBits(NS_FRAME_CAN_HAVE_ABSPOS_CHILDREN); > + if (display->IsAbsPosContainingBlock(newFrame)) { > + state.PushAbsoluteContainingBlock( > + contentFrame, newFrame, absoluteSaveState); (Same here.)
Attachment #8761889 -
Flags: review?(dholbert) → review+
Updated•8 years ago
|
Blocks: css-grid
Summary: "ASSERTION: Reference frame mismatch" with display:grid/flex → [css-grid][css-flexbox] "ASSERTION: Reference frame mismatch" with display:grid/flex
Assignee | ||
Comment 15•8 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #14) > > + nsFrameConstructorSaveState parentAbsoluteSaveState; > > Maybe s/parent/root/? (or "rootFrame"?) I'm not sure what nesting-level > "parent" is referring to here. I think it's meant to refer to the root frame? Going by this comment https://dxr.mozilla.org/mozilla-central/rev/ddb6b30149221f00eb5dd180530e9033093d4c2b/layout/base/nsCSSFrameConstructor.cpp#2759 the most accurate name would be docElementContainingBlockAbsoluteSaveState. We have the root frame and the root element frame (which are different frames), but this save state is for neither, so "root" or "root element" would be misleading.
Updated•8 years ago
|
Assignee: nobody → tnikkel
Comment 16•8 years ago
|
||
Pushed by tnikkel@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/31f0406a8595 If the root element is grid or flex, allow it to be an absolute containing block. r=dholbert
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/31f0406a8595
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•