Implement symbolic CSS3 list-style-types

• Component: CSS
• Status: RESOLVED FIXED

[Daniel Bates]

We should implement all of the symbolic CSS3 list-style-types as per section 4.5 of the CSS3 Lists module <http://www.w3.org/TR/css3-lists/#symbolic>.

[Daniel Bates]

Created attachment 51965 [details]
Patch with test case

[WebKit Review Bot]

Attachment 51965 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
File not a recognized type to check. Skipping: "WebCore/css/CSSValueKeywords.in"
File not a recognized type to check. Skipping: "WebCore/inspector/front-end/SourceCSSTokenizer.re2js"
WebCore/rendering/RenderListMarker.cpp:1048:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/rendering/RenderListMarker.cpp:1436:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

See bug 36724 for my ideas about how to make some text-only tests for list numbering.

[Darin Adler]

Mac early warning system says there was a failure, but I don't see what the error was.

[Darin Adler]

Comment on attachment 51965 [details]
Patch with test case

> -        isNegativeNumber = true;
> +        isNegativeNumber = type == SymbolicSequence ? false : true; // Symbolics cannot be negative.

For clarity I suggest writing either this:

    isNegativeNumber = type != SymbolicSequence; // Symbolics cannot be negative.

Or this:

    if (type != SymbolicSequence)
        isNegativeNumber = true; // Symbolics cannot be negative.

It seems to me that the ?: expression is too oblique. Especially "? false : true", which is just a sideways way to write "!".

Another way to do this is to let isNegativeNumber get set, and then change it below to:

    if (isNegativeNumber && type != SymbolicSequence)
        letters[lettersSize - ++length] = hyphenMinus;

Because symbolics can indeed be negative. They just don't show a minus sign when they are.

> +        unsigned numSymbols = ceil((double) (numberShadow + 1) / sequenceSize) - 1;

There is no need to involve floating point. The following expression gives the same result for all the positive numbers I tried:

    unsigned numSymbols = numberShadow / sequenceSize;

You should use that instead unless there's a reason not to.

> +            width = m_text.isEmpty() ? 0 : font.width(m_text); // no suffix for these types

I don't think you need a special case here for the empty string.

> +        case Asterisks:
> +        case Footnotes: {
> +            if (m_text.isEmpty())
> +                return IntRect();
> +            const Font& font = style()->font();
> +            return IntRect(x(), y() + font.ascent(), font.width(m_text), font.height());
> +        }

I'm not sure you need it here either.

review- for the gratuitous floating point

[Daniel Bates]

(In reply to comment #5)
> (From update of attachment 51965 [details])
> > -        isNegativeNumber = true;
> > +        isNegativeNumber = type == SymbolicSequence ? false : true; // Symbolics cannot be negative.
> 
> For clarity I suggest writing either this:
> 
>     isNegativeNumber = type != SymbolicSequence; // Symbolics cannot be
> negative.
> 
> Or this:
> 
>     if (type != SymbolicSequence)
>         isNegativeNumber = true; // Symbolics cannot be negative.
> 
> It seems to me that the ?: expression is too oblique. Especially "? false :
> true", which is just a sideways way to write "!".
> 
> Another way to do this is to let isNegativeNumber get set, and then change it
> below to:
> 
>     if (isNegativeNumber && type != SymbolicSequence)
>         letters[lettersSize - ++length] = hyphenMinus;

Will change, went with the last suggestion.

> Because symbolics can indeed be negative. They just don't show a minus sign
> when they are.
> 
> > +        unsigned numSymbols = ceil((double) (numberShadow + 1) / sequenceSize) - 1;

> 
> There is no need to involve floating point. The following expression gives the
> same result for all the positive numbers I tried:
> 
>     unsigned numSymbols = numberShadow / sequenceSize;

Will change.

> 
> You should use that instead unless there's a reason not to.
> 
> > +            width = m_text.isEmpty() ? 0 : font.width(m_text); // no suffix for these types
> 

Will change.

> I don't think you need a special case here for the empty string.
> 
> > +        case Asterisks:
> > +        case Footnotes: {
> > +            if (m_text.isEmpty())
> > +                return IntRect();
> > +            const Font& font = style()->font();
> > +            return IntRect(x(), y() + font.ascent(), font.width(m_text), font.height());
> > +        }
> 
> I'm not sure you need it here either.

Will change.

[Daniel Bates]

Created attachment 51998 [details]
Patch with test case

Updated patch based on Darin's suggestions.

I am not marking this patch for review, yet, as I am looking into Darin's idea on how to make a text-only layout test for this.

Also, I am aware of the style issues. I suggest we do a stye clean-up patch either preceding or proceeding this bug.

[Daniel Bates]

Comment on attachment 51998 [details]
Patch with test case

After looking over the discussion in bug #36724 which implied adding DRT support so that we can test using text-only tests, I suggest that we address this issue in a separate bug. I agree we should add this support since using render tree results seems unnecessary.

If Darin or anyone else feels that we should add DRT support/make it so that we can use text-only tests before proceeding with this patch then let me know.

[WebKit Review Bot]

Attachment 51998 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/rendering/RenderListMarker.cpp:1048:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/rendering/RenderListMarker.cpp:1436:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jakub Wieczorek]

