Bug 112615 – [css3-text] Implement support for vertical writing mode in -webkit-text-underline-position

• Component: CSS
• Status: RESOLVED DUPLICATE

[Lamarque V. Souza]

The CSS3 property "text-decoration-style" accepts the following values: "auto | alphabetic | [ under || [ left | right ] ]". Support for values "auto | alphabetic | under" was implemented in bugs https://bugs.webkit.org/show_bug.cgi?id=102491 and https://bugs.webkit.org/show_bug.cgi?id=102795. This bug intents to implement the support for values "under left" and "under right", which only applies for vertical writing mode according to the specification:

http://dev.w3.org/csswg/css-text-decor-3/#text-underline-position-property

[Yuki Sekiguchi]

Created attachment 198033 [details]
Patch

[Lamarque V. Souza]

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

> Source/WebCore/css/CSSParser.cpp:9340
>      // However, values 'left' and 'right' are not implemented yet, so we will parse sintax

This comment should be removed.

> Source/WebCore/css/CSSParser.cpp:9352
> +    case CSSValueRight:

The part of the parser is incorret. The only acceptable values are either 'left', 'right', 'under left' or 'under right'. No other combination should be accepted. Here you accept things like 'left right', 'under alphabetic', 'right auto', etc. I created a valid parser in https://bug-102491-attachments.webkit.org/attachment.cgi?id=191578 but I removed since I was not planning to implement the vertical text support so soon.

> Source/WebCore/rendering/InlineTextBox.cpp:995
>      // and to 'under Left' for vertical text (e.g. japanese). We support only horizontal text for now.

Remove the "We support only horizontal text for now." comment.

> Source/WebCore/rendering/InlineTextBox.cpp:996
> +    bool drawsUnderLeft;

You can detect 'under left' and 'under right' values from underlinePosition variable. You do not need those two bools. 'under left' and 'under right' are mutually exclusive by the way.

> Source/WebCore/rendering/InlineTextBox.cpp:1022
> +    bool isOpposite;

What does exactly isOpposite mean? Opposite to what?

> LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-horizontal.html:26
> +<span class="decoration" style="color: cyan; -webkit-text-underline-position: left right;">left right :p The line should be alphabetic.</span>

"left right" is not a valid value for text-underline-position according to the specification.

> LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-horizontal.html:29
> +<span class="decoration" style="color: black; -webkit-text-underline-position: alphabetic right;">alphabetic right :p The line should be alphabetic.</span>

also not a valid value.

> LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-horizontal.html:32
> +<span class="decoration" style="color: blue; -webkit-text-underline-position: under right left;">under right left :p The line should be alphabetic.</span>

also not a valid value.

> LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-vertical.html:26
> +<span class="decoration" style="color: cyan; -webkit-text-underline-position: left right;">left right :p The line should be alphabetic.</span>

not valid value.

> LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-vertical.html:29
> +<span class="decoration" style="color: black; -webkit-text-underline-position: alphabetic right;">alphabetic right :p The line should be alphabetic.</span>

not valid value.

> LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-vertical.html:32
> +<span class="decoration" style="color: blue; -webkit-text-underline-position: under right left;">under right left :p The line should be alphabetic.</span>

not valid value.

[Yuki Sekiguchi]

Created attachment 202789 [details]
Patch

[Yuki Sekiguchi]

Created attachment 202797 [details]
Patch

[Lamarque V. Souza]

I did not have time to review all the patch, so I will write only the things I spotted in the first glance:

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

> Source/WebCore/ChangeLog:12
> +        because these line should ignore veritcal-align like under underline in horizontal writing mode.

typo: "veritcal-align"

> Source/WebCore/rendering/InlineTextBox.cpp:1011
> +    default:

Do not move this code into the default statement. Read comment https://bugs.webkit.org/show_bug.cgi?id=102795#c63 about this.

> LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-horizontal.html:26
> +<span class="decoration" style="color: cyan; -webkit-text-underline-position: left right;">left right :p The line should be alphabetic.</span>

This is an invalid value. It is ok that your patch defaults to alphabetic here. However, your patch should update LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-underline-position.html to check for this and other invalid values. Anything that is not either "auto", "alphabetic", "under", "under left" or "under right" is invalid.

