Bug 174701 – Round-tripping stroke-width styles through getComputedStyle cause the text to gain a stroke.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Per Arne Vollan]

WebKit will currently render the text stroked in the following test case:

<div id=t>This text should not be stroked.</div>
<script>t.style=getComputedStyle(t).cssText</script>

[Per Arne Vollan]

<rdar://problem/32903679>

[Per Arne Vollan]

Created attachment 316047 [details]
Patch

[Build Bot]

Comment on attachment 316047 [details]
Patch

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

New failing tests:
fast/css/getComputedStyle/computed-style-without-renderer.html
fast/css/parsing-stroke-width.html
fast/css/getComputedStyle/computed-style.html

[Build Bot]

Created attachment 316052 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

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

[Darin Adler]

Comment on attachment 316047 [details]
Patch

The test looks good, but I am not sure the fix is correct. I’d like to see another test explicitly checking the value of computed style when stroke width was not set, and I’d like to compare with the CSS specification asks for and possibly also what other browsers do.

[Build Bot]

Comment on attachment 316047 [details]
Patch

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

New failing tests:
fast/css/getComputedStyle/computed-style-without-renderer.html
fast/css/parsing-stroke-width.html
fast/css/getComputedStyle/computed-style.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 316047 [details]
Patch

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

New failing tests:
fast/css/getComputedStyle/computed-style-without-renderer.html
fast/css/parsing-stroke-width.html
fast/css/getComputedStyle/computed-style.html

[Build Bot]

Created attachment 316055 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

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

[Simon Fraser (smfr)]

Today I searched for initialStrokeWidth() and was unable to find it. Why does StyleRareInheritedData use:

    , strokeWidth(RenderStyle::initialOneLength())

which I can't easily search for?

Question about the patch: why does the text get a black stroke, when the default stroke color is transparent?

[Build Bot]

Comment on attachment 316047 [details]
Patch

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

New failing tests:
fast/css/parsing-stroke-width.html

[Build Bot]

