<li value="0"> behaves like <li> (the same for negative numbers)

[Andrian Cucu]

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

[Florin Malita]

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.

[Florin Malita]

Created attachment 118117 [details]
Patch

[Alexey Proskuryakov]

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.

[Florin Malita]

Created attachment 118217 [details]
Patch

Added out-of-range value tests

[Florin Malita]

(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.

[Darin Adler]

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".

[Alexey Proskuryakov]

Thanks. Could be useful to also test programmatic changes to the value.

[WebKit Review Bot]

Comment on attachment 118217 [details]
Patch

Attachment 118217 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10767121

[Florin Malita]

Created attachment 118228 [details]
Patch

[Florin Malita]

(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.

[Darin Adler]

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.

[Alexey Proskuryakov]

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.

[Darin Adler]

(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.

[Florin Malita]

Created attachment 118249 [details]
Patch

[Florin Malita]

(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()).

[WebKit Review Bot]

Comment on attachment 118249 [details]
Patch

Clearing flags on attachment: 118249

Committed r102290: <http://trac.webkit.org/changeset/102290>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.