Add isTabStop attribute to Element

Add isTabStop attribute to Element

Add isTabStop attribute which exposes the "tab focusable flag" defined
in the HTML spec so that shadow hosts can control its own focusability.

By default this is disabled under "experimental web platform features" flag.

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

BUG=380445
TEST=fast/dom/shadow/{focus-navigation-with-istabstop.html,focusable-elements-with-istabstop.html}

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

[kochi, 2015-02-13 07:31:30]

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

[kochi, 2015-02-13 07:31:31]

PTAL

[vivekg, 2015-02-13 10:28:35]

vivekg@chromium.org changed reviewers:
+ vivekg@chromium.org

[vivekg, 2015-02-13 10:28:36]

https://codereview.chromium.org/917613004/diff/1/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/917613004/diff/1/Source/core/dom/Element.cpp#...
Source/core/dom/Element.cpp:2259: return supportsFocus() ? m_isTabStop : false;
nit: return supportsFocus() && m_isTabStop;

[vivekg, 2015-02-13 10:28:56]

vivekg@chromium.org changed reviewers:
- vivekg@chromium.org

[hayato, 2015-02-16 04:08:01]

Could you add a test how isTabStop affects existing other built-in elements than
div?

[kochi, 2015-02-18 07:57:49]

On 2015/02/16 04:08:01, hayato wrote:
> Could you add a test how isTabStop affects existing other built-in elements
than
> div?

Added a layout test for <a> and <button>, which are focusable by default.
Please take another look?

[kochi, 2015-02-19 06:10:47]

https://codereview.chromium.org/917613004/diff/1/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/917613004/diff/1/Source/core/dom/Element.cpp#...
Source/core/dom/Element.cpp:2259: return supportsFocus() ? m_isTabStop : false;
On 2015/02/13 10:28:36, vivekg_ wrote:
> nit: return supportsFocus() && m_isTabStop;

Done.

[hayato, 2015-02-19 08:54:54]

We, hayato@ and kochi@, are still discussing offline about this change.

[hayato, 2015-02-20 08:46:31]

https://codereview.chromium.org/917613004/diff/100001/LayoutTests/fast/dom/sh...
File LayoutTests/fast/dom/shadow/focusable-elements-with-istabstop.html (right):

https://codereview.chromium.org/917613004/diff/100001/LayoutTests/fast/dom/sh...
LayoutTests/fast/dom/shadow/focusable-elements-with-istabstop.html:55: var
expected_nav = [
Nit: We should avoid using snake case for variable names for consistency with
other tests.

https://codereview.chromium.org/917613004/diff/100001/LayoutTests/fast/dom/sh...
LayoutTests/fast/dom/shadow/focusable-elements-with-istabstop.html:73: var
expected_nav = [
|expected_nav| is declared twice. You should remove |var|, or introduce the new
variable name.

https://codereview.chromium.org/917613004/diff/100001/LayoutTests/fast/dom/sh...
LayoutTests/fast/dom/shadow/focusable-elements-with-istabstop.html:143:
anchor_host.offsetTop;
Why do we need this? Looks weird.

https://codereview.chromium.org/917613004/diff/100001/Source/core/dom/Element...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/917613004/diff/100001/Source/core/dom/Element...
Source/core/dom/Element.cpp:221: // isTabStop is overridden by setting tabindex.
Not sure how setting taxIndex attribute from JavaScript affects isTabStop
attribute and vice-verse.
What's the intentional behavior?

For example,

A)

element.setAttribute("isTabStop", false);
element.setAttribute("tabIndex", 1);
// What is the expectation of element.isTabStop here?

B)

element.setAttribute("tabIndex", 1);
element.setAttribute("isTabStop", false);
// What is the expectation of element.isTabStop here?

The result of element.isTabStop will differ for A and  B, won't it?
I'm afraid that that might be weird and will be likely to be misused.

https://codereview.chromium.org/917613004/diff/100001/Source/core/dom/Element.h
File Source/core/dom/Element.h (right):

https://codereview.chromium.org/917613004/diff/100001/Source/core/dom/Element...
Source/core/dom/Element.h:653: bool m_isTabStop;
How about moving this to ElementRareData?

[kochi, 2015-02-20 09:43:05]

https://codereview.chromium.org/917613004/diff/100001/LayoutTests/fast/dom/sh...
File LayoutTests/fast/dom/shadow/focusable-elements-with-istabstop.html (right):

