Last Comment Bug 679653 - li.value attribute reflection doesn't follow the specs (allow negative <li value>s)
: li.value attribute reflection doesn't follow the specs (allow negative <li va...
Status: RESOLVED FIXED
[mentor=volkmar]
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla9
Assigned To: Atul Aggarwal
:
:
Mentors:
Depends on: 669578
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-17 03:33 PDT by Mounir Lamouri (:mounir)
Modified: 2011-12-08 05:11 PST (History)
6 users (show)
mounir: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (925 bytes, patch)
2011-08-28 15:17 PDT, Atul Aggarwal
mounir: review+
Details | Diff | Splinter Review
Patch v2 (906 bytes, patch)
2011-08-29 02:19 PDT, Atul Aggarwal
mounir: review+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2011-08-17 03:33:59 PDT
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) 2011-08-17 03:42:23 PDT
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 2011-08-28 15:04:19 PDT
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 2011-08-28 15:17:24 PDT
Created attachment 556432 [details] [diff] [review]
Patch v1
Comment 4 Mounir Lamouri (:mounir) 2011-08-29 02:07:15 PDT
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 2011-08-29 02:13:55 PDT
(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) 2011-08-29 02:15:01 PDT
Yes, sorry for the confusion :)
Comment 7 Atul Aggarwal 2011-08-29 02:19:01 PDT
Created attachment 556491 [details] [diff] [review]
Patch v2
Comment 8 Mounir Lamouri (:mounir) 2011-08-29 02:24:12 PDT
Comment on attachment 556491 [details] [diff] [review]
Patch v2

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

r=mounir
Comment 9 Mounir Lamouri (:mounir) 2011-08-29 02:28:21 PDT
Sent to try:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=062603c0db20
Comment 10 :Ms2ger (⌚ UTC+1/+2) 2011-08-30 12:59:06 PDT
It would probably be good to check the effect of this patch on the list layout stuff.
Comment 11 Mounir Lamouri (:mounir) 2011-08-30 15:09:34 PDT
(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 2011-08-30 16:05:23 PDT
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 (⌚ UTC+1/+2) 2011-08-31 01:02:04 PDT
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 2011-08-31 01:38:19 PDT
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) 2011-08-31 03:29:35 PDT
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) 2011-08-31 03:37:25 PDT
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) 2011-08-31 08:00:11 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/2ace1d703abe
Comment 19 Jean-Yves Perrier [:teoli] 2011-12-08 05:11:51 PST
I also added https://developer.mozilla.org/en/DOM/HTMLLIElement

I think that's all what is needed.

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