Closed Bug 879659 Opened 7 years ago Closed 7 years ago

implement <marker orient="auto-start-reverse"> from SVG 2

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: heycam, Assigned: heycam)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-needed, feature)

Attachments

(3 files, 4 obsolete files)

At the F2F this week, we discussed a new value of <marker orient> that means the same as "auto", but for marker-start, means "auto" + 180deg.  This allows a single arrowhead marker to be defined and used in marker-start and marker-end and for this to result in both arrowheads pointing out beyond the ends of the path.

https://svgwg.org/svg2-draft/painting.html#OrientAttribute
There are a bunch of marker improvements in SVG 2, so let's put them all behind the one pref.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #760364 - Flags: review?(longsonr)
Attachment #760365 - Flags: review?(longsonr)
Attachment #760364 - Flags: review?(longsonr) → review+
Attachment #760365 - Attachment description: Part 2: Implement <marker orient="auto-start-reverse">. → Part 3: Implement <marker orient="auto-start-reverse">.
Blocks: 793613
Comment on attachment 760615 [details] [diff] [review]
Part 2: Refactor SVG marker handling to support more types of markers in the future.

>+  float xmid = (x1 + x2) / 2;
>+  float ymid = (y1 + y2) / 2;

These variables aren't used.

>+
>+  aMarks->AppendElement(nsSVGMark(x1, y1, angle, nsSVGMark::eStart));
>+  aMarks->AppendElement(nsSVGMark(x2, y2, angle, nsSVGMark::eEnd));
> }
> 
> 
>+  // info on the segment marker for this command:
>+  SVGPathTraversalState state;
>+  state.mode = SVGPathTraversalState::eUpdateAll;

eUpdateAll is the default so the second line is not required. More than that though, I can't see why this is required at all? Why create and keep the state variable up to date? You don't seem to be using it.
Sorry, I didn't do a good enough job splitting out the refactoring from the part that implements marker-segment (for bug 793613).  I'll upload another try soon.
Comment on attachment 760365 [details] [diff] [review]
Part 3: Implement <marker orient="auto-start-reverse">.

Waiting for that other try...
Attachment #760365 - Flags: review?(longsonr) → review-
Comment on attachment 760615 [details] [diff] [review]
Part 2: Refactor SVG marker handling to support more types of markers in the future.

ditto
Attachment #760615 - Flags: review?(longsonr) → review-
Attachment #760365 - Attachment is obsolete: true
Attachment #771817 - Flags: review?(longsonr)
Attachment #771816 - Flags: review?(longsonr) → review+
>   uint16_t GetBaseValue() const
>     { return mBaseVal; }
>+  // we want to avoid exposing SVG_MARKER_ORIENT_AUTO_START_REVERSE to
>+  // Web content
>   uint16_t GetAnimValue() const
>+    { return mAnimVal == SVG_MARKER_ORIENT_AUTO_START_REVERSE ?
>+               SVG_MARKER_ORIENT_UNKNOWN : mAnimVal; }

Why didn't you modify GetBaseValue like you did GetAnimValue?
Although on the whole I'm not sure it's the greatest idea in the world to do this for GetAnimValue. I would have thought this would be rather unexpected for users of this new functionality.
(In reply to Robert Longson from comment #10)
> Why didn't you modify GetBaseValue like you did GetAnimValue?

An oversight.

(In reply to Robert Longson from comment #11)
> Although on the whole I'm not sure it's the greatest idea in the world to do
> this for GetAnimValue. I would have thought this would be rather unexpected
> for users of this new functionality.

Yeah, I recently had second thoughts about that, and raised it on the mailing list (the general issue of avoiding new integer constants):

  http://lists.w3.org/Archives/Public/www-svg/2013Jul/0019.html

Looks like Tab thinks we should go ahead with adding a non-integer-constant API for this.  What do you think about implementing as in the patch now, and adding the string-based accessor when we get around to adding it to the spec?  I don't know if too many people would use the SVG DOM interface for this attribute, anyway.
Attachment #771817 - Attachment is obsolete: true
Attachment #771817 - Flags: review?(longsonr)
Attachment #773811 - Flags: review?(longsonr)
Fix a test problem.
Attachment #773811 - Attachment is obsolete: true
Attachment #773811 - Flags: review?(longsonr)
Attachment #773828 - Flags: review?(longsonr)
Attachment #773828 - Flags: review?(longsonr) → review+
Follow fix for unused variable warning/error:

https://hg.mozilla.org/integration/mozilla-inbound/rev/71de191da647
Depends on: 893484
Keywords: feature
Depends on: 1093327
Blocks: svg2
You need to log in before you can comment on or make changes to this bug.