(In reply to comment #8)
> (From update of attachment 51998 [details])
> After looking over the discussion in bug #36724 which implied adding DRT
> support so that we can test using text-only tests, I suggest that we address
> this issue in a separate bug. I agree we should add this support since using
> render tree results seems unnecessary.
> 
> If Darin or anyone else feels that we should add DRT support/make it so that we
> can use text-only tests before proceeding with this patch then let me know.

FYI: Text-only test support for lists is being added as part of #37060.

[Daniel Bates]

Comment on attachment 51998 [details]
Patch with test case

Will update test case with the landing of bug #37060, which includes DRT support.

[Daniel Bates]

Created attachment 62485 [details]
Patch with test case

With the resolution of Bug #42054, converted test case to text.

This patch depends on the changes to the JavaScript script LayoutTests/fast/lists/resources/dump-list.js in the patch "[Patch] Part 1 of 2 (Updated tests and expected results)" of Bug #42803.

[WebKit Review Bot]

Attachment 62485 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/rendering/RenderListMarker.cpp:1050:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/rendering/RenderListMarker.cpp:1438:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 62485 [details]
Patch with test case

>      const int lettersSize = sizeof(number) * 8 + 1; // Binary is the worst case; requires one character per bit plus a minus sign.

The new symbols sequence is now the worst case, much worse than binary. If you don't change something there will be buffer overflow.

> Index: LayoutTests/fast/lists/w3-css3-list-styles-symbolic-expected.txt
> ===================================================================
> --- LayoutTests/fast/lists/w3-css3-list-styles-symbolic-expected.txt	(revision 0)
> +++ LayoutTests/fast/lists/w3-css3-list-styles-symbolic-expected.txt	(revision 0)
> @@ -0,0 +1,13 @@
> +CSS3 Symbolic list-style-type
> +
> +asterisks
> +
> +PASS
> +PASS
> +PASS
> +footnotes
> +
> +PASS
> +PASS
> +PASS
> +

Tests that just write out "PASS PASS PASS" are suboptimal. Can you change this test so the output makes it clearer what is going on?

Do we need some kind of limit? I'm worried that you can generate millions of asterisks and cause problems that way. Some limit would prevent that possibility.

review- because I think this has a buffer overflow in it

[Daniel Bates]

(In reply to comment #14)
> (From update of attachment 62485 [details])
> >      const int lettersSize = sizeof(number) * 8 + 1; // Binary is the worst case; requires one character per bit plus a minus sign.
> 
> The new symbols sequence is now the worst case, much worse than binary. If you don't change something there will be buffer overflow.

Thank you Darin for catching this issue. Will fix.

> 
> > Index: LayoutTests/fast/lists/w3-css3-list-styles-symbolic-expected.txt
> > ===================================================================
> > --- LayoutTests/fast/lists/w3-css3-list-styles-symbolic-expected.txt	(revision 0)
> > +++ LayoutTests/fast/lists/w3-css3-list-styles-symbolic-expected.txt	(revision 0)
> > @@ -0,0 +1,13 @@
> > +CSS3 Symbolic list-style-type
> > +
> > +asterisks
> > +
> > +PASS
> > +PASS
> [...]
> 
> Tests that just write out "PASS PASS PASS" are suboptimal. Can you change this test so the output makes it clearer what is going on?
>

Sure.

We should also make this message more descriptive in the expected results for tests: w3-css3-list-styles-alphabetic.html and w3-css3-list-styles-numeric.html (included in the patch for Bug #42803). Instead of posting an updated patch for Bug #42803, I thought it would be easier to review just the change to the PASS message. So, I filed Bug #42938. 

I'll update the expected results in this patch to use the updated PASS message.

[Daniel Bates]

Created attachment 63028 [details]
Patch with test case

I picked the upper bound for the length of a symbolic marker to be lettersSize := sizeof(int) * 8 + 1 characters (i.e. the number of characters to represent a binary number plus a minus sign). Currently, we use a fixed sized array, but we want to reconsider this and/or special-case symbolics which use more characters per ordinal than other list style types.

[WebKit Review Bot]

Attachment 63028 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/rendering/RenderListMarker.cpp:1053:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/rendering/RenderListMarker.cpp:1441:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 63028 [details]
Patch with test case

> +        unsigned numSymbols = (numberShadow / sequenceSize) % lettersSize;

This is unusual math, given that both numberShadow and lettersSize are int, not unsigned. You are testing it enough, I guess, but I worry since if it goes wrong we could have a buffer overrun.

Why is modulo the right thing to do? We want to go to zero symbols when we hit the maximum length? I would expect a computation involving min instead, and it would be more obvious that prevents the overrun.

I suppose having a maximum is good anyway to avoid crazy behavior, but if there was any argument in favor of supporting long symbolic sequences then I would suggest changing the code to heap allocate in the cases where necessary.

[Daniel Bates]

Comment on attachment 63028 [details]
Patch with test case

Clearing review flag while I look into this further.

[Daniel Bates]

Created attachment 64434 [details]
Patch with test case

Updated patch that substitutes a Vector<UChar> for the fixed sized stack-allocated array so that we can handle arbitrary length symbolics (up to architecture-specific size of int).

The spec. is unclear how we should handle a list with a symbolic list-style-type  and that has a starting ordinal of 0 or some x < 0, where x is an integer. Currently, this patch outputs one symbol and |x| symbols for starting ordinals 0 and x < 0, respectively. From the description of the alphabetic- and numeric- systems, the spec. seems to imply we should output decimal ordinals for these cases. I will try to contact Ian Hickson for his insight on these cases.

[Daniel Bates]

(In reply to comment #20)
> Created an attachment (id=64434) [details]
> Patch with test case
> 
> Updated patch that substitutes a Vector<UChar> for the fixed sized stack-allocated array so that we can handle arbitrary length symbolics (up to architecture-specific size of int).
> 
> The spec. is unclear how we should handle a list with a symbolic list-style-type  and that has a starting ordinal of 0 or some x < 0, where x is an integer. Currently, this patch outputs one symbol and |x| symbols for starting ordinals 0 and x < 0, respectively. From the description of the alphabetic- and numeric- systems, the spec. seems to imply we should output decimal ordinals for these cases. I will try to contact Ian Hickson for his insight on these cases.

Spoke with Ian Hickson last weekend and he confirmed that we should fall back to the decimal list style for ordinals outside the representable range for a list style. In particular, for symbolic list style types we should fall back to the decimal list style for ordinals <= 0.

Bug #44486 implemented the necessary support infrastructure to support the fall back to the decimal list style.

[Daniel Bates]

Created attachment 65675 [details]
Patch with test case

[WebKit Review Bot]

Attachment 65675 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/rendering/RenderListMarker.cpp:1155:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/rendering/RenderListMarker.cpp:1543:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 65675 [details]
Patch with test case

> -static inline String toAlphabeticOrNumeric(int number, const UChar* sequence, int sequenceSize, SequenceType type)
> +static inline String toSequence(int number, const UChar* sequence, unsigned sequenceSize, SequenceType type)

The code for SymbolicSequence seems to share almost nothing with the alphabetic and numeric sequences. The assertion needed is different, the buffer size needed is different, the loop is different. About the only thing that's shared is the code to subtract one from the passed-in number.

I suggest implementing toSymbolic without calling toAlphabeticOrNumeric. There's no meaningful code sharing here.

Everything else looks fine.

r=me

[Daniel Bates]

(In reply to comment #24)
> (From update of attachment 65675 [details])
> > -static inline String toAlphabeticOrNumeric(int number, const UChar* sequence, int sequenceSize, SequenceType type)
> > +static inline String toSequence(int number, const UChar* sequence, unsigned sequenceSize, SequenceType type)
> 
> The code for SymbolicSequence seems to share almost nothing with the alphabetic and numeric sequences. The assertion needed is different, the buffer size needed is different, the loop is different. About the only thing that's shared is the code to subtract one from the passed-in number.

You're right. I will separate symbolics out from this function as its implementation has become disjoint. I'll leave the name of the function as toAlphabeticOrNumeric.

> 
> I suggest implementing toSymbolic without calling toAlphabeticOrNumeric. There's no meaningful code sharing here.
>

Will do. 

> Everything else looks fine.
> 
> r=me

Thank you for the review.

[Daniel Bates]

Committed r66296: <http://trac.webkit.org/changeset/66296>

[WebKit Review Bot]

http://trac.webkit.org/changeset/66296 might have broken Qt Linux Release minimal

[Adam Barth]

(In reply to comment #27)
> http://trac.webkit.org/changeset/66296 might have broken Qt Linux Release minimal

Looks like it broke compile there.

[Daniel Bates]

(In reply to comment #28)
> (In reply to comment #27)
> > http://trac.webkit.org/changeset/66296 might have broken Qt Linux Release minimal
> 
> Looks like it broke compile there.

From looking at the stdio output <http://build.webkit.org/builders/Qt%20Linux%20Release%20minimal/builds/7850/steps/compile-webkit/logs/stdio> the compile failed because the slave was lost: "remoteFailed: [Failure instance: Traceback (failure with no frames): <class 'twisted.internet.error.ConnectionLost'>: Connection to the other side was lost in a non-clean fashion.
]".