Bug 102491 – [css3-text] Add partial parsing support for text-underline-position property from CSS3 Text

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Lamarque V. Souza]

This bug intends to add parsing support for "text-underline-position" CSS3 property. Rendering support is going to be implemented on another bug. This is a sub-task for bug 101931.

[Lamarque V. Souza]

Created attachment 174668 [details]
Patch

Proposed patch

[Lamarque V. Souza]

Created attachment 179922 [details]
Patch

Updated to current git revision

[Build Bot]

Comment on attachment 179922 [details]
Patch

Attachment 179922 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15400222

New failing tests:
fast/frames/sandboxed-iframe-attribute-parsing.html

[Lamarque V. Souza]

Created attachment 183092 [details]
Parse text-underline-position

Fix getComputedStyle-text-underline-position-expected.txt

[Lamarque V. Souza]

Created attachment 183104 [details]
Parse text-underline-position (EWS only version)

Same patch with css3-text enabled by default so that bots can really test it.

[Build Bot]

Comment on attachment 183104 [details]
Parse text-underline-position (EWS only version)

Attachment 183104 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15909703

[Build Bot]

Comment on attachment 183104 [details]
Parse text-underline-position (EWS only version)

Attachment 183104 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15914671

[WebKit Review Bot]

Comment on attachment 183104 [details]
Parse text-underline-position (EWS only version)

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

New failing tests:
fast/css3-text/css3-text-decoration/text-decoration-style.html
fast/css3-text/css3-text-decoration/repaint/repaint-text-decoration-style.html

[WebKit Review Bot]

Comment on attachment 183104 [details]
Parse text-underline-position (EWS only version)

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

New failing tests:
fast/css3-text/css3-text-decoration/text-decoration-style.html
fast/css3-text/css3-text-decoration/repaint/repaint-text-decoration-style.html

[Build Bot]

Comment on attachment 183104 [details]
Parse text-underline-position (EWS only version)

Attachment 183104 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/15908986

[Lamarque V. Souza]

Created attachment 183171 [details]
Parse text-underline-position (EWS only version)

Enable css3-conditional-rules in some missing points.

[Build Bot]

Comment on attachment 183171 [details]
Parse text-underline-position (EWS only version)

Attachment 183171 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/15906898

[Build Bot]

Comment on attachment 183171 [details]
Parse text-underline-position (EWS only version)

Attachment 183171 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15913760

[Build Bot]

Comment on attachment 183171 [details]
Parse text-underline-position (EWS only version)

Attachment 183171 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15926694

[Lamarque V. Souza]

Created attachment 183222 [details]
Parse text-underline-position (EWS only version)

Attempt to fix compilation problem in some bots

[Build Bot]

Comment on attachment 183222 [details]
Parse text-underline-position (EWS only version)

Attachment 183222 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15944080

[WebKit Review Bot]

Comment on attachment 183222 [details]
Parse text-underline-position (EWS only version)

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

New failing tests:
fast/css3-text/css3-text-decoration/text-decoration-style.html
fast/css3-text/css3-text-decoration/repaint/repaint-text-decoration-style.html

[Lamarque V. Souza]

Created attachment 183309 [details]
Parse text-underline-position (EWS only version)

Disable css3-text decoration unit tests since they lack -expected.png files. See https://bugs.webkit.org/show_bug.cgi?id=100546.

[Build Bot]

Comment on attachment 183309 [details]
Parse text-underline-position (EWS only version)

Attachment 183309 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15947195

[Lamarque V. Souza]

Created attachment 189797 [details]
Parse text-underline-position

Update to current git revision.

[Lamarque V. Souza]

Created attachment 189798 [details]
Parse text-underline-position (EWS only version)

Same patch with css3-text enabled.

[Build Bot]

Comment on attachment 189798 [details]
Parse text-underline-position (EWS only version)

Attachment 189798 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16728997

[Julien Chaffraix]

Comment on attachment 189797 [details]
Parse text-underline-position

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

You are on the right tracks. This will need another round as you allow cases that are not compliant (and not tested).

> Source/WebCore/ChangeLog:24
> +        (WebCore):

These entries are not useful and should be removed.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1361
> +    if (list->length())
> +        return list;
> +
> +    ASSERT_NOT_REACHED();
> +    return cssValuePool().createExplicitInitialValue();

We should probably just have:

ASSERT(list->length());
return list;

> Source/WebCore/css/CSSParser.cpp:9100
> +        m_valueList->next();

Do you need this?

