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

RESOLVED FIXED 189144

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

https://bugs.webkit.org/show_bug.cgi?id=189144

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

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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component DOM

Assignee

Ryosuke Niwa

Reported

2018-08-29 20:45 PDT

Modified

2019-02-06 09:18 PST History

CC List

9 users Show

URL

Keywords InRadar

Duplicates (1)

190204 View as bug list

Depends on

189183

Blocks

148695

Dependencies

tree graph