Feature Request: Implement non-scaling-stroke

RESOLVED FIXED in mozilla15

Status

()

Core
SVG
--
enhancement
RESOLVED FIXED
8 years ago
4 months ago

People

(Reporter: Jeff Schiller, Assigned: Robert Longson)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

unspecified
mozilla15
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
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.
(Reporter)

Comment 1

8 years ago
er, WebKit and Gecko that is :)
(Assignee)

Updated

8 years ago
Severity: normal → enhancement

Comment 2

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

Updated

6 years ago
Duplicate of this bug: 677341

Comment 4

6 years ago
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

5 years ago
Please, implement this attribute.
(Assignee)

Comment 6

5 years ago
Created attachment 619524 [details] [diff] [review]
Part 1 - style system changes
Assignee: nobody → longsonr
Attachment #619524 - Flags: review?(dbaron)
(Assignee)

Comment 7

5 years ago
Created attachment 619525 [details] [diff] [review]
Part 2 - everything else
Attachment #619525 - Flags: review?(dholbert)
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 10

5 years ago
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.
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
(Assignee)

Comment 12

5 years ago
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 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 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+
(Assignee)

Comment 15

5 years ago
Created attachment 621922 [details] [diff] [review]
Part 2 - address review comments
Attachment #619525 - Attachment is obsolete: true
(Assignee)

Comment 16

5 years ago
> 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.
(Assignee)

Comment 17

5 years ago
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
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.
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/173ec720780e
https://hg.mozilla.org/integration/mozilla-inbound/rev/de36a918add7
Flags: in-testsuite+
Target Milestone: --- → mozilla15
(Assignee)

Updated

5 years ago
Keywords: dev-doc-needed
(Assignee)

Comment 20

5 years ago
Seems at least one of the new tests fails on Android
https://hg.mozilla.org/integration/mozilla-inbound/rev/35e50e954182
https://hg.mozilla.org/mozilla-central/rev/173ec720780e
https://hg.mozilla.org/mozilla-central/rev/de36a918add7
https://hg.mozilla.org/mozilla-central/rev/35e50e954182
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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?
(Assignee)

Comment 24

5 years ago
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: 829085
(Assignee)

Updated

4 years ago
Depends on: 835978
Blocks: 1328534
You need to log in before you can comment on or make changes to this bug.