Closed Bug 1429846 Opened 2 years ago Closed 2 years ago

Fix invalidation for ::slotted.

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(1 file)

I overlooked something while implementing it.

I think the last bug remaining is handled nested slots in invalidate_slotted_children. But that's less common.

I've left a FIXME commit for that too in https://github.com/servo/servo/pull/19747. This patch is on top.
Filed https://bugs.chromium.org/p/chromium/issues/detail?id=801225 for the Chromium issue, with a more detailed description, which I think it's what the bug is.
If it's not too hard, could you attach a patch that shows what the diff is from the state with the referenced commit entirely backed out, to the state with the partial backout, so I can more easily see which changes will remain?
Flags: needinfo?(emilio)
It doesn't apply cleanly on top of the other one, so I'm not sure how could I do that... But the only change remaining should be moving each_xbl_stylist to operate on CascadeData instead of Stylist (with the appropriate renaming), which I still want for Shadow DOM.
Flags: needinfo?(emilio)
Comment on attachment 8941900 [details]
Bug 1429846: Fix slotted invalidation.

https://reviewboard.mozilla.org/r/212116/#review219222

::: commit-message-46008:16
(Diff revision 2)
> +Curiously, Blink fails the test as written, presumably because they don't flush
> +styles from getComputedStyle incorrectly (in their test they do via

s/don't // or s/incorrectly/correctly/

::: servo/components/style/stylist.rs:2192
(Diff revision 2)
> +                            self.slotted_rules.get_or_insert_with(|| {
> +                                Box::new(Default::default())
> +                            })

self.slotted_rules.get_or_insert_with(Default::default)

would work too.

::: testing/web-platform/tests/css/css-scoping/slotted-invalidation.html:29
(Diff revision 2)
> +  var root = host.attachShadow({"mode":"open"});
> +  root.innerHTML = '<style>.outer ::slotted(#slotted) { background-color: red } .outer .inner::slotted(#slotted) { background-color: green }</style><div id="outer"><slot id="inner"></slot></div>';
> +
> +  assert_equals(window.getComputedStyle(slotted).backgroundColor, "rgba(0, 0, 0, 0)");
> +
> +  host.offsetTop;

Based on your comment, these flushes aren't needed, yes?  If so, please remove them.
Attachment #8941900 - Flags: review?(cam) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/bec4537b2efd
Fix slotted invalidation. r=heycam
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6eb62602664d
Skip the test in non-stylo on a CLOSED TREE. r=me
https://hg.mozilla.org/mozilla-central/rev/bec4537b2efd
https://hg.mozilla.org/mozilla-central/rev/6eb62602664d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.