https://codereview.chromium.org/917613004/diff/100001/LayoutTests/fast/dom/sh...
LayoutTests/fast/dom/shadow/focusable-elements-with-istabstop.html:55: var
expected_nav = [
On 2015/02/20 08:46:30, hayato wrote:
> Nit: We should avoid using snake case for variable names for consistency with
> other tests.

Done.

Renamed to expectedOrder and replaced all instances in this file and
focus-navigation-with-istabstop.html

https://codereview.chromium.org/917613004/diff/100001/LayoutTests/fast/dom/sh...
LayoutTests/fast/dom/shadow/focusable-elements-with-istabstop.html:73: var
expected_nav = [
On 2015/02/20 08:46:31, hayato wrote:
> |expected_nav| is declared twice. You should remove |var|, or introduce the
new
> variable name.

Done.

https://codereview.chromium.org/917613004/diff/100001/LayoutTests/fast/dom/sh...
LayoutTests/fast/dom/shadow/focusable-elements-with-istabstop.html:143:
anchor_host.offsetTop;
On 2015/02/20 08:46:31, hayato wrote:
> Why do we need this? Looks weird.

Removed.

This was inserted while I was debugging, but indeed it is unnecessary.

https://codereview.chromium.org/917613004/diff/100001/Source/core/dom/Element...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/917613004/diff/100001/Source/core/dom/Element...
Source/core/dom/Element.cpp:221: // isTabStop is overridden by setting tabindex.
On 2015/02/20 08:46:31, hayato wrote:
> Not sure how setting taxIndex attribute from JavaScript affects isTabStop
> attribute and vice-verse.
> What's the intentional behavior?
> 
> For example,
> 
> A)
> 
> element.setAttribute("isTabStop", false);
> element.setAttribute("tabIndex", 1);
> // What is the expectation of element.isTabStop here?
> 
> B)
> 
> element.setAttribute("tabIndex", 1);
> element.setAttribute("isTabStop", false);
> // What is the expectation of element.isTabStop here?
> 
> The result of element.isTabStop will differ for A and  B, won't it?
> I'm afraid that that might be weird and will be likely to be misused.

A) isTabStop = true
B) isTabStop = false

This is because setting tabindex is stronger than isTabStop, for compatibility
reason.
In case A, when you explicitly set isTabStop to false first, but setting
"tabindex=1"
will reset the effect.

I'll add a layout test for these cases.

https://codereview.chromium.org/917613004/diff/100001/Source/core/dom/Element.h
File Source/core/dom/Element.h (right):

https://codereview.chromium.org/917613004/diff/100001/Source/core/dom/Element...
Source/core/dom/Element.h:653: bool m_isTabStop;
On 2015/02/20 08:46:31, hayato wrote:
> How about moving this to ElementRareData?

Yes, I should have done...
Done.

[hayato, 2015-02-20 10:28:45]

https://codereview.chromium.org/917613004/diff/100001/Source/core/dom/Element...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/917613004/diff/100001/Source/core/dom/Element...
Source/core/dom/Element.cpp:221: // isTabStop is overridden by setting tabindex.
On 2015/02/20 09:43:05, Takayoshi Kochi wrote:
> On 2015/02/20 08:46:31, hayato wrote:
> > Not sure how setting taxIndex attribute from JavaScript affects isTabStop
> > attribute and vice-verse.
> > What's the intentional behavior?
> > 
> > For example,
> > 
> > A)
> > 
> > element.setAttribute("isTabStop", false);
> > element.setAttribute("tabIndex", 1);
> > // What is the expectation of element.isTabStop here?
> > 
> > B)
> > 
> > element.setAttribute("tabIndex", 1);
> > element.setAttribute("isTabStop", false);
> > // What is the expectation of element.isTabStop here?
> > 
> > The result of element.isTabStop will differ for A and  B, won't it?
> > I'm afraid that that might be weird and will be likely to be misused.
> 
> A) isTabStop = true
> B) isTabStop = false
> 
> This is because setting tabindex is stronger than isTabStop, for compatibility
> reason.
> In case A, when you explicitly set isTabStop to false first, but setting
> "tabindex=1"
> will reset the effect.
> 
> I'll add a layout test for these cases.

I understand the intention, however, I'm not sure whether it is good or not that
the result of isTabStop depends on the order in which attributes are specified.
Still looks strange to me...

For example (assuming that we cat set both attributes in declarative way):

<div tabindex="1" isTabStop="false">
<div isTabStop="false" tabindex="1">

In these cases, we expect that isTabStop == false in both cases. I expect that
the same thing will happen also in an imperative way.

Is there any way to make it order-independent?

[kochi, 2015-02-20 10:57:33]

