Stop clamping tabIndex to short range

Stop clamping tabIndex to short range

Stop clamping tabIndex to short range to match
the specification [1] and other implementations.

Setting this attribute with values in range [INT_MIN, INT_MAX]
will be accepted, otherwise rejected.

The test fast/events/max-tabindex-focus.html is taken
from WebKit and rewritten to use testharness.js.

Behavior matches Firefox and Safari.

BUG=654860
[1] https://html.spec.whatwg.org/multipage/interaction.html#dom-tabindex

Committed: https://crrev.com/dcfa87e4ecf3f9673f642f9292f9a69e50ef6ba4
Cr-Commit-Position: refs/heads/master@{#430080}

[rwlbuis, 2016-09-26 21:07:55]

Patchset #3 (id:40001) has been deleted

[rwlbuis, 2016-09-27 12:40:26]

Patchset #3 (id:60001) has been deleted

[rwlbuis, 2016-10-10 17:20:00]

Description was changed from

==========
tabIndex WIP

BUG=
==========

to

==========
Clamp tabIndex to int range

BUG=490511
==========

[rwlbuis, 2016-10-10 20:46:02]

Description was changed from

==========
Clamp tabIndex to int range

BUG=490511
==========

to

==========
Clamp tabIndex to int range

Clamp tabIndex to int range instead of short to match
the specification [1].


Matches Firefox and Safari.

BUG=490511

[1] https://html.spec.whatwg.org/multipage/interaction.html#dom-tabindex
==========

[rwlbuis, 2016-10-10 20:46:45]

Description was changed from

==========
Clamp tabIndex to int range

Clamp tabIndex to int range instead of short to match
the specification [1].


Matches Firefox and Safari.

BUG=490511

[1] https://html.spec.whatwg.org/multipage/interaction.html#dom-tabindex
==========

to

==========
Clamp tabIndex to int range

Clamp tabIndex to int range instead of short to match
the specification [1].
The test fast/events/max-tabindex-focus.html is taken
from WebKit and rewritten to use testharness.js.

Matches Firefox and Safari.

BUG=490511

[1] https://html.spec.whatwg.org/multipage/interaction.html#dom-tabindex
==========

[rwlbuis, 2016-10-10 20:47:26]

Description was changed from

==========
Clamp tabIndex to int range

Clamp tabIndex to int range instead of short to match
the specification [1].
The test fast/events/max-tabindex-focus.html is taken
from WebKit and rewritten to use testharness.js.

Matches Firefox and Safari.

BUG=490511

[1] https://html.spec.whatwg.org/multipage/interaction.html#dom-tabindex
==========

to

==========
Clamp tabIndex to int range

Clamp tabIndex to int range instead of short to match
the specification [1].
The test fast/events/max-tabindex-focus.html is taken
from WebKit and rewritten to use testharness.js.

Behavior matches Firefox and Safari.

BUG=490511

[1] https://html.spec.whatwg.org/multipage/interaction.html#dom-tabindex
==========

[rwlbuis, 2016-10-10 20:53:38]

rob.buis@samsung.com changed reviewers:
+ tkent@chromium.org

[rwlbuis, 2016-10-10 20:53:39]

PTAL.

[tkent, 2016-10-11 13:32:44]

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

[tkent, 2016-10-11 13:32:45]

kochi@ should review this.

> BUG=490511

Please file a dedicated bug. 490511 isn't good for a behavior change.

[rwlbuis, 2016-10-11 20:17:12]

Description was changed from

==========
Clamp tabIndex to int range

Clamp tabIndex to int range instead of short to match
the specification [1].
The test fast/events/max-tabindex-focus.html is taken
from WebKit and rewritten to use testharness.js.

Behavior matches Firefox and Safari.

BUG=490511

[1] https://html.spec.whatwg.org/multipage/interaction.html#dom-tabindex
==========

to

==========
Clamp tabIndex to int range

Clamp tabIndex to int range instead of short to match
the specification [1].
The test fast/events/max-tabindex-focus.html is taken
from WebKit and rewritten to use testharness.js.

Behavior matches Firefox and Safari.

BUG=654860

[1] https://html.spec.whatwg.org/multipage/interaction.html#dom-tabindex
==========

[rwlbuis, 2016-10-11 20:18:02]

On 2016/10/11 13:32:45, tkent wrote:
> Please file a dedicated bug. 490511 isn't good for a behavior change.

I made crbug.com/654860 for this and updated the Description.

[kochi, 2016-10-17 08:18:53]

On 2016/10/11 20:18:02, rwlbuis wrote:
> On 2016/10/11 13:32:45, tkent wrote:
> > Please file a dedicated bug. 490511 isn't good for a behavior change.
> 
> I made crbug.com/654860 for this and updated the Description.

As I commented on 654860, let's make sure the change is safe for interop/compat
first. It looks WPT tests already expect 32bit tabindex.

I took a glance at the change, and short->int conversion seems quite
straightforward and small, ant most of the rest are the test expectation
changes.

For WPT tests that pass all tests, you can remove *-expected.txt.

[rwlbuis, 2016-10-17 20:24:09]

On 2016/10/17 08:18:53, kochi wrote:
> On 2016/10/11 20:18:02, rwlbuis wrote:
> > On 2016/10/11 13:32:45, tkent wrote:
> > > Please file a dedicated bug. 490511 isn't good for a behavior change.
> > 
> > I made crbug.com/654860 for this and updated the Description.
> 
> As I commented on 654860, let's make sure the change is safe for
interop/compat
> first. It looks WPT tests already expect 32bit tabindex.
>
> I took a glance at the change, and short->int conversion seems quite
> straightforward and small, ant most of the rest are the test expectation
> changes.

I'll react on the bug.

> For WPT tests that pass all tests, you can remove *-expected.txt.

Right, I think a pre upload check told me as well. So far there are two
*-expected.txt not needed anymore, I removed them in the patch.

[kochi, 2016-10-18 02:34:21]

On 2016/10/17 20:24:09, rwlbuis wrote:
> On 2016/10/17 08:18:53, kochi wrote:
> > On 2016/10/11 20:18:02, rwlbuis wrote:
> > > On 2016/10/11 13:32:45, tkent wrote:
> > > > Please file a dedicated bug. 490511 isn't good for a behavior change.
> > > 
> > > I made crbug.com/654860 for this and updated the Description.
> > 
> > As I commented on 654860, let's make sure the change is safe for
> interop/compat
> > first. It looks WPT tests already expect 32bit tabindex.
> >
> > I took a glance at the change, and short->int conversion seems quite
> > straightforward and small, ant most of the rest are the test expectation
> > changes.
> 
> I'll react on the bug.

Thanks!
As I wrote in the bug, probably it is better to wait for LGTMs in blink-dev.

> > For WPT tests that pass all tests, you can remove *-expected.txt.
> 
> Right, I think a pre upload check told me as well. So far there are two
> *-expected.txt not needed anymore, I removed them in the patch.

I see, thanks!

[kochi, 2016-10-18 02:37:25]

BTW, "Clamp" means saturating to int min/max, so
the new behavior (if it overflows int range, you get 0)
doesn't sound "clamping".  Could you update subject &
description?

https://codereview.chromium.org/2362223002/diff/150001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/platform/win/fast/dom/tabindex-clamp-expected.txt
(right):

https://codereview.chromium.org/2362223002/diff/150001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/win/fast/dom/tabindex-clamp-expected.txt:3:

(this is not mandatory, you can ignore this)
The diff from the original -expected.txt and this win version is only
this line (number of whitespaces).
I'm not sure why, but with some tweak in the original test html file,
could you remove the diff and we only have one -expected.txt?

https://codereview.chromium.org/2362223002/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2362223002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:2436:
setTabIndexExplicitly(tabindex);
Could you add a comment here for the behavior when the parsed value is
out of int ragne?

As far as I read parseHTMLInteger(), if the value is out of int range,
it returns false and thus tabindex will be 0 (set on line 2427).

But in that case, setTabIndexExplicitly(tabindex) will not be called?

If so, it can make some difference in the following case:

1. <div tabindex="3"></div>
    => div.tabIndex = 3
2. div.setAttribute("tabindex", "12039810238102983");
3. the div becomes <div tabindex="12039810238102983"><div> but
   div.tabIndex = 3 or 0?

If you have a test case to catch this, it is nice.

[rwlbuis, 2016-10-18 20:07:25]

Description was changed from

==========
Clamp tabIndex to int range

Clamp tabIndex to int range instead of short to match
the specification [1].
The test fast/events/max-tabindex-focus.html is taken
from WebKit and rewritten to use testharness.js.

Behavior matches Firefox and Safari.

BUG=654860

[1] https://html.spec.whatwg.org/multipage/interaction.html#dom-tabindex
==========

to

==========
Stop clamping tabIndex to short range

Stop clamping tabIndex to short range to match
the specification [1] and other implementations.

The test fast/events/max-tabindex-focus.html is taken
from WebKit and rewritten to use testharness.js.

Behavior matches Firefox and Safari.

BUG=654860

[1] https://html.spec.whatwg.org/multipage/interaction.html#dom-tabindex
==========

[rwlbuis, 2016-10-18 20:18:00]

Description was changed from

==========
Stop clamping tabIndex to short range

Stop clamping tabIndex to short range to match
the specification [1] and other implementations.

The test fast/events/max-tabindex-focus.html is taken
from WebKit and rewritten to use testharness.js.

Behavior matches Firefox and Safari.

BUG=654860

[1] https://html.spec.whatwg.org/multipage/interaction.html#dom-tabindex
==========

to

==========
Stop clamping tabIndex to short range

Stop clamping tabIndex to short range to match
the specification [1] and other implementations.

Setting this attribute with values in range [INT_MIN, INT_MAX]
will be accepted, otherwise rejected.

The test fast/events/max-tabindex-focus.html is taken
from WebKit and rewritten to use testharness.js.

Behavior matches Firefox and Safari.

BUG=654860

[1] https://html.spec.whatwg.org/multipage/interaction.html#dom-tabindex
==========

[rwlbuis, 2016-10-19 00:43:43]

https://codereview.chromium.org/2362223002/diff/150001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/platform/win/fast/dom/tabindex-clamp-expected.txt
(right):

https://codereview.chromium.org/2362223002/diff/150001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/win/fast/dom/tabindex-clamp-expected.txt:3:

On 2016/10/18 02:37:24, kochi wrote:
> (this is not mandatory, you can ignore this)
> The diff from the original -expected.txt and this win version is only
> this line (number of whitespaces).
> I'm not sure why, but with some tweak in the original test html file,
> could you remove the diff and we only have one -expected.txt?

It does not seem needed, I removed it :)

