Bug 31438 – Implement non-scaling-stroke (from SVG Tiny 1.2, also in Opera)

• Component: SVG
• Status: RESOLVED FIXED

[Jeff Schiller]

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.

[Jeff Schiller]

Created attachment 43143 [details]
Test case showing some uses of non-scaling-stroke

[Jeff Schiller]

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.

[Jeff Schiller]

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 :/

[Jeff Schiller]

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.

[Jeff Schiller]

Created attachment 43185 [details]
Test case testing various scenarios of vector-effect

Will eventually turn this into a layout test.

[Eric Seidel (no email)]

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.

[Jeff Schiller]

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.

[Jeff Schiller]

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.

[Darin Adler]

Dan, could you briefly explain pixel tests?

[Darin Adler]

I’m concerned about adding SVG 1.2 features without discussion on webkit-dev.

[Jeff Schiller]

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

[Eric Seidel (no email)]

(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.

[Jeff Schiller]

Ok, then it might be that my membership has not been approved yet.

[Dirk Schulze]

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.

[Dirk Schulze]

(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.

[Dirk Schulze]

*** Bug 24634 has been marked as a duplicate of this bug. ***

[Adam Barth]

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

[WebKit Review Bot]

Attachment 43245 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/144676

[Dirk Schulze]

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.

[Helder Magalhães]

(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... :-)

[Laszlo Gombos]

> 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.

[Adam Barth]

> 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.

[Adam Barth]

Where is CSSValueNonScalingStroke declared?  I can't find it in WebCore or in the patch.  Maybe the patch is missing a file?

[Laszlo Gombos]

(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

[Adam Barth]

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.

[Jeff Schiller]

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?

[Dirk Schulze]

Created attachment 45700 [details]
vector-effect with gradients and pattern

[Dirk Schulze]

(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.

[Dirk Schulze]

Created attachment 57982 [details]
Patch

[WebKit Review Bot]

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.

[Nikolas Zimmermann]

Comment on attachment 57982 [details]
Patch

r- because of the style issues :( Looks excellent, other than that.

[Dirk Schulze]

(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.

[Nikolas Zimmermann]

(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.

[Dirk Schulze]

Committed r60858: <http://trac.webkit.org/changeset/60858>

[WebKit Review Bot]

http://trac.webkit.org/changeset/60858 might have broken Qt Linux ARMv5 Release and Qt Linux ARMv7 Release

[Dirk Schulze]

Committed r60885: <http://trac.webkit.org/changeset/60885>

[Dirk Schulze]

Comment on attachment 57982 [details]
Patch

Clearing review flag.