Don't let a slot do anything special if it isn't in a shadow tree #459

hayatoito · 2017-05-15T05:26:36Z

Fixes #447.

Preview | Diff

hayatoito · 2017-05-15T05:29:14Z

@surma is taking care of the PR for the tests: web-platform-tests/wpt#5705

DOM Standard chagne is whatwg/dom#459.

hayatoito · 2017-05-17T07:19:17Z

It looks we have to update existing tests in WPT too.
I am working on that at web-platform-tests/wpt#5954.

annevk · 2017-05-17T07:26:44Z

This change LGTM. We only need bugs against Chrome and WebKit I suspect, since nobody else is far enough along thus far.

hayatoito · 2017-05-18T02:13:46Z

Yeah, regarding Chrome, I'll take care of it. I'll also update WPT so that the test would fail if UA doesn't support this change.

hayatoito · 2017-06-02T09:52:21Z

I have updated the PR so that it can reflect the conclusion in #447.
@domenic , could you have a chance to review this?
cc: @annevk

domenic

I can merge this and check any editorial things while @annevk is out of office for the next month or so, but I would like @rniwa to review the normative contents and the tests.

domenic · 2017-06-02T17:18:53Z

dom.bs

- <var>slot</var>'s <a for=slot>assigned nodes</a> are not identical, then run
- <a>signal a slot change</a> for <var>slot</var>.
+ <li><p>If <var>slotables</var> and <var>slot</var>'s <a for=slot>assigned
+ nodes</a> are not identical, then run <a>signal a slot change</a> for <var>slot</var>.


