Closed
Bug 529328
Opened 16 years ago
Closed 14 years ago
DOMTokenList index out of bounds should return undefined, not the empty string
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
VERIFIED
FIXED
mozilla7
People
(Reporter: erik, Assigned: Ms2ger)
References
()
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 2 obsolete files)
13.90 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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•16 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).
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•15 years ago
|
||
(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•14 years ago
|
Assignee: nobody → Ms2ger
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•14 years ago
|
||
Attachment #534102 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 5•14 years ago
|
||
Attachment #534103 -
Flags: review?(Olli.Pettay)
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.
Attachment #534102 -
Flags: review?(Olli.Pettay) → review-
Attachment #534103 -
Flags: review?(Olli.Pettay) → review+
Assignee | ||
Comment 7•14 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?
Yes, changing all the cases and adding tests sounds ok.
Assignee | ||
Comment 9•14 years ago
|
||
Attachment #535936 -
Flags: review?(Olli.Pettay)
Assignee | ||
Updated•14 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•14 years ago
|
Attachment #534102 -
Attachment is obsolete: true
Assignee | ||
Updated•14 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•14 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•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Assignee | ||
Updated•14 years ago
|
Keywords: dev-doc-needed
Comment 16•14 years ago
|
||
Documentation updated by Trevor.
Keywords: dev-doc-needed → dev-doc-complete
Comment 17•14 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.
Description
•