https://codereview.chromium.org/2362223002/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2362223002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:2436:
setTabIndexExplicitly(tabindex);
On 2016/10/18 02:37:24, kochi wrote:
> Could you add a comment here for the behavior when the parsed value is
> out of int ragne?

Done.

> As far as I read parseHTMLInteger(), if the value is out of int range,
> it returns false and thus tabindex will be 0 (set on line 2427).
> 
> But in that case, setTabIndexExplicitly(tabindex) will not be called?

That is correct.

> If so, it can make some difference in the following case:
> 
> 1. <div tabindex="3"></div>
>     => div.tabIndex = 3
> 2. div.setAttribute("tabindex", "12039810238102983");
> 3. the div becomes <div tabindex="12039810238102983"><div> but
>    div.tabIndex = 3 or 0?
> 
> If you have a test case to catch this, it is nice.

I added a test for this. Turns out it ends up being 3 since the setAttribute
tries to set to an out of range value.

[rwlbuis, 2016-10-19 00:47:22]

On 2016/10/18 02:37:25, kochi wrote:
> BTW, "Clamp" means saturating to int min/max, so
> the new behavior (if it overflows int range, you get 0)
> doesn't sound "clamping".  Could you update subject &
> description?

You are right, it is not clamping, I changed the wording and described how
setAttribute works for tabIndex.
Note that setting tabIndex from JS will show integer overflow behavior, same in
Webkit and Firefox though. Still I can add it to the description if you want.

