1381017 - stylo: After dynamically modifying an element with -moz-binding, frames do not created under <xbl:children>

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Some XBL tests like http://searchfox.org/mozilla-central/source/layout/reftests/bugs/495354-1a.xhtml dynamically modify child contents for an element with -moz-binding. 

For gecko restyle manager, it sets those NODE_NEEDS_FRAME/NODE_DESCENDANTS_NEED_FRAMES bits in nsCSSFrameConstructor::MaybeConstructLazily, and creates frames by calling nsCSSFrameConstructor::CreateNeededFrames() in next refresh driver flush.

For servo restyle manager, it doesn't go to the CreateNeedFrames() path. Instead, it deals with those bits in ServoRestyleManager::ProcessPostTraversal(). However, when setting NODE_DESCENDANTS_NEED_FRAMES in MaybeConstructLazily(), the contents in <xbl:content> won't have NODE_DESCENDANTS_NEED_FRAMES bit set, so the frame for the modified content won't be generated.

Comment 1

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1381017 - Set NODE_DESCENDANTS_NEED_FRAMES from flatten tree parent.

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

Comment 2

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=449e8b22a625c3a81e8c60b41389b1dcc904f63e

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Wouldn't:

- nsIContent* content = aContainer;
+ nsIContent* content = aChild->GetFlattenedTreeParent();

be the correct fix, instead of this?

Comment 4

[Cameron McCormack (:heycam)]

Comment on attachment 8886613 [details]
Bug 1381017 - Set NODE_DESCENDANTS_NEED_FRAMES from flatten tree parent.

Redirect to Emilio since he's taken a look.

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8886613 [details]
Bug 1381017 - Set NODE_DESCENDANTS_NEED_FRAMES from flatten tree parent.

The solution in comment 3 is promising, but it hits an assertion in crashtest. I'll look into that. Clear the review request for now.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a6d651bd5dc4d5869f45aef413e9654ecb28cf8

Comment 6

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8886613 [details]
Bug 1381017 - Set NODE_DESCENDANTS_NEED_FRAMES from flatten tree parent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157410/diff/1-2/

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8886613 [details]
Bug 1381017 - Set NODE_DESCENDANTS_NEED_FRAMES from flatten tree parent.

https://reviewboard.mozilla.org/r/157410/#review162842

This looks fine to me, and I think it's the right fix, but I'd want tn to stamp the assertion change, or suggest to update the assertion count in that test-case, I think...

::: layout/base/nsCSSFrameConstructor.cpp:7231
(Diff revisions 1 - 2)
>        noPrimaryFrame = needsFrameBitSet = false;
>      }
> -    if (!noPrimaryFrame && !content->GetPrimaryFrame() &&
> -        !GetDisplayContentsStyleFor(content)) {
> -      noPrimaryFrame = true;
> +    if (!noPrimaryFrame && !content->GetPrimaryFrame()) {
> +      nsStyleContext* sc = GetUndisplayedContent(content);
> +      noPrimaryFrame = !GetDisplayContentsStyleFor(content) &&
> +        (sc && !sc->IsInDisplayNoneSubtree());

Hmm... It feels a bit fishy to set the needs frame bit on a display: none subtree... But I guess in the case of XBL it may be ok? Could we assert it more tightly? I guess this only hits in dom/xbl/crashtests/277523-1.xhtml... I'm not sure if it's worth to relax the assertion vs. bumping the expected count for that specific test-case.

::: layout/reftests/bugs/reftest.list:1248
(Diff revisions 1 - 2)
>  == 467084-1.html 467084-1-ref.html
>  == 467084-2.html 467084-2-ref.html
>  == 467444-1.html 467444-1-ref.html
>  == 467460-1.html 467460-1-ref.html
>  == 468473-1.xul 468473-1-ref.xul
> -fails-if(styloVsGecko||stylo) == 468546-1.xhtml 468546-1-ref.xhtml
> +fails-if(styloVsGecko) == 468546-1.xhtml 468546-1-ref.xhtml

Why do we still fail if `styloVsGecko`? Should we have another bug for that?

Comment 8

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8886613 [details]
Bug 1381017 - Set NODE_DESCENDANTS_NEED_FRAMES from flatten tree parent.

https://reviewboard.mozilla.org/r/157410/#review163154

Comment 9

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8886613 [details]
Bug 1381017 - Set NODE_DESCENDANTS_NEED_FRAMES from flatten tree parent.

https://reviewboard.mozilla.org/r/157410/#review162842

> Hmm... It feels a bit fishy to set the needs frame bit on a display: none subtree... But I guess in the case of XBL it may be ok? Could we assert it more tightly? I guess this only hits in dom/xbl/crashtests/277523-1.xhtml... I'm not sure if it's worth to relax the assertion vs. bumping the expected count for that specific test-case.

Relaxing the assertion could also remove expected assertion counts in `layout/reftests/css-display/reftest.list`, so I'm going for this path. Those tests are related to xul, which we do not enable for normal pages, so I'm not worry about them.

> Why do we still fail if `styloVsGecko`? Should we have another bug for that?

468546-1.xhtml has `::first-line` style, so bug 1324619 should fix that.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06b014083d03
Set NODE_DESCENDANTS_NEED_FRAMES from flatten tree parent. r=emilio,tnikkel

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/06b014083d03