The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in mozilla7

Status

()

Core
DOM: Core & HTML
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: Erik Arvidsson, Assigned: Ms2ger)

Tracking

({dev-doc-complete})

Trunk
mozilla7
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
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

7 years ago
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).
Blocks: 501257
Version: unspecified → Trunk
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]?
(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
(Assignee)

Updated

6 years ago
Assignee: nobody → Ms2ger
Status: NEW → ASSIGNED
(Assignee)

Comment 4

6 years ago
Created attachment 534102 [details] [diff] [review]
Patch v1 (code changes)
Attachment #534102 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 5

6 years ago
Created attachment 534103 [details] [diff] [review]
Patch v1 (tests)
Attachment #534103 - Flags: review?(Olli.Pettay)

Comment 6

6 years ago
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.

Updated

6 years ago
Attachment #534102 - Flags: review?(Olli.Pettay) → review-

Updated

6 years ago
Attachment #534103 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 7

6 years ago
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

6 years ago
Yes, changing all the cases and adding tests sounds ok.
(Assignee)

Comment 9

6 years ago
Created attachment 535936 [details] [diff] [review]
Patch v2, with tests
Attachment #535936 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

6 years ago
Summary: DOMTokenList index out of bounds should return null, not the empty string → DOMTokenList index out of bounds should return undefined, not the empty string
(Assignee)

Updated

6 years ago
Attachment #534102 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #534103 - Attachment is obsolete: true
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?
Attachment #535936 - Flags: review?(Olli.Pettay) → review+
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.
Attachment #535936 - Flags: review+ → review?(Olli.Pettay)
Are there any cases when Item() might return null when inside the bounds?
(Assignee)

Comment 13

6 years ago
I'm pretty sure that can't happen.
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.
Attachment #535936 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 15

6 years ago
http://hg.mozilla.org/mozilla-central/rev/2cd5af3aca9a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
(Assignee)

Updated

6 years ago
Keywords: dev-doc-needed
Documentation updated by Trevor.
Keywords: dev-doc-needed → dev-doc-complete

Comment 17

6 years ago
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)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.