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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=112615


Yuki Sekiguchi <yuki.sekiguchi at access-company.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |yuki.sekiguchi at access-compa
                   |                            |ny.com




--- Comment #8 from Yuki Sekiguchi <yuki.sekiguchi at access-company.com>  2013-05-27 04:20:46 PST ---
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.