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

RESOLVED FIXED in mozilla25

Status

()

RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

(Blocks: 2 bugs, {dev-doc-needed, feature})

Trunk
mozilla25
dev-doc-needed, feature
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

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

Comment 1

6 years ago
Created attachment 760364 [details] [diff] [review]
Part 1: Add a pref for SVG 2 marker improvements.

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)
(Assignee)

Comment 2

6 years ago
Created attachment 760365 [details] [diff] [review]
Part 3: Implement <marker orient="auto-start-reverse">.
Attachment #760365 - Flags: review?(longsonr)
Attachment #760364 - Flags: review?(longsonr) → review+
(Assignee)

Comment 3

6 years ago
Created attachment 760615 [details] [diff] [review]
Part 2: Refactor SVG marker handling to support more types of markers in the future.
Attachment #760615 - Flags: review?(longsonr)
(Assignee)

Updated

6 years ago
Attachment #760365 - Attachment description: Part 2: Implement <marker orient="auto-start-reverse">. → Part 3: Implement <marker orient="auto-start-reverse">.
(Assignee)

Updated

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

Comment 5

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

Comment 8

6 years ago
Created attachment 771816 [details] [diff] [review]
Part 2: Refactor SVG marker handling to support more types of markers in the future. (v1.1)
Attachment #760615 - Attachment is obsolete: true
Attachment #771816 - Flags: review?(longsonr)
(Assignee)

Comment 9

6 years ago
Created attachment 771817 [details] [diff] [review]
Part 3: Implement <marker orient="auto-start-reverse">. (v1.1)
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.
(Assignee)

Comment 12

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

Comment 13

6 years ago
Created attachment 773811 [details] [diff] [review]
Part 3: Implement <marker orient="auto-start-reverse">. (v1.2)
Attachment #771817 - Attachment is obsolete: true
Attachment #771817 - Flags: review?(longsonr)
Attachment #773811 - Flags: review?(longsonr)
(Assignee)

Comment 14

6 years ago
Created attachment 773828 [details] [diff] [review]
Part 3: Implement <marker orient="auto-start-reverse">. (v1.3)

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

Comment 16

6 years ago
Follow fix for unused variable warning/error:

https://hg.mozilla.org/integration/mozilla-inbound/rev/71de191da647
Depends on: 893484
(Assignee)

Updated

5 years ago
Keywords: feature
Depends on: 1093327
You need to log in before you can comment on or make changes to this bug.