1338678 - Missing textContent with display:contents

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Markus Stange [:mstange]]

STR:
 1. Load the testcase.

Expected results:
The testcase should have a piece of text that says "PASS".

Actual results:
The page is empty.

The text only disappears after the textContent property has been assigned to *twice*; removing the last line of the script makes the text "FAIL" show up correctly.

I encountered this bug while using display:contents within a display:grid element, as a workaround for lack of subgrid support.

Comment 1

[Markus Stange [:mstange]]

Attachment: testcase

Comment 2

[Emilio Cobos Álvarez (:emilio)]

So this is because there's no lazy frame construction for display: contents, so the first time we add the frame we go through[1], but we don't bother unsetting the child flags, so the next time we go through we stop at the top of the loop because the inner element already has the flag set and proceed normally.

[1]: http://searchfox.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#7066

Comment 3

[Emilio Cobos Álvarez (:emilio)]

I see why this may be problematic for direct children of display: contents nodes, but lazy frame construction for nested children should be ok looking at the code. I'll submit a patch.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1338678: Only disallow lazy frame construction for direct children of display: contents elements.

Signed-off-by: Emilio Cobos Álvarez <[e-mail]>

Review commit: https://reviewboard.mozilla.org/r/111806/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/111806/

Comment 5

[Emilio Cobos Álvarez (:emilio)]

If I'm missing something and that patch for some reason doesn't make sense I guess we should keep a stack with the parents we've looked for so far to restore the flag.

Comment 6

[Mats Palmgren (inactive)]

Attachment: wip

Right, I came to the same conclusion as you when I debugged this.
This wip seems to fix it; if we mark all ancestors with NODE_DESCENDANTS_NEED_FRAMES
to the root then we'll see that in CreateNeededFrames() and create the missing frames:
http://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/layout/base/nsCSSFrameConstructor.cpp#7176

This is a bit wallpaper-ish since I think it means we're actually doing lazy
frame construction for the second "element.textContent" assignment, but perhaps
that's OK; I can't say that I remember what the issue was with bug 979782.
It doesn't look hard to add a second loop in the add if-statement though
that unsets the bits.

Emilio, let me know if you're already working in this so we avoid doing it twice :-)
Feel free to take the bug if you want to.

Comment 7

[Mats Palmgren (inactive)]

Oh, you already did :-)

Comment 8

[Mats Palmgren (inactive)]

Comment on attachment 8836377 [details]
Bug 1338678: Only disallow lazy frame construction for direct children of display: contents elements.

https://reviewboard.mozilla.org/r/111806/#review113096

This looks fine to me if it pass regression tests.  r=mats
BTW, did you do any analysis of the non-child case so you know that lazy construction
actually works fine in that case?

Comment 9

[Mats Palmgren (inactive)]

Comment on attachment 8836377 [details]
Bug 1338678: Only disallow lazy frame construction for direct children of display: contents elements.

Perhaps Boris has time to take a quick look as well since he reviewed the original display:contents patches and is generally knowledgeable about lazy frame construction I think.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8836378 [details] [diff] [review]
wip

Review of attachment 8836378 [details] [diff] [review]:
-----------------------------------------------------------------

Ouch, sorry for the duplicated work :(.

Your patch has the side effect of actually running lazy frame construction the second time as you said, which means (I think) that we both think it's fine to do lazy frame construction in this case.

Your patch also means that we'd trigger lazy frame construction the second time even if the content inserted is a direct descendant of a display: contents node. I was wary of this case exactly (I'd have to do a bunch of tests for dynamic insertion of stuff in display: contents nodes to verify it works correctly, but given the hacks we have in place for shadow roots, I believe this may be the tricky part of bug 979782).

I believe my patch should be simpler, but if you think it's fine to recreate frames even as direct children of display contents we may as well skip all this mess and return true the first time too.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

(In reply to Mats Palmgren (:mats) from comment #8)
> Comment on attachment 8836377 [details]
> Bug 1338678: Only disallow lazy frame construction for direct children of
> display: contents elements.
> 
> https://reviewboard.mozilla.org/r/111806/#review113096
> 
> This looks fine to me if it pass regression tests.  r=mats
> BTW, did you do any analysis of the non-child case so you know that lazy
> construction
> actually works fine in that case?

Try is closed, so I couldn't push to try. It passes the css-display reftests though.

I couldn't think of a reason this would break lazy frame construction, since it only looks at the content tree looking at the flags, and ends up being a ContentRangeInserted call.

Comment 12

[Mats Palmgren (inactive)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> Your patch has the side effect of actually running lazy frame construction
> the second time as you said, which means (I think) that we both think it's
> fine to do lazy frame construction in this case.

I said "but perhaps that's OK", meaning "I have no clue" :-)

> I believe my patch should be simpler, but if you think it's fine to recreate
> frames even as direct children of display contents we may as well skip all
> this mess and return true the first time too.

I'm pretty sure I excluded it for a reason, and I don't think this code has
gone through any major architectural changes since then so I suspect the issue
is still there.  But perhaps it's only direct children that are problematic,
not deeper descendants.

> It passes the css-display reftests though.

OK, good enough for me.  I think most display:contents tests are there,
except maybe a few crashtests.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8836377 [details]
Bug 1338678: Only disallow lazy frame construction for direct children of display: contents elements.

https://reviewboard.mozilla.org/r/111806/#review114056

r=me with the nit below; I can't think of any reason we can't lazy-construct non-immediate-child descendants of display:contents.  Should work, I would think.

I'm not sure what the problem is for doing it for _immediate_ children; I wish the existing comment here described the problem in more detail.  :(

::: layout/base/nsCSSFrameConstructor.cpp:7077
(Diff revision 1)
>      }
> -#endif
> -    // XXXmats no lazy frames for display:contents descendants yet (bug 979782).
>      if (GetDisplayContentsStyleFor(content)) {
> -      return false;
> +      MOZ_ASSERT(content != aContainer, "Should've been discarded earlier!");
> +      noPrimaryFrame = false;

This is lossy; it will miss it if some _other_ content was missing a primary frame.

What you probably want instead is to check !GetDisplayContentsStyleFor(content) in the "!noPrimaryFrame && !content->GetPrimaryFrame()" condition.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb2a37ed9b76
Only disallow lazy frame construction for direct children of display: contents elements. r=mats,bz

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Thanks for the review boris!

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #13)
> r=me with the nit below; I can't think of any reason we can't lazy-construct
> non-immediate-child descendants of display:contents.  Should work, I would
> think.
> 
> I'm not sure what the problem is for doing it for _immediate_ children; I
> wish the existing comment here described the problem in more detail.  :(

Yeah, that may be right, I didn't want to take the risk, but seems that the stuff that finds for the content insertion parent should just work for display contents, huh. I'll make sure to write some tests for dynamic insertion in blink and come back to bug 979782 after running those through Gecko ;)

> ::: layout/base/nsCSSFrameConstructor.cpp:7077
> (Diff revision 1)
> >      }
> > -#endif
> > -    // XXXmats no lazy frames for display:contents descendants yet (bug 979782).
> >      if (GetDisplayContentsStyleFor(content)) {
> > -      return false;
> > +      MOZ_ASSERT(content != aContainer, "Should've been discarded earlier!");
> > +      noPrimaryFrame = false;
> 
> This is lossy; it will miss it if some _other_ content was missing a primary
> frame.
> 
> What you probably want instead is to check
> !GetDisplayContentsStyleFor(content) in the "!noPrimaryFrame &&
> !content->GetPrimaryFrame()" condition.

Heh, funny you mention it, I had that change locally already.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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