If you like current changes I can post on blink-dev tomorrow, let me know if
more needs to be done.

[kochi, 2016-10-19 02:33:23]

On 2016/10/19 00:47:22, rwlbuis wrote:
> On 2016/10/18 02:37:25, kochi wrote:
> > BTW, "Clamp" means saturating to int min/max, so
> > the new behavior (if it overflows int range, you get 0)
> > doesn't sound "clamping".  Could you update subject &
> > description?
> 
> You are right, it is not clamping, I changed the wording and described how
> setAttribute works for tabIndex.
> Note that setting tabIndex from JS will show integer overflow behavior, same
in
> Webkit and Firefox though. Still I can add it to the description if you want.

Hmm, do you mean setting div.tabIndex = 2147483648 (INT_MAX + 1) will
result in div.tabIndex = 0 or -2147483648 in WebKit and Firefox?

You seem to have changed the logic in Element.cpp so that when overflow happens
in attribute parsing, preserve the original .tabIndex property value,
but the comment in fast/dom/tabindex-clamp.html says
"Values outside of this range will be set to zero".
So I thought this is the behavior in WebKit/Firefox, but it was not, right?

According to HTML spec (2.7.1 Reflecting content attributes in IDL attributes)
https://html.spec.whatwg.org/multipage/infrastructure.html#reflecting-content...

> If a reflecting IDL attribute has a signed integer type (long) then, on
getting,
> the content attribute must be parsed according to the rules for parsing signed
integers,
> and if that is successful, and the value is in the range of the IDL
attribute's type,
> the resulting value must be returned. If, on the other hand, it fails or
returns an
> out of range value, or if the attribute is absent, then the default value must
be
> returned instead, or 0 if there is no default value.

So when you get a parse error on tabindex attribute, you have to clear
"explicitly set"
flag so .tabIndex getter can return the default value (0 or -1, depending on
element).

Probably you can also take actions:
1. File bugs for Firefox/WebKit about tabindex overflow integer behavior
2. Add a test in web-platform-tests about setting out-of-range/empty/non-integer
tabindex attribute

> If you like current changes I can post on blink-dev tomorrow, let me know if
> more needs to be done.

You can post blink-dev while addressing the out-of-range value handling above.
It's almost there, thanks for working on this!

[rwlbuis, 2016-10-19 20:44:03]

Patchset #10 (id:180001) has been deleted

[rwlbuis, 2016-10-20 00:10:52]

On 2016/10/19 02:33:23, kochi wrote:
> On 2016/10/19 00:47:22, rwlbuis wrote:
> > On 2016/10/18 02:37:25, kochi wrote:
> > > BTW, "Clamp" means saturating to int min/max, so
> > > the new behavior (if it overflows int range, you get 0)
> > > doesn't sound "clamping".  Could you update subject &
> > > description?
> > 
> > You are right, it is not clamping, I changed the wording and described how
> > setAttribute works for tabIndex.
> > Note that setting tabIndex from JS will show integer overflow behavior, same
> in
> > Webkit and Firefox though. Still I can add it to the description if you
want.
> 
> Hmm, do you mean setting div.tabIndex = 2147483648 (INT_MAX + 1) will
> result in div.tabIndex = 0 or -2147483648 in WebKit and Firefox?

