Closed
Bug 1341083
Opened 7 years ago
Closed 7 years ago
stylo: dynamic restyling for display: contents
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
References
Details
Attachments
(3 files)
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.
Assignee | ||
Comment 1•7 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4f59c552728f5acc19a540d55414195eaac0271
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8839407 [details] Bug 1341083: Don't fall back to reframing with display: contents. https://reviewboard.mozilla.org/r/114082/#review115562
Attachment #8839407 -
Flags: review?(cam) → review+
Comment 6•7 years ago
|
||
mozreview-review |
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?
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-review |
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.
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
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•7 years ago
|
||
mozreview-review |
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.
Attachment #8839736 -
Flags: review?(cam) → review+
Comment 14•7 years ago
|
||
mozreview-review |
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/
Attachment #8839408 -
Flags: review?(cam) → review+
Assignee | ||
Comment 15•7 years ago
|
||
Still need to split + land, will do tomorrow.
Assignee | ||
Comment 16•7 years ago
|
||
Gah, though I can't land in autoland I guess, so this requires bug 1322317 in central :(
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 20•7 years ago
|
||
Pushed by ecoal95@gmail.com: 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a48a9a64e7e2 https://hg.mozilla.org/mozilla-central/rev/16e6b3202e27 https://hg.mozilla.org/mozilla-central/rev/7fb6bef8f7d6
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•