The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla9

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mounir, Assigned: Atul Aggarwal)

Tracking

({dev-doc-complete})

Trunk
mozilla9
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=volkmar])

Attachments

(1 attachment, 1 obsolete attachment)

906 bytes, patch
mounir
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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".
(Reporter)

Updated

6 years ago
Depends on: 669578
(Reporter)

Comment 1

6 years ago
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]
(Assignee)

Comment 2

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

Comment 3

6 years ago
Created attachment 556432 [details] [diff] [review]
Patch v1
Attachment #556432 - Flags: review?(mounir)
(Reporter)

Comment 4

6 years ago
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+
(Assignee)

Comment 5

6 years ago
(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)?
(Reporter)

Comment 6

6 years ago
Yes, sorry for the confusion :)
(Assignee)

Comment 7

6 years ago
Created attachment 556491 [details] [diff] [review]
Patch v2
Attachment #556432 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #556491 - Flags: review?(mounir)
(Reporter)

Comment 8

6 years ago
Comment on attachment 556491 [details] [diff] [review]
Patch v2

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

r=mounir
Attachment #556491 - Flags: review?(mounir) → review+
(Reporter)

Comment 9

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

Comment 11

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

Comment 12

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

Comment 14

6 years ago
Yes, I can see negative integers in the layout of list after my change if it is what you meant.
(Reporter)

Comment 15

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

Comment 16

6 years ago
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]
(Reporter)

Comment 17

6 years ago
Pushed:
http://hg.mozilla.org/mozilla-central/rev/2ace1d703abe
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=volkmar][inbound] → [mentor=volkmar]
Target Milestone: --- → mozilla9

Updated

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

Comment 18

6 years ago
Updated:
https://developer.mozilla.org/en/HTML/Element/li
https://developer.mozilla.org/en/Firefox_9_for_developers
I also added https://developer.mozilla.org/en/DOM/HTMLLIElement

I think that's all what is needed.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.