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
Product: WebKit
Classification: Unclassified
Component: SVG
: 528+ (Nightly build)
: All All
: P2 Enhancement
Assigned To: Nobody
:
Depends on: 40325
Blocks:
  Show dependency treegraph
 
Reported: 2009-11-12 14:15 PST by Jeff Schiller
Modified: 2011-01-02 03:21 PST (History)
13 users (show)

See Also:


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 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-
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 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 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeff Schiller 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 Jeff Schiller 2009-11-13 00:55:13 PST
Created attachment 43143 [details]
Test case showing some uses of non-scaling-stroke
Comment 2 Jeff Schiller 2009-11-13 00:56:17 PST
Created attachment 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 Jeff Schiller 2009-11-13 01:01:41 PST
Created attachment 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 Jeff Schiller 2009-11-13 12:05:10 PST
Created attachment 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 Jeff Schiller 2009-11-13 12:06:25 PST
Created attachment 43185 [details]
Test case testing various scenarios of vector-effect

Will eventually turn this into a layout test.
Comment 6 Eric Seidel 2009-11-13 13:38:43 PST
Comment on attachment 43184 [details]
Patch that adds the vector-effect property, maps attribute to property and handles computed style

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 Jeff Schiller 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 Jeff Schiller 2009-11-15 08:32:29 PST
Created attachment 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 Darin Adler 2009-11-16 13:55:38 PST
Dan, could you briefly explain pixel tests?
Comment 10 Darin Adler 2009-11-16 13:56:05 PST
I’m concerned about adding SVG 1.2 features without discussion on webkit-dev.
Comment 11 Jeff Schiller 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 Eric Seidel 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 Jeff Schiller 2009-11-16 14:44:46 PST
Ok, then it might be that my membership has not been approved yet.
Comment 14 Dirk Schulze 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 Dirk Schulze 2009-11-17 00:12:25 PST
(In reply to comment #6)
> (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.

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 Dirk Schulze 2009-11-17 00:33:42 PST
*** Bug 24634 has been marked as a duplicate of this bug. ***
Comment 17 Adam Barth 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 WebKit Review Bot 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 Dirk Schulze 2009-12-27 03:09:47 PST
Comment on attachment 43245 [details]
Patch that adds vector-effect property/attribute, changes how the stroke is painted when non-scaling-stroke

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 Helder Magalhães 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 Laszlo Gombos 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 Adam Barth 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 Adam Barth 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 Laszlo Gombos 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 Adam Barth 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 Jeff Schiller 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 Dirk Schulze 2009-12-30 23:31:17 PST
Created attachment 45700 [details]
vector-effect with gradients and pattern
Comment 28 Dirk Schulze 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 Dirk Schulze 2010-06-06 13:32:52 PDT
Created attachment 57982 [details]
Patch
Comment 30 WebKit Review Bot 2010-06-06 13:34:31 PDT
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 Nikolas Zimmermann 2010-06-07 08:20:05 PDT
Comment on attachment 57982 [details]
Patch

r- because of the style issues :( Looks excellent, other than that.
Comment 32 Dirk Schulze 2010-06-07 09:18:19 PDT
(In reply to comment #31)
> (From update of attachment 57982 [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 Nikolas Zimmermann 2010-06-08 11:39:04 PDT
(In reply to comment #32)
> (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.
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 Dirk Schulze 2010-06-08 12:12:02 PDT
Committed r60858: <http://trac.webkit.org/changeset/60858>
Comment 35 WebKit Review Bot 2010-06-08 12:24:09 PDT
http://trac.webkit.org/changeset/60858 might have broken Qt Linux ARMv5 Release and Qt Linux ARMv7 Release
Comment 36 Dirk Schulze 2010-06-09 00:25:15 PDT
Committed r60885: <http://trac.webkit.org/changeset/60885>
Comment 37 Dirk Schulze 2011-01-02 03:21:28 PST
Comment on attachment 57982 [details]
Patch

Clearing review flag.