It will result in the overflow case (2147483648).

> You seem to have changed the logic in Element.cpp so that when overflow
happens
> in attribute parsing, preserve the original .tabIndex property value,
> but the comment in fast/dom/tabindex-clamp.html says
> "Values outside of this range will be set to zero".
> So I thought this is the behavior in WebKit/Firefox, but it was not, right?

I am sorry, the comment was probably wrong, I tried to fix it. You are correct
that that logic
in Element.cpp in the patch you reviewed would ignore out of range integer
values, which is what
WebKit currently does. For Firefox behavior is different, see below.

> According to HTML spec (2.7.1 Reflecting content attributes in IDL attributes)
>
https://html.spec.whatwg.org/multipage/infrastructure.html#reflecting-content...
> 
> > If a reflecting IDL attribute has a signed integer type (long) then, on
> getting,
> > the content attribute must be parsed according to the rules for parsing
signed
> integers,
> > and if that is successful, and the value is in the range of the IDL
> attribute's type,
> > the resulting value must be returned. If, on the other hand, it fails or
> returns an
> > out of range value, or if the attribute is absent, then the default value
must
> be
> > returned instead, or 0 if there is no default value.
> 
> So when you get a parse error on tabindex attribute, you have to clear
> "explicitly set"
> flag so .tabIndex getter can return the default value (0 or -1, depending on
> element).

I agree that the spec is pretty clear on this now that you summarized it.
Firefox does do above and I implemented this in my latest patch, at least for
setting the attribute and then calling tabIndex.

> Probably you can also take actions:
> 1. File bugs for Firefox/WebKit about tabindex overflow integer behavior
> 2. Add a test in web-platform-tests about setting
out-of-range/empty/non-integer
> tabindex attribute

I did not get around to this today but will keep this in mind as I find out
more.
I feel like we have now sane behavior for setting the attribute and then getting
it with tabIndex getter, i.e. return the value or 0/-1 for out of range
depending on whether the element can be focused or not. I am just not sure if
tabIndex setting and then getting needs the same behavior, i.e. no overflow
allowed? Is it clear from the specification? If yes, then we definitely need
action 1.

> You can post blink-dev while addressing the out-of-range value handling above.
> It's almost there, thanks for working on this!

No problem!

[kochi, 2016-10-21 07:21:57]

On 2016/10/20 00:10:52, rwlbuis wrote:
> On 2016/10/19 02:33:23, kochi wrote:
> > On 2016/10/19 00:47:22, rwlbuis wrote:
> > > On 2016/10/18 02:37:25, kochi wrote:
> > > > BTW, "Clamp" means saturating to int min/max, so
> > > > the new behavior (if it overflows int range, you get 0)
> > > > doesn't sound "clamping".  Could you update subject &
> > > > description?
> > > 
> > > You are right, it is not clamping, I changed the wording and described how
> > > setAttribute works for tabIndex.
> > > Note that setting tabIndex from JS will show integer overflow behavior,
same
> > in
> > > Webkit and Firefox though. Still I can add it to the description if you
> want.
> > 
> > Hmm, do you mean setting div.tabIndex = 2147483648 (INT_MAX + 1) will
> > result in div.tabIndex = 0 or -2147483648 in WebKit and Firefox?
> 
> It will result in the overflow case (2147483648).

I tried Firefox (49.0.2) and Safari Technology Preview 14.

On Firefox (used developer console), it returned -1.
>> div = document.createElement('div');
>> div.tabIndex = 2147483648;
>> div.tabIndex
<- -1

On Safari,
>> div = document.createElement('div');
>> div.tabIndex = 2147483648;
>> div.tabIndex
<- -2147483648

So Firefox implements "reverting to default" correctly,
WebKit has a bug in implementation. 

> > You seem to have changed the logic in Element.cpp so that when overflow
> happens
> > in attribute parsing, preserve the original .tabIndex property value,
> > but the comment in fast/dom/tabindex-clamp.html says
> > "Values outside of this range will be set to zero".
> > So I thought this is the behavior in WebKit/Firefox, but it was not, right?
> 
> I am sorry, the comment was probably wrong, I tried to fix it. You are correct
> that that logic
> in Element.cpp in the patch you reviewed would ignore out of range integer
> values, which is what
> WebKit currently does. For Firefox behavior is different, see below.
> 
> > According to HTML spec (2.7.1 Reflecting content attributes in IDL
attributes)
> >
>
https://html.spec.whatwg.org/multipage/infrastructure.html#reflecting-content...
> > 
> > > If a reflecting IDL attribute has a signed integer type (long) then, on
> > getting,
> > > the content attribute must be parsed according to the rules for parsing
> signed
> > integers,
> > > and if that is successful, and the value is in the range of the IDL
> > attribute's type,
> > > the resulting value must be returned. If, on the other hand, it fails or
> > returns an
> > > out of range value, or if the attribute is absent, then the default value
> must
> > be
> > > returned instead, or 0 if there is no default value.
> > 
> > So when you get a parse error on tabindex attribute, you have to clear
> > "explicitly set"
> > flag so .tabIndex getter can return the default value (0 or -1, depending on
> > element).
> 
> I agree that the spec is pretty clear on this now that you summarized it.
> Firefox does do above and I implemented this in my latest patch, at least for
> setting the attribute and then calling tabIndex.

