Bug 73911

Summary: <li value="0"> behaves like <li> (the same for negative numbers)
Product: WebKit Reporter: Andrian Cucu <acucu>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, darin, dglazkov, fmalita, WebkitBugTracker, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Andrian Cucu 2011-12-06 05:20:11 PST
Render an element like this:

<ol>
  <li value= "0" />
</ol>

or 

<ol>
  <li value= "-1" />
</ol>

Both will display 1.  like:

<ol>
  <li/>
</ol>

We should see the marker display 0. or -1. instead.

A similar issue is logged against Chrome, here: http://code.google.com/p/chromium/issues/detail?id=43565
Comment 1 Florin Malita 2011-12-06 14:54:45 PST
Previous versions of the spec are somewhat ambiguous, but the current draft is pretty clear:

http://www.w3.org/TR/2011/WD-html5-20110525/grouping-content.html#the-li-element

"The value attribute, if present, must be a valid integer giving the ordinal value of the list item.

If the value attribute is present, user agents must parse it as an integer, in order to determine the attribute's value. If the attribute's value cannot be converted to a number, the attribute must be treated as if it was absent."

http://www.w3.org/TR/2011/WD-html5-20110525/common-microsyntaxes.html#valid-integer

"A string is a valid integer if it consists of one or more characters in the range U+0030 DIGIT ZERO (0) to U+0039 DIGIT NINE (9), optionally prefixed with a U+002D HYPHEN-MINUS character (-)."

Patch coming up.
Comment 2 Florin Malita 2011-12-06 14:57:02 PST
Created attachment 118117 [details]
Patch
Comment 3 Alexey Proskuryakov 2011-12-06 15:21:29 PST
Comment on attachment 118117 [details]
Patch

Please add tests for list-style-types that have non-positive values out of range (and make sure that these pass).

E.g. upper-roman has range of 1...4999, and falls back to "decimal". And japanese-formal falls back to cjk-decimal.

Please see <http://www.w3.org/TR/css3-lists> for discussion of ranges and fallback.
Comment 4 Florin Malita 2011-12-07 08:56:29 PST
Created attachment 118217 [details]
Patch

Added out-of-range value tests
Comment 5 Florin Malita 2011-12-07 09:03:06 PST
(In reply to comment #3)
> (From update of attachment 118117 [details])
> Please add tests for list-style-types that have non-positive values out of range (and make sure that these pass).
> 
> E.g. upper-roman has range of 1...4999, and falls back to "decimal". And japanese-formal falls back to cjk-decimal.
> 
> Please see <http://www.w3.org/TR/css3-lists> for discussion of ranges and fallback.

Looks like fast/lists/w3-css3-list-styles-fallback-style.html covers out of range fallbacks using ol/start. I've added some li-values.html tests to validate ranged styles fallback using li/value too.
Comment 6 Darin Adler 2011-12-07 09:10:52 PST
Comment on attachment 118217 [details]
Patch

Patch seems OK. Three thoughts:

1) This logic should not be repeated twice. We should find a way to share the code between these two call sites.
2) There is no need for a special case for the null string in HTMLLIElement::attach.
3) Normally I would spell it "OK", not "Ok".
Comment 7 Alexey Proskuryakov 2011-12-07 09:19:08 PST
Thanks. Could be useful to also test programmatic changes to the value.
Comment 8 WebKit Review Bot 2011-12-07 10:03:55 PST
Comment on attachment 118217 [details]
Patch

Attachment 118217 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10767121
Comment 9 Florin Malita 2011-12-07 10:23:53 PST
Created attachment 118228 [details]
Patch
Comment 10 Florin Malita 2011-12-07 10:24:42 PST
(In reply to comment #6)
> (From update of attachment 118217 [details])
> Patch seems OK. Three thoughts:
> 
> 1) This logic should not be repeated twice. We should find a way to share the code between these two call sites.
> 2) There is no need for a special case for the null string in HTMLLIElement::attach.
> 3) Normally I would spell it "OK", not "Ok".

