528332 - Feature Request: Implement non-scaling-stroke

Status: RESOLVED FIXED

(Core :: SVG, enhancement)

Description

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

Comment 1

[Jeff Schiller]

er, WebKit and Gecko that is :)

Comment 2

[adeveria]

WebKit has support for this too now (it's in Chrome 6), so Gecko support would be appreciated.

Comment 4

[Martin Hejral]

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]

Please, implement this attribute.

Comment 6

[Robert Longson [:longsonr]]

Attachment: Part 1 - style system changes

Comment 7

[Robert Longson [:longsonr]]

Attachment: Part 2 - everything else

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

(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 [:longsonr]]

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)]

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 [:longsonr]]

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

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

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 [:longsonr]]

Attachment: Part 2 - address review comments

Comment 16

[Robert Longson [:longsonr]]

> 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 [:longsonr]]

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)]

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 19

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/173ec720780e
https://hg.mozilla.org/integration/mozilla-inbound/rev/de36a918add7

Comment 20

[Robert Longson [:longsonr]]

Seems at least one of the new tests fails on Android
https://hg.mozilla.org/integration/mozilla-inbound/rev/35e50e954182

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/173ec720780e
https://hg.mozilla.org/mozilla-central/rev/de36a918add7
https://hg.mozilla.org/mozilla-central/rev/35e50e954182

Comment 22

[Jonathan Watt [:jwatt]]

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

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 [:longsonr]]

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.