Okay, now the logic in the patch looks good.

> > Probably you can also take actions:
> > 1. File bugs for Firefox/WebKit about tabindex overflow integer behavior
> > 2. Add a test in web-platform-tests about setting
> out-of-range/empty/non-integer
> > tabindex attribute
> 
> I did not get around to this today but will keep this in mind as I find out
> more.
> I feel like we have now sane behavior for setting the attribute and then
getting
> it with tabIndex getter, i.e. return the value or 0/-1 for out of range
> depending on whether the element can be focused or not. I am just not sure if
> tabIndex setting and then getting needs the same behavior, i.e. no overflow
> allowed? Is it clear from the specification? If yes, then we definitely need
> action 1.

For setter, the IDL describes HTMLElement.tabIndex as "long" type, and how
long type is treated is defined in WebIDL spec:
https://heycam.github.io/webidl/#es-long

To be brief, as the tabIndex attribute in the IDL is not accompanied by
[EnforceRange] or [Clamp] extended attribute, ToInt32() (which ECMAScript
defines)
is applied and therefore the lower 32bit will be assigned to 'tabIndex'
property.

So in this regard Firefox implementation looks correct as well.

It may be good to have test cases for this reflection behavior for both ways
(setting attribute -> get property, set property -> get attribute).

> > You can post blink-dev while addressing the out-of-range value handling
above.
> > It's almost there, thanks for working on this!
> 
> No problem!

Let's wait for API owners' l-g-t-m's for intent to ship and implement for
landing
this.

[foolip, 2016-11-03 14:17:41]

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

[foolip, 2016-11-03 14:17:43]

https://codereview.chromium.org/2362223002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ElementRareData.h (right):

https://codereview.chromium.org/2362223002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ElementRareData.h:64:
setElementFlag(TabIndexWasSetExplicitly, true);
Is this flag actually needed? Can't hasAttribute(tabindexAttr) be used instead?

[rwlbuis, 2016-11-03 15:33:09]

https://codereview.chromium.org/2362223002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ElementRareData.h (right):

https://codereview.chromium.org/2362223002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ElementRareData.h:64:
setElementFlag(TabIndexWasSetExplicitly, true);
On 2016/11/03 14:17:42, foolip wrote:
> Is this flag actually needed? Can't hasAttribute(tabindexAttr) be used
instead?

I still think we need the flag. If we do setAttribute("tabIndex",
crazy_big_number), the attribute will be set.
If we change Element::supportsFocus to test for hasAttribute instead of the
flag, in this
case the element with out-of-range tabIndex would still be focusable, which I
think we do not want to allow.

[foolip, 2016-11-03 16:06:57]

https://codereview.chromium.org/2362223002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ElementRareData.h (right):

https://codereview.chromium.org/2362223002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ElementRareData.h:64:
setElementFlag(TabIndexWasSetExplicitly, true);
On 2016/11/03 15:33:09, rwlbuis wrote:
> On 2016/11/03 14:17:42, foolip wrote:
> > Is this flag actually needed? Can't hasAttribute(tabindexAttr) be used
> instead?
> 
> I still think we need the flag. If we do setAttribute("tabIndex",
> crazy_big_number), the attribute will be set.
> If we change Element::supportsFocus to test for hasAttribute instead of the
> flag, in this
> case the element with out-of-range tabIndex would still be focusable, which I
> think we do not want to allow.

<button tabindex=18446744073709552001> seems to already be focusable, or am I
testing it wrong? What do other implementations do?

[rwlbuis, 2016-11-03 16:11:43]

On 2016/11/03 16:06:57, foolip wrote:
>
https://codereview.chromium.org/2362223002/diff/240001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/dom/ElementRareData.h (right):
> 
>
https://codereview.chromium.org/2362223002/diff/240001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/ElementRareData.h:64:
> setElementFlag(TabIndexWasSetExplicitly, true);
> On 2016/11/03 15:33:09, rwlbuis wrote:
> > On 2016/11/03 14:17:42, foolip wrote:
> > > Is this flag actually needed? Can't hasAttribute(tabindexAttr) be used
> > instead?
> > 
> > I still think we need the flag. If we do setAttribute("tabIndex",
> > crazy_big_number), the attribute will be set.
> > If we change Element::supportsFocus to test for hasAttribute instead of the
> > flag, in this
> > case the element with out-of-range tabIndex would still be focusable, which
I
> > think we do not want to allow.
> 
> <button tabindex=18446744073709552001> seems to already be focusable, or am I
> testing it wrong? What do other implementations do?

