Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1072403010:
SVG Text should respect non-scaling-stroke. (Closed)
|
Created:
5 years, 8 months ago by Shanmuga Pandi Modified:
5 years, 8 months ago CC:
blink-reviews, dshwang, slimming-paint-reviews_chromium.org, blink-reviews-paint_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionSVG Text should respect non-scaling-stroke.
Stroke thickness of SVG text should be depends on non-scaling-stroke.
BUG=475203
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194496
Patch Set 1 #
Total comments: 12
Patch Set 2 : Added test with paint server #
Total comments: 4
Patch Set 3 : Added repaint test #Patch Set 4 : fixed nits #
Total comments: 5
Patch Set 5 : Align with review comments #Patch Set 6 : changed from expected.html to expected.txt #
Total comments: 5
Patch Set 7 : fixed review comments #Patch Set 8 : changed stroke-width #
Total comments: 2
Patch Set 9 : increased stroke-width #Patch Set 10 : Add TestExpectations #Patch Set 11 : rebased #Messages
Total messages: 34 (9 generated)
shanmuga.m@samsung.com changed reviewers: + ed@opera.com, fs@opera.com, pdr@chromium.org
Please review it
> SVG Text non-scaling-stroke doesn't work. Please use a better description (this is just the subject of the bug, which rarely make for a good CL subject.) https://codereview.chromium.org/1072403010/diff/1/LayoutTests/svg/stroke/non-... File LayoutTests/svg/stroke/non-scaling-stroke-attribute-text.svg (right): https://codereview.chromium.org/1072403010/diff/1/LayoutTests/svg/stroke/non-... LayoutTests/svg/stroke/non-scaling-stroke-attribute-text.svg:1: <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"> You don't need to open the xlink NS. Also, I think you might as well do this as an HTML testcase (then you don't need to open the svg NS either =)) https://codereview.chromium.org/1072403010/diff/1/LayoutTests/svg/stroke/non-... File LayoutTests/svg/stroke/non-scaling-stroke-css-text.svg (right): https://codereview.chromium.org/1072403010/diff/1/LayoutTests/svg/stroke/non-... LayoutTests/svg/stroke/non-scaling-stroke-css-text.svg:2: <style type="text/css" > I don't see that this test really test much differently (hopefully there's test for applying 'transform' as a property already.) If you want to keep it, then please drop the <style> element, and use inline style (and only for the transform.) https://codereview.chromium.org/1072403010/diff/1/Source/core/paint/SVGInline... File Source/core/paint/SVGInlineTextBoxPainter.cpp (right): https://codereview.chromium.org/1072403010/diff/1/Source/core/paint/SVGInline... Source/core/paint/SVGInlineTextBoxPainter.cpp:281: // FIXME: Non-scaling stroke is not applied here. Could you fix this too? (Other CL ok, using same bug probably too.) https://codereview.chromium.org/1072403010/diff/1/Source/core/paint/SVGInline... Source/core/paint/SVGInlineTextBoxPainter.cpp:328: paintServerTransform.scale(scalingFactor); Make a test with a paint-server (gradient/pattern) too, because this may be influenced by non-scaling-stroke as well. https://codereview.chromium.org/1072403010/diff/1/Source/core/paint/SVGInline... Source/core/paint/SVGInlineTextBoxPainter.cpp:332: // FIXME: Non-scaling stroke is not applied here. Can remove this now.
https://codereview.chromium.org/1072403010/diff/1/LayoutTests/svg/stroke/non-... File LayoutTests/svg/stroke/non-scaling-stroke-attribute-text.svg (right): https://codereview.chromium.org/1072403010/diff/1/LayoutTests/svg/stroke/non-... LayoutTests/svg/stroke/non-scaling-stroke-attribute-text.svg:1: <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"> On 2015/04/22 12:15:54, fs wrote: > You don't need to open the xlink NS. Also, I think you might as well do this as > an HTML testcase (then you don't need to open the svg NS either =)) Done. https://codereview.chromium.org/1072403010/diff/1/LayoutTests/svg/stroke/non-... File LayoutTests/svg/stroke/non-scaling-stroke-css-text.svg (right): https://codereview.chromium.org/1072403010/diff/1/LayoutTests/svg/stroke/non-... LayoutTests/svg/stroke/non-scaling-stroke-css-text.svg:2: <style type="text/css" > On 2015/04/22 12:15:55, fs wrote: > I don't see that this test really test much differently (hopefully there's test > for applying 'transform' as a property already.) If you want to keep it, then > please drop the <style> element, and use inline style (and only for the > transform.) removed this test file https://codereview.chromium.org/1072403010/diff/1/Source/core/paint/SVGInline... File Source/core/paint/SVGInlineTextBoxPainter.cpp (right): https://codereview.chromium.org/1072403010/diff/1/Source/core/paint/SVGInline... Source/core/paint/SVGInlineTextBoxPainter.cpp:281: // FIXME: Non-scaling stroke is not applied here. On 2015/04/22 12:15:55, fs wrote: > Could you fix this too? (Other CL ok, using same bug probably too.) Yes. I can fix this too. Probably Other CL. Could you please provide some test case to check this bug here to verify the fix incase? https://codereview.chromium.org/1072403010/diff/1/Source/core/paint/SVGInline... Source/core/paint/SVGInlineTextBoxPainter.cpp:328: paintServerTransform.scale(scalingFactor); On 2015/04/22 12:15:55, fs wrote: > Make a test with a paint-server (gradient/pattern) too, because this may be > influenced by non-scaling-stroke as well. Added test case https://codereview.chromium.org/1072403010/diff/1/Source/core/paint/SVGInline... Source/core/paint/SVGInlineTextBoxPainter.cpp:332: // FIXME: Non-scaling stroke is not applied here. On 2015/04/22 12:15:55, fs wrote: > Can remove this now. Done.
I almost forgot, but also need to test that repaints are correct with non-scaling-stroke. https://codereview.chromium.org/1072403010/diff/1/Source/core/paint/SVGInline... File Source/core/paint/SVGInlineTextBoxPainter.cpp (right): https://codereview.chromium.org/1072403010/diff/1/Source/core/paint/SVGInline... Source/core/paint/SVGInlineTextBoxPainter.cpp:281: // FIXME: Non-scaling stroke is not applied here. On 2015/04/22 13:08:35, Shanmuga Pandi wrote: > On 2015/04/22 12:15:55, fs wrote: > > Could you fix this too? (Other CL ok, using same bug probably too.) > > Yes. I can fix this too. Probably Other CL. Could you please provide some test > case to check this bug here to verify the fix incase? I think it should be sufficient to add text-decoration="underline" to <text> in the test for CL to see the bug. https://codereview.chromium.org/1072403010/diff/20001/LayoutTests/svg/stroke/... File LayoutTests/svg/stroke/non-scaling-stroke-pattern-text.html (right): https://codereview.chromium.org/1072403010/diff/20001/LayoutTests/svg/stroke/... LayoutTests/svg/stroke/non-scaling-stroke-pattern-text.html:1: <!DOCTYPE html> The name of this is *-pattern-*, but since it fills with a gradient *-gradient-* seems more appropriate. https://codereview.chromium.org/1072403010/diff/20001/LayoutTests/svg/stroke/... LayoutTests/svg/stroke/non-scaling-stroke-pattern-text.html:4: <stop offset="0" stop-color="red"/> I think we should use a more "boring" gradient here to avoid issue with rounding on different platforms: <stop offset="0" stop-color="green"/> <stop offset="0.5" stop-color="green"/> <stop offset="0.5" stop-color="blue"/> <stop offset="1" stop-color="blue"/> (I also prefer to not use the color "red" if there nothing "wrong" when you see it.)
Added repaint test case. https://codereview.chromium.org/1072403010/diff/1/Source/core/paint/SVGInline... File Source/core/paint/SVGInlineTextBoxPainter.cpp (right): https://codereview.chromium.org/1072403010/diff/1/Source/core/paint/SVGInline... Source/core/paint/SVGInlineTextBoxPainter.cpp:281: // FIXME: Non-scaling stroke is not applied here. On 2015/04/22 13:31:41, fs wrote: > On 2015/04/22 13:08:35, Shanmuga Pandi wrote: > > On 2015/04/22 12:15:55, fs wrote: > > > Could you fix this too? (Other CL ok, using same bug probably too.) > > > > Yes. I can fix this too. Probably Other CL. Could you please provide some test > > case to check this bug here to verify the fix incase? > > I think it should be sufficient to add text-decoration="underline" to <text> in > the test for CL to see the bug. Thanks for the test case , I will make another CL to fix this decoration bug. https://codereview.chromium.org/1072403010/diff/20001/LayoutTests/svg/stroke/... File LayoutTests/svg/stroke/non-scaling-stroke-pattern-text.html (right): https://codereview.chromium.org/1072403010/diff/20001/LayoutTests/svg/stroke/... LayoutTests/svg/stroke/non-scaling-stroke-pattern-text.html:1: <!DOCTYPE html> On 2015/04/22 13:31:41, fs wrote: > The name of this is *-pattern-*, but since it fills with a gradient *-gradient-* > seems more appropriate. Done. https://codereview.chromium.org/1072403010/diff/20001/LayoutTests/svg/stroke/... LayoutTests/svg/stroke/non-scaling-stroke-pattern-text.html:4: <stop offset="0" stop-color="red"/> On 2015/04/22 13:31:41, fs wrote: > I think we should use a more "boring" gradient here to avoid issue with rounding > on different platforms: > > <stop offset="0" stop-color="green"/> > <stop offset="0.5" stop-color="green"/> > <stop offset="0.5" stop-color="blue"/> > <stop offset="1" stop-color="blue"/> > > (I also prefer to not use the color "red" if there nothing "wrong" when you see > it.) Done.
https://codereview.chromium.org/1072403010/diff/60001/LayoutTests/svg/repaint... File LayoutTests/svg/repaint/repaint-non-scaling-stroke-text-expected.html (right): https://codereview.chromium.org/1072403010/diff/60001/LayoutTests/svg/repaint... LayoutTests/svg/repaint/repaint-non-scaling-stroke-text-expected.html:1: <!DOCTYPE html> This is not a proper reference for a repaint test. https://codereview.chromium.org/1072403010/diff/60001/LayoutTests/svg/repaint... File LayoutTests/svg/repaint/repaint-non-scaling-stroke-text.html (right): https://codereview.chromium.org/1072403010/diff/60001/LayoutTests/svg/repaint... LayoutTests/svg/repaint/repaint-non-scaling-stroke-text.html:21: <text id="t" x="0" y="50" stroke="blue" stroke-width="2" fill="none" font-size="36">Hello</text> I think you'd want to add the stroke as well, so the transition is from "no stroke" to "non-scaling-stroke"
https://codereview.chromium.org/1072403010/diff/60001/LayoutTests/svg/repaint... File LayoutTests/svg/repaint/repaint-non-scaling-stroke-text-expected.html (right): https://codereview.chromium.org/1072403010/diff/60001/LayoutTests/svg/repaint... LayoutTests/svg/repaint/repaint-non-scaling-stroke-text-expected.html:1: <!DOCTYPE html> On 2015/04/22 14:05:05, fs wrote: > This is not a proper reference for a repaint test. Acknowledged and corrected. https://codereview.chromium.org/1072403010/diff/60001/LayoutTests/svg/repaint... File LayoutTests/svg/repaint/repaint-non-scaling-stroke-text.html (right): https://codereview.chromium.org/1072403010/diff/60001/LayoutTests/svg/repaint... LayoutTests/svg/repaint/repaint-non-scaling-stroke-text.html:21: <text id="t" x="0" y="50" stroke="blue" stroke-width="2" fill="none" font-size="36">Hello</text> On 2015/04/22 14:05:05, fs wrote: > I think you'd want to add the stroke as well, so the transition is from "no > stroke" to "non-scaling-stroke" Done.
https://codereview.chromium.org/1072403010/diff/60001/LayoutTests/svg/repaint... File LayoutTests/svg/repaint/repaint-non-scaling-stroke-text-expected.html (right): https://codereview.chromium.org/1072403010/diff/60001/LayoutTests/svg/repaint... LayoutTests/svg/repaint/repaint-non-scaling-stroke-text-expected.html:1: <!DOCTYPE html> On 2015/04/22 14:31:02, Shanmuga Pandi wrote: > On 2015/04/22 14:05:05, fs wrote: > > This is not a proper reference for a repaint test. > > Acknowledged and corrected. I meant that the expected results for a repaint-test is a -expected.txt (hopefully run-webkit-tests would indicate this.)
On 2015/04/22 14:48:48, fs wrote: > https://codereview.chromium.org/1072403010/diff/60001/LayoutTests/svg/repaint... > File LayoutTests/svg/repaint/repaint-non-scaling-stroke-text-expected.html > (right): > > https://codereview.chromium.org/1072403010/diff/60001/LayoutTests/svg/repaint... > LayoutTests/svg/repaint/repaint-non-scaling-stroke-text-expected.html:1: > <!DOCTYPE html> > On 2015/04/22 14:31:02, Shanmuga Pandi wrote: > > On 2015/04/22 14:05:05, fs wrote: > > > This is not a proper reference for a repaint test. > > > > Acknowledged and corrected. > > I meant that the expected results for a repaint-test is a -expected.txt > (hopefully run-webkit-tests would indicate this.) Earlier I was using runRepaintAndPixelTest, so run-webkit-tests did not indicate any thing. Now As you suggested I replaced -expected.html to -expected.txt. Thank you
https://codereview.chromium.org/1072403010/diff/100001/LayoutTests/svg/repain... File LayoutTests/svg/repaint/repaint-non-scaling-stroke-text.html (right): https://codereview.chromium.org/1072403010/diff/100001/LayoutTests/svg/repain... LayoutTests/svg/repaint/repaint-non-scaling-stroke-text.html:21: <g transform="scale(2 2)"> Make this 0.5 instead so that the resulting stroke ends up being larger. Probably make sense to double the stroke-width at the same time. https://codereview.chromium.org/1072403010/diff/100001/LayoutTests/svg/repain... LayoutTests/svg/repaint/repaint-non-scaling-stroke-text.html:22: <text id="t" x="0" y="50" stroke="none" stroke-width="2" fill="none" font-size="36" vector-effect="none">Hello</text> Make x > 0 (10 perhaps?) so that the repaint rect isn't clipped.
https://codereview.chromium.org/1072403010/diff/100001/LayoutTests/svg/repain... File LayoutTests/svg/repaint/repaint-non-scaling-stroke-text.html (right): https://codereview.chromium.org/1072403010/diff/100001/LayoutTests/svg/repain... LayoutTests/svg/repaint/repaint-non-scaling-stroke-text.html:21: <g transform="scale(2 2)"> On 2015/04/23 08:24:22, fs wrote: > Make this 0.5 instead so that the resulting stroke ends up being larger. > Probably make sense to double the stroke-width at the same time. Done. https://codereview.chromium.org/1072403010/diff/100001/LayoutTests/svg/repain... LayoutTests/svg/repaint/repaint-non-scaling-stroke-text.html:22: <text id="t" x="0" y="50" stroke="none" stroke-width="2" fill="none" font-size="36" vector-effect="none">Hello</text> On 2015/04/23 08:24:22, fs wrote: > Make x > 0 (10 perhaps?) so that the repaint rect isn't clipped. > Done.
https://codereview.chromium.org/1072403010/diff/100001/LayoutTests/svg/repain... File LayoutTests/svg/repaint/repaint-non-scaling-stroke-text.html (right): https://codereview.chromium.org/1072403010/diff/100001/LayoutTests/svg/repain... LayoutTests/svg/repaint/repaint-non-scaling-stroke-text.html:21: <g transform="scale(2 2)"> On 2015/04/23 08:51:33, Shanmuga Pandi wrote: > On 2015/04/23 08:24:22, fs wrote: > > Make this 0.5 instead so that the resulting stroke ends up being larger. > > Probably make sense to double the stroke-width at the same time. > > Done. Please also increase stroke-width.
On 2015/04/23 09:11:33, fs wrote: > https://codereview.chromium.org/1072403010/diff/100001/LayoutTests/svg/repain... > File LayoutTests/svg/repaint/repaint-non-scaling-stroke-text.html (right): > > https://codereview.chromium.org/1072403010/diff/100001/LayoutTests/svg/repain... > LayoutTests/svg/repaint/repaint-non-scaling-stroke-text.html:21: <g > transform="scale(2 2)"> > On 2015/04/23 08:51:33, Shanmuga Pandi wrote: > > On 2015/04/23 08:24:22, fs wrote: > > > Make this 0.5 instead so that the resulting stroke ends up being larger. > > > Probably make sense to double the stroke-width at the same time. > > > > Done. > > Please also increase stroke-width. Done the changes. Hopefully it should be OK this time. Thanks for reviewing.
I'll start a try-bot for you (I'm a bit interested in how the gradient TC looks.) https://codereview.chromium.org/1072403010/diff/140001/LayoutTests/svg/repain... File LayoutTests/svg/repaint/repaint-non-scaling-stroke-text-expected.txt (right): https://codereview.chromium.org/1072403010/diff/140001/LayoutTests/svg/repain... LayoutTests/svg/repaint/repaint-non-scaling-stroke-text-expected.txt:13: [11, 15, 42, 24], Please also verify that this looks correct too (not clipped) - preferably by increasing the stroke-width even more (to make it more obvious.)
https://codereview.chromium.org/1072403010/diff/140001/LayoutTests/svg/repain... File LayoutTests/svg/repaint/repaint-non-scaling-stroke-text-expected.txt (right): https://codereview.chromium.org/1072403010/diff/140001/LayoutTests/svg/repain... LayoutTests/svg/repaint/repaint-non-scaling-stroke-text-expected.txt:13: [11, 15, 42, 24], On 2015/04/23 09:31:40, fs wrote: > Please also verify that this looks correct too (not clipped) - preferably by > increasing the stroke-width even more (to make it more obvious.) Increased stroke-width and it is not clipped
On 2015/04/23 10:03:52, Shanmuga Pandi wrote: > https://codereview.chromium.org/1072403010/diff/140001/LayoutTests/svg/repain... > File LayoutTests/svg/repaint/repaint-non-scaling-stroke-text-expected.txt > (right): > > https://codereview.chromium.org/1072403010/diff/140001/LayoutTests/svg/repain... > LayoutTests/svg/repaint/repaint-non-scaling-stroke-text-expected.txt:13: [11, > 15, 42, 24], > On 2015/04/23 09:31:40, fs wrote: > > Please also verify that this looks correct too (not clipped) - preferably by > > increasing the stroke-width even more (to make it more obvious.) > > Increased stroke-width and it is not clipped Buildbot fails in this case, but this change will not affect this. :( webkit_tests webkit_tests failures: http/tests/inspector/service-workers/service-workers-redundant.html
On 2015/04/23 11:04:24, Shanmuga Pandi wrote: > On 2015/04/23 10:03:52, Shanmuga Pandi wrote: > > > https://codereview.chromium.org/1072403010/diff/140001/LayoutTests/svg/repain... > > File LayoutTests/svg/repaint/repaint-non-scaling-stroke-text-expected.txt > > (right): > > > > > https://codereview.chromium.org/1072403010/diff/140001/LayoutTests/svg/repain... > > LayoutTests/svg/repaint/repaint-non-scaling-stroke-text-expected.txt:13: [11, > > 15, 42, 24], > > On 2015/04/23 09:31:40, fs wrote: > > > Please also verify that this looks correct too (not clipped) - preferably by > > > increasing the stroke-width even more (to make it more obvious.) > > > > Increased stroke-width and it is not clipped > > Buildbot fails in this case, but this change will not affect this. :( > webkit_tests webkit_tests > failures: > http/tests/inspector/service-workers/service-workers-redundant.html That indeed looks unrelated, so probably a flake. LGTM.
The CQ bit was checked by shanmuga.m@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1072403010/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/60494)
On 2015/04/23 17:06:09, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > win_blink_rel on tryserver.blink (JOB_FAILED, > http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/60494) Sorry, but I forgot to mention that the repaint test will need a line in TestExpectations (to get the baselines correct.) So drop the expected file for that and add a line there.
On 2015/04/24 08:37:30, fs wrote: > On 2015/04/23 17:06:09, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > win_blink_rel on tryserver.blink (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/60494) > > Sorry, but I forgot to mention that the repaint test will need a line in > TestExpectations (to get the baselines correct.) So drop the expected file for > that and add a line there. Thanks for you help. I have added the changes
The CQ bit was checked by shanmuga.m@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com Link to the patchset: https://codereview.chromium.org/1072403010/#ps180001 (title: "Add TestExpectations")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1072403010/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_compile_dbg on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
The CQ bit was unchecked by shanmuga.m@samsung.com
The CQ bit was checked by shanmuga.m@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com Link to the patchset: https://codereview.chromium.org/1072403010/#ps200001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1072403010/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://src.chromium.org/viewvc/blink?view=rev&revision=194496 |
