1341083 - stylo: dynamic restyling for display: contents

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Emilio Cobos Álvarez [:emilio]]

This is needed to avoid the assertion that prevents a bunch of reftests from even running, and also fixes a bunch of other restyling assertions.

I have a patch for this I'm running through try.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Quick update: It works, but I'm trying to diagnose whether it's fine to inherit from a display: contents node without blockifying for anonymous boxes. I believe it may just be fine, but otherwise we need a few more changes.

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1341083: Don't fall back to reframing with display: contents.

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

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1341083: Implement dynamic restyling for display: contents.

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

Comment 4

[Emilio Cobos Álvarez [:emilio]]

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

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 8839407 [details]
Bug 1341083: Don't fall back to reframing with display: contents.

https://reviewboard.mozilla.org/r/114082/#review115562

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8839408 [details]
Bug 1341083: Implement dynamic restyling for display: contents.

https://reviewboard.mozilla.org/r/114084/#review115564

Looking good but a couple of questions in here.

::: layout/base/ServoRestyleManager.cpp:203
(Diff revision 1)
>  
> +  bool isDisplayContentsElement = false;
> +
> +  // FIXME(emilio, bug 1303605): This can be simpler for Servo.
> +  //
> +  // Note that we intentionally don't check for display: none content.

Can you explain why?  I don't think it's obvious.

::: layout/base/ServoRestyleManager.cpp:251
(Diff revision 1)
> +      PresContext()->FrameConstructor()->SetDisplayContents(aElement,
> +                                                            newContext);

Why do we need to call this here, when GeckoRestyleManager doesn't?

::: layout/base/ServoRestyleManager.cpp:292
(Diff revision 1)
> +  nsStyleContext* upToDateContext =
> +    recreateContext ? newContext : oldStyleContext;

Move this into the if statement.

::: layout/base/ServoRestyleManager.cpp:338
(Diff revision 1)
>  
>    if (!aPseudoTagOrNull) {
>      return primaryFrame;
>    }
>  
> +  // FIXME(emilio): Need to take into account display: contents pseudos!

