Bug 169718

Summary:

Changes to slot children should trigger slotchange

Product:

WebKit

Reporter:

Jan Miksovsky <jan>

Component:

DOM

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, cmarcelo, darin, dbates, esprehn+autocc, ews-watchlist, gyuyoung.kim, kangil.han, rniwa, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

Safari Technology Preview

Hardware:

Mac

OS:

macOS 10.12

Bug Depends on:

Bug Blocks:

148695

Attachments:

Description	Flags
WIP	none
WIP2	none
Archive of layout-test-results from ews126 for ios-simulator-wk2	none
Archive of layout-test-results from ews121 for ios-simulator-wk2	none
Fixes the bug	none
Archive of layout-test-results from ews121 for ios-simulator-wk2	none
Fixed rebaseline	none
Fixes the bug	darin: review+

Jan Miksovsky

Reported 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.)

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, EWS Watchlist	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, EWS Watchlist	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, EWS Watchlist	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
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Jan Miksovsky

Comment 1 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.

Radar WebKit Bug Importer

Comment 2 2018-08-14 20:35:37 PDT

<rdar://problem/43317496>

Ryosuke Niwa

Comment 3 2018-08-24 16:49:39 PDT

Created attachment 348058 [details] WIP

Ryosuke Niwa

Comment 4 2018-08-27 21:22:28 PDT

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

Ryosuke Niwa

Comment 5 2018-08-27 21:24:04 PDT

Created attachment 348262 [details] WIP2

Ryosuke Niwa

Comment 6 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.

Ryosuke Niwa

Comment 7 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.

EWS Watchlist

Comment 8 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

EWS Watchlist

Comment 9 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

EWS Watchlist

Comment 10 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

EWS Watchlist

Comment 11 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

Ryosuke Niwa

Comment 12 2018-08-28 17:20:41 PDT

Created attachment 348364 [details] Fixes the bug

EWS Watchlist

Comment 13 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

EWS Watchlist

Comment 14 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

Ryosuke Niwa

Comment 15 2018-08-28 20:23:17 PDT

Created attachment 348382 [details] Fixed rebaseline

Darin Adler

Comment 16 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.

Ryosuke Niwa

Comment 17 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.

Ryosuke Niwa

Comment 18 2018-08-28 21:26:42 PDT

Created attachment 348390 [details] Fixes the bug

Ryosuke Niwa

Comment 19 2018-08-28 22:30:34 PDT

Thanks for the review!

Ryosuke Niwa

Comment 20 2018-08-28 22:31:59 PDT

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

Note You need to log in before you can comment on or make changes to this bug.