529328 - DOMTokenList index out of bounds should return undefined, not the empty string

Status: VERIFIED FIXED

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

Description

[Erik Arvidsson]

http://www.whatwg.org/specs/web-apps/current-work/multipage/urls.html#dom-tokenlist-item

The following test fails

document.body.className = 'a b';

is(document.body.classList[2], null, 'Wrong value for out of bounds index');
is(document.body.classList[-1], undefined, 'Wrong value for non index access');

Comment 1

[Sylvain Pasche]

Actually, in this case document.body.classList[2] should return undefined (but document.body.classList.item(2) should return null) according to the Web IDL spec (if that's still true in the latest version, see bug 501257 comment 6 for the explanation).

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think the first cause of action should be to bring this up to the [e-mail] and/or [e-mail] lists and get clarification on what the defined behavior should be.

For example should classList[-1] behave the same as classList.item(-1)? And if so, should that throw or be the same as classList.item(-1 >>> 0) == classList.item(2^32 - 1)?

And what about classList.item[1.5]?

Comment 3

[Cameron McCormack (:heycam)]

(In reply to comment #2)
> For example should classList[-1] behave the same as classList.item(-1)? And if
> so, should that throw or be the same as classList.item(-1 >>> 0) ==
> classList.item(2^32 - 1)?
>
> And what about classList.item[1.5]?

I don't know if this was brought up on the list, but:

  classList[-1] should return undefined, because there is no property named "-1"
  classList.item(-1) should return null, because ToUint32(-1) is equal to or
    greater than the number of tokens, which HTML5 says means null is returned.
  classList.item(-1 >>> 0) returns null, since it's the same as the previous
    case.
  classList.item[1.5] returns undefined, because there is no property named
    "1.5" on the Function object for "item". :)
  classList.item(1.5) returns whatever classList.item(1) returns.

There was some recent discussion on how exactly out-of-range numbers are coerced into unsigned longs.  The spec currently says to do a ToUint32 on it.  But perhaps that should change:

  http://krijnhoetmer.nl/irc-logs/whatwg/20090714#l-102
  http://www.w3.org/mid/[e-mail]

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1 (code changes)

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1 (tests)

Comment 6

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

Comment on attachment 534102 [details] [diff] [review]
Patch v1 (code changes)

What about other users of nsStringArraySH? Is it specified for those cases
that undefined can be returned?

Would it make sense to add a new success code which nsDOMTokenList::Item
could return in some case; NS_SUCCESS_UNDEFINED. Then this code could
check that, and return undefined, or if the string is null, return null, otherwise
the string.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Looking at the other things this patch affects:

* nsHistorySH (History): no idea what this is supposed to be, it isn't in the spec and I've only got SECURITY_ERRs out of it
* nsStringListSH (DOMStringList): needs this fix
* nsMediaListSH (MediaList) and nsCSSStyleDeclSH (CSSStyleDeclaration): don't have the actual definitions WebIDL wants, but would need the fix
* nsOfflineResourceListSH: proprietary, but should match DOMStringList, IMO

So I'd rather change these cases (and add tests) than leave them returning the empty string. Is that okay with you?

Comment 8

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

Yes, changing all the cases and adding tests sounds ok.

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2, with tests

Comment 10

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

Comment on attachment 535936 [details] [diff] [review]
Patch v2, with tests

Is CSSStyleDeclaration [] behavior defined anywhere?
Should we open a w3c bug to fix it to work like other interfaces?

Comment 11

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

Comment on attachment 535936 [details] [diff] [review]
Patch v2, with tests

Actually, the patch does something else than what the summary says.
undefined might be returned even when inside the bounds.

Comment 12

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

Are there any cases when Item() might return null when inside the bounds?

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I'm pretty sure that can't happen.

Comment 14

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

Comment on attachment 535936 [details] [diff] [review]
Patch v2, with tests

OK. Could we add some #ifdef DEBUG code to each GetStringAt implementation
to check that null is returned only when index out of bounds.

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/2cd5af3aca9a

Comment 16

[Eric Shepherd [:sheppy]]

Documentation updated by Trevor.

Comment 17

[Ioana (away)]

dom/tests/mochitest/bugs/test_bug529328.html was run and it passed on:
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (X11; Linux i686; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0
+ Aurora (Fx 8.0a2) & Central (Fx 9.0a1)