Bug 169718 - Changes to slot children should trigger slotchange
Summary: Changes to slot children should trigger slotchange
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: Safari Technology Preview
Hardware: Macintosh macOS 10.12
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
: 188752 (view as bug list)
Depends on:
Blocks: 148695
  Show dependency treegraph
 
Reported: 2017-03-15 16:31 PDT by Jan Miksovsky
Modified: 2018-08-28 22:31 PDT (History)
10 users (show)

See Also:


Attachments
WIP (5.47 KB, patch)
2018-08-24 16:49 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
WIP2 (24.57 KB, patch)
2018-08-27 21:24 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.28 MB, application/zip)
2018-08-27 23:25 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.27 MB, application/zip)
2018-08-28 01:23 PDT, Build Bot
no flags Details
Fixes the bug (9.54 KB, patch)
2018-08-28 17:20 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.25 MB, application/zip)
2018-08-28 19:03 PDT, Build Bot
no flags Details
Fixed rebaseline (8.43 KB, patch)
2018-08-28 20:23 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixes the bug (25.99 KB, patch)
2018-08-28 21:26 PDT, Ryosuke Niwa
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Miksovsky 2017-03-15 16:31:32 PDT
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.)
Comment 1 Jan Miksovsky 2017-11-14 16:52:55 PST
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.
Comment 2 Radar WebKit Bug Importer 2018-08-14 20:35:37 PDT
<rdar://problem/43317496>
Comment 3 Ryosuke Niwa 2018-08-24 16:49:39 PDT
Created attachment 348058 [details]
WIP
Comment 4 Ryosuke Niwa 2018-08-27 21:22:28 PDT
*** Bug 188752 has been marked as a duplicate of this bug. ***
Comment 5 Ryosuke Niwa 2018-08-27 21:24:04 PDT
Created attachment 348262 [details]
WIP2
Comment 6 Ryosuke Niwa 2018-08-27 21:26:22 PDT
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.
Comment 7 Ryosuke Niwa 2018-08-27 22:28:55 PDT
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.
Comment 8 Build Bot 2018-08-27 23:25:17 PDT
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
Comment 9 Build Bot 2018-08-27 23:25:19 PDT
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
Comment 10 Build Bot 2018-08-28 01:23:21 PDT
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
Comment 11 Build Bot 2018-08-28 01:23:23 PDT
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
Comment 12 Ryosuke Niwa 2018-08-28 17:20:41 PDT
Created attachment 348364 [details]
Fixes the bug
Comment 13 Build Bot 2018-08-28 19:03:00 PDT
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
Comment 14 Build Bot 2018-08-28 19:03:02 PDT
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
Comment 15 Ryosuke Niwa 2018-08-28 20:23:17 PDT
Created attachment 348382 [details]
Fixed rebaseline
Comment 16 Darin Adler 2018-08-28 20:41:22 PDT
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.
Comment 17 Ryosuke Niwa 2018-08-28 21:24:41 PDT
(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.
Comment 18 Ryosuke Niwa 2018-08-28 21:26:42 PDT
Created attachment 348390 [details]
Fixes the bug
Comment 19 Ryosuke Niwa 2018-08-28 22:30:34 PDT
Thanks for the review!
Comment 20 Ryosuke Niwa 2018-08-28 22:31:59 PDT
Committed r235458: <https://trac.webkit.org/changeset/235458>