I notice that this instance of "signal a slot change" has fewer checks before it than the other two. Is that correct? (I don't understand the algorithms that well so I am just pattern-matching in the hopes of finding something useful :)

Yes, that is correct and intentional.

For assigned nodes' changes, the condition became relaxed.

For other cases (aka fallback contents), the condition became more tight.

domenic · 2017-06-02T17:19:41Z

dom.bs

- <var>slot</var>'s <a for=slot>assigned nodes</a> are not identical, then run
- <a>signal a slot change</a> for <var>slot</var>.
+ <li><p>If <var>slotables</var> and <var>slot</var>'s <a for=slot>assigned
+ nodes</a> are not identical, then run <a>signal a slot change</a> for <var>slot</var>.


Nit: annevk prefers tags all on one line in his specs, so no linebreak in the middle of an <a>stuff stuff</a>. So the whole <a for=slot>assigned nodes</a> should get pushed to the next line.

Thanks. Let me put it in the same line.

hayatoito · 2017-06-05T06:04:26Z

@domenic Thank you for the review.
I have updated the PR, by squashing the commits.

I am now working on updating WPT. I'll ping @rniwa in PR for WPT too.

hayatoito · 2017-06-05T06:06:03Z

dom.bs

@@ -1719,7 +1722,8 @@ for a given <a>slot</a> <var>slot</var>, run these steps:</p>

  <ol>
   <li>
-    <p>If <var>node</var> is a <a>slot</a>, then:
+    <p>If <var>node</var> is a <a>slot</a> whose <a for=tree>root</a> is a <a for=/>shadow root</a>,


I also updated this line, which I forgot to fix in the previous PR.

DOM Standard chagne is whatwg/dom#459.

This corresponds to the DOM Standard chagne: whatwg/dom#459. We might want to clean up tests further because this patch is the minimum effort to catch up the DOM Standard's change, but should be good enough.

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.

hayatoito · 2017-06-07T04:24:23Z

@rniwa, could you have a chance to take a look?

PR for WPT is here: web-platform-tests/wpt#5954

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.

…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 <[email protected]> Reviewed-by: Takayoshi Kochi <[email protected]> Reviewed-by: Kent Tamura <[email protected]> Cr-Commit-Position: refs/heads/master@{#480013}

hayatoito · 2017-06-19T08:46:55Z

FYI. I implemented this new behavior in Chrome Canary.

hayatoito · 2017-06-26T07:44:44Z

@rniwa, friendly ping. I am assuming that this change sounds good to all of us.

rniwa · 2017-06-26T07:52:18Z

The intention seems fine but did you check that we don't end up signaling the same slot multiple times per a single DOM change especially nested ones?

I haven't gotten around to fix WebKit's implementation but I intend to do it in near future.

hayatoito · 2017-06-26T08:04:40Z

The intention seems fine but did you check that we don't end up signaling the same slot multiple times per a single DOM change especially nested ones?

That shouldn't happen, as far as my understanding is correct. A slotchange is signaled at most once per a slot per a single DOM change for any tree structures.

rniwa · 2017-06-26T08:12:17Z

I understand that's the intention but it's really hard for me to tell if that's what's gonna happen with this change or not.

In general, I find it really hard to read these spec diffs and figure out what's actually happening. It would be ideal if we could look at the spec with the change applied. Is there a way to do that?

hayatoito · 2017-06-26T08:25:23Z

It looks this DOM repository adds a Preview or Diff link automatically in the first comment in PR.
I guess such a nice feature was recently introduced.

Though Diff link looks ugly for unknown reasons (Hmm, I remember that it showed a clean diff when I updated this PR), I think Preview link still works.

annevk · 2017-06-28T12:21:52Z

@hayatoito it looks like you need to rebase on master and force push. I suspect your change conflicts with 93db340.

hayatoito · 2017-06-29T04:59:14Z

@annevk Thanks. I have rebased on master and force push.

annevk · 2017-06-29T14:17:20Z

Great!

@rniwa you can review changes in a rendered way starting here (I think there's nothing before this point): https://s3.amazonaws.com/pr-preview/whatwg/dom/d9cd30d...hayatoito:32e6669.html#find-flattened-slotables.

rniwa · 2017-06-30T03:08:54Z

That diff link was very useful! The change looks good to me.

r=me.

annevk · 2017-06-30T17:30:04Z

dom.bs

- <var>slot</var>'s <a for=slot>assigned nodes</a> are not identical, then run
- <a>signal a slot change</a> for <var>slot</var>.
+ <li><p>If <var>slotables</var> and <var>slot</var>'s <a for=slot>assigned nodes</a> are not
+identical, then run <a>signal a slot change</a> for <var>slot</var>.


Could you add a space before identical? (I would do it, but I cannot modify this branch for reasons unclear.)

My bad. Done. I've updated the PR.

A slot's children are no longer considered as the slot's fallback contents if the slot is not in a shadow tree. Such a slot no longer signals a slot change even if its children are updated. This patch also removes a concept of suppress signaling flag for a slot change entirely. A slot change is always signaled when a slot's assigned nodes are changed. Fixes whatwg#447.

annevk · 2017-07-04T09:03:25Z

Due to jetlag I forgot to complete the usual checklist:

Tests haven't landed yet. @hayatoito will take care of that per Add shadowdom-customelements tests web-platform-tests/wpt#5705 (comment).
Browser bugs haven't been filed. @rniwa and @hayatoito, I suspect you'll take care of that for your respective engines? Nobody else implements this yet so I'm guessing nothing is needed for them.

hayatoito · 2017-07-05T03:18:43Z

Thanks for merging.
Regarding tests, I have updated existing tests for slotshange to reflect this change in another PR: web-platform-tests/wpt#5954
I'll review web-platform-tests/wpt#5705 (comment) too.

Regarding implementation, I could take care of Blink. I think @rniwa would take care of WebKit.

annevk · 2017-07-05T07:34:19Z

Thanks, I'm going to assume all is in order then.

hayatoito mentioned this pull request May 17, 2017

Update tests for slot change events web-platform-tests/wpt#5954

Merged

hayatoito added a commit to hayatoito/wpt that referenced this pull request May 17, 2017

Update slotchange event tests for slots being inserted

627ed45

DOM Standard chagne is whatwg/dom#459.

hayatoito mentioned this pull request May 19, 2017

Shadow: Specify when slotchange fires #447

Closed

hayatoito force-pushed the slotchange-when-inserted branch from ff0fdaf to e943a7e Compare June 2, 2017 09:03

hayatoito changed the title ~~Don't set a suppress signaling flag when a slot is inserted~~ Don't let slot do anything special if it isn't in a shadow tree Jun 2, 2017

hayatoito force-pushed the slotchange-when-inserted branch from e943a7e to 35064d2 Compare June 2, 2017 09:09

hayatoito changed the title ~~Don't let slot do anything special if it isn't in a shadow tree~~ Don't let a slot do anything special if it isn't in a shadow tree Jun 2, 2017

hayatoito force-pushed the slotchange-when-inserted branch from 35064d2 to 2bd8992 Compare June 2, 2017 09:57

hayatoito mentioned this pull request Jun 2, 2017

Add shadowdom-customelements tests web-platform-tests/wpt#5705

Merged

domenic reviewed Jun 2, 2017

View reviewed changes

hayatoito force-pushed the slotchange-when-inserted branch from 2bd8992 to 2a0e664 Compare June 5, 2017 06:01

hayatoito commented Jun 5, 2017

View reviewed changes

hayatoito added a commit to hayatoito/wpt that referenced this pull request Jun 5, 2017

Update a slot change event tests

eebb9d0

DOM Standard chagne is whatwg/dom#459.

hayatoito force-pushed the slotchange-when-inserted branch from 2a0e664 to 32e6669 Compare June 29, 2017 04:53

annevk approved these changes Jun 30, 2017

View reviewed changes

hayatoito force-pushed the slotchange-when-inserted branch from 32e6669 to e1db083 Compare July 3, 2017 04:12

annevk merged commit 8f8c1c3 into whatwg:master Jul 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't let a slot do anything special if it isn't in a shadow tree #459

Don't let a slot do anything special if it isn't in a shadow tree #459

hayatoito commented May 15, 2017 •

edited by pr-preview bot

Loading

hayatoito commented May 15, 2017

hayatoito commented May 17, 2017 •

edited

Loading

annevk commented May 17, 2017

hayatoito commented May 18, 2017 •

edited

Loading

hayatoito commented Jun 2, 2017

domenic left a comment •

edited

Loading

domenic Jun 2, 2017

hayatoito Jun 5, 2017 •

edited

Loading

domenic Jun 2, 2017

hayatoito Jun 5, 2017

hayatoito commented Jun 5, 2017

hayatoito Jun 5, 2017

hayatoito commented Jun 7, 2017 •

edited

Loading

hayatoito commented Jun 19, 2017 •

edited

Loading

hayatoito commented Jun 26, 2017

rniwa commented Jun 26, 2017

hayatoito commented Jun 26, 2017

rniwa commented Jun 26, 2017

hayatoito commented Jun 26, 2017 •

edited

Loading

annevk commented Jun 28, 2017

hayatoito commented Jun 29, 2017

annevk commented Jun 29, 2017

rniwa commented Jun 30, 2017

annevk Jun 30, 2017

hayatoito Jul 3, 2017

annevk commented Jul 4, 2017

hayatoito commented Jul 5, 2017

annevk commented Jul 5, 2017

Don't let a slot do anything special if it isn't in a shadow tree #459

Don't let a slot do anything special if it isn't in a shadow tree #459

Conversation

hayatoito commented May 15, 2017 • edited by pr-preview bot Loading

hayatoito commented May 15, 2017

hayatoito commented May 17, 2017 • edited Loading

annevk commented May 17, 2017

hayatoito commented May 18, 2017 • edited Loading

hayatoito commented Jun 2, 2017

domenic left a comment • edited Loading

Choose a reason for hiding this comment

domenic Jun 2, 2017

Choose a reason for hiding this comment

hayatoito Jun 5, 2017 • edited Loading

Choose a reason for hiding this comment

domenic Jun 2, 2017

Choose a reason for hiding this comment

hayatoito Jun 5, 2017

Choose a reason for hiding this comment

hayatoito commented Jun 5, 2017

hayatoito Jun 5, 2017

Choose a reason for hiding this comment

hayatoito commented Jun 7, 2017 • edited Loading

hayatoito commented Jun 19, 2017 • edited Loading

hayatoito commented Jun 26, 2017

rniwa commented Jun 26, 2017

hayatoito commented Jun 26, 2017

rniwa commented Jun 26, 2017

hayatoito commented Jun 26, 2017 • edited Loading

annevk commented Jun 28, 2017

hayatoito commented Jun 29, 2017

annevk commented Jun 29, 2017

rniwa commented Jun 30, 2017

annevk Jun 30, 2017

Choose a reason for hiding this comment

hayatoito Jul 3, 2017

Choose a reason for hiding this comment

annevk commented Jul 4, 2017

hayatoito commented Jul 5, 2017

annevk commented Jul 5, 2017

hayatoito commented May 15, 2017 •

edited by pr-preview bot

Loading

hayatoito commented May 17, 2017 •

edited

Loading

hayatoito commented May 18, 2017 •

edited

Loading

domenic left a comment •

edited

Loading

hayatoito Jun 5, 2017 •

edited

Loading

hayatoito commented Jun 7, 2017 •

edited

Loading

hayatoito commented Jun 19, 2017 •

edited

Loading

hayatoito commented Jun 26, 2017 •

edited

Loading