(In reply to comment #7)
> Thanks. Could be useful to also test programmatic changes to the value.

Updated per comments, thanks.
Comment 11 Darin Adler 2011-12-07 10:55:33 PST
Comment on attachment 118228 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118228&action=review

> Source/WebCore/html/HTMLLIElement.h:43
> +    void setValue(const AtomicString&);

This is great refactoring. I don’t think the name of the function is good, though, because it doesn’t set the value on the element. Maybe setValueInRenderer or parseValue.
Comment 12 Alexey Proskuryakov 2011-12-07 10:59:00 PST
View in context: https://bugs.webkit.org/attachment.cgi?id=118228&action=review

> Source/WebCore/html/HTMLLIElement.cpp:-113
> -        const AtomicString& requestedValueString = fastGetAttribute(valueAttr);
> -        if (requestedValueString.isNull())
> -            render->clearExplicitValue();

Are you saying that the isNull check is no longer needed, because valueOK will be false? Things like this should be explained in ChangeLog (this is why prepare-Script generates a list of modified functions).

How will this affect performance? At a first glance, it feels like a bigger issue than "inline" on setValue that you added.

> Source/WebCore/html/HTMLLIElement.cpp:111
> +    if (renderer() && renderer()->isListItem()) {

WebKit style prefers early return.
Comment 13 Darin Adler 2011-12-07 11:00:39 PST
(In reply to comment #12)
> View in context: https://bugs.webkit.org/attachment.cgi?id=118228&action=review
> 
> > Source/WebCore/html/HTMLLIElement.cpp:-113
> > -        const AtomicString& requestedValueString = fastGetAttribute(valueAttr);
> > -        if (requestedValueString.isNull())
> > -            render->clearExplicitValue();
> 
> Are you saying that the isNull check is no longer needed, because valueOK will be false?

Yes. I suggested that change.

> How will this affect performance?

Should be minimal. The toInt function quickly returns failure with null string.
Comment 14 Florin Malita 2011-12-07 12:04:50 PST
Created attachment 118249 [details]
Patch
Comment 15 Florin Malita 2011-12-07 12:11:23 PST
(In reply to comment #11)
> I don’t think the name of the function is good, though, because it doesn’t set the value on the element. Maybe setValueInRenderer or parseValue.

Good point. Using parseValue().


(In reply to comment #12)
> View in context: https://bugs.webkit.org/attachment.cgi?id=118228&action=review
> 
> > Source/WebCore/html/HTMLLIElement.cpp:-113
> > -        const AtomicString& requestedValueString = fastGetAttribute(valueAttr);
> > -        if (requestedValueString.isNull())
> > -            render->clearExplicitValue();
> 
> Are you saying that the isNull check is no longer needed, because valueOK will be false? Things like this should be explained in ChangeLog (this is why prepare-Script generates a list of modified functions).

ChangeLog updated.


> How will this affect performance? At a first glance, it feels like a bigger issue than "inline" on setValue that you added.

As far as the isNull() check goes, if the function gets inlined the optimized result should be equivalent (as Darin mentioned, toInt() performs pretty much the same check).

I've also extracted the renderer() && renderer()->isListItem() test back into parseMappedAttribute() to avoid evaluating it twice on the attach path.


> 
> > Source/WebCore/html/HTMLLIElement.cpp:111
> > +    if (renderer() && renderer()->isListItem()) {
> 
> WebKit style prefers early return.

Done (no longer present in parseValue()).
Comment 16 WebKit Review Bot 2011-12-07 17:45:07 PST
Comment on attachment 118249 [details]
Patch

Clearing flags on attachment: 118249

Committed r102290: <http://trac.webkit.org/changeset/102290>
Comment 17 WebKit Review Bot 2011-12-07 17:45:13 PST
All reviewed patches have been landed.  Closing bug.