Bug 189144

Summary:

slotchange event doesn't get fired when inserting, removing, or renaming slot elements

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

DOM

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, dbates, esprehn+autocc, ews-watchlist, gyuyoung.kim, kangil.han, koivisto, tomalecpub, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

189183

Bug Blocks:

148695

Attachments:

Description	Flags
WIP	ews-watchlist: commit-queue-
Archive of layout-test-results from ews123 for ios-simulator-wk2	none
Archive of layout-test-results from ews125 for ios-simulator-wk2	none
WIP2	none
WIP3	none
WIP4	none
WIP4 (rebased for trunk)	none
WIP5	none
WIP6	none
WIP6 (with code change)	none
Fixes the bug	koivisto: review+

Ryosuke Niwa

Reported 2018-08-29 20:45:26 PDT

There are multiple WPT failures in shadow-dom/slotchange.html and shadow-dom/slotchange-event.html because of this.

Attachments
WIP (22.74 KB, patch) 2018-08-29 20:47 PDT, Ryosuke Niwa	ews-watchlist: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.26 MB, application/zip) 2018-08-29 22:51 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.25 MB, application/zip) 2018-08-30 00:51 PDT, EWS Watchlist	no flags	Details
WIP2 (52.09 KB, patch) 2018-08-30 19:31 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
WIP3 (57.48 KB, patch) 2018-08-30 21:33 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
WIP4 (61.10 KB, patch) 2018-08-31 19:33 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
WIP4 (rebased for trunk) (54.75 KB, patch) 2018-08-31 19:45 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
WIP5 (58.81 KB, patch) 2018-08-31 23:23 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
WIP6 (40.51 KB, patch) 2018-09-01 00:55 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
WIP6 (with code change) (59.41 KB, patch) 2018-09-01 00:56 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Fixes the bug (105.45 KB, patch) 2018-09-04 12:36 PDT, Ryosuke Niwa	koivisto: review+	Details Formatted Diff Diff
Show Obsolete (10) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2018-08-29 20:45:50 PDT

<rdar://problem/43871061>

Ryosuke Niwa

Comment 2 2018-08-29 20:47:45 PDT

Created attachment 348477 [details] WIP

EWS Watchlist

Comment 3 2018-08-29 22:51:55 PDT

Comment on attachment 348477 [details] WIP Attachment 348477 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9031498 New failing tests: imported/w3c/web-platform-tests/shadow-dom/slotchange.html

EWS Watchlist

Comment 4 2018-08-29 22:51:57 PDT

Created attachment 348486 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4

EWS Watchlist

Comment 5 2018-08-30 00:51:22 PDT

Comment on attachment 348477 [details] WIP Attachment 348477 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9032135 New failing tests: imported/w3c/web-platform-tests/shadow-dom/slotchange.html

EWS Watchlist

Comment 6 2018-08-30 00:51:24 PDT

Created attachment 348490 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4

Ryosuke Niwa

Comment 7 2018-08-30 19:31:14 PDT

Created attachment 348583 [details] WIP2

Ryosuke Niwa

Comment 8 2018-08-30 21:33:47 PDT

Created attachment 348594 [details] WIP3

EWS Watchlist

Comment 9 2018-08-30 21:36:33 PDT

Attachment 348594 [details] did not pass style-queue: ERROR: Source/WebCore/dom/SlotAssignment.cpp:129: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Ryosuke Niwa

Comment 10 2018-08-31 19:33:55 PDT

Created attachment 348697 [details] WIP4 Finally I have something satisfactory in terms of perf & behavior. Will clean this up, and add more test cases. We already have 53 test cases but still doesn't seem to cover all the edge cases.

Ryosuke Niwa

Comment 11 2018-08-31 19:45:08 PDT

Created attachment 348699 [details] WIP4 (rebased for trunk)

EWS Watchlist

Comment 12 2018-08-31 19:58:25 PDT

Attachment 348699 [details] did not pass style-queue: ERROR: Source/WebCore/dom/SlotAssignment.cpp:242: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Ryosuke Niwa

Comment 13 2018-08-31 23:23:12 PDT

Created attachment 348709 [details] WIP5 This bug is absolutely insane... There are so many crazy edge cases I have to worry about for both insertion & removal.

EWS Watchlist

Comment 14 2018-08-31 23:25:03 PDT

Attachment 348709 [details] did not pass style-queue: ERROR: Source/WebCore/dom/SlotAssignment.cpp:261: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Ryosuke Niwa

Comment 15 2018-09-01 00:55:46 PDT

Created attachment 348710 [details] WIP6 Done some cleanup. The patch looks much better! One more test case to fix and we should be good to go.

