Fix for calculation of event.path for closed mode shadow root

Fix for calculation of event.path for closed mode shadow root

Old code assumed that only one level of UA shadow tree
nodes should be excluded from event path, while it is
no longer true after introduction of shadow root
closed mode.

This change properly hides all the nodes behind closed
shadow root in the event path.

BUG=459136
TEST=event-path-closed-shadowroot.html, event-path-closed-shadowroot2.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201391

[kochi, 2015-08-20 12:22:22]

kochi@chromium.org changed reviewers:
+ hayato@chromium.org

[kochi, 2015-08-20 12:22:33]

The CQ bit was checked by kochi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-20 12:22:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1305733002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1305733002/1

[commit-bot: I haz the power, 2015-08-20 13:28:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-20 13:28:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kochi, 2015-08-24 10:15:39]

PTAL

[hayato, 2015-08-25 01:34:20]

https://codereview.chromium.org/1305733002/diff/160001/LayoutTests/fast/dom/s...
File LayoutTests/fast/dom/shadow/event-path-closed-shadowroot.html (right):

https://codereview.chromium.org/1305733002/diff/160001/LayoutTests/fast/dom/s...
LayoutTests/fast/dom/shadow/event-path-closed-shadowroot.html:9:
createShadowRoot({mode: 'open'},
Could you add an id attribute to Shadow Roots so the expected texts could be
more readable?

https://codereview.chromium.org/1305733002/diff/160001/LayoutTests/fast/dom/s...
LayoutTests/fast/dom/shadow/event-path-closed-shadowroot.html:28:
debug('Event.path should include only unclosed elements.');
s/elements/nodes/

https://codereview.chromium.org/1305733002/diff/160001/Source/core/events/Tre...
File Source/core/events/TreeScopeEventContext.h (right):

https://codereview.chromium.org/1305733002/diff/160001/Source/core/events/Tre...
Source/core/events/TreeScopeEventContext.h:77: int
calculateTreeOrderAndFindClosedTrees(int, TreeScopeEventContext*);
You shouldn't omit a parameter name for int.

calculateTreeOrderAndSetNearestAncestorClosedTree might be a better name.

https://codereview.chromium.org/1305733002/diff/160001/Source/core/events/Tre...
Source/core/events/TreeScopeEventContext.h:88: bool shouldIncludeTreeScope(const
TreeScopeEventContext& other);
More descriptive name would be preferred. What 'Include' means here might be
unclear for readers.

[kochi, 2015-08-25 02:18:31]

Addressed comments.
PTAL

https://codereview.chromium.org/1305733002/diff/160001/LayoutTests/fast/dom/s...
File LayoutTests/fast/dom/shadow/event-path-closed-shadowroot.html (right):

https://codereview.chromium.org/1305733002/diff/160001/LayoutTests/fast/dom/s...
LayoutTests/fast/dom/shadow/event-path-closed-shadowroot.html:9:
createShadowRoot({mode: 'open'},
On 2015/08/25 01:34:20, hayato wrote:
> Could you add an id attribute to Shadow Roots so the expected texts could be
> more readable?

Done.

https://codereview.chromium.org/1305733002/diff/160001/LayoutTests/fast/dom/s...
LayoutTests/fast/dom/shadow/event-path-closed-shadowroot.html:28:
debug('Event.path should include only unclosed elements.');
On 2015/08/25 01:34:20, hayato wrote:
> s/elements/nodes/

Done.

https://codereview.chromium.org/1305733002/diff/160001/Source/core/events/Tre...
File Source/core/events/TreeScopeEventContext.h (right):

https://codereview.chromium.org/1305733002/diff/160001/Source/core/events/Tre...
Source/core/events/TreeScopeEventContext.h:77: int
calculateTreeOrderAndFindClosedTrees(int, TreeScopeEventContext*);
On 2015/08/25 01:34:20, hayato wrote:
> You shouldn't omit a parameter name for int.
> 
> calculateTreeOrderAndSetNearestAncestorClosedTree might be a better name.

Done.

https://codereview.chromium.org/1305733002/diff/160001/Source/core/events/Tre...
Source/core/events/TreeScopeEventContext.h:88: bool shouldIncludeTreeScope(const
TreeScopeEventContext& other);
On 2015/08/25 01:34:20, hayato wrote:
> More descriptive name would be preferred. What 'Include' means here might be
> unclear for readers.

How about shouldIncludeTreeScopeInEventPath?

[hayato, 2015-08-26 05:37:58]

https://codereview.chromium.org/1305733002/diff/180001/Source/core/events/Tre...
File Source/core/events/TreeScopeEventContext.cpp (right):

https://codereview.chromium.org/1305733002/diff/180001/Source/core/events/Tre...
Source/core/events/TreeScopeEventContext.cpp:37: bool
TreeScopeEventContext::shouldIncludeTreeScopeInEventPath(const
TreeScopeEventContext& other)
Why don't you use a term of *unclosed* here?
The behavior of this function is exactly equal to the definition of "unclosed"?

https://codereview.chromium.org/1305733002/diff/180001/Source/core/events/Tre...
Source/core/events/TreeScopeEventContext.cpp:41: // Exclude nodes in UA shadow. 
E.g. <details> distributes elements under its tree.
Can we merge this check to the following checks? I'm wondering we should treat
UA shadow and closed shadow in the same manner.

https://codereview.chromium.org/1305733002/diff/180001/Source/core/events/Tre...
Source/core/events/TreeScopeEventContext.cpp:122: int
TreeScopeEventContext::calculateTreeOrderAndSetNearestAncestorClosedTree(int
orderNumber, TreeScopeEventContext* parentClosedTreeScopeEventContext)
It looks `parent` is inappropriate. It could be an ancestor?

https://codereview.chromium.org/1305733002/diff/180001/Source/core/events/Tre...
File Source/core/events/TreeScopeEventContext.h (right):

https://codereview.chromium.org/1305733002/diff/180001/Source/core/events/Tre...
Source/core/events/TreeScopeEventContext.h:77: int
calculateTreeOrderAndSetNearestAncestorClosedTree(int orderNumber,
TreeScopeEventContext*);
It looks TreeScopeEventContext parameter should have name here too.

[kochi, 2015-08-26 06:24:05]

PTAL

https://codereview.chromium.org/1305733002/diff/180001/Source/core/events/Tre...
File Source/core/events/TreeScopeEventContext.cpp (right):

https://codereview.chromium.org/1305733002/diff/180001/Source/core/events/Tre...
Source/core/events/TreeScopeEventContext.cpp:37: bool
TreeScopeEventContext::shouldIncludeTreeScopeInEventPath(const
TreeScopeEventContext& other)
On 2015/08/26 05:37:58, hayato wrote:
> Why don't you use a term of *unclosed* here?
> The behavior of this function is exactly equal to the definition of
"unclosed"?

How about isTreeScopeUnclosed?

(Unclosed only used for nodes?)

https://codereview.chromium.org/1305733002/diff/180001/Source/core/events/Tre...
Source/core/events/TreeScopeEventContext.cpp:41: // Exclude nodes in UA shadow. 
E.g. <details> distributes elements under its tree.
On 2015/08/26 05:37:58, hayato wrote:
> Can we merge this check to the following checks? I'm wondering we should treat
> UA shadow and closed shadow in the same manner.

No, UA shadow is special - even under a closed shadow root, elements can be
exposed if the event.path is for an element in that shadow tree. On the other
hand, nodes in UA shadow tree should never be exposed to any event paths.
(This assumes that you can never install an event handler on elements under UA
shadow).

So we need separate check for UA and closed shadow.

https://codereview.chromium.org/1305733002/diff/180001/Source/core/events/Tre...
Source/core/events/TreeScopeEventContext.cpp:122: int
TreeScopeEventContext::calculateTreeOrderAndSetNearestAncestorClosedTree(int
orderNumber, TreeScopeEventContext* parentClosedTreeScopeEventContext)
On 2015/08/26 05:37:58, hayato wrote:
> It looks `parent` is inappropriate. It could be an ancestor?

How about "nearestAncestor~" to match the function name?

https://codereview.chromium.org/1305733002/diff/180001/Source/core/events/Tre...
File Source/core/events/TreeScopeEventContext.h (right):

https://codereview.chromium.org/1305733002/diff/180001/Source/core/events/Tre...
Source/core/events/TreeScopeEventContext.h:77: int
calculateTreeOrderAndSetNearestAncestorClosedTree(int orderNumber,
TreeScopeEventContext*);
On 2015/08/26 05:37:58, hayato wrote:
> It looks TreeScopeEventContext parameter should have name here too.

Done.

[hayato, 2015-08-26 10:15:30]

https://codereview.chromium.org/1305733002/diff/180001/Source/core/events/Tre...
File Source/core/events/TreeScopeEventContext.cpp (right):

https://codereview.chromium.org/1305733002/diff/180001/Source/core/events/Tre...
Source/core/events/TreeScopeEventContext.cpp:37: bool
TreeScopeEventContext::shouldIncludeTreeScopeInEventPath(const
TreeScopeEventContext& other)
On 2015/08/26 06:24:05, Takayoshi Kochi wrote:
> On 2015/08/26 05:37:58, hayato wrote:
> > Why don't you use a term of *unclosed* here?
> > The behavior of this function is exactly equal to the definition of
> "unclosed"?
> 
> How about isTreeScopeUnclosed?
> 
> (Unclosed only used for nodes?)

The spec defines "unclosed tree". You might want to use this term.
https://w3c.github.io/webcomponents/spec/shadow/#dfn-unclosed-tree

https://codereview.chromium.org/1305733002/diff/180001/Source/core/events/Tre...
Source/core/events/TreeScopeEventContext.cpp:41: // Exclude nodes in UA shadow. 
E.g. <details> distributes elements under its tree.
On 2015/08/26 06:24:05, Takayoshi Kochi wrote:
> On 2015/08/26 05:37:58, hayato wrote:
> > Can we merge this check to the following checks? I'm wondering we should
treat
> > UA shadow and closed shadow in the same manner.
> 
> No, UA shadow is special - even under a closed shadow root, elements can be
> exposed if the event.path is for an element in that shadow tree. On the other
> hand, nodes in UA shadow tree should never be exposed to any event paths.
> (This assumes that you can never install an event handler on elements under UA
> shadow).
> 
> So we need separate check for UA and closed shadow.

Could you have a concrete example?
I don't see what this comment means... :(

https://codereview.chromium.org/1305733002/diff/180001/Source/core/events/Tre...
Source/core/events/TreeScopeEventContext.cpp:122: int
TreeScopeEventContext::calculateTreeOrderAndSetNearestAncestorClosedTree(int
orderNumber, TreeScopeEventContext* parentClosedTreeScopeEventContext)
On 2015/08/26 06:24:05, Takayoshi Kochi wrote:
> On 2015/08/26 05:37:58, hayato wrote:
> > It looks `parent` is inappropriate. It could be an ancestor?
> 
> How about "nearestAncestor~" to match the function name?

Acknowledged.

[kochi, 2015-08-27 07:24:32]

PTAL

https://codereview.chromium.org/1305733002/diff/180001/Source/core/events/Tre...
File Source/core/events/TreeScopeEventContext.cpp (right):

https://codereview.chromium.org/1305733002/diff/180001/Source/core/events/Tre...
Source/core/events/TreeScopeEventContext.cpp:41: // Exclude nodes in UA shadow. 
E.g. <details> distributes elements under its tree.
On 2015/08/26 10:15:30, hayato wrote:
> On 2015/08/26 06:24:05, Takayoshi Kochi wrote:
> > On 2015/08/26 05:37:58, hayato wrote:
> > > Can we merge this check to the following checks? I'm wondering we should
> treat
> > > UA shadow and closed shadow in the same manner.
> > 
> > No, UA shadow is special - even under a closed shadow root, elements can be
> > exposed if the event.path is for an element in that shadow tree. On the
other
> > hand, nodes in UA shadow tree should never be exposed to any event paths.
> > (This assumes that you can never install an event handler on elements under
UA
> > shadow).
> > 
> > So we need separate check for UA and closed shadow.
> 
> Could you have a concrete example?
> I don't see what this comment means... :(

Scratch my previous comment, your intuition seems correct.
Removed code for handling UA shadow specially.

[hayato, 2015-08-28 08:37:51]

lgtm

https://codereview.chromium.org/1305733002/diff/230001/Source/core/events/Tre...
File Source/core/events/TreeScopeEventContext.cpp (right):

https://codereview.chromium.org/1305733002/diff/230001/Source/core/events/Tre...
Source/core/events/TreeScopeEventContext.cpp:37: bool
TreeScopeEventContext::isUnclosedTree(const TreeScopeEventContext& other)
Can we rename this to `isUnclosedTreeOf`?
(and swap the role of `this` and `other`?)

https://codereview.chromium.org/1305733002/diff/230001/Source/core/events/Tre...
Source/core/events/TreeScopeEventContext.cpp:70: if
(isUnclosedTree(path[i].treeScopeEventContext()))
After the renaming, this can be 

 if (path[i].treeScopeEventContext().isUnclosedTreeOf(*this))

[kochi, 2015-08-28 09:01:19]

Thanks for the review.
isUnclosedTreeOf() more matches other part of the code,
and looks better.

https://codereview.chromium.org/1305733002/diff/230001/Source/core/events/Tre...
File Source/core/events/TreeScopeEventContext.cpp (right):

https://codereview.chromium.org/1305733002/diff/230001/Source/core/events/Tre...
Source/core/events/TreeScopeEventContext.cpp:37: bool
TreeScopeEventContext::isUnclosedTree(const TreeScopeEventContext& other)
On 2015/08/28 08:37:51, hayato wrote:
> Can we rename this to `isUnclosedTreeOf`?
> (and swap the role of `this` and `other`?)
> 

Done.

https://codereview.chromium.org/1305733002/diff/230001/Source/core/events/Tre...
Source/core/events/TreeScopeEventContext.cpp:70: if
(isUnclosedTree(path[i].treeScopeEventContext()))
On 2015/08/28 08:37:51, hayato wrote:
> After the renaming, this can be 
> 
>  if (path[i].treeScopeEventContext().isUnclosedTreeOf(*this))

Done.

[kochi, 2015-08-28 09:01:32]

The CQ bit was checked by kochi@chromium.org

[kochi, 2015-08-28 09:01:32]

The patchset sent to the CQ was uploaded after l-g-t-m from hayato@chromium.org
Link to the patchset: https://codereview.chromium.org/1305733002/#ps250001
(title: "isUnclosedTreeOf()")

[commit-bot: I haz the power, 2015-08-28 09:01:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1305733002/250001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1305733002/250001

[commit-bot: I haz the power, 2015-08-28 10:36:22]

Committed patchset #14 (id:250001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201391