Form controls seem special:
HTMLFormControlElement::supportsFocus
This is suggested by w3c here:
http://w3c.github.io/html/editing.html#the-tabindex-attribute

[foolip, 2016-11-03 16:25:45]

On 2016/11/03 16:11:43, rwlbuis wrote:
> On 2016/11/03 16:06:57, foolip wrote:
> >
>
https://codereview.chromium.org/2362223002/diff/240001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/dom/ElementRareData.h (right):
> > 
> >
>
https://codereview.chromium.org/2362223002/diff/240001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/dom/ElementRareData.h:64:
> > setElementFlag(TabIndexWasSetExplicitly, true);
> > On 2016/11/03 15:33:09, rwlbuis wrote:
> > > On 2016/11/03 14:17:42, foolip wrote:
> > > > Is this flag actually needed? Can't hasAttribute(tabindexAttr) be used
> > > instead?
> > > 
> > > I still think we need the flag. If we do setAttribute("tabIndex",
> > > crazy_big_number), the attribute will be set.
> > > If we change Element::supportsFocus to test for hasAttribute instead of
the
> > > flag, in this
> > > case the element with out-of-range tabIndex would still be focusable,
which
> I
> > > think we do not want to allow.
> > 
> > <button tabindex=18446744073709552001> seems to already be focusable, or am
I
> > testing it wrong? What do other implementations do?
> 
> Form controls seem special:
> HTMLFormControlElement::supportsFocus
> This is suggested by w3c here:
> http://w3c.github.io/html/editing.html#the-tabindex-attribute

Are there differences from
https://html.spec.whatwg.org/multipage/interaction.html#the-tabindex-attribute
other than the warning?

In any event, both say "Each element can have a tabindex focus flag set", is
that this flag?

From what I can tell, parseHTMLInteger doesn't strictly match
https://html.spec.whatwg.org/#rules-for-parsing-integers in that it fails if the
value doesn't fit in an int, but there is a generic allowance for things like
it: "User agents may impose implementation-specific limits on otherwise
unconstrained inputs, e.g. to prevent denial of service attacks, to guard
against running out of memory, or to work around platform-specific limitations."

So, yeah, parseHTMLInteger will sometimes fail, and then a flag should be set.
Sorry for the noise.

[rwlbuis, 2016-11-03 23:18:43]

On 2016/10/21 07:21:57, kochi wrote:
> I tried Firefox (49.0.2) and Safari Technology Preview 14.
> 
> On Firefox (used developer console), it returned -1.
> >> div = document.createElement('div');
> >> div.tabIndex = 2147483648;
> >> div.tabIndex
> <- -1
> 
> On Safari,
> >> div = document.createElement('div');
> >> div.tabIndex = 2147483648;
> >> div.tabIndex
> <- -2147483648
> 
> So Firefox implements "reverting to default" correctly,
> WebKit has a bug in implementation. 

I am not sure, with my FF nightly on linux I get the same behavior as Safari
above in FireFox. Are you willing to try a FF nightly?

> It may be good to have test cases for this reflection behavior for both ways
> (setting attribute -> get property, set property -> get attribute).

I think tabindex-clamp.html is already covering this. Note I added a few
subtests for testing what happens with empty strings and assigning strings to
tabIndex property. This too seems to match FF.

> Let's wait for API owners' l-g-t-m's for intent to ship and implement for
> landing
> this.

I think we had nice feedback. Because of this, I had to look at the issue of
tabIndex taking up room in rare data, and think we don't need it and can just
use the attribute. It would be nice if you could have another look and please
let me know if more tests are needed.

[kochi, 2016-11-04 06:49:35]

https://codereview.chromium.org/2362223002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ElementRareData.h (left):

https://codereview.chromium.org/2362223002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ElementRareData.h:195: short m_tabindex;
Didn't you get any error for SameSizeAsElementRareData?

I think if you remove this member, you have to also remove
'short indices[1]' member in SameSizeAsElementRareData in
Element.cpp.

https://codereview.chromium.org/2362223002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ElementRareData.h (right):

