Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Should setting an enum IDL to an invalid value throw an error? #547

Closed
AmeliaBR opened this issue Sep 20, 2018 · 3 comments · Fixed by #641
Closed

Should setting an enum IDL to an invalid value throw an error? #547

AmeliaBR opened this issue Sep 20, 2018 · 3 comments · Fixed by #641
Assignees
@AmeliaBR
Labels
Basic Data Types and Interfaces chapter Needs editing Needs tests SVG Core
Milestone
SVG 2.0 Proposed ...

Comments

@AmeliaBR
Copy link
Contributor

SVG DOM has many IDL properties represented as short-integer enumerations with matching sets of constants. All the enumeration constant sets define a XXX_UNKNOWN = 0 value.

The general rule for setting an invalid value via the DOM (from the old SVGDOM appendix) is:

If a script sets a DOM attribute to an invalid value (e.g., a negative number for an attribute that requires a non-negative number or an out-of-range value for an enumeration), unless this specification indicates otherwise, no exception shall be raised on setting, but the given document fragment shall become technically in error as described in Error processing.

The detailed setter algorithm for SVGAnimatedEnumeration, in SVG 2, also says:

If value is 0 or is not the numeric type value for any value of the reflected attribute, then set the reflected attribute to the empty string.

However, to confuse matters further, the setting algorithm for SVGPreserveAspectRatio (which encompasses two separate constants, so doesn't use SVGAnimatedEnumeration), says that setting to an invalid value or UNKNOWN should fail silently without changing anything.

Actual results, based on this test case: https://codepen.io/AmeliaBR/pen/0d981477f3832275f7313cbb1caacd9d/
(you'll need to check the real console, the codepen console doesn't show errors)

Blink (Chrome 69), WebKit (Safari 11), and Gecko (Firefox 63) all currently throw errors when you set an enum (SVGAnimatedEnumeration or SVGPreserveAspectRatio) to a value not in the enum OR to 0.

Edge (EdgeHTML 17) silently ignores the SVGAnimatedEnumeration set, but errors out for SVGPreserveAspectRatio (the opposite of the spec).

Edge also silently ignores setting a length attribute to an invalid negative value, while the other browsers all pass it through to the attribute (which is what causes the pattern to disappear, since it's now in error for rendering).

So…

Should we match the majority behavior here?

Relevant context: in #393, we agreed to add new enum values for new attribute values, so being able to catch an error could be a useful way of detecting support. But then, so could reading back the value to see if it was set correctly.
@birtles
Copy link
Contributor

birtles commented Sep 20, 2018

In WebIDL they throw: https://heycam.github.io/webidl/#es-enumeration

@AmeliaBR AmeliaBR mentioned this issue Sep 20, 2018
Closed
AmeliaBR added a commit that referenced this issue Sep 20, 2018
@AmeliaBR
More updating of SVGDOM appendix
cc48d1a 
Section "Elements in the SVG DOM":

- Add a preamble paragraph to clarify normative requirements.
- Make it clear that the rule applies to SVG-namespaced elements
  defined in other specifications (e.g., filters, masking).
- Add a requirement for handling unknown elements.
  (Should implement SVGUnknownElement;
  must implement SVGElement, since that's what browsers do.)
- Add a warning about SVGUnknownElement being at-risk.
- Mark up the "for example" as a proper example.

Section "Naming Conventions":

- Make the naming conventions section reference modern DOM + HTML,
  and remove the informative reference to DOM Level 1.
- Add a paragraph on the naming conventions of interfaces.
- Remove mention of (long-since-removed) CDATA type;
  Case preservation when reflecting strings is already covered
  by the rules in HTML about reflecting basic types.
  (and either way, that's not part of Naming Conventions!)

Section "Invalid Values":

- Clarify that this applies to reflected attributes
  & validity restrictions on the content attributes.
- Add a link to Github Issue #547, re matching reality.
@AmeliaBR
Copy link
Contributor Author

In WebIDL they throw

Relevant context. But WebIDL enumerations are somewhat different from SVG's legacy enumerations, which are actually short values in the IDL definitions.

svgeesus pushed a commit that referenced this issue Sep 23, 2018
@AmeliaBR @svgeesus
No normative appendices! (#548)
7abc5d5 
* Move normative parts of implnote.html to chapters

Section "Error Processing":

- moved to the end of the "Document Conformance Classes"
  (in conform.html)
- passive-voiced "shall" commands rewritten
  as requirements on user agents
- the example of a general error rewritten to refer
  to the document conformance requirements
- it's made explicit that these general error processing
  rules can be overridden by more specific rules
  elsewhere in the spec.

Section "Clamping values which are restricted to a particular range":

- moved to types.html (Basic Data Types and Interfaces chapter)
- paragraphs re color and opacity removed,
  since clamping behavior of those properties is defined in CSS Color
- remaining paragraph generalized, with clarifications that
  * it only applies if more specific rules aren't given elsewhere
  * it applies to clamping for device capabilities as well as per spec

Section "Elliptical Arc Implementation Notes":

- the normative section on out-of-range parameters
  is moved to the Paths chapter implementation notes.
- the syntax is updated to use parameter names defined in the chapter,
  not the mathematical notation defined in the appendix
- the remaining sections on arc parameterization and conversion
  are tidied up to stand alone in the appendix
- Equation numbering is made independent of
  Appendix/section numbering (which was no longer correct).

The rest of the "Implementation Notes" section is paths.html
is reorganized into logical sections.
The contradictory statement that
"A non-positive radius value is an error."
 is removed.
(Elsewhere, the spec says to use the absolute value of negative radii,
and most browser implementations match; see #324.)

Section "Text selection implementation notes":

- moved to the end of the "Text selection and clipboard operations"
  section (in text.html)

Section "Printing implementation notes":

- moved to the end of "Conforming SVG Viewers" section (in conform.html)

Cross-references to these sections have also been updated.

Changes appendix entries have been made.
Links to PR will need to be updated once a PR is generated.

* Update dependencies in SVG DOM appendix

- Clarify which software the "SVG DOM requires" statements apply to
  (viewers/interpreters with scripting support).
- Make DOM 4 a minimum requirement, with DOM living standard a "should".
  Note: W3C DOM 4 isn't currently listed in the refs.
  Would it be better to make the minimum requirement a timepoint
  in the DOM LS history?
- Remove requirement for support of obsolete DOM View spec.
- Update the requirement on DOM Level 2 Style to
  only include interfaces also defined in CSSOM,
  with full support for CSSOM as a "should"
  (CSSOM is still WD, but it drops a lot of DOM Style
  that was never implemented.
  Alternative would be to list which DOM2 Style interfaces to support.)
  DOM Level 2 is included as an informative reference
- Remove the "Relationship with DOM Level 2 CSS" section.
- Add Geometry Interfaces Module Level 1 as a normative ref,
  since it didn't seem to be included anywhere
  (despite being referenced in many WebIDL dependencies).

Also: fix some alphabetization issues in refs.html. Add changes entries.

* Reorganize SVGDOM content re initializing values

In prep for moving the content to the Types chapter.

- Separate out overview paragraphs on reflecting attributes
  from specifics on initializing objects to `(none)` initial values.
- Rename the section on initialization to clarify when it applies,
  and add new intro paragraph.
- Make it clear that the rules for initializing compound interfaces
  are not restricted to those defined in SVG.
  (Reintroduce example of DOMRect &
  include other numeric primitive numeric types in initialization list.)
- Remove the special rule about `SVGTextContentElement::textLength`
  in this section, and replace it with:
  * clearer text defining the initial value of the content attribute
  * a note in the IDL definition explaining the impact
    of the dynamic initial value
- Use proper example formatting for the "for example"
  paragraph, add links/clarification,
  and add a matching example of initializing a `(none)` value.

* More updating of SVGDOM appendix

Section "Elements in the SVG DOM":

- Add a preamble paragraph to clarify normative requirements.
- Make it clear that the rule applies to SVG-namespaced elements
  defined in other specifications (e.g., filters, masking).
- Add a requirement for handling unknown elements.
  (Should implement SVGUnknownElement;
  must implement SVGElement, since that's what browsers do.)
- Add a warning about SVGUnknownElement being at-risk.
- Mark up the "for example" as a proper example.

Section "Naming Conventions":

- Make the naming conventions section reference modern DOM + HTML,
  and remove the informative reference to DOM Level 1.
- Add a paragraph on the naming conventions of interfaces.
- Remove mention of (long-since-removed) CDATA type;
  Case preservation when reflecting strings is already covered
  by the rules in HTML about reflecting basic types.
  (and either way, that's not part of Naming Conventions!)

Section "Invalid Values":

- Clarify that this applies to reflected attributes
  & validity restrictions on the content attributes.
- Add a link to Github Issue #547, re matching reality.

* Update UI Events section in SVGDOM appendix

In preparation for merging into the interactivity chapter.

- Add a normative reference to the Clipboard API spec,
  since we're including event handlers for the events it defines.
- Add normative wording about which events need to be implemented
  (with exceptions for non interactive environments,
  and an exclusion for the legacy events defined in UI Events spec).
- Clarify that most onEvent attributes/properties
  are available on all elements in the SVG namespace.
- remove statements and example
  about onclick, etc. attributes
  only being valid on certain elements.
- Keep the general rule about event attributes not working
  where invalid
  (since it applies to & is implemented for onbegin, onend, etc.)
  But remove statements about addEventListener working,
  because those events don't support addEventListener,
  per the table in https://svgwg.org/svg2-draft/interact.html#SVGEvents
- Remove a cross-reference to the deleted paragraph from pservers.html
- Mark up the example as such.

* Move SVGDOM appendix content to chapters

And re-arrange some content to match.
With additional edits only when required for
merge conflicts with existing content.

In types.html:

- #SVGDOMOverview is now a major section in types.html,
  with the following subsections taken from svgdom.html and
  from the length intro to
  types.html#DOMInterfacesForReflectingSVGAttributes
  * Dependencies (new heading for content from svgdom.html)
  * Naming conventions (from svgdom.html)
  * Elements in the SVG DOM (from svgdom.html)
  * Reflecting content attributes in the DOM (merging intros,
    with a little extra effort to actually define the term "reflect")
  * Synchronizing reflected values
    (new heading on content from types.html)
  * Reflecting an empty initial value (from svgdom.html)
  * Invalid values (from svgdom.html)
- Without its intro section on what reflecting attributes means,
  the major section for DOM interfaces that reflect attributes
  is renamed to clarify these are the *Animated* interfaces,
  with a brief intro about the baseVal/animVal structure.
- The one non-animated interface in that section
  (SVGStringList) is moved up to the previous section.
- Elsewhere in types.html, the short sections on
  number precision and clamping are made sub-sections
  to the attribute syntax.

In interact.html:

- The "Relationship with UI Events" section is inserted as a sub-section
  to "Supported Events" except:
- The paragraphs about possible implementation of event attributes,
  and the example of their use, is added at the end of the
  "Event attributes" section.
- A mention of that section as being in another chapter is removed.

svgdom.html, with its content removed, is turned into a stub file
(and publish.xml updated accordingly)

Cross-references are updated in other files.

Changes section is updated
(including some updates that should have been on the last commit).
@css-meeting-bot
Copy link
Member

The SVG Working Group just discussed Should setting an enum IDL to an invalid value throw an error?, and agreed to the following:

  • RESOLUTION: Setting a enum-like DOM property to a value that doesn't represent a valid attribute value should throw a TypeError
The full IRC log of that discussion <AmeliaBR> Topic: Should setting an enum IDL to an invalid value throw an error?
<AmeliaBR> github: https://github.com//issues/547
<AmeliaBR> Amelia (describes issue)
<AmeliaBR> Chris: Blink, WebKit, Gecko throw errors: but what happens to the value?
<AmeliaBR> Amelia: It doesn't change.
<AmeliaBR> Chris: Well, that seems sensible.
<AmeliaBR> Amelia: I find it a bit confusing, as an author, to have an UNKNOWN constant that you can't use. But that's my only complaint.
<AmeliaBR> Chris: And we have three matching browsers.
<AmeliaBR> Amelia: And as Brian notes, it's consistent with behavior of WebIDL enum data type.
<AmeliaBR> Chris: So what needs to happen?
<AmeliaBR> Amelia: Need to find all relevant sections of the spec to update. I found three different bits of text.
<AmeliaBR> ... I can do that, eventually.
<AmeliaBR> RESOLUTION: Setting a enum-like DOM property to a value that doesn't represent a valid attribute value should throw a TypeError

@AmeliaBR AmeliaBR self-assigned this Sep 27, 2018
svgeesus pushed a commit that referenced this issue Sep 28, 2018
@AmeliaBR @svgeesus
Tidying up, post-"No normative appendices" (#550)
fea119b 
* Move normative parts of implnote.html to chapters

Section "Error Processing":

- moved to the end of the "Document Conformance Classes"
  (in conform.html)
- passive-voiced "shall" commands rewritten
  as requirements on user agents
- the example of a general error rewritten to refer
  to the document conformance requirements
- it's made explicit that these general error processing
  rules can be overridden by more specific rules
  elsewhere in the spec.

Section "Clamping values which are restricted to a particular range":

- moved to types.html (Basic Data Types and Interfaces chapter)
- paragraphs re color and opacity removed,
  since clamping behavior of those properties is defined in CSS Color
- remaining paragraph generalized, with clarifications that
  * it only applies if more specific rules aren't given elsewhere
  * it applies to clamping for device capabilities as well as per spec

Section "Elliptical Arc Implementation Notes":

- the normative section on out-of-range parameters
  is moved to the Paths chapter implementation notes.
- the syntax is updated to use parameter names defined in the chapter,
  not the mathematical notation defined in the appendix
- the remaining sections on arc parameterization and conversion
  are tidied up to stand alone in the appendix
- Equation numbering is made independent of
  Appendix/section numbering (which was no longer correct).

The rest of the "Implementation Notes" section is paths.html
is reorganized into logical sections.
The contradictory statement that
"A non-positive radius value is an error."
 is removed.
(Elsewhere, the spec says to use the absolute value of negative radii,
and most browser implementations match; see #324.)

Section "Text selection implementation notes":

- moved to the end of the "Text selection and clipboard operations"
  section (in text.html)

Section "Printing implementation notes":

- moved to the end of "Conforming SVG Viewers" section (in conform.html)

Cross-references to these sections have also been updated.

Changes appendix entries have been made.
Links to PR will need to be updated once a PR is generated.

* Update dependencies in SVG DOM appendix

- Clarify which software the "SVG DOM requires" statements apply to
  (viewers/interpreters with scripting support).
- Make DOM 4 a minimum requirement, with DOM living standard a "should".
  Note: W3C DOM 4 isn't currently listed in the refs.
  Would it be better to make the minimum requirement a timepoint
  in the DOM LS history?
- Remove requirement for support of obsolete DOM View spec.
- Update the requirement on DOM Level 2 Style to
  only include interfaces also defined in CSSOM,
  with full support for CSSOM as a "should"
  (CSSOM is still WD, but it drops a lot of DOM Style
  that was never implemented.
  Alternative would be to list which DOM2 Style interfaces to support.)
  DOM Level 2 is included as an informative reference
- Remove the "Relationship with DOM Level 2 CSS" section.
- Add Geometry Interfaces Module Level 1 as a normative ref,
  since it didn't seem to be included anywhere
  (despite being referenced in many WebIDL dependencies).

Also: fix some alphabetization issues in refs.html. Add changes entries.

* Reorganize SVGDOM content re initializing values

In prep for moving the content to the Types chapter.

- Separate out overview paragraphs on reflecting attributes
  from specifics on initializing objects to `(none)` initial values.
- Rename the section on initialization to clarify when it applies,
  and add new intro paragraph.
- Make it clear that the rules for initializing compound interfaces
  are not restricted to those defined in SVG.
  (Reintroduce example of DOMRect &
  include other numeric primitive numeric types in initialization list.)
- Remove the special rule about `SVGTextContentElement::textLength`
  in this section, and replace it with:
  * clearer text defining the initial value of the content attribute
  * a note in the IDL definition explaining the impact
    of the dynamic initial value
- Use proper example formatting for the "for example"
  paragraph, add links/clarification,
  and add a matching example of initializing a `(none)` value.

* More updating of SVGDOM appendix

Section "Elements in the SVG DOM":

- Add a preamble paragraph to clarify normative requirements.
- Make it clear that the rule applies to SVG-namespaced elements
  defined in other specifications (e.g., filters, masking).
- Add a requirement for handling unknown elements.
  (Should implement SVGUnknownElement;
  must implement SVGElement, since that's what browsers do.)
- Add a warning about SVGUnknownElement being at-risk.
- Mark up the "for example" as a proper example.

Section "Naming Conventions":

- Make the naming conventions section reference modern DOM + HTML,
  and remove the informative reference to DOM Level 1.
- Add a paragraph on the naming conventions of interfaces.
- Remove mention of (long-since-removed) CDATA type;
  Case preservation when reflecting strings is already covered
  by the rules in HTML about reflecting basic types.
  (and either way, that's not part of Naming Conventions!)

Section "Invalid Values":

- Clarify that this applies to reflected attributes
  & validity restrictions on the content attributes.
- Add a link to Github Issue #547, re matching reality.

* Update UI Events section in SVGDOM appendix

In preparation for merging into the interactivity chapter.

- Add a normative reference to the Clipboard API spec,
  since we're including event handlers for the events it defines.
- Add normative wording about which events need to be implemented
  (with exceptions for non interactive environments,
  and an exclusion for the legacy events defined in UI Events spec).
- Clarify that most onEvent attributes/properties
  are available on all elements in the SVG namespace.
- remove statements and example
  about onclick, etc. attributes
  only being valid on certain elements.
- Keep the general rule about event attributes not working
  where invalid
  (since it applies to & is implemented for onbegin, onend, etc.)
  But remove statements about addEventListener working,
  because those events don't support addEventListener,
  per the table in https://svgwg.org/svg2-draft/interact.html#SVGEvents
- Remove a cross-reference to the deleted paragraph from pservers.html
- Mark up the example as such.

* Move SVGDOM appendix content to chapters

And re-arrange some content to match.
With additional edits only when required for
merge conflicts with existing content.

In types.html:

- #SVGDOMOverview is now a major section in types.html,
  with the following subsections taken from svgdom.html and
  from the length intro to
  types.html#DOMInterfacesForReflectingSVGAttributes
  * Dependencies (new heading for content from svgdom.html)
  * Naming conventions (from svgdom.html)
  * Elements in the SVG DOM (from svgdom.html)
  * Reflecting content attributes in the DOM (merging intros,
    with a little extra effort to actually define the term "reflect")
  * Synchronizing reflected values
    (new heading on content from types.html)
  * Reflecting an empty initial value (from svgdom.html)
  * Invalid values (from svgdom.html)
- Without its intro section on what reflecting attributes means,
  the major section for DOM interfaces that reflect attributes
  is renamed to clarify these are the *Animated* interfaces,
  with a brief intro about the baseVal/animVal structure.
- The one non-animated interface in that section
  (SVGStringList) is moved up to the previous section.
- Elsewhere in types.html, the short sections on
  number precision and clamping are made sub-sections
  to the attribute syntax.

In interact.html:

- The "Relationship with UI Events" section is inserted as a sub-section
  to "Supported Events" except:
- The paragraphs about possible implementation of event attributes,
  and the example of their use, is added at the end of the
  "Event attributes" section.
- A mention of that section as being in another chapter is removed.

svgdom.html, with its content removed, is turned into a stub file
(and publish.xml updated accordingly)

Cross-references are updated in other files.

Changes section is updated
(including some updates that should have been on the last commit).

* Remove link to DOM 4 in required dependencies

Point to WHATWG DOM Review Drafts for the "must"
link to a stable specification as of the time of our publication,
and add an explicit exception for any removed/deprecated features.

The "should" requirement still points to the latest DOM LS.

As discussed with @svgeesus while reviewing PR #548:
#548 (comment)

* Update changes re PR #548

To add links to the PR & related issue #520.

Closes #520.
AmeliaBR added a commit to AmeliaBR/svgwg that referenced this issue Jan 31, 2019
@AmeliaBR
Enum-like DOM properties throw TypeErrors on setting to unknown values
0880730 
As resolved in w3c#547 (comment)

> RESOLUTION: Setting a enum-like DOM property to a value
> that doesn't represent a valid attribute value
> should throw a TypeError

This affects the setter algorithm for
SVGAnimatedEnumeration
and for SVGPreserveAspectRatio.

Clarifying changes made to:
-  the note regarding reserializing an SVGAnimatedEnumeration,
- the general statement about
silently ignoring invalid values.

Closes w3c#547.
@AmeliaBR AmeliaBR mentioned this issue Jan 31, 2019
Merged
AmeliaBR added a commit that referenced this issue Feb 11, 2019
@AmeliaBR
Enum-like DOM properties throw TypeErrors on setting to unknown values
f756827 
As resolved in #547 (comment)

> RESOLUTION: Setting a enum-like DOM property to a value
> that doesn't represent a valid attribute value
> should throw a TypeError

This affects the setter algorithm for
SVGAnimatedEnumeration
and for SVGPreserveAspectRatio.

Clarifying changes made to:
-  the note regarding reserializing an SVGAnimatedEnumeration,
- the general statement about
silently ignoring invalid values.

Closes #547.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@AmeliaBR AmeliaBR

Labels
Basic Data Types and Interfaces chapter Needs editing Needs tests SVG Core
Projects
None yet
Milestone
SVG 2.0 Proposed Recommendation
Development

Successfully merging a pull request may close this issue.

Enum-like DOM properties throw TypeErrors on setting to unknown values AmeliaBR/svgwg
4 participants
@birtles @dirkschulze @AmeliaBR @css-meeting-bot