1315238 - Setting tabIndex with out of range values does the wrong thing

Status: RESOLVED INVALID

(Core :: DOM: Core & HTML, defect, P3)

Description

[Rob Buis]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20161026030210

Steps to reproduce:

In inspector:
> div = document.createElement('div');
> div.tabIndex = 2147483648;
> div.tabIndex


Actual results:

> div.tabIndex
<- -2147483648


Expected results:

> div.tabIndex
<- -1

Note that this happened in Firefox (49.0.2)

Since the value used for tabIndex is out of range for the idl long type, I would expect setting it would be handled as under "If the attribute is omitted or parsing the value returns an error" and a default value would be returned:
http://w3c.github.io/html/editing.html#the-tabindex-attribute

Note that by using setAttribute with the same value we do get the default value:
> div.setAttribute("tabIndex", "2147483648");
> div.tabIndex
-1

Comment 1

[YF (Yang)]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c109d0b45e8aa440b588c1a862b0ca5b83ee050e&tochange=085bacd46edf878ae2f23d98cfdddda49ffa0dc6

Comment 2

[Andrew Overholt [:overholt]]

Gordon, can you please take a look? Thanks.

Comment 3

[Gordon Su]

I'll take a look.

Comment 4

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

We reviewed this in platform triage meeting today. This doesn't seem release blocking. Let's keep it around for a fix in 50.1.0.

Comment 5

[John Dai[:jdai]]

After I reverted Bug 1271126 back, it can't really solve this problem. It can avoid tabIndex is 2147483648 case, but if I set tabIndex to 2147483649, it will show -2147483647.
I think the best way to solve this problem is when JSValue convert to C++ type [1][2]. If we get a overflow notification, it would be easier to make sure the value is an overflow value.

[1] https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-pc-linux-gnu/dom/bindings/HTMLElementBinding.cpp#410
[2] https://dxr.mozilla.org/mozilla-central/source/dom/bindings/PrimitiveConversions.h#348

Comment 6

[John Dai[:jdai]]

firefox version 50.1.0
div.tabIndex = 2147483648;
result: -2147483648
div.tabIndex = 2147483649;
result: -2147483647
div.tabIndex = 2147483650;
result: -2147483646

Chrome version 55.0.2883.95
div.tabIndex = 2147483648;
result: -32768
div.tabIndex = 2147483649;
result: -32768
div.tabIndex = 2147483650;
result: -32768

Safari version 9.1.3
div.tabIndex = 2147483648;
result: -1
div.tabIndex = 2147483649;
result: -32768
div.tabIndex = 2147483650;
result: -32768

Edge version 38.14393.0.0
div.tabIndex = 2147483648;
result: 0
div.tabIndex = 2147483649;
result: 1
div.tabIndex = 2147483650;
result: 2

Comment 7

[John Dai[:jdai]]

(In reply to John Dai[:jdai] from comment #5)
> After I reverted Bug 1271126 back, it can't really solve this problem. It
> can avoid tabIndex is 2147483648 case, but if I set tabIndex to 2147483649,
> it will show -2147483647.
> I think the best way to solve this problem is when JSValue convert to C++
> type [1][2]. If we get a overflow notification, it would be easier to make
> sure the value is an overflow value.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-pc-linux-gnu/dom/
> bindings/HTMLElementBinding.cpp#410
> [2]
> https://dxr.mozilla.org/mozilla-central/source/dom/bindings/
> PrimitiveConversions.h#348

Hi Boris,
Could you help to shed some light here? Thank you.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Light on what?  What we're currently doing matches what the HTML spec says to do, which was the whole point of bug 1271126.  The HTML spec defines:

  attribute long tabIndex;

If you set it to something outside the 32-bit signed int range, it will get moved back into that range by adding or subtracting enough multiples of 2^32.  This is very clearly defined at https://heycam.github.io/webidl/#es-to-long

Then this resulting value will be passed to the spec algorithm defined at https://html.spec.whatwg.org/multipage/interaction.html#dom-tabindex which is what's being quoted in comment 0.  At this point the value is not out of range for 32-bit signed ints, because IDL coerced it into that range.

If you want to change the spec behavior here, please feel free to raise a spec issue, of course, but you'll have to explain why the new behavior is really better than what the spec has right now.  Not to mention deciding what this new behavior should be.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Marking invalid, since we're following the spec.  Spec issues can be reported at https://github.com/whatwg/html/issues/new

Comment 10

[Gerry Chang [:gchang]]

Mark 51/52 won't fix as we are following the spec.