https://codereview.chromium.org/2362223002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ElementRareData.h:63: void
setTabIndexExplicitly(int index) {
You can remove the parameter here.

[kochi, 2016-11-04 06:52:04]

Thanks for all the work!

On 2016/11/03 23:18:43, rwlbuis wrote:
> On 2016/10/21 07:21:57, kochi wrote:
> > I tried Firefox (49.0.2) and Safari Technology Preview 14.
> > 
> > On Firefox (used developer console), it returned -1.
> > >> div = document.createElement('div');
> > >> div.tabIndex = 2147483648;
> > >> div.tabIndex
> > <- -1
> > 
> > On Safari,
> > >> div = document.createElement('div');
> > >> div.tabIndex = 2147483648;
> > >> div.tabIndex
> > <- -2147483648
> > 
> > So Firefox implements "reverting to default" correctly,
> > WebKit has a bug in implementation. 
> 
> I am not sure, with my FF nightly on linux I get the same behavior as Safari
> above in FireFox. Are you willing to try a FF nightly?

Yes, I tried FF nightly and indeed I see the same behavior as WebKit.
Could you file a bug on Mozilla's bugzilla for this?
As far as we saw in this review rounds, FF49 looks the spec compliant
behavior.

> > It may be good to have test cases for this reflection behavior for both ways
> > (setting attribute -> get property, set property -> get attribute).
> 
> I think tabindex-clamp.html is already covering this. Note I added a few
> subtests for testing what happens with empty strings and assigning strings to
> tabIndex property. This too seems to match FF.
> 
> > Let's wait for API owners' l-g-t-m's for intent to ship and implement for
> > landing
> > this.
> 
> I think we had nice feedback. Because of this, I had to look at the issue of
> tabIndex taking up room in rare data, and think we don't need it and can just
> use the attribute. It would be nice if you could have another look and please
> let me know if more tests are needed.

Yeah, got 3 l-g-t-m's and even you reduced the size of ElementRareData!

Once the same-size issue is addressed, you can land this CL.

[kochi, 2016-11-04 07:17:04]

https://codereview.chromium.org/2362223002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ElementRareData.h (left):

https://codereview.chromium.org/2362223002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ElementRareData.h:195: short m_tabindex;
On 2016/11/04 06:49:35, kochi wrote:
> Didn't you get any error for SameSizeAsElementRareData?
> 
> I think if you remove this member, you have to also remove
> 'short indices[1]' member in SameSizeAsElementRareData in
> Element.cpp.

I figured out.
The last member of NodeRareData (which is the parent of
ElementRareData class) is a bitfield and take one byte.
As one short can fit in the padding to the next
"LayoutSize" (4-byte aligned).

So the ElementRareData size doesn't change with this anyway,
but you should still remove the 'short indices[1]' line in
SameSizeAsElementRareData in Element.cpp to avoid future
confusion.

[kochi, 2016-11-04 08:19:08]

On 2016/11/04 07:17:04, kochi wrote:
> I figured out.
> The last member of NodeRareData (which is the parent of
> ElementRareData class) is a bitfield and take one byte.
> As one short can fit in the padding to the next
> "LayoutSize" (4-byte aligned).

I was wrong, verified on Linux 64bit environment:

The end of NodeRareData or bitfield was irrelevant.  The last member
of NodeRaredata, "unsigned m_isElementRareData : 1" occupies 4 bytes.

The first 3 members of ElementRareData (excluding NodeRareData) are:
  LayoutSize m_minimumSizeForResizing;  // 4bytes, 4byte-aligned
  ScrollOffset m_savedLayerScrollOffset; // 8bytes, 4byte-aligned
  Member<DataSetDOMStringMap> m_dataset; // 8byte-aligned

So the whole structure is 8byte-aligned, and the 3rd member is
8byte-aligned, thus for member 1 and 2, 16bytes can be used.
So adding one 'int' (32bit) before or after m_minimumSizeForResizing
doesn't make the static_assert fail.

On 32-bit platforms, the above constraints (alignment) can be
different.

> So the ElementRareData size doesn't change with this anyway,
> but you should still remove the 'short indices[1]' line in
> SameSizeAsElementRareData in Element.cpp to avoid future
> confusion.

[kochi, 2016-11-04 09:20:10]

On 2016/11/04 08:19:08, kochi wrote:
> On 2016/11/04 07:17:04, kochi wrote:
> > I figured out.
> > The last member of NodeRareData (which is the parent of
> > ElementRareData class) is a bitfield and take one byte.
> > As one short can fit in the padding to the next
> > "LayoutSize" (4-byte aligned).
> 
> I was wrong, verified on Linux 64bit environment:
> 
> The end of NodeRareData or bitfield was irrelevant.  The last member
> of NodeRaredata, "unsigned m_isElementRareData : 1" occupies 4 bytes.
> 
> The first 3 members of ElementRareData (excluding NodeRareData) are:
>   LayoutSize m_minimumSizeForResizing;  // 4bytes, 4byte-aligned
>   ScrollOffset m_savedLayerScrollOffset; // 8bytes, 4byte-aligned
>   Member<DataSetDOMStringMap> m_dataset; // 8byte-aligned
> 
> So the whole structure is 8byte-aligned, and the 3rd member is
> 8byte-aligned, thus for member 1 and 2, 16bytes can be used.
> So adding one 'int' (32bit) before or after m_minimumSizeForResizing
> doesn't make the static_assert fail.
> 
> On 32-bit platforms, the above constraints (alignment) can be
> different.

Yeah, a 32bit build caught this:
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

> > So the ElementRareData size doesn't change with this anyway,
> > but you should still remove the 'short indices[1]' line in
> > SameSizeAsElementRareData in Element.cpp to avoid future
> > confusion.

[rwlbuis, 2016-11-04 13:03:18]

https://codereview.chromium.org/2362223002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ElementRareData.h (left):

https://codereview.chromium.org/2362223002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ElementRareData.h:195: short m_tabindex;
On 2016/11/04 06:49:35, kochi wrote:
> Didn't you get any error for SameSizeAsElementRareData?
> 
> I think if you remove this member, you have to also remove
> 'short indices[1]' member in SameSizeAsElementRareData in
> Element.cpp.

Acknowledged.

https://codereview.chromium.org/2362223002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ElementRareData.h:195: short m_tabindex;
On 2016/11/04 07:17:04, kochi wrote:
> On 2016/11/04 06:49:35, kochi wrote:
> > Didn't you get any error for SameSizeAsElementRareData?
> > 
> > I think if you remove this member, you have to also remove
> > 'short indices[1]' member in SameSizeAsElementRareData in
> > Element.cpp.
> 
> I figured out.
> The last member of NodeRareData (which is the parent of
> ElementRareData class) is a bitfield and take one byte.
> As one short can fit in the padding to the next
> "LayoutSize" (4-byte aligned).
> 
> So the ElementRareData size doesn't change with this anyway,
> but you should still remove the 'short indices[1]' line in
> SameSizeAsElementRareData in Element.cpp to avoid future
> confusion.

Thanks for looking into it, I forgot about this. I simply removed the entry in
the struct in my latest patch.

https://codereview.chromium.org/2362223002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ElementRareData.h (right):

https://codereview.chromium.org/2362223002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ElementRareData.h:63: void
setTabIndexExplicitly(int index) {
On 2016/11/04 06:49:35, kochi wrote:
> You can remove the parameter here.

Done.

[rwlbuis, 2016-11-04 13:04:08]

On 2016/11/04 06:52:04, kochi wrote:
> > I am not sure, with my FF nightly on linux I get the same behavior as Safari
> > above in FireFox. Are you willing to try a FF nightly?
> 
> Yes, I tried FF nightly and indeed I see the same behavior as WebKit.
> Could you file a bug on Mozilla's bugzilla for this?

Done:
https://bugzilla.mozilla.org/show_bug.cgi?id=1315238

[kochi, 2016-11-04 14:43:16]

LGTM!

[rwlbuis, 2016-11-04 14:48:19]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2016-11-04 14:48:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 17:04:49]

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

[commit-bot: I haz the power, 2016-11-04 17:04:51]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rwlbuis, 2016-11-04 17:27:42]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2016-11-04 17:28:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rwlbuis, 2016-11-04 19:36:16]

