1435632 - slotchange example not working

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Chris Mills [:cmills]]

I've been writing a bunch of web components examples to go along with the web components documentation on MDN. In this example, when I click a list item, the corresponding description paragraph is displayed by applying a slot attribute to it so it appears in a particular slot:

https://mdn.github.io/web-components-examples/slotchange/

In Chrome this seems to work as expected, but in Firefox it doesn't. The slotchange event seems to fire just fine, but the paragraph doesn't appear in the slot - it doesn't get put in the slot properly.

Is this a bug, or am I doing something screwy in my code?

Comment 1

[Marcos Caceres [:marcosc]]

I'm seeing the following in the error console:

> TypeError: this.attachShadow is not a function

Which it seems we don't yet support. That means that everything below line 9 in the main script is never run. 

I guess until we actually support `attachShadow` on HTMLTemplateElement (yet), this won't worky. 

Closing as duplicate of 1205323 (implement shadow dom), as that's what's needed here.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I'm pretty sure Chris had the pref enabled.
(And attachShadow should not work on template element, but the test isn't about that.)

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Seems to work fine now. Probably a dupe of bug 1438210. Mind confirming Chris?

Comment 4

[Chris Mills [:cmills]]

It works better than it did. But there is still strange behavior here. When you first load the example and click an item in the list, the corresponding element appears in the slot.

If you click an item after the previously clicked item in the list, the element in the slot updates as expected.

However, if you click an item *before* the previously clicked item in the list, the element does not appear in the slot. It only appears in the slot the second time you click it. And the slot attribute doesn't seem to be set on the <li> elements when they are clicked, looking at the DOM inspector.

again, this all seems to work absolutely fine in Chrome.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: reduced testcase

Oh, interesting, I see what's going on, thanks!

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Fix the logic to do layout stuff after a slot attribute changes.

The dirty bit fiddling is nontrivial, but it's pretty much what we do for
invalidation and allows to keep this as incrementally as it can get.

The only caller that we cared about for the GetFlattenedTreeParent check in DestroyFramesForAndRestyle was the old ShadowRoot invalidation functions that went away, rest of the callers are XBL bound element stuff and the pretty print stuff on the root element, so shouldn't be out of the flat tree, or at least not as a result of that call.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Simpler, less incremental patch.

If we deem that changing slot="" should be rare enough, we may as well do it, but I'd prefer to do the fast thing.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Test.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8963261 [details] [diff] [review]
Simpler, less incremental patch.

I might not be against this, but let me look at the patch 1.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8963258 [details] [diff] [review]
Fix the logic to do layout stuff after a slot attribute changes.

>+  if (aNewSlot) {
>+    // Ditto if the new slot will stop showing fallback content.
>+    if (aNewSlot->AssignedNodes().IsEmpty()) {
>+      DestroyFramesForAndRestyle(aNewSlot);
>+    // Otherwise we just care about the element, but we need to ensure
>+    // that something takes care of traversing to the relevant slot.
>+    } else if (aElement.HasServoData()) {
>+      DestroyFramesForAndRestyle(&aElement);
This part isn't too clear from this code.
Could you perhaps clarify somehow what "traversing to the relevant slot" means here.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/284b9d43b113
Fix the logic to do layout stuff after reassigning a slot. r=smaug,xidorn

Comment 12

[Emilio Cobos Álvarez (:emilio)]

(In reply to Olli Pettay [:smaug] (only webcomponents and event handling reviews, please) from comment #10)
> Comment on attachment 8963258 [details] [diff] [review]
> Fix the logic to do layout stuff after a slot attribute changes.
> 
> >+  if (aNewSlot) {
> >+    // Ditto if the new slot will stop showing fallback content.
> >+    if (aNewSlot->AssignedNodes().IsEmpty()) {
> >+      DestroyFramesForAndRestyle(aNewSlot);
> >+    // Otherwise we just care about the element, but we need to ensure
> >+    // that something takes care of traversing to the relevant slot.
> >+    } else if (aElement.HasServoData()) {
> >+      DestroyFramesForAndRestyle(&aElement);
> This part isn't too clear from this code.
> Could you perhaps clarify somehow what "traversing to the relevant slot"
> means here.

I changed it a bit so this wasn't needed.

Comment 13

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/284b9d43b113

Comment 14

[Chris Mills [:cmills]]

Wow, nice work. Thanks Emilio!

Comment 15

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10310 for changes under testing/web-platform/tests