Ryosuke Niwa

Comment 16 2018-09-01 00:56:26 PDT

Created attachment 348711 [details] WIP6 (with code change)

EWS Watchlist

Comment 17 2018-09-01 00:59:41 PDT

Attachment 348711 [details] did not pass style-queue: ERROR: Source/WebCore/dom/SlotAssignment.cpp:246: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Ryosuke Niwa

Comment 18 2018-09-04 12:36:59 PDT

Created attachment 348839 [details] Fixes the bug

Antti Koivisto

Comment 19 2018-09-04 16:50:28 PDT

Comment on attachment 348839 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=348839&action=review r=me > Source/WebCore/ChangeLog:10 > + Let us consider each scenairo separately. 'scenario' > Source/WebCore/ChangeLog:51 > + node insertion, removal, or rename but no longer has after the mutation. This patch also adds a slot mutation > + version number, m_slotMutationVersion, which is incremented whenever a node is about to be inserted or removed, > + and slot resolution version, m_slotResolutionVersion, which is set to the current slot mutation version number > + when the full slot resolution is triggered during slot mutations. They are used to avoid redundant tree traversals I like the versioning approach. > Source/WebCore/ChangeLog:74 > + While the DOM specification currently specifies the former beavhior, this patch implements the latter behavior 'behavior' > Source/WebCore/dom/SlotAssignment.cpp:181 > + if (!slot->element) // A previous invocation to resolveSlotsAfterSlotMutation during this removal has updated this slot. > + ASSERT(m_slotResolutionVersion == m_slotMutationVersion && !findSlotElement(shadowRoot, name)); > + else { Maybe just ASSERT(slot->element || (m_slotResolutionVersion == m_slotMutationVersion && !findSlotElement(shadowRoot, name))); A branch with nothing but assert doesn't look nice.

Antti Koivisto

Comment 20 2018-09-04 16:52:24 PDT

Comment on attachment 348839 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=348839&action=review > Source/WebCore/dom/SlotAssignment.cpp:175 > + bool elementWasRenamed = !oldParentOfRemovedTree; This is a rather magical way to signal the rename case.

Ryosuke Niwa

Comment 21 2018-09-04 17:15:50 PDT

(In reply to Antti Koivisto from comment #19) > Comment on attachment 348839 [details] > Fixes the bug > > View in context: > https://bugs.webkit.org/attachment.cgi?id=348839&action=review > > r=me > > > Source/WebCore/ChangeLog:10 > > + Let us consider each scenairo separately. > > 'scenario' Fixed. > > Source/WebCore/ChangeLog:51 > > + node insertion, removal, or rename but no longer has after the mutation. This patch also adds a slot mutation > > + version number, m_slotMutationVersion, which is incremented whenever a node is about to be inserted or removed, > > + and slot resolution version, m_slotResolutionVersion, which is set to the current slot mutation version number > > + when the full slot resolution is triggered during slot mutations. They are used to avoid redundant tree traversals > > I like the versioning approach. Great! > > Source/WebCore/ChangeLog:74 > > + While the DOM specification currently specifies the former beavhior, this patch implements the latter behavior > > 'behavior' Fixed. > > Source/WebCore/dom/SlotAssignment.cpp:181 > > + if (!slot->element) // A previous invocation to resolveSlotsAfterSlotMutation during this removal has updated this slot. > > + ASSERT(m_slotResolutionVersion == m_slotMutationVersion && !findSlotElement(shadowRoot, name)); > > + else { > > Maybe just > > ASSERT(slot->element || (m_slotResolutionVersion == m_slotMutationVersion && > !findSlotElement(shadowRoot, name))); > > A branch with nothing but assert doesn't look nice. Sure. Fixed.

Ryosuke Niwa

Comment 22 2018-09-04 17:16:09 PDT

(In reply to Antti Koivisto from comment #20) > Comment on attachment 348839 [details] > Fixes the bug > > View in context: > https://bugs.webkit.org/attachment.cgi?id=348839&action=review > > > Source/WebCore/dom/SlotAssignment.cpp:175 > > + bool elementWasRenamed = !oldParentOfRemovedTree; > > This is a rather magical way to signal the rename case. Renamed oldParentOfRemovedTree to oldParentOfRemovedTreeForRemoval as we discussed.

Ryosuke Niwa

Comment 23 2018-09-04 17:22:45 PDT

Committed r235650: <https://trac.webkit.org/changeset/235650>

Ryosuke Niwa

Comment 24 2018-10-04 14:41:12 PDT

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

Lucas Forschler

Comment 25 2019-02-06 09:18:36 PST

Mass move bugs into the DOM component.

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