Closed
Bug 528332
Opened 15 years ago
Closed 13 years ago
Feature Request: Implement non-scaling-stroke
Categories
(Core :: SVG, enhancement)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: codedread, Assigned: longsonr)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-needed)
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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•15 years ago
|
||
er, WebKit and Gecko that is :)
Assignee | ||
Updated•15 years ago
|
Severity: normal → enhancement
WebKit has support for this too now (it's in Chrome 6), so Gecko support would be appreciated.
Comment 4•13 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•13 years ago
|
||
Please, implement this attribute.
Assignee | ||
Comment 6•13 years ago
|
||
Assignee: nobody → longsonr
Attachment #619524 -
Flags: review?(dbaron)
Assignee | ||
Comment 7•13 years ago
|
||
Attachment #619525 -
Flags: review?(dholbert)
Assignee | ||
Updated•13 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Comment 8•13 years ago
|
||
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+
Comment 9•13 years ago
|
||
(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•13 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.
Comment 11•13 years ago
|
||
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•13 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 13•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
Attachment #619525 -
Attachment is obsolete: true
Assignee | ||
Comment 16•13 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•13 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
Comment 18•13 years ago
|
||
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•13 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•13 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 20•13 years ago
|
||
Seems at least one of the new tests fails on Android
https://hg.mozilla.org/integration/mozilla-inbound/rev/35e50e954182
Comment 21•13 years ago
|
||
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
Closed: 13 years ago
Resolution: --- → FIXED
Comment 22•12 years ago
|
||
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•12 years ago
|
||
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•12 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.
You need to log in
before you can comment on or make changes to this bug.
Description
•