Bug 174737 – CSS properties stroke-width/stroke-color and -webkit-text-stroke-width/-webkit-text-stroke-color should not be mixed

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

[Per Arne Vollan]

It is currently possible to apply a mix of the css properties stroke-width/stroke-color and -webkit-text-stroke-width/-webkit-text-stroke-color. This is unfortunate, we should either use the combination stroke-width/stroke-color or the combination -webkit-text-stroke-width/-webkit-text-stroke-color.

Example:

<div style="stroke-width: 1px">Text</div>

Since stroke-color is not specified here, WebKit will currently fall back to the -webkit-text-stroke-color value, and not use the default transparent value of the stroke-color property. The above text is currently rendered with stroke, this is incorrect.

[Per Arne Vollan]

<rdar://problem/33466294>

[Per Arne Vollan]

Created attachment 316318 [details]
Patch

[Brent Fulgham]

Comment on attachment 316318 [details]
Patch

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

> LayoutTests/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=174737

<rdar://problem/33466294>

[Brent Fulgham]

I think Dean or Simon should review this. It seems like a reasonable change, and it's a shame it hasn't been landed.

[Simon Fraser (smfr)]

Comment on attachment 316318 [details]
Patch

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

Please consult Antti and Hyatt.

> Source/WebCore/ChangeLog:3
> +        The css properties stroke-width/stroke-color and -webkit-text-stroke-width/-webkit-text-stroke-color should not be mixed.

CSS (uppercase)

> Source/WebCore/ChangeLog:12
> +        Previously, the stroke width and stroke color would independently fall back to the -webkit-text-stroke-width
> +        and -webkit-text-stroke-color values, if stroke-width and/or stroke-color were not explicitly specified. The
> +        new strategy is to fall back to the -webkit-text-stroke-width and -webkit-text-stroke-color value combination,
> +        if the stroke-width and stroke-color combination does not result in a visible stroke.

The fallback is problematic because of the differing initial values for stroke-color and -webkit-stroke-color, right (transparent vs. currentColor?). I wonder if it would be simpler to just alias these properties, and change the default -webkit-text-stroke-color. We'd have to see if that breaks content that we care about.

[Per Arne Vollan]

(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 316318 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316318&action=review
> 
> Please consult Antti and Hyatt.
> 
> > Source/WebCore/ChangeLog:3
> > +        The css properties stroke-width/stroke-color and -webkit-text-stroke-width/-webkit-text-stroke-color should not be mixed.
> 
> CSS (uppercase)
> 
> > Source/WebCore/ChangeLog:12
> > +        Previously, the stroke width and stroke color would independently fall back to the -webkit-text-stroke-width
> > +        and -webkit-text-stroke-color values, if stroke-width and/or stroke-color were not explicitly specified. The
> > +        new strategy is to fall back to the -webkit-text-stroke-width and -webkit-text-stroke-color value combination,
> > +        if the stroke-width and stroke-color combination does not result in a visible stroke.
> 
> The fallback is problematic because of the differing initial values for
> stroke-color and -webkit-stroke-color, right (transparent vs.
> currentColor?). I wonder if it would be simpler to just alias these
> properties, and change the default -webkit-text-stroke-color. We'd have to
> see if that breaks content that we care about.

That's a good point, I will look into it :)

Thanks for reviewing, all!

[Per Arne Vollan]

Created attachment 321900 [details]
Patch

[Build Bot]

Comment on attachment 321900 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 321912 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 321900 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 321914 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 321900 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 321917 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 321900 [details]
Patch

Attachment 321900 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4671205

New failing tests:
tables/mozilla/bugs/bug46623-1.html
fast/css/remove-shorthand.html
fast/selectors/017.html
fast/css/stroke-miterlimit-large.html
fast/css/getPropertyValue-webkit-text-stroke.html
fast/css/child-style-can-override-visited-style.html
css2.1/t051103-c21-hover-ln-00-e-i.html
css2.1/t060403-c21-pseu-cls-00-e-i.html
css3/selectors3/xhtml/css3-modsel-17.xml
fast/css/style-tag-display-none.html
svg/css/text-shadow-multiple.xhtml
css2.1/t0511-c21-pseud-link-03-e.html
css2.1/t051103-c21-activ-ln-00-e-i.html
css2.1/t051103-c21-focus-ln-00-e-i.html
css3/selectors3/xml/css3-modsel-17.xml
css2.1/t060403-c21-pseu-id-00-e-i.html
fast/css/shadow-multiple.html
fast/selectors/visited-descendant.html
tables/mozilla/bugs/bug8950.html
css3/selectors3/html/css3-modsel-17.html
fast/dom/34176.html
fast/selectors/061.html
media/track/track-css-stroke-cues.html
fast/css/stroke-miterlimit-zero.html
tables/mozilla/bugs/bug7342.html
tables/mozilla/bugs/bug51140.html
css2.1/t0511-c21-pseud-link-02-e.html

[Build Bot]

Created attachment 321923 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Per Arne Vollan]

(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 316318 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316318&action=review
> 
> Please consult Antti and Hyatt.
> 
> > Source/WebCore/ChangeLog:3
> > +        The css properties stroke-width/stroke-color and -webkit-text-stroke-width/-webkit-text-stroke-color should not be mixed.
> 
> CSS (uppercase)
> 
> > Source/WebCore/ChangeLog:12
> > +        Previously, the stroke width and stroke color would independently fall back to the -webkit-text-stroke-width
> > +        and -webkit-text-stroke-color values, if stroke-width and/or stroke-color were not explicitly specified. The
> > +        new strategy is to fall back to the -webkit-text-stroke-width and -webkit-text-stroke-color value combination,
> > +        if the stroke-width and stroke-color combination does not result in a visible stroke.
> 
> The fallback is problematic because of the differing initial values for
> stroke-color and -webkit-stroke-color, right (transparent vs.
> currentColor?). I wonder if it would be simpler to just alias these
> properties, and change the default -webkit-text-stroke-color. We'd have to
> see if that breaks content that we care about.

After having looked closer into this, it seems aliasing will break setting stroke properties like '-webkit-text-stroke-width: thin;', and '-webkit-text-stroke: thin orange;'.

[Per Arne Vollan]

Created attachment 323200 [details]
Patch

[Build Bot]

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


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 14 files


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

[Per Arne Vollan]

Created attachment 323246 [details]
Patch

[Per Arne Vollan]

(In reply to Per Arne Vollan from comment #19)
> Created attachment 323246 [details]
> Patch

Since there is a minor risk of breaking existing content with the aliasing approach, I have suggested a different fix to the bug in the last patch.

[Darin Adler]

Simon, Antti, Hyatt?

[Antti Koivisto]

Comment on attachment 323246 [details]
Patch

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

Seems reasonable, r=me

> Source/WebCore/rendering/style/RenderStyle.cpp:2358
> -    if (!hasExplicitlySetStrokeWidth())
> +    if (!hasExplicitlySetStrokeColor())
>          return textStrokeWidth();

This needs a comment. It really reads like a bug.

[Per Arne Vollan]

(In reply to Antti Koivisto from comment #22)
> Comment on attachment 323246 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323246&action=review
> 
> Seems reasonable, r=me
> 
> > Source/WebCore/rendering/style/RenderStyle.cpp:2358
> > -    if (!hasExplicitlySetStrokeWidth())
> > +    if (!hasExplicitlySetStrokeColor())
> >          return textStrokeWidth();
> 
> This needs a comment. It really reads like a bug.

Thanks for reviewing! I will add a comment before landing.

[Per Arne Vollan]

Committed r224780: <https://trac.webkit.org/changeset/224780/webkit>.