Created attachment 316061 [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.5

[Per Arne Vollan]

(In reply to Simon Fraser (smfr) from comment #10)
> Today I searched for initialStrokeWidth() and was unable to find it. Why
> does StyleRareInheritedData use:
> 
>     , strokeWidth(RenderStyle::initialOneLength())
> 
> which I can't easily search for?
> 
> Question about the patch: why does the text get a black stroke, when the
> default stroke color is transparent?

These are very good points. From what I can see, the default color does not seem to be transparent, but black (opaque), which is not correct. I will update the patch shortly.

Thanks for reviewing, all!

[Per Arne Vollan]

Created attachment 316090 [details]
Patch

[Per Arne Vollan]

(In reply to Darin Adler from comment #5)
> Comment on attachment 316047 [details]
> Patch
> 
> The test looks good, but I am not sure the fix is correct. I’d like to see
> another test explicitly checking the value of computed style when stroke
> width was not set, and I’d like to compare with the CSS specification asks
> for and possibly also what other browsers do.

Yes, I agree the fix is not correct. After the suggestion from Simon, I have changed the patch to instead set the correct initial value of the stroke-color to transparent, see https://www.w3.org/TR/fill-stroke-3/#stroke-color.

Thanks for reviewing!

Comment on attachment 316090 [details]
Patch

This patch also changes behavior outside of getting computed style, right? Currently <div style="stroke-width: 1px">Text</div> renders with a stroke, but after the patch it won’t, unless a non-transparent stroke-color is also specified. Seems like this should be covered in a test as well.

[Per Arne Vollan]

(In reply to mitz from comment #16)
> Comment on attachment 316090 [details]
> Patch
> 
> This patch also changes behavior outside of getting computed style, right?
> Currently <div style="stroke-width: 1px">Text</div> renders with a stroke,
> but after the patch it won’t, unless a non-transparent stroke-color is also
> specified. Seems like this should be covered in a test as well.

Yes, this also changes the behavior outside of getting the computed style. I will add a new test.

Thanks for reviewing!

[Per Arne Vollan]

(In reply to mitz from comment #16)
> Comment on attachment 316090 [details]
> Patch
> 
> This patch also changes behavior outside of getting computed style, right?
> Currently <div style="stroke-width: 1px">Text</div> renders with a stroke,
> but after the patch it won’t, unless a non-transparent stroke-color is also
> specified. Seems like this should be covered in a test as well.

Actually, no, it doesn't. When stroke-color is not explicitly specified, we fall back to the value of -webkit-text-stroke-color, which means the text in your example will be stroked.

[Per Arne Vollan]

Created attachment 316109 [details]
Patch

[Darin Adler]

Comment on attachment 316109 [details]
Patch

Might be OK. Test seems just right. I am not as sure that the behavior change is OK. I don’t see the additional testing that Mitz asked for, covering various behavior changes outside editing.

[Simon Fraser (smfr)]

(In reply to Per Arne Vollan from comment #18)
> (In reply to mitz from comment #16)
> > Comment on attachment 316090 [details]
> > Patch
> > 
> > This patch also changes behavior outside of getting computed style, right?
> > Currently <div style="stroke-width: 1px">Text</div> renders with a stroke,
> > but after the patch it won’t, unless a non-transparent stroke-color is also
> > specified. Seems like this should be covered in a test as well.
> 
> Actually, no, it doesn't. When stroke-color is not explicitly specified, we
> fall back to the value of -webkit-text-stroke-color, which means the text in
> your example will be stroked.

If <div style="stroke-width: 1px">Text</div> now renders with a stroke, then we have problems.

initialtextstrokecolor doesn't exist so I can't easily tell what the default value is.

[Per Arne Vollan]

(In reply to Simon Fraser (smfr) from comment #21)
> (In reply to Per Arne Vollan from comment #18)
> > (In reply to mitz from comment #16)
> > > Comment on attachment 316090 [details]
> > > Patch
> > > 
> > > This patch also changes behavior outside of getting computed style, right?
> > > Currently <div style="stroke-width: 1px">Text</div> renders with a stroke,
> > > but after the patch it won’t, unless a non-transparent stroke-color is also
> > > specified. Seems like this should be covered in a test as well.
> > 
> > Actually, no, it doesn't. When stroke-color is not explicitly specified, we
> > fall back to the value of -webkit-text-stroke-color, which means the text in
> > your example will be stroked.
> 
> If <div style="stroke-width: 1px">Text</div> now renders with a stroke, then
> we have problems.
> 
> initialtextstrokecolor doesn't exist so I can't easily tell what the default
> value is.

The initial value of -webkit-text-stroke-color is an invalid color value. Since it is invalid, it will get the value of the css color property (in RenderStyle::visitedDependentColor), which is initially black. This explains the black stroke in the test case above.

I don't think this patch has behavior changes outside of getting the computed style, since stroke-color falls back to -webkit-text-stroke-color if not explicitly specified, but I might be mistaken.

Since the result of the test case '<div style="stroke-width: 1px">Text</div>' does not change with this patch, should we address the problem that it renders with a stroke in a separate bug?

[Simon Fraser (smfr)]

(In reply to Per Arne Vollan from comment #22)
> (In reply to Simon Fraser (smfr) from comment #21)
> > (In reply to Per Arne Vollan from comment #18)
> > > (In reply to mitz from comment #16)
> > > > Comment on attachment 316090 [details]
> > > > Patch
> > > > 
> > > > This patch also changes behavior outside of getting computed style, right?
> > > > Currently <div style="stroke-width: 1px">Text</div> renders with a stroke,
> > > > but after the patch it won’t, unless a non-transparent stroke-color is also
> > > > specified. Seems like this should be covered in a test as well.
> > > 
> > > Actually, no, it doesn't. When stroke-color is not explicitly specified, we
> > > fall back to the value of -webkit-text-stroke-color, which means the text in
> > > your example will be stroked.
> > 
> > If <div style="stroke-width: 1px">Text</div> now renders with a stroke, then
> > we have problems.
> > 
> > initialtextstrokecolor doesn't exist so I can't easily tell what the default
> > value is.
> 
> The initial value of -webkit-text-stroke-color is an invalid color value.
> Since it is invalid, it will get the value of the css color property (in
> RenderStyle::visitedDependentColor), which is initially black. This explains
> the black stroke in the test case above.

I think that's wrong, because https://www.w3.org/TR/fill-stroke-3/#propdef-stroke-color states that the initial value of stroke-color is transparent, not currentColor (c.f. border-color https://www.w3.org/TR/css3-background/#border-color).

So this aliasing to -webkit-text-stroke-color which has unspecified behavior is causing problems.

[Jon Lee]

(In reply to Simon Fraser (smfr) from comment #23)
> So this aliasing to -webkit-text-stroke-color which has unspecified behavior
> is causing problems.

Seems like we could either

1) Keep the fallback, and change -webkit-text-stroke-color to default to transparent; or,
2) Not fall back to -webkit-text-stroke-color if stroke-color is specified.

[Per Arne Vollan]

(In reply to Jon Lee from comment #24)
> (In reply to Simon Fraser (smfr) from comment #23)
> > So this aliasing to -webkit-text-stroke-color which has unspecified behavior
> > is causing problems.
> 
> Seems like we could either
> 
> 1) Keep the fallback, and change -webkit-text-stroke-color to default to
> transparent; or,
> 2) Not fall back to -webkit-text-stroke-color if stroke-color is specified.

Thanks for the input! I would prefer 2), since 1) might break sites using -webkit-text-stroke-width and -webkit-text-stroke-color. I assume these are used more frequently than stroke-width and stroke-color.

[Per Arne Vollan]

(In reply to Per Arne Vollan from comment #25)
> (In reply to Jon Lee from comment #24)
> > (In reply to Simon Fraser (smfr) from comment #23)
> > > So this aliasing to -webkit-text-stroke-color which has unspecified behavior
> > > is causing problems.
> > 
> > Seems like we could either
> > 
> > 1) Keep the fallback, and change -webkit-text-stroke-color to default to
> > transparent; or,
> > 2) Not fall back to -webkit-text-stroke-color if stroke-color is specified.
> 
> Thanks for the input! I would prefer 2), since 1) might break sites using
> -webkit-text-stroke-width and -webkit-text-stroke-color. I assume these are
> used more frequently than stroke-width and stroke-color.

Although, I would like to fix this as a separate issue, and just focus on the round-trip issue in this bug. This is fixed by the current patch, without changes in behavior outside of getting the computed style, I believe.

[Per Arne Vollan]

(In reply to Per Arne Vollan from comment #26)
> (In reply to Per Arne Vollan from comment #25)
> > (In reply to Jon Lee from comment #24)
> > > (In reply to Simon Fraser (smfr) from comment #23)
> > > > So this aliasing to -webkit-text-stroke-color which has unspecified behavior
> > > > is causing problems.
> > > 
> > > Seems like we could either
> > > 
> > > 1) Keep the fallback, and change -webkit-text-stroke-color to default to
> > > transparent; or,
> > > 2) Not fall back to -webkit-text-stroke-color if stroke-color is specified.
> > 
> > Thanks for the input! I would prefer 2), since 1) might break sites using
> > -webkit-text-stroke-width and -webkit-text-stroke-color. I assume these are
> > used more frequently than stroke-width and stroke-color.
> 
> Although, I would like to fix this as a separate issue, and just focus on
> the round-trip issue in this bug. This is fixed by the current patch,
> without changes in behavior outside of getting the computed style, I believe.

I have filed a new bug to address this issue: https://bugs.webkit.org/show_bug.cgi?id=174737

[Per Arne Vollan]

Thanks for the input and review, all!

[WebKit Commit Bot]

Comment on attachment 316109 [details]
Patch

Clearing flags on attachment: 316109

Committed r219755: <http://trac.webkit.org/changeset/219755>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.