Bug 189144 - slotchange event doesn't get fired when inserting, removing, or renaming slot elements
Summary: slotchange event doesn't get fired when inserting, removing, or renaming slot...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
: 190204 (view as bug list)
Depends on: 189183
Blocks: 148695
  Show dependency treegraph
 
Reported: 2018-08-29 20:45 PDT by Ryosuke Niwa
Modified: 2019-02-06 09:18 PST (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 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.
Comment 1 Radar WebKit Bug Importer 2018-08-29 20:45:50 PDT
<rdar://problem/43871061>
Comment 2 Ryosuke Niwa 2018-08-29 20:47:45 PDT
Created attachment 348477 [details]
WIP
Comment 3 EWS Watchlist 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
Comment 4 EWS Watchlist 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
Comment 5 EWS Watchlist 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
Comment 6 EWS Watchlist 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
Comment 7 Ryosuke Niwa 2018-08-30 19:31:14 PDT
Created attachment 348583 [details]
WIP2
Comment 8 Ryosuke Niwa 2018-08-30 21:33:47 PDT
Created attachment 348594 [details]
WIP3
Comment 9 EWS Watchlist 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.
Comment 10 Ryosuke Niwa 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.
Comment 11 Ryosuke Niwa 2018-08-31 19:45:08 PDT
Created attachment 348699 [details]
WIP4 (rebased for trunk)
Comment 12 EWS Watchlist 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.
Comment 13 Ryosuke Niwa 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.
Comment 14 EWS Watchlist 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.
Comment 15 Ryosuke Niwa 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.
Comment 16 Ryosuke Niwa 2018-09-01 00:56:26 PDT
Created attachment 348711 [details]
WIP6 (with code change)
Comment 17 EWS Watchlist 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.
Comment 18 Ryosuke Niwa 2018-09-04 12:36:59 PDT
Created attachment 348839 [details]
Fixes the bug
Comment 19 Antti Koivisto 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.
Comment 20 Antti Koivisto 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.
Comment 21 Ryosuke Niwa 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.
Comment 22 Ryosuke Niwa 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.
Comment 23 Ryosuke Niwa 2018-09-04 17:22:45 PDT
Committed r235650: <https://trac.webkit.org/changeset/235650>
Comment 24 Ryosuke Niwa 2018-10-04 14:41:12 PDT
*** Bug 190204 has been marked as a duplicate of this bug. ***
Comment 25 Lucas Forschler 2019-02-06 09:18:36 PST
Mass move bugs into the DOM component.