slotchange example not working

RESOLVED FIXED in Firefox 61

Status

()

defect
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: cmills, Assigned: emilio)

Tracking

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(4 attachments)

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?
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.
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → DUPLICATE
Duplicate of bug: shadowdom-initial-release
I'm pretty sure Chris had the pref enabled.
(And attachShadow should not work on template element, but the test isn't about that.)
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Flags: needinfo?(emilio)
Seems to work fine now. Probably a dupe of bug 1438210. Mind confirming Chris?
Flags: needinfo?(emilio) → needinfo?(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.
Flags: needinfo?(cmills)
Posted file reduced testcase
Oh, interesting, I see what's going on, thanks!
Assignee: nobody → emilio
Status: REOPENED → ASSIGNED
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.
Attachment #8963258 - Flags: review?(xidorn+moz)
Attachment #8963258 - Flags: review?(bugs)
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.
Attachment #8963261 - Flags: review?(bugs)
Posted patch Test.Splinter Review
Attachment #8963264 - Flags: review?(bugs)
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.
Attachment #8963261 - Flags: review?(bugs) → review+
Attachment #8963264 - Flags: review?(bugs) → review+
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.
Attachment #8963258 - Flags: review?(bugs) → review+
Attachment #8963258 - Flags: review?(xidorn+moz) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/284b9d43b113
Fix the logic to do layout stuff after reassigning a slot. r=smaug,xidorn
(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.
https://hg.mozilla.org/mozilla-central/rev/284b9d43b113
Status: ASSIGNED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Wow, nice work. Thanks Emilio!
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10310 for changes under testing/web-platform/tests
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.