Closed Bug 679653 Opened 8 years ago Closed 8 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: mounir, Assigned: atulagrwl)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [mentor=volkmar])

Attachments

(1 file, 1 obsolete file)

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".
Depends on: 669578
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 :)
Whiteboard: [mentor=volkmar]
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.
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #556432 - Flags: review?(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.
Attachment #556432 - Flags: review?(mounir) → review+
(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)?
Yes, sorry for the confusion :)
Attached patch Patch v2Splinter Review
Attachment #556432 - Attachment is obsolete: true
Attachment #556491 - Flags: review?(mounir)
Comment on attachment 556491 [details] [diff] [review]
Patch v2

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

r=mounir
Attachment #556491 - Flags: review?(mounir) → review+
Sent to try:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=062603c0db20
Assignee: nobody → atulagrwl
It would probably be good to check the effect of this patch on the list layout stuff.
(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.
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.
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.
Yes, I can see negative integers in the layout of list after my change if it is what you meant.
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.
Pushed to m-i and will be merged to m-c soon hopefully.

The tests will be done with bug 669578.
Flags: in-testsuite?
Whiteboard: [mentor=volkmar] → [mentor=volkmar][inbound]
Pushed:
http://hg.mozilla.org/mozilla-central/rev/2ace1d703abe
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=volkmar][inbound] → [mentor=volkmar]
Target Milestone: --- → mozilla9
Keywords: dev-doc-needed
Summary: li.value attribute reflection doesn't follow the specs → li.value attribute reflection doesn't follow the specs (allow negative <li value>s)
I also added https://developer.mozilla.org/en/DOM/HTMLLIElement

I think that's all what is needed.
You need to log in before you can comment on or make changes to this bug.