[Yuki Sekiguchi]

Created attachment 202967 [details]
Patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-underline-position-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-underline-position.js', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/style.css', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-double.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-left-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-left.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-horizontal.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-vertical.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right.html', u'LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-left-expected.png', u'LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-expected.png', u'LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-horizontal-expected.png', u'LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-horizontal-expected.txt', u'LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-vertical-expected.png', u'LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-vertical-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSPrimitiveValueMappings.h', u'Source/WebCore/rendering/InlineFlowBox.cpp', u'Source/WebCore/rendering/InlineTextBox.cpp', u'Source/WebCore/rendering/style/RenderStyleConstants.h', u'Source/WebCore/rendering/style/StyleRareNonInheritedData.h']" exit_code: 1
Source/WebCore/rendering/InlineTextBox.cpp:984:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 22 files


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

[Yuki Sekiguchi]

Thank you for your advice.

(In reply to comment #2)
> View in context: https://bugs.webkit.org/attachment.cgi?id=198033&action=review
> 
> > Source/WebCore/css/CSSParser.cpp:9340
> >      // However, values 'left' and 'right' are not implemented yet, so we will parse sintax
> 
> This comment should be removed.

Removed.

> > Source/WebCore/css/CSSParser.cpp:9352
> > +    case CSSValueRight:
> 
> The part of the parser is incorret. The only acceptable values are either 'left', 'right', 'under left' or 'under right'. No other combination should be accepted. Here you accept things like 'left right', 'under alphabetic', 'right auto', etc. I created a valid parser in https://bug-102491-attachments.webkit.org/attachment.cgi?id=191578 but I removed since I was not planning to implement the vertical text support so soon.

Fixed.

> > Source/WebCore/rendering/InlineTextBox.cpp:995
> >      // and to 'under Left' for vertical text (e.g. japanese). We support only horizontal text for now.
> 
> Remove the "We support only horizontal text for now." comment.

Removed.

> > Source/WebCore/rendering/InlineTextBox.cpp:996
> > +    bool drawsUnderLeft;
> 
> You can detect 'under left' and 'under right' values from underlinePosition variable. You do not need those two bools. 'under left' and 'under right' are mutually exclusive by the way.

Changed to use one value.

> > Source/WebCore/rendering/InlineTextBox.cpp:1022
> > +    bool isOpposite;
> 
> What does exactly isOpposite mean? Opposite to what?

Just removed.

> > +<span class="decoration" style="color: cyan; -webkit-text-underline-position: left right;">left right :p The line should be alphabetic.</span>
> 
> "left right" is not a valid value for text-underline-position according to the specification.
> 
> > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-horizontal.html:29
> > +<span class="decoration" style="color: black; -webkit-text-underline-position: alphabetic right;">alphabetic right :p The line should be alphabetic.</span>
> 
> also not a valid value.
> 
> > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-horizontal.html:32
> > +<span class="decoration" style="color: blue; -webkit-text-underline-position: under right left;">under right left :p The line should be alphabetic.</span>
> 
> also not a valid value.
> 
> > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-vertical.html:26
> > +<span class="decoration" style="color: cyan; -webkit-text-underline-position: left right;">left right :p The line should be alphabetic.</span>
> 
> not valid value.
> 
> > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-vertical.html:29
> > +<span class="decoration" style="color: black; -webkit-text-underline-position: alphabetic right;">alphabetic right :p The line should be alphabetic.</span>
> 
> not valid value.
> 
> > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-vertical.html:32
> > +<span class="decoration" style="color: blue; -webkit-text-underline-position: under right left;">under right left :p The line should be alphabetic.</span>
> 
> not valid value.

Yes, these are not valid value.
We should check invalid case.
These test cases found my bugs, so I think adding this to trunk is valuable.

[Yuki Sekiguchi]

(In reply to comment #5)
> I did not have time to review all the patch, so I will write only the things I spotted in the first glance:
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=202797&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        because these line should ignore veritcal-align like under underline in horizontal writing mode.
> 
> typo: "veritcal-align"

Fixed.

> > Source/WebCore/rendering/InlineTextBox.cpp:1011
> > +    default:
> 
> Do not move this code into the default statement. Read comment https://bugs.webkit.org/show_bug.cgi?id=102795#c63 about this.

Fixed.

> > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-horizontal.html:26
> > +<span class="decoration" style="color: cyan; -webkit-text-underline-position: left right;">left right :p The line should be alphabetic.</span>
> 
> This is an invalid value. It is ok that your patch defaults to alphabetic here. However, your patch should update LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-underline-position.html to check for this and other invalid values.

Added test cases to getComputedStyle-text-underline-position.html.
I found that I should implement CSSValueList behavior, so I implemented it.

>  Anything that is not either "auto", "alphabetic", "under", "under left" or "under right" is invalid.

This is not correct.
"left under" and "right under" are also valid value.
There is the definition of || in CSS2.1[1]
> A double bar (||) separates two or more options: one or more of them must occur, in any order.

This says "in any order", so we can write "left under" or "right under".

[1]: http://www.w3.org/TR/CSS21/about.html#property-defs

[Yuki Sekiguchi]

(In reply to comment #7)
> Attachment 202967 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-underline-position-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-underline-position.js', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/style.css', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-double.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-left-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-left.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-horizontal.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-vertical.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right.html', u'LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-left-expected.png', u'LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-expected.png', u'LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-horizontal-expected.png', u'LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-horizontal-expected.txt', u'LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-vertical-expected.png', u'LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-vertical-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSPrimitiveValueMappings.h', u'Source/WebCore/rendering/InlineFlowBox.cpp', u'Source/WebCore/rendering/InlineTextBox.cpp', u'Source/WebCore/rendering/style/RenderStyleConstants.h', u'Source/WebCore/rendering/style/StyleRareNonInheritedData.h']" exit_code: 1
> Source/WebCore/rendering/InlineTextBox.cpp:984:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
> Total errors found: 1 in 22 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

The outer "if" dose not conclude with a return.
This may be bug of style checker.

[Lamarque V. Souza]

(In reply to comment #8)
> Thank you for your advice.

You are welcome :-)
 
> (In reply to comment #2)
> Yes, these are not valid value.
> We should check invalid case.
> These test cases found my bugs, so I think adding this to trunk is valuable.

I agree, just that you added them to the wrong file. You should have used getComputedStyle-text-underline-position.html for checking the parser of this patch. The other unit tests are for the rendering part. Keep in mind that getComputedStyle-text-underline-position.html is text only test and the others are pixel tests. You do not need pixel tests to test the parser.

(In reply to comment #9)
> Added test cases to getComputedStyle-text-underline-position.html.
> I found that I should implement CSSValueList behavior, so I implemented it.

Good.
 
> >  Anything that is not either "auto", "alphabetic", "under", "under left" or "under right" is invalid.
> 
> This is not correct.
> "left under" and "right under" are also valid value.
> There is the definition of || in CSS2.1[1]
> > A double bar (||) separates two or more options: one or more of them must occur, in any order.
> 
> This says "in any order", so we can write "left under" or "right under".
> 
> [1]: http://www.w3.org/TR/CSS21/about.html#property-defs

Ok about that then.(In reply to comment #10)
> (In reply to comment #7)
> > Attachment 202967 [details] [details] did not pass style-queue:
> > 
> > Source/WebCore/rendering/InlineTextBox.cpp:984:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
> > Total errors found: 1 in 22 files
> > 
> > 
> > If any of these errors are false positives, please file a bug against check-webkit-style.
> 
> The outer "if" dose not conclude with a return.
> This may be bug of style checker.

What the error message want to say is that you should do:

if (...)
    return <something>;
if (...)
    return <something>;

instead of

if (...)
    return <something>;
else if (...)
    return <something>;

You just need to remove the "else"'s in your source code. They are not necessary if there is a return inside the if statement.

[Lamarque V. Souza]

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1389
> +    return list;

You should add an assert here, list must never be empty. Besides, I do not see a need to replace CSSPrimitiveValue::CSSPrimitiveValue(TextUnderlinePosition e) with this function. This just makes your patch bigger. I see that you used one of my draft patch for text-underline-position to implement this function but since WebKit already uses CSSPrimitiveValue::CSSPrimitiveValue(TextUnderlinePosition e) you should implement your patch using it unless there is a really good reason for not to.

> Source/WebCore/css/CSSPrimitiveValueMappings.h:2468
>      // FIXME: Implement support for 'under left' and 'under right' values.

Remove obsolete comment.

> Source/WebCore/rendering/InlineTextBox.cpp:1251
> +                FloatPoint start(localOrigin.x(), localOrigin.y() + underlineOffset + (isTopSide ? -doubleOffset : doubleOffset));

We should add tests similar to LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-style.html to check for this change. Maybe a text-decoration-style-underline-left.html and text-decoration-style-underline-right.html

[Yuki Sekiguchi]

Created attachment 203027 [details]
Patch

[Yuki Sekiguchi]

(In reply to comment #11)
> I agree, just that you added them to the wrong file. You should have used getComputedStyle-text-underline-position.html for checking the parser of this patch. The other unit tests are for the rendering part. Keep in mind that getComputedStyle-text-underline-position.html is text only test and the others are pixel tests. You do not need pixel tests to test the parser.

OK. I removed rendering part of invalid value test.

> > > Attachment 202967 [details] [details] [details] did not pass style-queue:
> > > 
> > > Source/WebCore/rendering/InlineTextBox.cpp:984:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
> > > Total errors found: 1 in 22 files
> > > 
> > > 
> > > If any of these errors are false positives, please file a bug against check-webkit-style.
> > 
> > The outer "if" dose not conclude with a return.
> > This may be bug of style checker.
> 
> What the error message want to say is that you should do:
> 
> if (...)
>     return <something>;
> if (...)
>     return <something>;
> 
> instead of
> 
> if (...)
>     return <something>;
> else if (...)
>     return <something>;
> 
> You just need to remove the "else"'s in your source code. They are not necessary if there is a return inside the if statement.

I understand it. Thank you.
Fixed.

(In reply to comment #12)
> View in context: https://bugs.webkit.org/attachment.cgi?id=202967&action=review
> 
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1389
> > +    return list;
> 
> You should add an assert here, list must never be empty.

I don't think so.
I think your draft patch needs assert because it is not clear whether list is empty or not.
However, my implementation is copied from renderTextDecorationFlagsToCSSValue().
Since "if" at 2nd line above the return line assures that list must not be empty, and there is no assert in renderTextDecorationFlagsToCSSValue().
I think the following assert is too duplicate.
> if (condition)
>     return foo;
> ASSERT(!condition);
> return bar;

> Besides, I do not see a need to replace CSSPrimitiveValue::CSSPrimitiveValue(TextUnderlinePosition e) with this function. This just makes your patch bigger. I see that you used one of my draft patch for text-underline-position to implement this function but since WebKit already uses CSSPrimitiveValue::CSSPrimitiveValue(TextUnderlinePosition e) you should implement your patch using it unless there is a really good reason for not to.

Hmm. How to use CSSPrimitiveValue::CSSPrimitiveValue(TextUnderlinePosition e)?

Should we renderTextUnderlinePositionFlagsToCSSValue() like the following?
> for (int flag = 1; flag <= TextUnderlinePositionRight; flag <<= 1) {
>     if (style->textUnderlinePosition() & flag)
>         list->append(cssValuePool().createValue(flag))
> }

or
At first, we change CSSPrimitiveValue(TextUnderlinePosition e) to read flag.
After that we change renderTextUnderlinePositionFlagsToCSSValue() like the following:
> TextUnderlinePosition position = style->textUnderlinePosition();
> while (position) {
>     RefPtr<CSSPrimitiveValue> newValue = cssValuePool().createValue(position);
>     TextUnderlinePosition usedPosition = newValue;
>     position &= ~usedPosition;
>     list->append(newValue.release());
> }

I think neither of them are clear code.
Reducing diff is good, but writing clear code is also good.

> > Source/WebCore/css/CSSPrimitiveValueMappings.h:2468
> >      // FIXME: Implement support for 'under left' and 'under right' values.
> 
> Remove obsolete comment.

Removed.

> > Source/WebCore/rendering/InlineTextBox.cpp:1251
> > +                FloatPoint start(localOrigin.x(), localOrigin.y() + underlineOffset + (isTopSide ? -doubleOffset : doubleOffset));
> 
> We should add tests similar to LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-style.html to check for this change. Maybe a text-decoration-style-underline-left.html and text-decoration-style-underline-right.html

text-underline-position-double.html will cover this change.
I changed this to reftest.

[Lamarque V. Souza]

(In reply to comment #14)
> (In reply to comment #12)
> > View in context: https://bugs.webkit.org/attachment.cgi?id=202967&action=review
> > 
> > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1389
> > > +    return list;
> > 
> > You should add an assert here, list must never be empty.
> 
> I don't think so.
> I think your draft patch needs assert because it is not clear whether list is empty or not.
> However, my implementation is copied from renderTextDecorationFlagsToCSSValue().
> Since "if" at 2nd line above the return line assures that list must not be empty, and there is no assert in renderTextDecorationFlagsToCSSValue().

"none" is a valid value for TextDecoration (http://dev.w3.org/csswg/css-text-decor-3/#text-decoration-property), that is why defaulting to CSSValueNone when the list is empty is Ok there and no assert is needed there. However, "none" is not a valid value for text-underline-position, so the list must not be empty, an assert is needed and we should not default to CSSValueNone.

> I think the following assert is too duplicate.
> > if (condition)
> >     return foo;
> > ASSERT(!condition);
> > return bar;

asserts are used to detect bugs not to make code more readable. Besides, they are not compiled if debugging is disabled, so they do not cause any runtime overhead in release mode. In our case the code above should be something like:

if (condition)
    list->append(...);
ASSERT(!list->isEmpty())
return list;

See? No return inside the if statement.
 
> > Besides, I do not see a need to replace CSSPrimitiveValue::CSSPrimitiveValue(TextUnderlinePosition e) with this function. This just makes your patch bigger. I see that you used one of my draft patch for text-underline-position to implement this function but since WebKit already uses CSSPrimitiveValue::CSSPrimitiveValue(TextUnderlinePosition e) you should implement your patch using it unless there is a really good reason for not to.
> 
> Hmm. How to use CSSPrimitiveValue::CSSPrimitiveValue(TextUnderlinePosition e)?

Ok, I tested it here and trying to use CSSPrimitiveValue::CSSPrimitiveValue() for a list is a big pain. I am ok with using renderTextUnderlinePositionFlagsToCSSValue() instead of CSSPrimitiveValue::CSSPrimitiveValue().
 
> > > Source/WebCore/rendering/InlineTextBox.cpp:1251
> > > +                FloatPoint start(localOrigin.x(), localOrigin.y() + underlineOffset + (isTopSide ? -doubleOffset : doubleOffset));
> > 
> > We should add tests similar to LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-style.html to check for this change. Maybe a text-decoration-style-underline-left.html and text-decoration-style-underline-right.html
> 
> text-underline-position-double.html will cover this change.
> I changed this to reftest.

Ok, that's better than using pixel test :-)

[Yuki Sekiguchi]

Created attachment 205669 [details]
Patch

[Yuki Sekiguchi]

(In reply to comment #15)
> > I don't think so.
> > I think your draft patch needs assert because it is not clear whether list is empty or not.
> > However, my implementation is copied from renderTextDecorationFlagsToCSSValue().
> > Since "if" at 2nd line above the return line assures that list must not be empty, and there is no assert in renderTextDecorationFlagsToCSSValue().
> 
> "none" is a valid value for TextDecoration (http://dev.w3.org/csswg/css-text-decor-3/#text-decoration-property), that is why defaulting to CSSValueNone when the list is empty is Ok there and no assert is needed there. However, "none" is not a valid value for text-underline-position, so the list must not be empty, an assert is needed and we should not default to CSSValueNone.

Changed to use "auto", and changed definition of TextUnderlinePosition constants.
It make underline-position's code more like text-decoration's.

[yisibl]

Blink support.

[Brady Eidson]

Comment on attachment 205669 [details]
Patch

Assuming that patches for review since 2013 are stale, r-

[Tim Nguyen (:ntim)]

*** This bug has been marked as a duplicate of bug 245559 ***