1456004 - Set angle unit type in setOrientToAngle and to UNKNOWN when setting orient to auto or auto-start-reverse

Status: RESOLVED FIXED

(Core :: SVG, enhancement, P3)

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

SetOrientToAngle should not ignore the angle's unit type
SetBaseValue should not support SVG_MARKER_ORIENT_AUTO_START_REVERSE ever.
auto and auto-start-reverse should set the unitType to UNKNOWN.

With this and bug 1455854 we pass https://wpt.fyi/svg/types/scripted/SVGAnimatedEnumeration-SVGMarkerElement.html

Comment 2

[Robert Longson [:longsonr]]

Not this bug, that's bug 1455854. This bug is unconnected with any w3c issues.

Comment 3

[Robert Longson [:longsonr]]

Ahh, OK I see. Do you want me to pull out the nsSVGOrientType::SetBaseValue changes into a separate part.

Comment 4

[Robert Longson [:longsonr]]

Attachment: Part 1 - ensure unitType is set properly

Comment 5

[Robert Longson [:longsonr]]

Attachment: Part 2 - Disallow setting baseVal to 3 (auto-start-reverse)

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8971853 [details] [diff] [review]
Part 1 - ensure unitType is set properly

Review of attachment 8971853 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the correct unit passed in, if I'm not mistaken.

::: dom/svg/SVGAngle.cpp
@@ +48,4 @@
>      return;
>    }
>    bool isBaseVal = mType == BaseValue;
> +  mVal->SetBaseValue(aValue, mVal->mBaseValUnit,

Shouldn't this be passing in SVG_ANGLETYPE_UNSPECIFIED or SVG_ANGLETYPE_DEG?

Comment 7

[Robert Longson [:longsonr]]

Both Chrome and Safari leave the units unchanged i.e. we're consistent with/without this patch with them both.

I think it would seem odd if markerElement.orientAngle.baseVal.value = 10 changed the angle's units.

Comment 8

[Cameron McCormack (:heycam)]

(In reply to Robert Longson [:longsonr] from comment #7)
> Both Chrome and Safari leave the units unchanged i.e. we're consistent
> with/without this patch with them both.
> 
> I think it would seem odd if markerElement.orientAngle.baseVal.value = 10
> changed the angle's units.

OK, my memory of how this was meant to work is different from what our code does.  Do other browsers also leave the unit as is for SVGLength too?

Given this I think the spec needs fixing since https://svgwg.org/svg2-draft/types.html#__svg__SVGAngle__value says that assigning to value changes the internal value to a <number>, which means the unit will be SVG_ANGLETYPE_UNSPECIFIED.  Similarly for https://svgwg.org/svg2-draft/types.html#__svg__SVGLength__value.  (That is all text I wrote so maybe I just got it wrong.)

Comment 9

[Robert Longson [:longsonr]]

Attachment: testcase

Comment 10

[Robert Longson [:longsonr]]

Attachment: testcase

Safari, Firefox and Chrome all produce identical results with this testcase.

Comment 11

[Cameron McCormack (:heycam)]

Great.  Could you file an issue on the spec then to fix this up?  Thank you!

Comment 12

[Robert Longson [:longsonr]]

Raised https://github.com/w3c/svgwg/issues/478

Comment 13

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7160917a98e580371029559304cc18e8d7208ebb

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da2d9d2b562e
Set angle unit type in setOrientToAngle and to UNKNOWN when setting orient to auto or auto-start-reverse r=heycam

Comment 15

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/da2d9d2b562e