Bug 31438 - Implement non-scaling-stroke (from SVG Tiny 1.2, also in Opera)
: Implement non-scaling-stroke (from SVG Tiny 1.2, also in Opera)
Status: RESOLVED FIXED
: WebKit
SVG
: 528+ (Nightly build)
: All All
: P2 Enhancement
Assigned To:
:
:
: 40325
:
  Show dependency treegraph
 
Reported: 2009-11-12 14:15 PST by
Modified: 2011-01-02 03:21 PST (History)


Attachments
Test case showing some uses of non-scaling-stroke (2.01 KB, image/svg+xml)
2009-11-13 00:55 PST, Jeff Schiller
no flags Details
Patch that only adds the vector-effect property - for feedback (8.06 KB, patch)
2009-11-13 00:56 PST, Jeff Schiller
no flags Review Patch | Details | Formatted Diff | Diff
Test case showing some uses of non-scaling-stroke (2.17 KB, image/svg+xml)
2009-11-13 01:01 PST, Jeff Schiller
no flags Details
Patch that adds the vector-effect property, maps attribute to property and handles computed style (8.50 KB, patch)
2009-11-13 12:05 PST, Jeff Schiller
eric: review-
Review Patch | Details | Formatted Diff | Diff
Test case testing various scenarios of vector-effect (2.67 KB, image/svg+xml)
2009-11-13 12:06 PST, Jeff Schiller
no flags Details
Patch that adds vector-effect property/attribute, changes how the stroke is painted when non-scaling-stroke (13.34 KB, patch)
2009-11-15 08:32 PST, Jeff Schiller
no flags Review Patch | Details | Formatted Diff | Diff
vector-effect with gradients and pattern (2.17 KB, image/svg+xml)
2009-12-30 23:31 PST, Dirk Schulze
no flags Details
Patch (67.50 KB, patch)
2010-06-06 13:32 PST, Dirk Schulze
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-11-12 14:15:01 PST
http://www.w3.org/TR/SVGTiny12/painting.html#NonScalingStroke

SVG Tiny 1.2 has a vector-effect property which allows a non-scaling-stroke.  Non-scaling strokes are actually a big missing feature in SVGF 1.1.

Opera 9.5+ has implemented this property-value.  I think WebKit should do the same.
------- Comment #1 From 2009-11-13 00:55:13 PST -------
Created an attachment (id=43143) [details]
Test case showing some uses of non-scaling-stroke
------- Comment #2 From 2009-11-13 00:56:17 PST -------
Created an attachment (id=43144) [details]
Patch that only adds the vector-effect property - for feedback

I've just started to look at this.  Since I've never added a CSS property to WebKit before, I'm attaching the patch that _only_ adds the property first to get feedback.  It seems to work in my simple test case.
------- Comment #3 From 2009-11-13 01:01:41 PST -------
Created an attachment (id=43145) [details]
Test case showing some uses of non-scaling-stroke

Updated test showing that my patch does not handle the getComputedStyle() case when the property is inherited :/
------- Comment #4 From 2009-11-13 12:05:10 PST -------
Created an attachment (id=43184) [details]
Patch that adds the vector-effect property, maps attribute to property and handles computed style

This patch still does not do anything visually yet - but I think I got how to add a property/attribute to WebKit now.
------- Comment #5 From 2009-11-13 12:06:25 PST -------
Created an attachment (id=43185) [details]
Test case testing various scenarios of vector-effect

Will eventually turn this into a layout test.
------- Comment #6 From 2009-11-13 13:38:43 PST -------
(From update of attachment 43184 [details])
This is missing a ChangeLog.  See http://webkit.org/coding/contributing.html

We've generally avoided SVG 1.2, since we're not even done with 1.1 yet.  I'm not sure we really want to add 1.2 features at all.

r- for the missing ChangeLog.
------- Comment #7 From 2009-11-13 13:55:40 PST -------
Woops, missed the ChangeLog, yes - sorry.

Actually this patch does nothing more than add the property so far.  Was hoping for just a quick review to make sure I've got that part right.

Also, I realize the general avoidance of SVG 1.2.  However, I think it makes sense to cherry-pick a couple of features from SVGT 1.2 as Opera has done to improve SVG on the web.  In particular, this feature is really helpful with GIS/mapping.

The following SVG file illustrates the current problem with SVG 1.1.

<svg xmlns="http://www.w3.org/2000/svg">
  <rect transform="scale(6,1)" x="10" y="10" width="100" height="100" fill="yellow" stroke="black" stroke-width="5" />
</svg>