On 2015/02/20 10:28:45, hayato wrote:
>
https://codereview.chromium.org/917613004/diff/100001/Source/core/dom/Element...
> File Source/core/dom/Element.cpp (right):
> 
>
https://codereview.chromium.org/917613004/diff/100001/Source/core/dom/Element...
> Source/core/dom/Element.cpp:221: // isTabStop is overridden by setting
tabindex.
> On 2015/02/20 09:43:05, Takayoshi Kochi wrote:
> > On 2015/02/20 08:46:31, hayato wrote:
> > > Not sure how setting taxIndex attribute from JavaScript affects isTabStop
> > > attribute and vice-verse.
> > > What's the intentional behavior?
> > > 
> > > For example,
> > > 
> > > A)
> > > 
> > > element.setAttribute("isTabStop", false);
> > > element.setAttribute("tabIndex", 1);
> > > // What is the expectation of element.isTabStop here?
> > > 
> > > B)
> > > 
> > > element.setAttribute("tabIndex", 1);
> > > element.setAttribute("isTabStop", false);
> > > // What is the expectation of element.isTabStop here?
> > > 
> > > The result of element.isTabStop will differ for A and  B, won't it?
> > > I'm afraid that that might be weird and will be likely to be misused.
> > 
> > A) isTabStop = true
> > B) isTabStop = false
> > 
> > This is because setting tabindex is stronger than isTabStop, for
compatibility
> > reason.
> > In case A, when you explicitly set isTabStop to false first, but setting
> > "tabindex=1"
> > will reset the effect.
> > 
> > I'll add a layout test for these cases.
> 
> I understand the intention, however, I'm not sure whether it is good or not
that
> the result of isTabStop depends on the order in which attributes are
specified.
> Still looks strange to me...
> 
> For example (assuming that we cat set both attributes in declarative way):
> 
> <div tabindex="1" isTabStop="false">
> <div isTabStop="false" tabindex="1">
> 
> In these cases, we expect that isTabStop == false in both cases. I expect that
> the same thing will happen also in an imperative way.
> 
> Is there any way to make it order-independent?

One way to solve this is that we never expose isTabStop as an attribute,
only property on DOM object.  Then the ordering issue will never happen,
(authours have to set isTabStop *after* tabindex explicitly).

[hayato, 2015-02-23 05:57:12]

I'm not convinced, but this feature is under the flag, so it's okay to land this
patch and let early users experience this feature to get early feedbacks.

Have you verified that this CL doesn't break anything when this "experimental
web platform features" is not enabled?

[kochi, 2015-02-23 06:10:49]

On 2015/02/23 05:57:12, hayato wrote:
> I'm not convinced, but this feature is under the flag, so it's okay to land
this
> patch and let early users experience this feature to get early feedbacks.
> 
> Have you verified that this CL doesn't break anything when this "experimental
> web platform features" is not enabled?

Thanks, the spec is still not yet fixed and thus this is experimental.
So this is for those who want to test this feature experimentally.
I'll discuss on the spec doc about how we deal with attribute declaration order.

I've flipped the RuntimeEnabledFeatures to stable in patchset 10, and ran tests.
I expect no tests will fail (though some, like element-instance-property-listing
may fail).
Let's see how they go.

[hayato, 2015-02-23 06:53:45]

lgtm

[kochi, 2015-02-23 07:04:22]

The CQ bit was checked by kochi@chromium.org

[commit-bot: I haz the power, 2015-02-23 07:05:15]

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

[commit-bot: I haz the power, 2015-02-23 07:21:08]

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

[commit-bot: I haz the power, 2015-02-23 07:21:10]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/2...)

[kochi, 2015-02-23 07:28:38]

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

[kochi, 2015-02-23 07:29:00]

+haraken for Source/platform/RuntimeEnabledFeatures.in

[haraken, 2015-02-23 07:44:29]

On 2015/02/23 07:29:00, Takayoshi Kochi wrote:
> +haraken for Source/platform/RuntimeEnabledFeatures.in

platform/ LGTM

[kochi, 2015-02-23 07:46:08]

The CQ bit was checked by kochi@chromium.org

[commit-bot: I haz the power, 2015-02-23 07:46:54]

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

[commit-bot: I haz the power, 2015-02-23 07:50:05]

Committed patchset #11 (id:200001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190640

[tkent, 2015-02-23 07:55:01]

Is this defined by any public standard?

[kochi, 2015-02-23 08:01:09]

On 2015/02/23 07:55:01, tkent wrote:
> Is this defined by any public standard?

Not yet, and we intend to merge it to Shadow DOM spec first.