Implement ShadowRoot.delegatesFocus 4/4 (match CSS :focus for shadow host)

Implement ShadowRoot.delegatesFocus 4/4 (match CSS :focus for shadow host)

This Implements CSS :focus pseudo style selector
on shadow host to match when
1. focused element is descendant of its shadow root
2. its shadow root's delegatesFocus is true

This depends on previous CLs.
https://codereview.chromium.org/1174893002/
https://codereview.chromium.org/1174933002/
https://codereview.chromium.org/1177673004/

Design doc:
https://docs.google.com/document/d/1k93Ez6yNSyWQDtGjdJJqTBPmljk9l2WS3JTe5OHHB50/edit

BUG=496005
TEST=fast/dom/shadow/*

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

[kochi, 2015-06-10 11:15:15]

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

[commit-bot: I haz the power, 2015-06-10 11:15:24]

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

[commit-bot: I haz the power, 2015-06-10 12:34:23]

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

[commit-bot: I haz the power, 2015-06-10 12:34:24]

Dry run: This issue passed the CQ dry run.

[kochi, 2015-06-11 02:14:34]

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

[kochi, 2015-06-11 06:19:57]

PTAL

This CL is ready for review.

[kochi, 2015-06-11 08:48:19]

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

[commit-bot: I haz the power, 2015-06-11 08:48:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1177673005/40001

[commit-bot: I haz the power, 2015-06-11 10:10:32]

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

[commit-bot: I haz the power, 2015-06-11 10:10:33]

Dry run: This issue passed the CQ dry run.

[hayato, 2015-06-12 04:37:57]

https://codereview.chromium.org/1177673005/diff/40001/Source/core/dom/Contain...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/1177673005/diff/40001/Source/core/dom/Contain...
Source/core/dom/ContainerNode.cpp:1087: // Recurse up the shadow trees to mark
shadow hosts if it matches :focus.
Could you add a layout test which involves a dom mutation?

If a focused element is moved to somewhere, the focus state of each nodes are
set (or unset) correctly?

https://codereview.chromium.org/1177673005/diff/40001/Source/core/dom/Contain...
Source/core/dom/ContainerNode.cpp:1088: if (isInShadowTree() &&
shadowHost()->shadowRoot())
Not intuitive to me. Is this checking that `this` is in an author shadow tree or
not?

https://codereview.chromium.org/1177673005/diff/40001/Source/core/dom/Contain...
Source/core/dom/ContainerNode.cpp:1096: Element* host = toElement(this);
1. Looks isElementNode() is used in L1112.
 `this` is always Element here?

2. `host` looks confusing name to me since `this` is not always a shadow host,
at least until 1098.

https://codereview.chromium.org/1177673005/diff/40001/Source/core/dom/Contain...
Source/core/dom/ContainerNode.cpp:1111: // If :focus sets display: none, we lose
focus but still need to recalc our style.
Looks focusStateChanged() and the code around here are very similar. Can we
remove duplication?

[kochi, 2015-06-12 07:34:17]

Thanks for the review!

https://codereview.chromium.org/1177673005/diff/40001/Source/core/dom/Contain...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/1177673005/diff/40001/Source/core/dom/Contain...
Source/core/dom/ContainerNode.cpp:1087: // Recurse up the shadow trees to mark
shadow hosts if it matches :focus.
On 2015/06/12 04:37:57, hayato wrote:
> Could you add a layout test which involves a dom mutation?
> 
> If a focused element is moved to somewhere, the focus state of each nodes are
> set (or unset) correctly?

Added 'focus-with-dom-mutation.html' layout test which tests :focus
state is properly cleared when focused element is moved out of shadow tree.

https://codereview.chromium.org/1177673005/diff/40001/Source/core/dom/Contain...
Source/core/dom/ContainerNode.cpp:1088: if (isInShadowTree() &&
shadowHost()->shadowRoot())
On 2015/06/12 04:37:57, hayato wrote:
> Not intuitive to me. Is this checking that `this` is in an author shadow tree
or
> not?

Yes, it's author shadow only.
Updated the comment and moved the TODO comment below (line 1093) here.

https://codereview.chromium.org/1177673005/diff/40001/Source/core/dom/Contain...
Source/core/dom/ContainerNode.cpp:1096: Element* host = toElement(this);
On 2015/06/12 04:37:57, hayato wrote:
> 1. Looks isElementNode() is used in L1112.
>  `this` is always Element here?
> 
> 2. `host` looks confusing name to me since `this` is not always a shadow host,
> at least until 1098.

1. Good catch.  I guess no one is calling setFocus() on Node or ContainerNode,
but adding isElementNode() here looks correct to me.  The same bug existed for
the old tabStop implementation.

2. replaced toElement() for both occurrences.

https://codereview.chromium.org/1177673005/diff/40001/Source/core/dom/Contain...
Source/core/dom/ContainerNode.cpp:1111: // If :focus sets display: none, we lose
focus but still need to recalc our style.
On 2015/06/12 04:37:57, hayato wrote:
> Looks focusStateChanged() and the code around here are very similar. Can we
> remove duplication?

I'm afraid their conditions are subtly different and they can't be simplified
by unifying, just more complex.

[kochi, 2015-06-12 07:34:27]

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

[commit-bot: I haz the power, 2015-06-12 07:34:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1177673005/80001

[hayato, 2015-06-12 07:59:30]

https://codereview.chromium.org/1177673005/diff/80001/LayoutTests/fast/dom/sh...
File LayoutTests/fast/dom/shadow/focus-with-dom-mutation.html (right):

https://codereview.chromium.org/1177673005/diff/80001/LayoutTests/fast/dom/sh...
LayoutTests/fast/dom/shadow/focus-with-dom-mutation.html:28: createDOM('input',
{id:'input'})),
Nit: space is required after ':'.

https://codereview.chromium.org/1177673005/diff/80001/Source/core/dom/Contain...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/1177673005/diff/80001/Source/core/dom/Contain...
Source/core/dom/ContainerNode.cpp:1090: if (isInShadowTree() &&
shadowHost()->shadowRoot())
How about this? The intention would becomes more clear.

if (ShadowRoot* root = element.containingShadowRoot())
  if (root->type() == ShadowRoot::AuthorShadowRoot)

Or, you might want to add a new utility function.  e.g
Node::isInAuthorShadowTree().

[kochi, 2015-06-12 08:21:35]

https://codereview.chromium.org/1177673005/diff/80001/LayoutTests/fast/dom/sh...
File LayoutTests/fast/dom/shadow/focus-with-dom-mutation.html (right):

https://codereview.chromium.org/1177673005/diff/80001/LayoutTests/fast/dom/sh...
LayoutTests/fast/dom/shadow/focus-with-dom-mutation.html:28: createDOM('input',
{id:'input'})),
On 2015/06/12 07:59:29, hayato wrote:
> Nit: space is required after ':'.

Done.

https://codereview.chromium.org/1177673005/diff/80001/Source/core/dom/Contain...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/1177673005/diff/80001/Source/core/dom/Contain...
Source/core/dom/ContainerNode.cpp:1090: if (isInShadowTree() &&
shadowHost()->shadowRoot())
On 2015/06/12 07:59:29, hayato wrote:
> How about this? The intention would becomes more clear.
> 
> if (ShadowRoot* root = element.containingShadowRoot())
>   if (root->type() == ShadowRoot::AuthorShadowRoot)
> 
> Or, you might want to add a new utility function.  e.g
> Node::isInAuthorShadowTree().

Adopted your first suggestion.

[kochi, 2015-06-12 08:21:47]

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

[commit-bot: I haz the power, 2015-06-12 08:21:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1177673005/100001

[commit-bot: I haz the power, 2015-06-12 09:42:10]

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

[commit-bot: I haz the power, 2015-06-12 09:42:11]

Dry run: This issue passed the CQ dry run.

[kochi, 2015-06-15 06:36:20]

Ping?

[hayato, 2015-06-15 06:45:44]

lgtm

[kochi, 2015-06-15 06:49:26]

The CQ bit was checked by kochi@chromium.org

[commit-bot: I haz the power, 2015-06-15 06:49:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1177673005/100001

[commit-bot: I haz the power, 2015-06-15 09:13:53]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197104