It would be really nice to be able to keep a constant stroke width around the rectangle (basically ignoring the scale).  That's what vector-effect="non-scaling-stroke" does.
------- Comment #8 From 2009-11-15 08:32:29 PST -------
Created an attachment (id=43245) [details]
Patch that adds vector-effect property/attribute, changes how the stroke is painted when non-scaling-stroke

This patch is intentionally missing layout tests as I am not quite sure how to handle that yet.  If someone could please review the patch and then give me some help, that would be much appreciated.  

Following is the DumpRenderTree output from running the test case file attached to this bug:

layer at (0,0) size 800x600
  RenderView at (0,0) size 800x600
layer at (0,0) size 800x600
  RenderSVGRoot {svg} at (92.50,42.50) size 315x165
    RenderSVGContainer {g} at (92.50,42.50) size 115x65 [transform={m=((1.00,0.00)(0.00,1.00)) t=(100.00,50.00)}]
      RenderPath {rect} at (92.50,42.50) size 115x65 [stroke={[type=SOLID] [color=#FF0000] [stroke width=15.00]}] [data="M0.00,0.00 L100.00,0.00 L100.00,50.00 L0.00,50.00 Z"]
      RenderPath {rect} at (98.12,42.50) size 103.75x65 [transform={m=((0.25,0.00)(0.00,1.00)) t=(0.00,0.00)}] [stroke={[type=SOLID] [color=#00FF00] [stroke width=15.00]}] [data="M0.00,0.00 L400.00,0.00 L400.00,50.00 L0.00,50.00 Z"]
    RenderSVGContainer {g} at (92.50,142.50) size 115x65 [transform={m=((1.00,0.00)(0.00,1.00)) t=(100.00,150.00)}]
      RenderPath {rect} at (92.50,142.50) size 115x65 [stroke={[type=SOLID] [color=#FF0000] [stroke width=15.00]}] [data="M0.00,0.00 L100.00,0.00 L100.00,50.00 L0.00,50.00 Z"]
      RenderPath {rect} at (98.12,142.50) size 103.75x65 [transform={m=((0.25,0.00)(0.00,1.00)) t=(0.00,0.00)}] [stroke={[type=SOLID] [color=#00FF00] [stroke width=15.00]}] [data="M0.00,0.00 L400.00,0.00 L400.00,50.00 L0.00,50.00 Z"]
    RenderSVGContainer {g} at (292.50,42.50) size 115x65 [transform={m=((1.00,0.00)(0.00,1.00)) t=(300.00,50.00)}]
      RenderPath {rect} at (292.50,42.50) size 115x65 [stroke={[type=SOLID] [color=#FF0000] [stroke width=15.00]}] [data="M0.00,0.00 L100.00,0.00 L100.00,50.00 L0.00,50.00 Z"]
      RenderPath {rect} at (298.12,42.50) size 103.75x65 [transform={m=((0.25,0.00)(0.00,1.00)) t=(0.00,0.00)}] [stroke={[type=SOLID] [color=#00FF00] [stroke width=15.00]}] [data="M0.00,0.00 L400.00,0.00 L400.00,50.00 L0.00,50.00 Z"]
    RenderSVGContainer {g} at (292.50,142.50) size 115x65 [transform={m=((1.00,0.00)(0.00,1.00)) t=(300.00,150.00)}]
      RenderPath {rect} at (292.50,142.50) size 115x65 [stroke={[type=SOLID] [color=#FF0000] [stroke width=15.00]}] [data="M0.00,0.00 L100.00,0.00 L100.00,50.00 L0.00,50.00 Z"]
      RenderPath {rect} at (298.12,142.50) size 103.75x65 [transform={m=((0.25,0.00)(0.00,1.00)) t=(0.00,0.00)}] [data="M0.00,0.00 L400.00,0.00 L400.00,50.00 L0.00,50.00 Z"]


As can be seen, the green (lime) paths still show in the DumpRenderTree as how they are in the DOM (narrow rects with transforms on them), despite the strokes being painted differently.  

Are there any suggestions on the right way to test this - basically is there a way to test if any red pixels are present in WebKit's output?  I've heard someone mention pixel tests, but not sure how those are handled.
------- Comment #9 From 2009-11-16 13:55:38 PST -------
Dan, could you briefly explain pixel tests?
------- Comment #10 From 2009-11-16 13:56:05 PST -------
I’m concerned about adding SVG 1.2 features without discussion on webkit-dev.
------- Comment #11 From 2009-11-16 14:25:31 PST -------
Hi Darin,

I found webkit-dev, subscribed to it, typed up an email for discussion and sent it only to find out it bounced because the mailing list is moderated :)

So that email is probably sitting in someone's queue if someone cares to approve it on through.

Jeff
------- Comment #12 From 2009-11-16 14:43:13 PST -------
(In reply to comment #11)
> Hi Darin,
> 
> I found webkit-dev, subscribed to it, typed up an email for discussion and sent
> it only to find out it bounced because the mailing list is moderated :)
> 
> So that email is probably sitting in someone's queue if someone cares to
> approve it on through.
> 
> Jeff

webkit-dev is not moderated for members.  At least to my knowledge.
------- Comment #13 From 2009-11-16 14:44:46 PST -------
Ok, then it might be that my membership has not been approved yet.
------- Comment #14 From 2009-11-17 00:06:18 PST -------
I still wounder it this works with transformed gradients. This is the main problem I see on Cg. Cg is not able to transform gradients, so we transform the context to draw the gradient correctly. Can you add a testcase where you use not scaled strokes and fill/stroke the patch with a transformed gradient?
And please scale and skew the gradient using gradientTransform.

Also, did youalready run pixel-tests? There might me other issues.
------- Comment #15 From 2009-11-17 00:12:25 PST -------
(In reply to comment #6)
> (From update of attachment 43184 [details] [details])
> This is missing a ChangeLog.  See http://webkit.org/coding/contributing.html
> 
> We've generally avoided SVG 1.2, since we're not even done with 1.1 yet.  I'm
> not sure we really want to add 1.2 features at all.
> 
> r- for the missing ChangeLog.

The main parts that we miss on SVG 1.1 are filters. Sure, we still have some issues with texts and some other parts. I don't talk about implementing the complete SVG 1.2 (and I would avoid implementing parts like SVG uDOM that is incompatible to our current SVG DOM) but features like non-scaling-strokes are very useful.
------- Comment #16 From 2009-11-17 00:33:42 PST -------
*** Bug 24634 has been marked as a duplicate of this bug. ***
------- Comment #17 From 2009-11-30 12:24:46 PST -------
Attachment 43245 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/css/CSSPrimitiveValueMappings.h:2351:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/css/CSSPrimitiveValueMappings.h:2366:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Done processing WebCore/css/CSSPrimitiveValueMappings.h
Done processing WebCore/rendering/style/SVGRenderStyle.h
Done processing WebCore/svg/SVGStyledElement.cpp
Done processing WebCore/css/CSSComputedStyleDeclaration.cpp
Done processing WebCore/css/SVGCSSParser.cpp
WebCore/css/SVGCSSComputedStyleDeclaration.cpp:171:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Done processing WebCore/css/SVGCSSComputedStyleDeclaration.cpp
WebCore/css/SVGCSSStyleSelector.cpp:561:  One line control clauses should not use braces.  [whitespace/braces] [4]
Done processing WebCore/css/SVGCSSStyleSelector.cpp
WebCore/rendering/RenderPath.cpp:192:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Done processing WebCore/rendering/RenderPath.cpp
WebCore/rendering/style/SVGRenderStyleDefs.h:126:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Done processing WebCore/rendering/style/SVGRenderStyleDefs.h
Total errors found: 6
------- Comment #18 From 2009-12-25 23:55:37 PST -------
Attachment 43245 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/144676
------- Comment #19 From 2009-12-27 03:09:47 PST -------
(From update of attachment 43245 [details])
For the style issues, the build problem on Qt and missing detailed tests with gradients and patterns.

I would like to see this SVG 1.2 features in WebKit. I hope others agree to add this features.
------- Comment #20 From 2009-12-27 06:53:19 PST -------
(In reply to comment #19)

Response inline...


[krit: review -]
> For the style issues, the build problem on Qt and missing detailed tests with
> gradients and patterns.

Yeah, I noticed this and the issues should be fairly easy to fix, although I currently haven't a WebKit build environment. Jeff, would you like a hand in fixing the issues?


> I would like to see this SVG 1.2 features in WebKit. I hope others agree to add
> this features.

Me too, there are a few SVG Tiny 1.2 features which seem pretty valuable even when the current trend seems to be supporting SVG Full 1.1. Just my 2 cents, also... :-)
------- Comment #21 From 2009-12-28 06:18:47 PST -------
> Yeah, I noticed this and the issues should be fairly easy to fix, although I
> currently haven't a WebKit build environment. Jeff, would you like a hand in
> fixing the issues?

QtWebkit (r52595) builds for me with the patch (https://bug-31438-attachments.webkit.org/attachment.cgi?id=43245) - both with SVG support turned on or off build time. I can not explain the build failure from the Review Bot.

I also would like to see this SVG 1.2 features in WebKit.
------- Comment #22 From 2009-12-28 08:33:56 PST -------
> I can not explain the build failure from the Review Bot.

Maybe it's a bug with the review bot?  It's very new, so there could be some issues we need to work out.
------- Comment #23 From 2009-12-28 09:51:24 PST -------
Where is CSSValueNonScalingStroke declared?  I can't find it in WebCore or in the patch.  Maybe the patch is missing a file?
------- Comment #24 From 2009-12-28 10:16:01 PST -------
(In reply to comment #23)
> Where is CSSValueNonScalingStroke declared?  I can't find it in WebCore or in
> the patch.  Maybe the patch is missing a file?

Perhaps it is generated from here ? 

> ===================================================================
> --- WebCore/css/SVGCSSValueKeywords.in	(revision 50998)
> +++ WebCore/css/SVGCSSValueKeywords.in	(working copy)

> +# CSS_PROP_VECTOR_EFFECT
> +# none
> +non-scaling-stroke
> +# inherit
------- Comment #25 From 2009-12-28 10:18:53 PST -------
I see.  I bet there's some kind of dependency problem in the Qt build that it doesn't understand what it needs to do when SVGCSSValueKeywords.in changes.
------- Comment #26 From 2009-12-30 20:17:33 PST -------
Ok, I'm set to start working on this again.

  * I can easily fix the style issues

  * I no nothing about the Qt build issues, sounds like it's only a build-bot issue so should I ignore it and let Adam or whoever sort that out?

  * I will put together some tests to add to WebKit (see the test case attached).  Any suggestions on this would help.

  * I will start to look at gradients and patterns in strokes next - would it make sense to include that as a second bug/patch or should it be the full implementation out of the gate?
------- Comment #27 From 2009-12-30 23:31:17 PST -------
Created an attachment (id=45700) [details]
vector-effect with gradients and pattern
------- Comment #28 From 2009-12-30 23:40:16 PST -------
(In reply to comment #26)
>   * I no nothing about the Qt build issues, sounds like it's only a build-bot
> issue so should I ignore it and let Adam or whoever sort that out?

We can look at the Qt-bots once you have a patch. We can also ask the qt folks to help fixing build problems.

>   * I will start to look at gradients and patterns in strokes next - would it
> make sense to include that as a second bug/patch or should it be the full
> implementation out of the gate?

Yes. It just makes sense to add vector-effect's with working pattern and gradients. It looks like they depend to much on the transformation of the context. Otherwise it may need a bigger redesign once your patch gets landed and we figure out that we can't do it that way. 
I added a test case above. We need to take care about patternUnits, gradientUnits, patternContentUnits as well as gradientTransform, patternTransform and object transformations. I can help on creating test cases if you want.
------- Comment #29 From 2010-06-06 13:32:52 PST -------
Created an attachment (id=57982) [details]
Patch
------- Comment #30 From 2010-06-06 13:34:31 PST -------
Attachment 57982 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/rendering/style/SVGRenderStyle.h:172:  _vectorEffect is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/css/CSSPrimitiveValueMappings.h:2603:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
WebCore/css/CSSPrimitiveValueMappings.h:2603:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #31 From 2010-06-07 08:20:05 PST -------
(From update of attachment 57982 [details])
r- because of the style issues :( Looks excellent, other than that.
------- Comment #32 From 2010-06-07 09:18:19 PST -------
(In reply to comment #31)
> (From update of attachment 57982 [details] [details])
> r- because of the style issues :( Looks excellent, other than that.

The space before the closing brace is missing on the switch statement, thats correct.
I know the naming convention about leading '_', but won't renaming _vectorEffect cause problems in the perl build code script? Renaming would mean, that all variable names in SVGRenderStyle.h need to be renamed.
------- Comment #33 From 2010-06-08 11:39:04 PST -------
(In reply to comment #32)
> (In reply to comment #31)
> > (From update of attachment 57982 [details] [details] [details])
> > r- because of the style issues :( Looks excellent, other than that.
> 
> The space before the closing brace is missing on the switch statement, thats correct.
> I know the naming convention about leading '_', but won't renaming _vectorEffect cause problems in the perl build code script? Renaming would mean, that all variable names in SVGRenderStyle.h need to be renamed.
Oops, you're right about the last one. So when fixing the first, it's done, I'll set r+, and you can change the space before landing.
------- Comment #34 From 2010-06-08 12:12:02 PST -------
Committed r60858: <http://trac.webkit.org/changeset/60858>
------- Comment #35 From 2010-06-08 12:24:09 PST -------
http://trac.webkit.org/changeset/60858 might have broken Qt Linux ARMv5 Release and Qt Linux ARMv7 Release
------- Comment #36 From 2010-06-09 00:25:15 PST -------
Committed r60885: <http://trac.webkit.org/changeset/60885>
------- Comment #37 From 2011-01-02 03:21:28 PST -------
(From update of attachment 57982 [details])
Clearing review flag.