Last Comment Bug 528332 - Feature Request: Implement non-scaling-stroke
: Feature Request: Implement non-scaling-stroke
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: SVG (show other bugs)
: unspecified
: All All
: -- enhancement with 7 votes (vote)
: mozilla15
Assigned To: Robert Longson
:
Mentors:
: 677341 (view as bug list)
Depends on: 829085 835978
Blocks:
  Show dependency treegraph
 
Reported: 2009-11-12 14:16 PST by Jeff Schiller
Modified: 2014-08-19 15:40 PDT (History)
11 users (show)
longsonr: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 - style system changes (14.68 KB, patch)
2012-04-30 04:57 PDT, Robert Longson
dbaron: review+
Details | Diff | Splinter Review
Part 2 - everything else (31.21 KB, patch)
2012-04-30 04:58 PDT, Robert Longson
dholbert: review+
Details | Diff | Splinter Review
Part 2 - address review comments (35.88 KB, patch)
2012-05-08 02:53 PDT, Robert Longson
no flags Details | Diff | Splinter Review

Description Jeff Schiller 2009-11-12 14:16:22 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-12 14:16:38 PST
er, WebKit and Gecko that is :)
Comment 2 adeveria 2010-10-08 13:27:01 PDT
WebKit has support for this too now (it's in Chrome 6), so Gecko support would be appreciated.
Comment 3 Martin Hejral 2011-08-09 00:47:40 PDT
*** Bug 677341 has been marked as a duplicate of this bug. ***
Comment 4 Martin Hejral 2011-08-09 01:05:07 PDT
I highly suggest implement this attribute, because good usability for example in CAD-like technical drawings... 


I found this problem just at the moment when implementing SVG web project working with huge CAD documents ;-)
Comment 5 Jimmy Gilles 2012-01-26 11:15:42 PST
Please, implement this attribute.
Comment 6 Robert Longson 2012-04-30 04:57:59 PDT
Created attachment 619524 [details] [diff] [review]
Part 1 - style system changes
Comment 7 Robert Longson 2012-04-30 04:58:36 PDT
Created attachment 619525 [details] [diff] [review]
Part 2 - everything else
Comment 8 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-04-30 08:46:45 PDT
Comment on attachment 619524 [details] [diff] [review]
Part 1 - style system changes

>diff --git a/content/base/src/nsGkAtomList.h b/content/base/src/nsGkAtomList.h
>--- a/content/base/src/nsGkAtomList.h
>+++ b/content/base/src/nsGkAtomList.h
>@@ -1370,16 +1370,17 @@ GK_ATOM(x2, "x2")
> GK_ATOM(xChannelSelector, "xChannelSelector")
> GK_ATOM(xor_, "xor")
> GK_ATOM(y, "y")
> GK_ATOM(y1, "y1")
> GK_ATOM(y2, "y2")
> GK_ATOM(yChannelSelector, "yChannelSelector")
> GK_ATOM(z, "z")
> GK_ATOM(zoomAndPan, "zoomAndPan")
>+GK_ATOM(vector_effect, "vector-effect")
> 
> GK_ATOM(accumulate, "accumulate")
> GK_ATOM(additive, "additive")
> GK_ATOM(attributeName, "attributeName")
> GK_ATOM(attributeType, "attributeType")
> GK_ATOM(auto_reverse, "auto-reverse")
> GK_ATOM(begin, "begin")
> GK_ATOM(beginEvent, "beginEvent")

This change isn't needed as part of this patch (though maybe it's needed in patch 2).

r=dbaron with that moved to patch 2 or removed

