Bug 112615 - [css3-text] Implement support for vertical writing mode in -webkit-text-underline-position
Summary: [css3-text] Implement support for vertical writing mode in -webkit-text-under...
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Nobody
URL: http://dev.w3.org/csswg/css-text-deco...
Keywords:
Depends on:
Blocks: 101931
  Show dependency treegraph
 
Reported: 2013-03-18 13:36 PDT by Lamarque V. Souza
Modified: 2016-05-24 22:01 PDT (History)
10 users (show)

See Also:


Attachments
Patch (139.16 KB, patch)
2013-04-15 01:38 PDT, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Patch (140.21 KB, patch)
2013-05-24 02:09 PDT, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Patch (140.28 KB, patch)
2013-05-24 03:15 PDT, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Patch (160.49 KB, patch)
2013-05-27 04:16 PDT, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Patch (145.43 KB, patch)
2013-05-28 01:09 PDT, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Patch (143.94 KB, patch)
2013-06-27 23:23 PDT, Yuki Sekiguchi
beidson: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lamarque V. Souza 2013-03-18 13:36:03 PDT
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
Comment 1 Yuki Sekiguchi 2013-04-15 01:38:49 PDT
Created attachment 198033 [details]
Patch
Comment 2 Lamarque V. Souza 2013-04-15 07:43:30 PDT
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.
Comment 3 Yuki Sekiguchi 2013-05-24 02:09:47 PDT
Created attachment 202789 [details]
Patch
Comment 4 Yuki Sekiguchi 2013-05-24 03:15:42 PDT
Created attachment 202797 [details]
Patch
Comment 5 Lamarque V. Souza 2013-05-24 08:12:13 PDT
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.
Comment 6 Yuki Sekiguchi 2013-05-27 04:16:12 PDT
Created attachment 202967 [details]
Patch
Comment 7 WebKit Commit Bot 2013-05-27 04:18:32 PDT
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.
Comment 8 Yuki Sekiguchi 2013-05-27 04:20:46 PDT
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.
Comment 9 Yuki Sekiguchi 2013-05-27 04:27:04 PDT
(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
Comment 10 Yuki Sekiguchi 2013-05-27 04:29:09 PDT
(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.
Comment 11 Lamarque V. Souza 2013-05-27 06:14:01 PDT
(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.
Comment 12 Lamarque V. Souza 2013-05-27 08:17:59 PDT
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
Comment 13 Yuki Sekiguchi 2013-05-28 01:09:08 PDT
Created attachment 203027 [details]
Patch
Comment 14 Yuki Sekiguchi 2013-05-28 01:40:31 PDT
(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.
Comment 15 Lamarque V. Souza 2013-05-28 18:05:09 PDT
(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 :-)
Comment 16 Yuki Sekiguchi 2013-06-27 23:23:35 PDT
Created attachment 205669 [details]
Patch
Comment 17 Yuki Sekiguchi 2013-06-27 23:30:25 PDT
(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.
Comment 18 yisibl 2015-12-14 23:28:56 PST
Blink support.
Comment 19 Brady Eidson 2016-05-24 22:01:33 PDT
Comment on attachment 205669 [details]
Patch

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