1429846 - Fix invalidation for ::slotted.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1429846: Fix slotted invalidation.

This is a partial revert of
https://github.com/servo/servo/commit/ce1d8cd232dfbc9e0a52f9467ba2bc209087ea63

If you're in a shadow tree, you may not be slotted but you still need to look at
the slotted rules, since a <slot> could be a descendant of yours.

Just use the same invalidation map everywhere, and remove complexity.

This means that we can do some extra work while trying to gather invalidation
if there are slotted rules, but I don't think it's a problem.

The test is ported from https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/css/invalidation/slotted.html?l=1&rcl=58d68fdf783d7edde1c82a642e037464861f2787

Curiously, Blink fails the test as written, presumably because they don't flush
styles from getComputedStyle incorrectly (in their test they do via
updateStyleAndReturnAffectedElementCount), due to <slot>s not being in the flat
tree in their implementation.

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 3

[Cameron McCormack (:heycam)]

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?

Comment 4

[Emilio Cobos Álvarez (: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.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8941900 [details]
Bug 1429846: Fix slotted invalidation.

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

Comment 6

[Cameron McCormack (:heycam)]

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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bec4537b2efd
Fix slotted invalidation. r=heycam

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6eb62602664d
Skip the test in non-stylo on a CLOSED TREE. r=me

Comment 9

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/bec4537b2efd
https://hg.mozilla.org/mozilla-central/rev/6eb62602664d