679653 - li.value attribute reflection doesn't follow the specs (allow negative <li value>s)

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

Testcase:
data:text/html,<script>var li = document.createElement("li"); li.value = -222; alert(li.value);</script>

It should show "-222" but it shows "0" while Presto and Webkit shows "-222".

Comment 1

[Mounir Lamouri (:mounir)]

Specs for li element:
http://www.whatwg.org/specs/web-apps/current-work/multipage/grouping-content.html#dom-li-value

Specs for attribute reflection:
http://www.whatwg.org/specs/web-apps/current-work/multipage/urls.html#reflecting-content-attributes-in-idl-attributes

The corresponding code is in:
content/html/content/src/nsHTMLLIElement.cpp
and the macro (NS_IMPL_INT_ATTR) is defined in:
content/html/content/src/nsGenericHTMLElement.h

Ainsley is trying to write a test suite for int attributes reflection found this bug so we might not need specific tests, the test suite is going to cover this case.

@Ainsley, feel free to give this bug a try :)

Comment 2

[Atul Aggarwal]

Problem is in line 143 of content/html/content/src/nsHTMLLIElement.cpp in ParseAttribute function. 
The line is return aResult.ParseIntWithBounds(aValue, 0); The second parameter determine the minimum value returned by the function which in this case is 0.

I will create a patch and will attach with this bug in few mins.

Comment 3

[Atul Aggarwal]

Attachment: Patch v1

Comment 4

[Mounir Lamouri (:mounir)]

Comment on attachment 556432 [details] [diff] [review]
Patch v1

Review of attachment 556432 [details] [diff] [review]:
-----------------------------------------------------------------

Could you use |ParseInt(aValue);| instead?

r=mounir with that.

Comment 5

[Atul Aggarwal]

(In reply to Mounir Lamouri (:volkmar) from comment #4)
> Comment on attachment 556432 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 556432 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Could you use |ParseInt(aValue);| instead?
> 
> r=mounir with that.

Do you mean ParseIntValue (http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsAttrValue.h#245)?

Comment 6

[Mounir Lamouri (:mounir)]

Yes, sorry for the confusion :)

Comment 7

[Atul Aggarwal]

Attachment: Patch v2

Comment 8

[Mounir Lamouri (:mounir)]

Comment on attachment 556491 [details] [diff] [review]
Patch v2

Review of attachment 556491 [details] [diff] [review]:
-----------------------------------------------------------------

r=mounir

Comment 9

[Mounir Lamouri (:mounir)]

Sent to try:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=062603c0db20

Comment 10

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

It would probably be good to check the effect of this patch on the list layout stuff.

Comment 11

[Mounir Lamouri (:mounir)]

(In reply to Ms2ger from comment #10)
> It would probably be good to check the effect of this patch on the list
> layout stuff.

Atul, can you do that? Once you confirm everything is ok, I will push the patch. It seems to pass our tests.

Comment 12

[Atul Aggarwal]

Frankly, I don't understand what Ms2ger meant by list layout stuff. Let me summarize my change:
After this changeset, negative value of list item will start showing up. Only this has changed, no impact on layout etc. Layout of the list item will be changed as the list item index has been changed but that is intentional and consistent with what all other browsers show.

Comment 13

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

See the code at <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBulletFrame.cpp#395>. It looks like that would handle negative numbers, actually.

Comment 14

[Atul Aggarwal]

Yes, I can see negative integers in the layout of list after my change if it is what you meant.

Comment 15

[Mounir Lamouri (:mounir)]

I think the change is correct. Currently. Presto, Webkit and Gecko all have different behavior when a <li> have a negative value. With this change, we are going to behave like Presto which is the behavior that makes the most sense to me.

Comment 16

[Mounir Lamouri (:mounir)]

Pushed to m-i and will be merged to m-c soon hopefully.

The tests will be done with bug 669578.

Comment 17

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/2ace1d703abe

Comment 18

[Trevor]

Updated:
https://developer.mozilla.org/en/HTML/Element/li
https://developer.mozilla.org/en/Firefox_9_for_developers

Comment 19

[Jean-Yves Perrier [:teoli]]

I also added https://developer.mozilla.org/en/DOM/HTMLLIElement

I think that's all what is needed.