Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1606153002:
Implement v1 slot logic in EventPath (Closed)
DescriptionImplement v1 slot logic in EventPath
This CL makes sure that when an event happens on an element which is assigned to a slot, the event path goes to the slot after the element and goes up the v1 tree.
Described here in spec.
http://w3c.github.io/webcomponents/spec/shadow/#event-paths
BUG=531990
Committed: https://crrev.com/b457ca4d9a86e52c11d906b56f55bdf4c6f1136f
Cr-Commit-Position: refs/heads/master@{#370648}
Patch Set 1 #Patch Set 2 : Remove an unintended change #Patch Set 3 : Remove an unintended change #Patch Set 4 : Remove an unintended change #
Total comments: 16
Patch Set 5 : Add a test case in layout test #
Total comments: 7
Patch Set 6 : Add done() in each test case #Patch Set 7 : Modify the layout test #
Messages
Total messages: 19 (5 generated)
Description was changed from ========== Implement slot logic in EventPath BUG= ========== to ========== Implement v1 slot logic in EventPath This CL makes sure that when an event happens on an element which is assigned to a slot, the event path goes to the slot after the element and goes up the v1 tree. Described here in spec. http://w3c.github.io/webcomponents/spec/shadow/#event-paths BUG=531990 ==========
yuzus@chromium.org changed reviewers: + hayato@chromium.org, kochi@chromium.org, kojii@chromium.org
https://codereview.chromium.org/1606153002/diff/50001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html (right): https://codereview.chromium.org/1606153002/diff/50001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html:6: <img id="img" src="../../images/resources/test-load.jpg" slot='slot-1'> nit: please use single quote and double quote consistently within a file. For this html, all attribute values should be quoted by double quotes, and in JavaScript code strings should be quoted by single quotes. https://codereview.chromium.org/1606153002/diff/50001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html:32: var img = document.getElementById('img'); Loading an image will make this test slower. For example, <input> can substitute this, and use input.focus() to dispatch focus event, then handle onfocus event. https://codereview.chromium.org/1606153002/diff/50001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html:38: expected_array = [img, slot, shadowRoot, host, document.body, document.documentElement, document]; I thought the last element in event.path would be window. Could you check? https://codereview.chromium.org/1606153002/diff/50001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html:44: var img2 = document.getElementById('img2'); Same as above, could you avoid using <img>? https://codereview.chromium.org/1606153002/diff/50001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html:47: var srv1 = host2.shadowRoot; nit: as other ids are not abbreviated, could this and "srv0" below be shadowRootV0 and shadowRootV1 for consistency? https://codereview.chromium.org/1606153002/diff/50001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html:56: expected_array = [img2, slot2, slotParent, content, srv0, host3, srv1, host2, document.body, document.documentElement, document]; Same as above, could you check if the path ends in document, not in window? https://codereview.chromium.org/1606153002/diff/50001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html:61: Could you also add a test that original event target is not the directly slotted element, but a descendant element of the slotted element? e.g. <div id="host"> #shadow-root <slot> <div> <input> </div> </div> => [input, div, slot, #shadow-root, div#host, ...]
https://codereview.chromium.org/1606153002/diff/50001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html (right): https://codereview.chromium.org/1606153002/diff/50001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html:38: expected_array = [img, slot, shadowRoot, host, document.body, document.documentElement, document]; On 2016/01/20 09:22:07, kochi wrote: > I thought the last element in event.path would be window. > Could you check? I was told that "load" event is a special case that the event path stops at document and does not reach window. Probably if you change the event type to something else, you will see window at the end of the array.
https://codereview.chromium.org/1606153002/diff/50001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html (right): https://codereview.chromium.org/1606153002/diff/50001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html:6: <img id="img" src="../../images/resources/test-load.jpg" slot='slot-1'> On 2016/01/20 09:22:07, kochi wrote: > nit: please use single quote and double quote consistently within a file. > > For this html, all attribute values should be quoted by double quotes, > and in JavaScript code strings should be quoted by single quotes. Done. https://codereview.chromium.org/1606153002/diff/50001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html:32: var img = document.getElementById('img'); On 2016/01/20 09:22:07, kochi wrote: > Loading an image will make this test slower. > For example, <input> can substitute this, and use input.focus() to > dispatch focus event, then handle onfocus event. Done. https://codereview.chromium.org/1606153002/diff/50001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html:38: expected_array = [img, slot, shadowRoot, host, document.body, document.documentElement, document]; On 2016/01/20 09:22:07, kochi wrote: > I thought the last element in event.path would be window. > Could you check? Done. https://codereview.chromium.org/1606153002/diff/50001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html:38: expected_array = [img, slot, shadowRoot, host, document.body, document.documentElement, document]; On 2016/01/20 09:39:05, kochi wrote: > On 2016/01/20 09:22:07, kochi wrote: > > I thought the last element in event.path would be window. > > Could you check? > > I was told that "load" event is a special case that the event path stops > at document and does not reach window. > > Probably if you change the event type to something else, you will see window > at the end of the array. Done. https://codereview.chromium.org/1606153002/diff/50001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html:44: var img2 = document.getElementById('img2'); On 2016/01/20 09:22:07, kochi wrote: > Same as above, could you avoid using <img>? Done. https://codereview.chromium.org/1606153002/diff/50001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html:47: var srv1 = host2.shadowRoot; On 2016/01/20 09:22:07, kochi wrote: > nit: as other ids are not abbreviated, could this and "srv0" below be > shadowRootV0 and shadowRootV1 for consistency? Done. https://codereview.chromium.org/1606153002/diff/50001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html:56: expected_array = [img2, slot2, slotParent, content, srv0, host3, srv1, host2, document.body, document.documentElement, document]; On 2016/01/20 09:22:07, kochi wrote: > Same as above, could you check if the path ends in document, not in window? Done. https://codereview.chromium.org/1606153002/diff/50001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html:61: On 2016/01/20 09:22:07, kochi wrote: > Could you also add a test that original event target is not the directly slotted > element, but a descendant element of the slotted element? > > e.g. > > <div id="host"> > #shadow-root > <slot> > <div> > <input> > </div> > </div> > > => [input, div, slot, #shadow-root, div#host, ...] Done.
https://codereview.chromium.org/1606153002/diff/70001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html (right): https://codereview.chromium.org/1606153002/diff/70001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html:36: var host1 = document.getElementById('host1'); nit: if an element with ID "host1" (in this file, <div id='host1'>) exists, HTML parser implicitly creates window.host1 which can be accessed without "window." in <script></script>. So you can omit this line if you like, though I prefer having this line, which is explicit. I'm not so familiar with how this is spec'ed, but there should be exceptions, e.g. <div id="host-1"> doesn't work this way, or such element appears after script execution, if you set id via script, etc. etc. https://codereview.chromium.org/1606153002/diff/70001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html:47: test(function() { If the event doesn't happen accidentally, this test also passes. It would be better to wrap this whole test in test(function() {..}). (you may end up in needing test_async(), as it includes sending an event.) Then this test html file has 3 tests eventually.
https://codereview.chromium.org/1606153002/diff/70001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html (right): https://codereview.chromium.org/1606153002/diff/70001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html:47: test(function() { On 2016/01/21 01:26:33, kochi wrote: > If the event doesn't happen accidentally, this test also passes. > It would be better to wrap this whole test in test(function() {..}). > (you may end up in needing test_async(), as it includes sending an event.) The "focus" event is synchronous (i.e. when such event happens, its associated handler function is called immediately without waiting for the current script function to finish, so you should not need test_async() here.
https://codereview.chromium.org/1606153002/diff/70001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html (right): https://codereview.chromium.org/1606153002/diff/70001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html:36: var host1 = document.getElementById('host1'); On 2016/01/21 01:26:33, kochi wrote: > nit: if an element with ID "host1" (in this file, <div id='host1'>) exists, > HTML parser implicitly creates window.host1 which can be accessed without > "window." in <script></script>. So you can omit this line if you like, though > I prefer having this line, which is explicit. > > I'm not so familiar with how this is spec'ed, but there should be exceptions, > e.g. <div id="host-1"> doesn't work this way, or such element appears after > script execution, if you set id via script, etc. etc. Done. https://codereview.chromium.org/1606153002/diff/70001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html:47: test(function() { Thanks for pointing out the issue. How about adding "setup({ explicit_done: true });" and having "done();" in each test, which would catch failure cases when the events do not happen? On 2016/01/21 01:26:33, kochi wrote: > If the event doesn't happen accidentally, this test also passes. > It would be better to wrap this whole test in test(function() {..}). > (you may end up in needing test_async(), as it includes sending an event.) > > Then this test html file has 3 tests eventually.
https://codereview.chromium.org/1606153002/diff/70001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html (right): https://codereview.chromium.org/1606153002/diff/70001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html:47: test(function() { On 2016/01/21 05:10:22, yuzuchan wrote: > Thanks for pointing out the issue. How about adding "setup({ explicit_done: true > });" and having "done();" in each test, which would catch failure cases when the > events do not happen? One setup() can only check one "done();"? So if you call setup() only once and call done() 3 times from 3 tests, it thinks all tests finished at the first done() call? (I was referring to https://github.com/w3c/testharness.js/blob/master/docs/api.md#determining-whe... )
https://codereview.chromium.org/1606153002/diff/70001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html (right): https://codereview.chromium.org/1606153002/diff/70001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/dom/shadow/event-path-with-slot.html:47: test(function() { On 2016/01/21 05:22:58, kochi wrote: > On 2016/01/21 05:10:22, yuzuchan wrote: > > Thanks for pointing out the issue. How about adding "setup({ explicit_done: > true > > });" and having "done();" in each test, which would catch failure cases when > the > > events do not happen? > > One setup() can only check one "done();"? > > So if you call setup() only once and call done() 3 times from 3 tests, it > thinks all tests finished at the first done() call? > > (I was referring to > https://github.com/w3c/testharness.js/blob/master/docs/api.md#determining-whe... > ) Done.
lgtm Hayato-san, could you OWENERS review this?
lgtm We might want to add more tests, e.g. a slot is assigned to another slot, but we can add a test later. You can land this CL as long as this does not break any existing layout tests.
The CQ bit was checked by yuzus@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1606153002/110001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1606153002/110001
Message was sent while issue was closed.
Description was changed from ========== Implement v1 slot logic in EventPath This CL makes sure that when an event happens on an element which is assigned to a slot, the event path goes to the slot after the element and goes up the v1 tree. Described here in spec. http://w3c.github.io/webcomponents/spec/shadow/#event-paths BUG=531990 ========== to ========== Implement v1 slot logic in EventPath This CL makes sure that when an event happens on an element which is assigned to a slot, the event path goes to the slot after the element and goes up the v1 tree. Described here in spec. http://w3c.github.io/webcomponents/spec/shadow/#event-paths BUG=531990 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:110001)
Message was sent while issue was closed.
Description was changed from ========== Implement v1 slot logic in EventPath This CL makes sure that when an event happens on an element which is assigned to a slot, the event path goes to the slot after the element and goes up the v1 tree. Described here in spec. http://w3c.github.io/webcomponents/spec/shadow/#event-paths BUG=531990 ========== to ========== Implement v1 slot logic in EventPath This CL makes sure that when an event happens on an element which is assigned to a slot, the event path goes to the slot after the element and goes up the v1 tree. Described here in spec. http://w3c.github.io/webcomponents/spec/shadow/#event-paths BUG=531990 Committed: https://crrev.com/b457ca4d9a86e52c11d906b56f55bdf4c6f1136f Cr-Commit-Position: refs/heads/master@{#370648} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b457ca4d9a86e52c11d906b56f55bdf4c6f1136f Cr-Commit-Position: refs/heads/master@{#370648} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
