Bug 169718 – Changes to slot children should trigger slotchange

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Jan Miksovsky]

See http://jsbin.com/pimijip/edit?html,console,output.

This adds a child node to a `slot`.
Expect: this triggers a `slotchange` event. Blink does this as expected.
Actual: this doesn't trigger `slotchange`.

Rationale: If no nodes are assigned to a slot, then invoking `assignedNodes` on the slot will return any children of the slot itself. That is, the slot's own children serve as default content. Those children will be returned when asking for `assignedNodes`. Changes in the slot's children will be reflected in subsequent requests for `assignedNodes`. Since the set of `assignedNodes` is changing, the slot should be raising `slotchange`.

(If nodes *are* assigned to a slot, changes in the slot's direct children will not affect `assignedNodes`, and hence should *not* raise `slotchange`. Blink gets this right.)

[Jan Miksovsky]

Just spent an hour isolating a repro for a WebKit bug — and discovered that it's the same as this bug I'd filed half a year ago.

FWIW, here's a more complex repro involving an outer element and an inner element: http://jsbin.com/raciraw/edit?html,output.

the outer element changes its default slot content, which changes what's assigned to the inner element's slot — but the inner element's slot never raises slotchange. This works as expected in Blink, but not in WebKit.

[Radar WebKit Bug Importer]

<rdar://problem/43317496>

[Ryosuke Niwa]

Created attachment 348058 [details]
WIP

[Ryosuke Niwa]

*** Bug 188752 has been marked as a duplicate of this bug. ***

[Ryosuke Niwa]

Created attachment 348262 [details]
WIP2

[Ryosuke Niwa]

There is quite a bit of change involved in order to not keep resolving the slot assignments. A very native implementation would end up resolving the assignments of each slot whenever a child node is inserted or removed from a slot. I don't think that's acceptable implementation.

[Ryosuke Niwa]

Ugh... this is really shitty. There is no way to avoid at least one O(n) operation when the first slot is inserted because we'd have to figure out whether each slot uses its assigned content or fallback because we don't want to do be doing all the house-keeping when there are no slows in a shadow tree.

[EWS Watchlist]

Comment on attachment 348262 [details]
WIP2

Attachment 348262 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9006368

New failing tests:
imported/w3c/web-platform-tests/shadow-dom/slotchange.html

[EWS Watchlist]

Created attachment 348266 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 348262 [details]
WIP2

Attachment 348262 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9007251

New failing tests:
imported/w3c/web-platform-tests/shadow-dom/slotchange.html

[EWS Watchlist]

Created attachment 348276 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Ryosuke Niwa]

Created attachment 348364 [details]
Fixes the bug

[EWS Watchlist]

Comment on attachment 348364 [details]
Fixes the bug

Attachment 348364 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9017745

New failing tests:
imported/w3c/web-platform-tests/shadow-dom/slotchange.html

[EWS Watchlist]

Created attachment 348374 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Ryosuke Niwa]

Created attachment 348382 [details]
Fixed rebaseline

[Darin Adler]

Comment on attachment 348382 [details]
Fixed rebaseline

Wait, why no test progression in the patch? I see a change log entry, but no test result.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #16)
> Comment on attachment 348382 [details]
> Fixed rebaseline
> 
> Wait, why no test progression in the patch? I see a change log entry, but no
> test result.

Yeah, there was a bug in this latest patch. I'm uploading a new patch with more test cases. The test in WPT isn't so good.

[Ryosuke Niwa]

Created attachment 348390 [details]
Fixes the bug

[Ryosuke Niwa]

Thanks for the review!

[Ryosuke Niwa]

Committed r235458: <https://trac.webkit.org/changeset/235458>