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.
Created attachment 43143 [details] Test case showing some uses of non-scaling-stroke
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.
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 :/
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.
Created attachment 43185 [details] Test case testing various scenarios of vector-effect Will eventually turn this into a layout test.
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.
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.
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.
Dan, could you briefly explain pixel tests?
I’m concerned about adding SVG 1.2 features without discussion on webkit-dev.
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
(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.
Ok, then it might be that my membership has not been approved yet.
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.
(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.
*** Bug 24634 has been marked as a duplicate of this bug. ***
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
Attachment 43245 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/144676
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.
(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... :-)
> 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.
> 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.
Where is CSSValueNonScalingStroke declared? I can't find it in WebCore or in the patch. Maybe the patch is missing a file?
(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
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.
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?
Created attachment 45700 [details] vector-effect with gradients and pattern
(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.
Created attachment 57982 [details] Patch
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 on attachment 57982 [details] Patch r- because of the style issues :( Looks excellent, other than that.
(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.
(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.
Committed r60858: <http://trac.webkit.org/changeset/60858>
http://trac.webkit.org/changeset/60858 might have broken Qt Linux ARMv5 Release and Qt Linux ARMv7 Release
Committed r60885: <http://trac.webkit.org/changeset/60885>
Comment on attachment 57982 [details] Patch Clearing review flag.