(I'm not crazy about the name of this property, though.  Do you happen to know if the SVG WG still likes it?)
Comment 9 Jonathan Watt [:jwatt] 2012-04-30 08:53:37 PDT
(In reply to David Baron [:dbaron] from comment #8)
> I'm not crazy about the name of this property

If we want to try and get it changed we could prefix it for now.
Comment 10 Robert Longson 2012-04-30 08:54:29 PDT
They still like it according to http://www.w3.org/Graphics/SVG/WG/wiki/SVG2_Requirements_Input#Non-scaling_stroke

I'll include the atom change in part 2 when I land it. Perhaps Daniel can imagine I did that so I don't need to post updated patches just for that.
Comment 11 Cameron McCormack (:heycam) 2012-04-30 17:07:05 PDT
I'm not fond of the vector-effect="non-scaling-stroke" naming either, especially since we don't know exactly how the full Vector Effects feature is going to end up looking.  Although I see in the linked issue claims that Opera and WebKit have it implemented with that name:

  http://www.w3.org/Graphics/SVG/WG/track/issues/2400
Comment 12 Robert Longson 2012-05-01 01:35:24 PDT
The webkit bug is https://bugs.webkit.org/show_bug.cgi?id=31438 so they've had it a while now. I can confirm that Opera supports it too. It is part of SVG 1.2 Tiny after all.
Comment 13 Daniel Holbert [:dholbert] 2012-05-06 17:35:37 PDT
Comment on attachment 619525 [details] [diff] [review]
Part 2 - everything else

>+++ b/layout/reftests/svg/non-scaling-stroke-01.svg
[...]
>+  <pattern id="pattern" x="0" y="0" width="20" height="20" patternUnits="userSpaceOnUse" patternTransform="scale(4,0.5), skewX(45)">
>+	  <rect x="0" y="0" width="10" height="10" fill="red"/>
>+	  <rect x="10" y="0" width="10" height="10" fill="green"/>
>+	  <rect x="0" y="10" width="10" height="10" fill="blue"/>
>+	  <rect x="10" y="10" width="10" height="10" fill="yellow"/>
>+  </pattern>

Nit: The testcase has tab characters at the beginning of the line here, which are the only think that make this block different from the reference case.  Nix those tabs, so this chunk matches between testcase vs. reference.

>+<rect width="400" height="50" fill="none" stroke="url(#grad2)" transform="translate(20,100) scale(0.25,1)"/>
>+      
>+<use xlink:href="#rect" transform="translate(20, 180) scale(0.25,1)" stroke="url(#pattern)"/>

Whitespace on the blank line there (between <rect> and <use>)

> nsSVGGeometryFrame::SetupCairoStrokeGeometry(gfxContext *aContext)
> {
[...]
>+  if (GetStyleSVGReset()->mVectorEffect ==
>+        NS_STYLE_VECTOR_EFFECT_NON_SCALING_STROKE) {
>+ 
[etc]

Looks like this line and the next ~10 lines are effectively duplicated in three places:
 (1) here, in nsSVGGeometryFrame::SetupCairoStrokeGeometry
 (2) nsSVGPaintServerFrame::ApplyStrokeTransform
 (3) PathExtentsToMaxStrokeExtents

Could we share that code?  Perhaps with a method like
 nsSVGUtils::RevertCTMIfNonScalingStroke(nsIFrame *aSource,
                                         gfxMatrix &transform)
(which would basically be a more public version of nsSVGPaintServerFrame::ApplyStrokeTransform)

[The next 2 comments relate to this chunk of to-be-shared code:]

>+    nsSVGElement *ctx = static_cast<nsSVGElement*>
>+                                 (mContent->IsNodeOfType(nsINode::eTEXT) ?
>+                                     mContent->GetParent() : mContent);

Before we static_cast our nsIContent* to nsSVGElement, we should probably assert that it's IsSVG().

In fact, if this is moving to nsSVGUtils and taking an arbitrary nsIFrame* pointer, perhaps we should be checking IsSVG instead of IsNodeOfType there?  That's what we do elsewhere in nsSVGUtils.

>+    gfxMatrix transform = nsSVGUtils::GetCTM(ctx, true);
>+    if (!transform.IsSingular()) {
>+      aContext->Multiply(transform.Invert());
>+    }

Are we sure that our CTM is purely a scale here? The scale is the only thing we want to be un-applying at this point, right?  Perhaps we should assert that the CTM's translation components are 0?  (Or maybe I'm misunderstanding something -- either way, a comment here would probably help. :))
Comment 14 Daniel Holbert [:dholbert] 2012-05-07 10:38:32 PDT
Comment on attachment 619525 [details] [diff] [review]
Part 2 - everything else

Also: AFAICT, this doesn't affect full-page-zoom -- which is fine, I think. (That is to say: full-page-zoom will still increase the painted size of a stroke, despite non-scaling-stroke being in effect.)

This seems like the correct behavior to me, since applying a full-page-zoom is like increasing the resolution rather than introducing a transform.  I think this patch should include a reftest (probably just "non-scaling-stroke-02.svg") with reftest-zoom applied to a non-scaling stroke, to enforce that as our current behavior.  

r=me with that and the previous comment addressed
Comment 15 Robert Longson 2012-05-08 02:53:07 PDT
Created attachment 621922 [details] [diff] [review]
Part 2 - address review comments
Comment 16 Robert Longson 2012-05-08 02:56:47 PDT
> Are we sure that our CTM is purely a scale here? The scale is the only thing we 
> want to be un-applying at this point, right?  Perhaps we should assert that the 
> CTM's translation components are 0?  (Or maybe I'm misunderstanding something -- 
> either way, a comment here would probably help. :))

It isn't necessarily purely a scale. non-scaling-stroke just reverts the screen CTM transform. I've added some comments and a pointer to the specification.
Comment 17 Robert Longson 2012-05-08 08:55:14 PDT
Try server seems happy: https://tbpl.mozilla.org/?tree=Try&rev=2eb9b8dc93eb

I would like to land this patch as is, despite nobody liking the name. Opera and Webkit have implemented it like this. The SVGEdit editor creates content with this feature (http://code.google.com/p/svg-edit/wiki/BrowserBugs#Missing_SVG_support_desired_in_SVG-edit) and I suspect Adobe Illustrator may do too.

There's plenty of folks wanting this and they're generally being advised to use non-scaling-stroke: http://stackoverflow.com/search?q=[svg]non-scaling-stroke
Comment 18 Cameron McCormack (:heycam) 2012-05-17 01:05:14 PDT
At some point in the future we can work out how (or whether) the vector-effect property will handle multiple vector effects, and whether we should have a dedicated non-scaling-stroke property as well.  But for now I am happy for it to live solely in vector-effect, given that other implementations have it there and there is some content expecting it.
Comment 20 Robert Longson 2012-05-18 03:31:47 PDT
Seems at least one of the new tests fails on Android
https://hg.mozilla.org/integration/mozilla-inbound/rev/35e50e954182
Comment 22 Jonathan Watt [:jwatt] 2012-06-29 18:23:46 PDT
Wait, Robert, this isn't implemented correctly, is it? It seems like these patches just draws the path and stroke in outer-<svg> space. In other words it prevents both the path outline and the stroke from scaling. But the path outline _is_ supposed to scale - it's only the stroke width that's not.
Comment 23 Jonathan Watt [:jwatt] 2012-06-29 18:35:37 PDT
Or maybe I should try rereading this some time other that 2:30 am. If this does work, can you explain why?
Comment 24 Robert Longson 2012-06-30 00:55:07 PDT
There are tests for whether we're drawing the fill or the stroke and all of the scaling is only applied in the stroke drawing pass.

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