Last Comment Bug 529328 - DOMTokenList index out of bounds should return undefined, not the empty string
: DOMTokenList index out of bounds should return undefined, not the empty string
Status: VERIFIED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
:
Mentors:
http://mxr.mozilla.org/mozilla-centra...
Depends on:
Blocks: 501257
  Show dependency treegraph
 
Reported: 2009-11-17 11:17 PST by Erik Arvidsson
Modified: 2011-09-14 06:07 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (code changes) (1.77 KB, patch)
2011-05-20 14:08 PDT, :Ms2ger (⌚ UTC+1/+2)
bugs: review-
Details | Diff | Splinter Review
Patch v1 (tests) (6.34 KB, patch)
2011-05-20 14:09 PDT, :Ms2ger (⌚ UTC+1/+2)
bugs: review+
Details | Diff | Splinter Review
Patch v2, with tests (13.90 KB, patch)
2011-05-29 07:39 PDT, :Ms2ger (⌚ UTC+1/+2)
bugs: review+
Details | Diff | Splinter Review

Description Erik Arvidsson 2009-11-17 11:17:05 PST
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 2009-11-17 13:10:35 PST
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 2009-11-17 15:47:12 PST
I think the first cause of action should be to bring this up to the public-webapps@w3.org and/or public-script-coord@w3.org 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) 2010-10-16 18:55:55 PDT
(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/Pine.LNX.4.62.0907140132530.9397@hixie.dreamhostps.com
Comment 4 :Ms2ger (⌚ UTC+1/+2) 2011-05-20 14:08:13 PDT
Created attachment 534102 [details] [diff] [review]
Patch v1 (code changes)
Comment 5 :Ms2ger (⌚ UTC+1/+2) 2011-05-20 14:09:00 PDT
Created attachment 534103 [details] [diff] [review]
Patch v1 (tests)
Comment 6 Olli Pettay [:smaug] (reviewing overload) 2011-05-21 12:44:15 PDT
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 (⌚ UTC+1/+2) 2011-05-22 06:25:26 PDT
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] (reviewing overload) 2011-05-22 10:12:45 PDT
Yes, changing all the cases and adding tests sounds ok.
Comment 9 :Ms2ger (⌚ UTC+1/+2) 2011-05-29 07:39:24 PDT
Created attachment 535936 [details] [diff] [review]
Patch v2, with tests
Comment 10 Olli Pettay [:smaug] (reviewing overload) 2011-05-29 07:55:52 PDT
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] (reviewing overload) 2011-05-29 07:58:26 PDT
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] (reviewing overload) 2011-05-29 07:59:23 PDT
Are there any cases when Item() might return null when inside the bounds?
Comment 13 :Ms2ger (⌚ UTC+1/+2) 2011-05-29 09:42:05 PDT
I'm pretty sure that can't happen.
Comment 14 Olli Pettay [:smaug] (reviewing overload) 2011-05-29 12:26:53 PDT
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 (⌚ UTC+1/+2) 2011-05-30 08:03:44 PDT
http://hg.mozilla.org/mozilla-central/rev/2cd5af3aca9a
Comment 16 Eric Shepherd [:sheppy] 2011-06-23 06:40:01 PDT
Documentation updated by Trevor.
Comment 17 Ioana (away) 2011-09-14 06:07:54 PDT
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)

Note You need to log in before you can comment on or make changes to this bug.