Bug 102491 - [css3-text] Add partial parsing support for text-underline-position property from CSS3 Text
Summary: [css3-text] Add partial parsing support for text-underline-position property ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Lamarque V. Souza
URL:
Keywords:
Depends on:
Blocks: 101931 102795
  Show dependency treegraph
 
Reported: 2012-11-16 05:58 PST by Lamarque V. Souza
Modified: 2013-03-12 10:28 PDT (History)
22 users (show)

See Also:


Attachments
Patch (32.76 KB, patch)
2012-11-16 06:23 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
Patch (32.86 KB, patch)
2012-12-18 04:44 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
Parse text-underline-position (33.06 KB, patch)
2013-01-16 18:56 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
Parse text-underline-position (EWS only version) (44.65 KB, patch)
2013-01-16 20:12 PST, Lamarque V. Souza
buildbot: commit-queue-
Details | Formatted Diff | Diff
Parse text-underline-position (EWS only version) (45.78 KB, patch)
2013-01-17 05:40 PST, Lamarque V. Souza
buildbot: commit-queue-
Details | Formatted Diff | Diff
Parse text-underline-position (EWS only version) (46.43 KB, patch)
2013-01-17 11:10 PST, Lamarque V. Souza
buildbot: commit-queue-
Details | Formatted Diff | Diff
Parse text-underline-position (EWS only version) (46.68 KB, patch)
2013-01-17 16:26 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
Parse text-underline-position (33.02 KB, patch)
2013-02-22 11:24 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
Parse text-underline-position (EWS only version) (47.04 KB, patch)
2013-02-22 11:28 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
Parse text-underline-position (36.10 KB, patch)
2013-03-05 15:19 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
Parse text-underline-position (EWS only version) (50.00 KB, patch)
2013-03-05 15:22 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
Parse text-underline-position (29.53 KB, patch)
2013-03-06 09:47 PST, Lamarque V. Souza
jchaffraix: review+
jchaffraix: commit-queue-
Details | Formatted Diff | Diff
Parse text-underline-position (EWS only version) (43.42 KB, patch)
2013-03-06 09:54 PST, Lamarque V. Souza
buildbot: commit-queue-
Details | Formatted Diff | Diff
Parse text-underline-position (29.38 KB, patch)
2013-03-11 13:29 PDT, Lamarque V. Souza
no flags 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 2012-11-16 05:58:27 PST
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.
Comment 1 Lamarque V. Souza 2012-11-16 06:23:26 PST
Created attachment 174668 [details]
Patch

Proposed patch
Comment 2 Lamarque V. Souza 2012-12-18 04:44:57 PST
Created attachment 179922 [details]
Patch

Updated to current git revision
Comment 3 Build Bot 2012-12-18 09:18:20 PST
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
Comment 4 Lamarque V. Souza 2013-01-16 18:56:16 PST
Created attachment 183092 [details]
Parse text-underline-position

Fix getComputedStyle-text-underline-position-expected.txt
Comment 5 Lamarque V. Souza 2013-01-16 20:12:01 PST
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.
Comment 6 Build Bot 2013-01-16 20:54:35 PST
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
Comment 7 Build Bot 2013-01-16 22:24:37 PST
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
Comment 8 WebKit Review Bot 2013-01-16 22:31:45 PST
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
Comment 9 WebKit Review Bot 2013-01-17 00:02:10 PST
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
Comment 10 Build Bot 2013-01-17 05:01:44 PST
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
Comment 11 Lamarque V. Souza 2013-01-17 05:40:34 PST
Created attachment 183171 [details]
Parse text-underline-position (EWS only version)

Enable css3-conditional-rules in some missing points.
Comment 12 Build Bot 2013-01-17 06:28:33 PST
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
Comment 13 Build Bot 2013-01-17 06:32:28 PST
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
Comment 14 Build Bot 2013-01-17 09:33:06 PST
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
Comment 15 Lamarque V. Souza 2013-01-17 11:10:50 PST
Created attachment 183222 [details]
Parse text-underline-position (EWS only version)

Attempt to fix compilation problem in some bots
Comment 16 Build Bot 2013-01-17 12:21:21 PST
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
Comment 17 WebKit Review Bot 2013-01-17 13:01:07 PST
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
Comment 18 Lamarque V. Souza 2013-01-17 16:26:39 PST
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.
Comment 19 Build Bot 2013-01-17 18:52:22 PST
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
Comment 20 Lamarque V. Souza 2013-02-22 11:24:25 PST
Created attachment 189797 [details]
Parse text-underline-position

Update to current git revision.
Comment 21 Lamarque V. Souza 2013-02-22 11:28:00 PST
Created attachment 189798 [details]
Parse text-underline-position (EWS only version)

Same patch with css3-text enabled.
Comment 22 Build Bot 2013-02-24 22:46:56 PST
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
Comment 23 Julien Chaffraix 2013-03-04 16:58:20 PST
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.
Comment 24 Lamarque V. Souza 2013-03-05 15:19:54 PST
Created attachment 191578 [details]
Parse text-underline-position

Fix issues reported.
Comment 25 Lamarque V. Souza 2013-03-05 15:22:29 PST
Created attachment 191580 [details]
Parse text-underline-position (EWS only version)

Same patch with css3-text enabled.
Comment 26 Lamarque V. Souza 2013-03-05 15:33:39 PST
(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.
Comment 27 Build Bot 2013-03-06 02:46:18 PST
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
Comment 28 Lamarque V. Souza 2013-03-06 09:47:45 PST
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.
Comment 29 Lamarque V. Souza 2013-03-06 09:54:08 PST
Created attachment 191778 [details]
Parse text-underline-position (EWS only version)

Same patch with css3-text enabled.
Comment 30 Build Bot 2013-03-06 14:25:18 PST
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
Comment 31 Julien Chaffraix 2013-03-11 11:42:05 PDT
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.
Comment 32 Lamarque V. Souza 2013-03-11 13:29:56 PDT
Created attachment 192542 [details]
Parse text-underline-position

Patch for landing.
Comment 33 WebKit Review Bot 2013-03-11 18:12:47 PDT
Comment on attachment 192542 [details]
Parse text-underline-position

Clearing flags on attachment: 192542

Committed r145450: <http://trac.webkit.org/changeset/145450>
Comment 34 Bruno Abinader (history only) 2013-03-12 10:28:02 PDT
All reviewed patches have been landed. Closing bug.