-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update tests for slot change events #5954
Update tests for slot change events #5954
Conversation
Firefox (nightly)Testing web-platform-tests at revision 65932ee All results4 tests ran/shadow-dom/slotchange-event.html
/shadow-dom/slots-fallback-in-document.html
/shadow-dom/slots-fallback.html
/shadow-dom/slots.html
|
Sauce (safari)Testing web-platform-tests at revision 65932ee All results4 tests ran/shadow-dom/slotchange-event.html
/shadow-dom/slots-fallback-in-document.html
/shadow-dom/slots-fallback.html
/shadow-dom/slots.html
|
Sauce (MicrosoftEdge)Testing web-platform-tests at revision 65932ee All results4 tests ran/shadow-dom/slotchange-event.html
/shadow-dom/slots-fallback-in-document.html
/shadow-dom/slots-fallback.html
/shadow-dom/slots.html
|
Chrome (unstable)Testing web-platform-tests at revision 65932ee All results4 tests ran/shadow-dom/slotchange-event.html
/shadow-dom/slots-fallback-in-document.html
/shadow-dom/slots-fallback.html
/shadow-dom/slots.html
|
759b487
to
627ed45
Compare
eebb9d0
to
00f36c3
Compare
This corresponds to the DOM Standard change: whatwg/dom#459. We might want to clean up tests further because this patch is rather the minimum effort to catch up the DOM Standard's change, but should be good enough.
00f36c3
to
d28a7f3
Compare
I have updated the PR so that it reflects the latest whatwg/dom#459. cc: @domenic |
@TakayoshiKochi Could you review this too? |
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…nge concept Rewrite the core part of the Shadow DOM v1 distribution engine so that distribution recalculation happens *if and only if* a newly-defined concept of a slotchange actually happens. Benefits: - Performance improvement - Eliminated code complexity - Becomes spec-compliant (as a side effect, this is one of the motivations of rewriting) 1. Performance improvement: The result of PerformanceTests/ShadowDOM performance tests: v1-distribution-disconnected-and-reconnected Before CL: 201.4 ms After CL: 41.7 ms (5x times faster) 2. Eliminated code complexity: Though I have a plan to explain the detail about how the v1 distribution engine works in core/dom/shadow/README.md file, let me explain the benefits of the new design here other than the performance tests: - Eliminated false-positive for setting a dirty flag for distribution recalculation Before this CL, the engine sets a dirty flag for distribution conservatively. As a result, a false-positive can happen, which has been difficult to avoid because distribution is not a local effect. We don't have much budget of time in DOM mutation. After this CL, the engine sets a dirty flag if and only if a slotchange actually happens. No longer needs to set a dirty flag in other places. Note that the engine only detects the fact of "a slotchange happens", but it doesn't try to know an exact distributed nodes for each slot at the timing of DOM mutation so that DOM mutation should not be blocked more than necessary. Distributed nodes are lazily-calculated later. - Life cycle of slot's distribution nodes became more clear The engine no longer clears out each slot's distributed nodes in shadow-including descendant subtrees when the subtree is disconnected from the parent tree. e.g. We don't need to update distribution for a custom element which has deeply nested other custom elements inside of it at each insertion/removal from a tree. The performance test's improvement came mostly from this result. - Eliminated a lot of tricky code which is needed to support <slot> in non-shadow trees. I successfully stopped to support <slot> in non-shadow trees, and upstreamed the decision to WHATWG DOM Standard [1], getting agreement from other browser vendors [2]. I have already updated web platform tests [3] too. These tests no longer fail after this CL. The support of <slot> in non-shadow trees has been difficult to support, and has been the cause of crashes. We no longer have to fight with crashes. 3. Becomes spec-compliant (as a side effect, this is one of the motivations of rewriting) From the Web developers' perspective, this CL shouldn't have any practical impact, as long as a slot element is only used in shadow trees. The only practical visible change is: A slotchange event is always signaled as a microtask whenever a slot's distributed nodes are changed. For example, when a slot is inserted or removed from a tree, a slotchange event can be fired. Before this CL, a slotchange is never fired in this case. See DOM Standard issue [2] for details, which is rather a feature request from web developers. This CL satisfies this requirement, as an intended side effect. I am aware that it would be better to separate the engine rewriting from the user visible changes, but it would require unnecessary efforts to keep the old behavior in the new engine. Thus, I put all together. Supporting [2] is one of the reasons I decided to rewrite the engine. Note that only Shadow DOM v1 can get these benefits. Shadow DOM v0 is out of the scope of this CL. - [1] DOM Standard change: whatwg/dom#459 - [2] DOM Standard issue: whatwg/dom#447 - [3] Web platform tests change: web-platform-tests/wpt#5954 Links: Change-Id: I41f29e781185c46739377ab3939d20fa24fb69bf Reviewed-on: https://chromium-review.googlesource.com/532734 Commit-Queue: Hayato Ito <hayato@chromium.org> Reviewed-by: Takayoshi Kochi <kochi@chromium.org> Reviewed-by: Kent Tamura <tkent@chromium.org> Cr-Commit-Position: refs/heads/master@{#480013}
…t is inserted
DOM Standard chagne is whatwg/dom#459.