Yeah, I don't know what's meant to happen here. :(

::: servo/components/style/matching.rs
(Diff revision 1)
> -            // Propagate the "can be fragmented" bit. It would be nice to
> -            // encapsulate this better.
> -            if let Some(ref p) = parent_values {
> -                let can_be_fragmented =
> -                    p.is_multicol() || parent_el.unwrap().as_node().can_be_fragmented();
> -                unsafe { self.as_node().set_can_be_fragmented(can_be_fragmented); }

I guess we don't really need to set the can_be_fragmented flag again when we resolve style for a pseudo-element, since we should have already done it when resolving the element's primary style, right?

::: servo/components/style/matching.rs:489
(Diff revision 1)
> +            parent_el = Some(self.clone());
>              Some(primary_style.values())
>          };
> +
> +
> +        // FIXME(emilio): This looks slightly awful, can we make this cleaner?

Maybe factoring out the finding of the layout parent into a separate function would make this more readable?

::: servo/components/style/matching.rs:496
(Diff revision 1)
> +                let is_display_contents =
> +                    layout_parent_el.as_ref()
> +                        .map_or(false, |e| {

Because display:contents gets converted to block on the root element, we should never get None for layout_parent_el here.  Can you assert or maybe expect() this?

::: servo/components/style/properties/properties.mako.rs:1977
(Diff revision 1)
>      // FIXME(heycam): We should look past any display:contents ancestors to
>      // determine if we are a flex or grid item, but we don't have access to
>      // grandparent or higher style here.

This comment can be removed now.

::: servo/components/style/stylist.rs:326
(Diff revision 1)
> +        // In practice, I don't think any anonymous content can be a direct
> +        // descendant of a display: contents element where display: contents is
> +        // the actual used value, and the computed value of it would need
> +        // blockification.

It might be worth checking what shadow content does here, e.g.:

  <div style="display: contents"></div>
  <script> 
     var root = div.createShadowRoot();
     var e = document.createElement(...);
     root.appendChild(e);
  </script>

Although that's not related to precomputed pseudo styles here.

::: servo/components/style/values/computed/mod.rs:45
(Diff revision 1)
> +    /// The style of the layout parent node. This will almost always be
> +    /// `inherited_style`, except when `display: contents` is at play.

Can you describe in the comment what layout_parent_style should be when there are display:contents elements around?

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8839408 [details]
Bug 1341083: Implement dynamic restyling for display: contents.

https://reviewboard.mozilla.org/r/114084/#review115574

This is a first reply, I haven't had the time to address the comments.

::: layout/base/ServoRestyleManager.cpp:203
(Diff revision 1)
>  
> +  bool isDisplayContentsElement = false;
> +
> +  // FIXME(emilio, bug 1303605): This can be simpler for Servo.
> +  //
> +  // Note that we intentionally don't check for display: none content.

We don't need it, AFAIK we don't need to recalc the style for those AFAIK, and we have the old style , context map up-to-date for those I think (indeed, I don't think we need to keep it at all).

(That is, unless other user-visible stuff like GetComputedStyle can see what it's in that map, in which case I think there should be a lot of tests broken, which I hope there aren't).

But that just reminded me of another thing I have to fix: this in its current state works, and fixes the restyling of display: contents children, but we fall back to reframing when the display: contents style changes. I'll post another patch to address that..

::: layout/base/ServoRestyleManager.cpp:251
(Diff revision 1)
> +      PresContext()->FrameConstructor()->SetDisplayContents(aElement,
> +                                                            newContext);

Gecko iterates over the `UndisplayedNode`s directly, and sets the `mStyle` field there instead of doing the lookup again.

::: layout/base/ServoRestyleManager.cpp:338
(Diff revision 1)
>  
>    if (!aPseudoTagOrNull) {
>      return primaryFrame;
>    }
>  
> +  // FIXME(emilio): Need to take into account display: contents pseudos!

Well, we definitely need to style them and hook them up into the frame tree, that's why the recreateContext stuff still goes through them.

I just need to find them, I believe there are some LayoutUtils methods for that, though I'd rather do that as a followup.

::: servo/components/style/matching.rs
(Diff revision 1)
> -            // Propagate the "can be fragmented" bit. It would be nice to
> -            // encapsulate this better.
> -            if let Some(ref p) = parent_values {
> -                let can_be_fragmented =
> -                    p.is_multicol() || parent_el.unwrap().as_node().can_be_fragmented();
> -                unsafe { self.as_node().set_can_be_fragmented(can_be_fragmented); }

Right, good catch.

::: servo/components/style/matching.rs:489
(Diff revision 1)
> +            parent_el = Some(self.clone());
>              Some(primary_style.values())
>          };
> +
> +
> +        // FIXME(emilio): This looks slightly awful, can we make this cleaner?

Yes, I initially wanted to avoid re-borrowing as much as possible, but the borrow-checker didn't like it a lot, so I just gave up and borrowed at the end of the loop, so I'll factor this out.

::: servo/components/style/matching.rs:496
(Diff revision 1)
> +                let is_display_contents =
> +                    layout_parent_el.as_ref()
> +                        .map_or(false, |e| {

This is asserted in `cascade` with the `debug_assert_eq!(inherited_style.is_some(), layout_parent_style.is_some())`.

That being said, I believe we blockify right now (and gecko too) for disconnected nodes, which is why this holds, and that is dubious.

::: servo/components/style/values/computed/mod.rs:45
(Diff revision 1)
> +    /// The style of the layout parent node. This will almost always be
> +    /// `inherited_style`, except when `display: contents` is at play.

Sure.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8839408 [details]
Bug 1341083: Implement dynamic restyling for display: contents.

https://reviewboard.mozilla.org/r/114084/#review115574

> This is asserted in `cascade` with the `debug_assert_eq!(inherited_style.is_some(), layout_parent_style.is_some())`.
> 
> That being said, I believe we blockify right now (and gecko too) for disconnected nodes, which is why this holds, and that is dubious.

Correction: That assertion doesn't imply that we find a non-display: contents parent. But see above why I think it may be the right thing to do.

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1341083: Cleanup infallible ProcessRestyledFrames.

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

Comment 10

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8839407 [details]
Bug 1341083: Don't fall back to reframing with display: contents.

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

Comment 11

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8839408 [details]
Bug 1341083: Implement dynamic restyling for display: contents.

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

Comment 12

[Emilio Cobos Álvarez [:emilio]]

Last review request should have most of the comments addressed. I'm still doing another build to see if the failure I see on try is caused by my changes or it's a test that didn't run on automation because of the assertion. Assuming it's the latest, this is ready for review.

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8839736 [details]
Bug 1341083: Cleanup infallible ProcessRestyledFrames.

https://reviewboard.mozilla.org/r/114310/#review115776

(I don't know why this patch appears twice in the review...)

::: layout/style/ServoBindings.cpp:287
(Diff revision 1)
>                                                 aPseudoTagOrNull);
> -  if (!relevantFrame) {
> -    return nullptr;
> +  if (relevantFrame) {
> +    return relevantFrame->StyleContext();
>    }
>  
> -  return relevantFrame->StyleContext();
> +  // FIXME(emilio): Is there a shorter path?

I can't think of one...

::: layout/style/ServoBindings.cpp:291
(Diff revision 1)
> +  // NB: we handle correctly the display: none case since servo still has the
> +  // older style.

Nit: I think this comment only makes sense if you know that Gecko_GetStyleContext is only used for getting the old style when computing restyle damage, so maybe mention that fact here.

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8839408 [details]
Bug 1341083: Implement dynamic restyling for display: contents.

https://reviewboard.mozilla.org/r/114084/#review115780

::: servo/components/style/matching.rs:453
(Diff revisions 1 - 2)
> +                // NOTE(emilio): This is unreachable right now for connected
> +                // nodes, because we always blockify the root element.
> +                //
> +                // It should be also unreachable for disconnected nodes right
> +                // now, but it's dubious, at best, if we need to blockify the
> +                // root of disconnected subtrees.

I guess this is true only because we don't call layout_parent() on a root element.  Can you either adjust the comment above the function to say that it returns the current element if it doesn't have a parent, or just add a debug_assert!() at the top of the function to ensure we don't pass in a parent-less element.

::: servo/components/style/matching.rs:527
(Diff revisions 1 - 2)
>          // Propagate the "can be fragmented" bit. It would be nice to
>          // encapsulate this better.
> +        if pseudo_style.is_none() {

Can you extend the comment here to say why we only do this if pseudo_style is None?

::: servo/components/style/values/computed/mod.rs:47
(Diff revisions 1 - 2)
>      /// The style we're inheriting from.
>      pub inherited_style: &'a ComputedValues,
>  
>      /// The style of the layout parent node. This will almost always be
> -    /// `inherited_style`, except when `display: contents` is at play.
> +    /// `inherited_style`, except when `display: contents` is at play, in which
> +    /// case is the style of the last ancestor with a `display` value that isn't

s/case is/case it is/

Comment 15

[Emilio Cobos Álvarez [:emilio]]

Still need to split + land, will do tomorrow.

Comment 16

[Emilio Cobos Álvarez [:emilio]]

Gah, though I can't land in autoland I guess, so this requires bug 1322317 in central :(

Comment 17

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8839736 [details]
Bug 1341083: Cleanup infallible ProcessRestyledFrames.

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

Comment 18

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8839408 [details]
Bug 1341083: Implement dynamic restyling for display: contents.

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

Comment 19

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8839407 [details]
Bug 1341083: Don't fall back to reframing with display: contents.

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a48a9a64e7e2
Cleanup infallible ProcessRestyledFrames. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/16e6b3202e27
Implement dynamic restyling for display: contents. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fb6bef8f7d6
Don't fall back to reframing with display: contents. r=heycam

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a48a9a64e7e2
https://hg.mozilla.org/mozilla-central/rev/16e6b3202e27
https://hg.mozilla.org/mozilla-central/rev/7fb6bef8f7d6