1455854 - Throw a TypeError rather than a SyntaxError for enum out of range

Status: RESOLVED FIXED

(Core :: SVG, enhancement, P3)

Comment 1

[Robert Longson [:longsonr]]

Attachment: 1455854.txt

Seems good value for a two line change.

Comment 2

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4849be361f0b5878b135c0aaebc321196fb00eff

Comment 3

[Robert Longson [:longsonr]]

Attachment: updated patch - post try server

Comment 4

[Robert Longson [:longsonr]]

Attachment: one more spot required for marker orient

Comment 5

[Cameron McCormack (:heycam)]

The specification says that invalid values assigned to SVGAnimatedEnumeration.baseVal should be handled by setting the reflected DOM attribute to the emtpy string, rather than throwing any kind of exception.

https://svgwg.org/svg2-draft/types.html#__svg__SVGAnimatedEnumeration__baseVal

Can you verify that throwing an exception (and throwing TypeError) is what other implementations do, and if so, file a spec issue on this?

Comment 6

[Robert Longson [:longsonr]]

This isn't what this is for. This is for setAttribute("foo") on an enum, which should set the enum in the DOM to UNKNOWN.

Comment 7

[Robert Longson [:longsonr]]

Oops sorry, wrong bug.

Chrome does this and web platform tests check that. Chrome passes everyone else fails.

Edge does not throw, we throw a SyntaxError, Chrome throws a TypeError and Safari some kind of SVG_INVALID_VALUE_ERR

Comment 8

[Robert Longson [:longsonr]]

Here's an example https://wpt.fyi/svg/types/scripted/SVGAnimatedEnumeration-SVGClipPathElement.html

Comment 9

[Cameron McCormack (:heycam)]

OK, then I think it would good to clarify with the WG that the current spec text is what they want everyone to move towards, or if it's something they haven't considered.  I don't have an opinion on whether throwing or not is better.  But if you could file an issue pointing out the discrepancy between the spec text and the WPT tests to find out what we should do, that would be great.

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8969953 [details] [diff] [review]
one more spot required for marker orient

Cancelling review awaiting the issue to be discussed in the WG.

Comment 11

[Robert Longson [:longsonr]]

Comment on attachment 8969953 [details] [diff] [review]
one more spot required for marker orient

Looks like the resolution in https://github.com/w3c/svgwg/issues/423 was to throw a TypeError. That's consistent with WPT, chrome and this patch.

Comment 12

[Cameron McCormack (:heycam)]

(In reply to Robert Longson [:longsonr] from comment #11)
> Looks like the resolution in https://github.com/w3c/svgwg/issues/423 was to
> throw a TypeError. That's consistent with WPT, chrome and this patch.

Great, thanks for following up with the WG.

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8969953 [details] [diff] [review]
one more spot required for marker orient

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

::: dom/svg/nsSVGEnum.cpp
@@ +56,3 @@
>  
>    // only a warning since authors may mistype attribute values
>    NS_WARNING("unknown enumeration key");

(FWIW I don't think we should emit a warning just because content uses an invalid value.)

Comment 14

[Robert Longson [:longsonr]]

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f50366fc815
Throw a TypeError rather than a SyntaxError for enum out of range. r=heycam

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/9f50366fc815