Closed Bug 528332 Opened 10 years ago Closed 8 years ago

Feature Request: Implement non-scaling-stroke


(Core :: SVG, enhancement)

Not set





(Reporter: codedread, Assigned: longsonr)


(Blocks 1 open bug)


(Keywords: dev-doc-needed)


(2 files, 1 obsolete file)

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
er, WebKit and Gecko that is :)
Severity: normal → enhancement
WebKit has support for this too now (it's in Chrome 6), so Gecko support would be appreciated.
Duplicate of this bug: 677341
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 ;-)
Please, implement this attribute.
Assignee: nobody → longsonr
Attachment #619524 - Flags: review?(dbaron)
Attached patch Part 2 - everything else (obsolete) — Splinter Review
Attachment #619525 - Flags: review?(dholbert)
OS: Mac OS X → All
Hardware: x86 → All
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?)
Attachment #619524 - Flags: review?(dbaron) → review+
(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.
They still like it according to

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.
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:
The webkit bug is 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 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 ==

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 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
Attachment #619525 - Flags: review?(dholbert) → review+
Attachment #619525 - Attachment is obsolete: true
> 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.
Try server seems happy:

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 ( 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:[svg]non-scaling-stroke
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.
Keywords: dev-doc-needed
Seems at least one of the new tests fails on Android
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.
Or maybe I should try rereading this some time other that 2:30 am. If this does work, can you explain why?
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.
Depends on: 835978
Blocks: svg2
You need to log in before you can comment on or make changes to this bug.