Closed Bug 1455854 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: SVG, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: longsonr, Assigned: longsonr)

References

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Summary: Return a TypeError rather than a SyntaxError for enum out of range → Throw a TypeError rather than a SyntaxError for enum out of range
Assignee: nobody → longsonr
Attached patch 1455854.txt (obsolete) — Splinter Review
Seems good value for a two line change.
Attachment #8969896 - Flags: review?(cam)
Attachment #8969896 - Attachment is patch: true
Attached patch updated patch - post try server (obsolete) — Splinter Review
Attachment #8969896 - Attachment is obsolete: true
Attachment #8969896 - Flags: review?(cam)
Attachment #8969949 - Flags: review?(cam)
Attachment #8969949 - Attachment is obsolete: true
Attachment #8969949 - Flags: review?(cam)
Attachment #8969953 - Flags: review?(cam)
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?
Flags: needinfo?(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
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 on attachment 8969953 [details] [diff] [review]
one more spot required for marker orient

Cancelling review awaiting the issue to be discussed in the WG.
Attachment #8969953 - Flags: review?(cam)
Priority: -- → P3
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.
Attachment #8969953 - Flags: review?(cam)
(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 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.)
Attachment #8969953 - Flags: review?(cam) → review+
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f50366fc815
Throw a TypeError rather than a SyntaxError for enum out of range. r=heycam
https://hg.mozilla.org/mozilla-central/rev/9f50366fc815
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.