The CQ bit was checked by rob.buis@samsung.com

[rwlbuis, 2016-11-04 19:36:17]

The patchset sent to the CQ was uploaded after l-g-t-m from kochi@chromium.org
Link to the patchset: https://codereview.chromium.org/2362223002/#ps300001
(title: "Fix tabindex-clamp.html")

[commit-bot: I haz the power, 2016-11-04 19:36:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-05 00:22:27]

Description was changed from

==========
Stop clamping tabIndex to short range

Stop clamping tabIndex to short range to match
the specification [1] and other implementations.

Setting this attribute with values in range [INT_MIN, INT_MAX]
will be accepted, otherwise rejected.

The test fast/events/max-tabindex-focus.html is taken
from WebKit and rewritten to use testharness.js.

Behavior matches Firefox and Safari.

BUG=654860

[1] https://html.spec.whatwg.org/multipage/interaction.html#dom-tabindex
==========

to

==========
Stop clamping tabIndex to short range

Stop clamping tabIndex to short range to match
the specification [1] and other implementations.

Setting this attribute with values in range [INT_MIN, INT_MAX]
will be accepted, otherwise rejected.

The test fast/events/max-tabindex-focus.html is taken
from WebKit and rewritten to use testharness.js.

Behavior matches Firefox and Safari.

BUG=654860

[1] https://html.spec.whatwg.org/multipage/interaction.html#dom-tabindex
==========

[commit-bot: I haz the power, 2016-11-05 00:22:28]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-11-05 00:25:24]

Description was changed from

==========
Stop clamping tabIndex to short range

Stop clamping tabIndex to short range to match
the specification [1] and other implementations.

Setting this attribute with values in range [INT_MIN, INT_MAX]
will be accepted, otherwise rejected.

The test fast/events/max-tabindex-focus.html is taken
from WebKit and rewritten to use testharness.js.

Behavior matches Firefox and Safari.

BUG=654860

[1] https://html.spec.whatwg.org/multipage/interaction.html#dom-tabindex
==========

to

==========
Stop clamping tabIndex to short range

Stop clamping tabIndex to short range to match
the specification [1] and other implementations.

Setting this attribute with values in range [INT_MIN, INT_MAX]
will be accepted, otherwise rejected.

The test fast/events/max-tabindex-focus.html is taken
from WebKit and rewritten to use testharness.js.

Behavior matches Firefox and Safari.

BUG=654860

[1] https://html.spec.whatwg.org/multipage/interaction.html#dom-tabindex

Committed: https://crrev.com/dcfa87e4ecf3f9673f642f9292f9a69e50ef6ba4
Cr-Commit-Position: refs/heads/master@{#430080}
==========

[commit-bot: I haz the power, 2016-11-05 00:25:25]

Patchset 16 (id:??) landed as
https://crrev.com/dcfa87e4ecf3f9673f642f9292f9a69e50ef6ba4
Cr-Commit-Position: refs/heads/master@{#430080}