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>
<rdar://problem/32903679>
Created attachment 316047 [details] Patch
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
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
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.
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
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
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
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
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?
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
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
(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!
Created attachment 316090 [details] Patch
(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.
(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!
(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.
Created attachment 316109 [details] Patch
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.
(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.
(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?
(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.
(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.
(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.
(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.
(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
Thanks for the input and review, all!
Comment on attachment 316109 [details] Patch Clearing flags on attachment: 316109 Committed r219755: <http://trac.webkit.org/changeset/219755>
All reviewed patches have been landed. Closing bug.