> Source/WebCore/css/CSSParser.cpp:9104
> +    if (value->id == CSSValueAlphabetic) {

This could be added to the 'if' above.

> Source/WebCore/css/CSSParser.cpp:9106
> +        m_valueList->next();

Ditto.

> Source/WebCore/css/CSSParser.cpp:9111
> +    bool isValid = true;

I don't think we need this |isValid| variable: if you are not valid, I would expect you to bail out from the function as there is no point in continuing.

> Source/WebCore/css/CSSParser.cpp:9113
> +    while (isValid && value) {

We could probably get away without the loop here as we really expect to parse only 2 values but in any order.

> Source/WebCore/css/CSSParser.cpp:9132
> +

You would wrongly accept:
* -webkit-text-underline-position: under under;
* -webkit-text-underline-position: under under under;
* -webkit-text-underline-position: left under under;

These cases should be added to your test.

> Source/WebCore/css/CSSValueKeywords.in:949
> +#if !(defined(ENABLE_SVG) && ENABLE_SVG) && defined(ENABLE_CSS3_TEXT) && ENABLE_CSS3_TEXT

It's probably a bad idea to have the same property defined in 2 places. I would rather see it defined here only.

> Source/WebCore/css/CSSValueKeywords.in:952
> +#endif // CSS3_TEXT

Nit: No need for the "// CSS3_TEXT", especially since it's wrong.

> Source/WebCore/css/StyleBuilder.cpp:1960
> +    setPropertyHandler(CSSPropertyWebkitTextUnderlinePosition, ApplyPropertyTextUnderlinePosition::createHandler());

If you handle the value here (which is fine), there is a giant switch to update in StyleResolver.cpp to say that you do so. Unfortunately, it doesn't break the build because of SVG defining the 'default' case.

> Source/WebCore/rendering/style/RenderStyleConstants.h:368
> +inline TextUnderlinePosition operator| (TextUnderlinePosition a, TextUnderlinePosition b) { return TextUnderlinePosition(int(a) | int(b)); }
> +inline TextUnderlinePosition& operator|=(TextUnderlinePosition& a, TextUnderlinePosition b) { return a = a | b; }

Not a huge fan of these operators for 2 reasons:
* What you get is *not* a TextUnderlinePosition as it isn't a defined value.
* This doesn't match our current style, which is to use flags.

It's a smart way of handling this situation but I think a simple

typedef unsigned TextUnderlinePositionFlags;
(or a plain unsigned)

would do a better job.

> LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-underline-position.js:99
> +

Missing the bad cases: left left, left right, right right.

[Lamarque V. Souza]

Created attachment 191578 [details]
Parse text-underline-position

Fix issues reported.

[Lamarque V. Souza]

Created attachment 191580 [details]
Parse text-underline-position (EWS only version)

Same patch with css3-text enabled.

[Lamarque V. Souza]

(In reply to comment #23)
> These entries are not useful and should be removed.
> > Source/WebCore/css/CSSParser.cpp:9100
> > +        m_valueList->next();
> 
> Do you need this?

No, but I have changed the parser in the latest patch, so this is not a issue anymore.

> > Source/WebCore/css/CSSValueKeywords.in:949
> > +#if !(defined(ENABLE_SVG) && ENABLE_SVG) && defined(ENABLE_CSS3_TEXT) && ENABLE_CSS3_TEXT
> 
> It's probably a bad idea to have the same property defined in 2 places. I would rather see it defined here only.

Can we define a value and not use it? There are the cases where both ENABLE_SVG and ENABLE_CSS3_TEXT are enabled, when both are disabled and when only one of them is enabled. I added the extra ENABLE_SVG to prevent alphabetic being defined twice when both ENABLE_SVG and ENABLE_CSS3_TEXT are enabled. I do not see how I can define it here without causing a conflict in the svg file that also defines the 'alphabetic' value without such a check, unless we define it regardless of the svg and css3-text feature flags values.

> > Source/WebCore/css/CSSValueKeywords.in:952
> > +#endif // CSS3_TEXT
> 
> Nit: No need for the "// CSS3_TEXT", especially since it's wrong.

Ok, the feature flag was renamed some months ago and I forgot to change the text above.

> > Source/WebCore/css/StyleBuilder.cpp:1960
> > +    setPropertyHandler(CSSPropertyWebkitTextUnderlinePosition, ApplyPropertyTextUnderlinePosition::createHandler());
> 
> If you handle the value here (which is fine), there is a giant switch to update in StyleResolver.cpp to say that you do so. Unfortunately, it doesn't break the build because of SVG defining the 'default' case.

Which of the switches in StyleResolver.cpp are you talking about?

> > Source/WebCore/rendering/style/RenderStyleConstants.h:368
> > +inline TextUnderlinePosition operator| (TextUnderlinePosition a, TextUnderlinePosition b) { return TextUnderlinePosition(int(a) | int(b)); }
> > +inline TextUnderlinePosition& operator|=(TextUnderlinePosition& a, TextUnderlinePosition b) { return a = a | b; }
> 
> Not a huge fan of these operators for 2 reasons:
> * What you get is *not* a TextUnderlinePosition as it isn't a defined value.
> * This doesn't match our current style, which is to use flags.
> 
> It's a smart way of handling this situation but I think a simple
> 
> typedef unsigned TextUnderlinePositionFlags;
> (or a plain unsigned)
> 
> would do a better job.

That does not do the trick but I managed remove those operators and refactor the only place that use them.

The other issues reported have been fixed in the latest patch.

[Build Bot]

Comment on attachment 191580 [details]
Parse text-underline-position (EWS only version)

Attachment 191580 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/16999094

[Lamarque V. Souza]

Created attachment 191775 [details]
Parse text-underline-position

According to http://www.w3.org/TR/CSS/#partial we should not accept values that do not have rendering support. 'under left' and 'under right' are not implement in https://bugs.webkit.org/show_bug.cgi?id=102795 so remove the support for them in this patch.

[Lamarque V. Souza]

Created attachment 191778 [details]
Parse text-underline-position (EWS only version)

Same patch with css3-text enabled.

[Build Bot]

Comment on attachment 191778 [details]
Parse text-underline-position (EWS only version)

Attachment 191778 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/16987183

[Julien Chaffraix]

Comment on attachment 191775 [details]
Parse text-underline-position

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

> Source/WebCore/ChangeLog:3
> +        [css3-text] Add parsing support text-underline-position property from CSS3 Text

This should be renamed as you don't totally match the specification in this change.

> Source/WebCore/ChangeLog:12
> +        OBS: values 'under left' and 'under right' are not implemented in this

OBS? If you meant observation, either state it or use 'note'.

Also, you should just mention what you did here instead of just observing it:

This patch extends the existing parsing to support 'alphabetic' and 'under'. We don't fully match the specification as we don't support [ left | right ] and this is left for another implementation as the rendering will need to be added.

> Source/WebCore/ChangeLog:42
> +        * rendering/style/RenderStyleConstants.h:
> +        (WebCore::operator| ):
> +        (WebCore::operator|=):
> +        Added bitwise TextUnderlinePosition enum with operator functions | and |= operator (used in StyleBuilder).

The ChangeLog is stale. It should be updated before landing.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1362
> +static PassRefPtr<CSSValue> renderTextUnderlinePositionFlagsToCSSValue(TextUnderlinePosition textUnderlinePosition)
> +{
> +    if (textUnderlinePosition == TextUnderlinePositionAuto)
> +        return cssValuePool().createIdentifierValue(CSSValueAuto);
> +
> +    if (textUnderlinePosition == TextUnderlinePositionAlphabetic)
> +        return cssValuePool().createIdentifierValue(CSSValueAlphabetic);
> +
> +    if (textUnderlinePosition == TextUnderlinePositionUnder)
> +        return cssValuePool().createIdentifierValue(CSSValueUnder);
> +
> +    // TODO: Implement support for 'under left' and 'under right' values.
> +
> +    ASSERT_NOT_REACHED();
> +    return cssValuePool().createExplicitInitialValue();
> +}

Nit: This could be replaced with defining CSSPrimitiveValue::CSSPrimitiveValue(TextUnderlinePosition) in CSSPrimitiveValueMapping.h AFAICT.

> Source/WebCore/css/CSSParser.cpp:9130
> +        break;

No need for the break.

> Source/WebCore/css/CSSPrimitiveValueMappings.h:2372
> +    // TODO: Implement support for 'under left' and 'under right' values.

We don't have TODO's in WebKit, only FIXME's.

> Source/WebCore/css/CSSValueKeywords.in:949
> +#if !(defined(ENABLE_SVG) && ENABLE_SVG) && defined(ENABLE_CSS3_TEXT) && ENABLE_CSS3_TEXT

This will basically work but you still have 2 definitions of the same property which means that they can get out of sync.

I would rather go with a single definition than 2: either by defining the property here only using (my preference)

#if (defined(ENABLE_SVG) && ENABLE_SVG) || (defined(ENABLE_CSS3_TEXT) && ENABLE_CSS3_TEXT)

and removing the SVG bit or just enabling it always (not great but better than having to maintain 2 definitions).

> Source/WebCore/rendering/style/RenderStyleConstants.h:363
> +    // TODO: Implement support for 'under left' and 'under right' values.

s/TODO/FIXME.

[Lamarque V. Souza]

Created attachment 192542 [details]
Parse text-underline-position

Patch for landing.

[WebKit Review Bot]

Comment on attachment 192542 [details]
Parse text-underline-position

Clearing flags on attachment: 192542

Committed r145450: <http://trac.webkit.org/changeset/145450>

[Bruno Abinader (history only)]

All reviewed patches have been landed. Closing bug.