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.

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

Do we really need not to reflect additional enum values in SVG 2 #424

Open
longsonr opened this issue Apr 23, 2018 · 15 comments
Open

Do we really need not to reflect additional enum values in SVG 2 #424

longsonr opened this issue Apr 23, 2018 · 15 comments

Comments

@longsonr
Copy link

Looks like Firefox forgot to do this in bug https://wpt.fyi/svg/types/scripted/SVGAnimatedEnumeration-SVGFEBlendElement.html although it does not expose the additional values. Nobody seems to have noticed except me.

Implementers have to record any new values for enums somewhere so we need to write logic along the lines of if the enum value is > the maximum SVG 1.1 value and someone calls baseValue or animValue return 0. Then we need some kind of baseValueInternal implementation so we can work with the real value internally. Why not just add the new values to the webidl and reflect everything? It's simpler to implement. There's not that many changes, feBlend's additional modes, marker auto-start-reverse and feComposite's operator.

@dirkschulze
Copy link
Contributor

@longsonr There is probably not much existing code that actual makes use of enumerations for FIlters. So we might be able to use WebIDL enums in filters and maybe even get away with animVal and baseVal all together for filters.
I remember seeing a lot of code using SVG enumerations with SVG DOM in general though. But maybe not that much anymore. So is your request to change SVGAnimatedEnumeration in general or just the filters code?
If just the filters code, what do you think about removing baseVal and animVal in general? If you want to change it in general, how can we make this change interoperable with existing JS code?

@longsonr
Copy link
Author

longsonr commented Apr 23, 2018

My request is to change SVGMarkerElement.orient so that setting baseVal = 3 gives you auto-start-reverse and SVGFEBlendElement.mode so that all the new values from the filters specifications can be set as numeric values and SVGFECompositeElement.operator so that 7 sets lighter. With appropriate numeric additions to the webidl files too.

I'm not proposing here to make any changes to baseVal/animVal for filters.

@dirkschulze
Copy link
Contributor

@longsonr Oh I am sorry. Now I got it. You want the new enumeration values we added to the 2 enums to be reflected in SVG DOM as well. Currently they are not.

There was a WG decision long ago that we were not going to extend SVGDOM even for enumeration values. That is why they weren't added. This is a way to discourage the use of SVGDOM I suppose. A change would need:

  1. Implementers interest,
  2. A new WG resolution

You added a test for feBlend that implies that Blink added those new enums to SVGDOM for feBlend. Though it is unclear from the test which value feBlend would have been set to. So needs testing if Blink does really support the new enumerations or simply accepts all values you throw at it.
Is there an example for SVGMarkerElement.orient as well? Or wasn't it implemented anywhere yet?

Is there implementation interest from Mozilla to add new enumeration values to SVGDOM?

@longsonr
Copy link
Author

longsonr commented Apr 24, 2018

Blink does not accept new values for feBlend/feComposite, Firefox does. Neither Blink, nor Firefox accept additional values for marker orient.

Basically someone wrote the additional logic in Firefox to ban additional values for marker orient but that was not done elsewhere when additional values were introduced for other enums. Looks like that was implemented by Blink though. Not sure any other UA actually supports additional values at all.

When faced with writing additional code to check enum ranges for all enums I'm wondering if its really worth the hassle.

@dirkschulze
Copy link
Contributor

@longsonr agree. As long as other implementations add it as well, I am fine to add it to Filter Effects. Probably the same applies for SVGMarkerElement.orient.

@dstorey
Copy link
Member

dstorey commented May 2, 2018

I support @longsonr's suggestion here (as long as we get Wpt tests with the spec change)

@longsonr
Copy link
Author

longsonr commented May 6, 2018

@dstorey there are existing tests. Firefox fails them which is why I noticed this.

@css-meeting-bot
Copy link
Member

The Working Group just discussed Do we really need not to reflect additional enum values in SVG 2, and agreed to the following:

  • RESOLVED: Where SVG 2 has added a new attribute value for an attribute that has an existing DOM enumeration, we will add matching new enumeration values
The full IRC log of that discussion <BogdanBrinza> topic: Do we really need not to reflect additional enum values in SVG 2
<BogdanBrinza> GitHub: https://github.com//issues/424
<AmeliaBR> Bogdan: We discussed this briefly on the last calll, but deferred a resolution
<AmeliaBR> Amelia: I think, if we aren't going to add new values for new attributes, then we should just deprecate the entire enum, because it's rather confusing as is.
<AmeliaBR> Dirk: For WebKit, I think I had to add special cases to test whether a value was in the SVG 1.1 set or a new value.
<AmeliaBR> Amelia: So it's possible that by not adding the new values, we just complicate implementations anyway?
<AmeliaBR> Bogdan: I agree with David's point on the issue.
<AmeliaBR> Amelia: Meaning, you agree with Robert Longson's original proposal, that we SHOULD expose the new attribute values through the DOM?
<AmeliaBR> David: Yes, and it looks like Firefox already has a bug for exposing a new value?
<AmeliaBR> Amelia: But what new value are they exposing, if it's never defined as an enum constant?
<AmeliaBR> Dirk: But internally they need a value, and it's probably just an ordered int. So we can follow Firefox.
<AmeliaBR> Amelia: Well, it looks like we have agreement between implementers. And for authors it would definitely be an improvement.
<AmeliaBR> ... Do we know all the places this is an issue?
<AmeliaBR> Dirk: There are two places, for markers and for filter blend mode.
<AmeliaBR> ... I'm not sure if both are even implemented.
<AmeliaBR> Amelia: Both are existing attributes, existing enums, with new attribute values but not yet new enum values.
<AmeliaBR> ... I'd assume we want both to be consistent.
<AmeliaBR> RESOLVED: Where SVG 2 has added a new attribute value for an attribute that has an existing DOM enumeration, we will add matching new enumeration values

@dstorey
Copy link
Member

dstorey commented May 7, 2018

RE SVGFEBlendElement, Firefox includes the following consts that are not in the spec:

  const unsigned short SVG_FEBLEND_MODE_OVERLAY = 6;
  const unsigned short SVG_FEBLEND_MODE_COLOR_DODGE = 7;
  const unsigned short SVG_FEBLEND_MODE_COLOR_BURN = 8;
  const unsigned short SVG_FEBLEND_MODE_HARD_LIGHT = 9;
  const unsigned short SVG_FEBLEND_MODE_SOFT_LIGHT = 10;
  const unsigned short SVG_FEBLEND_MODE_DIFFERENCE = 11;
  const unsigned short SVG_FEBLEND_MODE_EXCLUSION = 12;
  const unsigned short SVG_FEBLEND_MODE_HUE = 13;
  const unsigned short SVG_FEBLEND_MODE_SATURATION = 14;
  const unsigned short SVG_FEBLEND_MODE_COLOR = 15;
  const unsigned short SVG_FEBLEND_MODE_LUMINOSITY = 16;

Edge matches these. Chrome doesn't include them.

@BigBadaboom
Copy link
Contributor

BigBadaboom commented May 8, 2018

What about SVGLength.SVG_LENGTHTYPE_XXX constants for the new unit types (vw etc)? Should they also be considered for inclusion now ?

@dstorey
Copy link
Member

dstorey commented May 8, 2018

Edge has:
const unsigned short SVG_LENGTHTYPE_UNKNOWN = 0;
const unsigned short SVG_LENGTHTYPE_NUMBER = 1;
const unsigned short SVG_LENGTHTYPE_PERCENTAGE = 2;
const unsigned short SVG_LENGTHTYPE_EMS = 3;
const unsigned short SVG_LENGTHTYPE_EXS = 4;
const unsigned short SVG_LENGTHTYPE_PX = 5;
const unsigned short SVG_LENGTHTYPE_CM = 6;
const unsigned short SVG_LENGTHTYPE_MM = 7;
const unsigned short SVG_LENGTHTYPE_IN = 8;
const unsigned short SVG_LENGTHTYPE_PT = 9;
const unsigned short SVG_LENGTHTYPE_PC = 10;

it doesn't have vw etc.

@boggydigital
Copy link
Contributor

Not blocking updated 2.0 CR publication - assigning 2.0 Recommendation milestone to clean this up before 2.0 REC

@css-meeting-bot
Copy link
Member

The Working Group just discussed Enum values, and agreed to the following:

  • RESOLVED: Do not extend the SVG_LENGTHTYPE_* and SVG_ANGLETYPE_* constants & enumeration for new CSS units.
  • RESOLVED: If a length/angle was declared using a unit not in the enum, it should be exposed as type UNKNOWN.
The full IRC log of that discussion <AmeliaBR> Topic: Enum values
<AmeliaBR> Github: https://github.com//issues/424
<AmeliaBR> Dirk: We agreed to add new enums for things like marker orientation and blend modes, where we have new attribute values.
<AmeliaBR> ... But then a question was raised about things like SVGLength unit types, do we want to support the new CSS units.
<AmeliaBR> ... David commented that Edge does not have these, and I know that WebKit doesn't. I added a few hacks to work around that.
<AmeliaBR> ... So do we want to add things? CSS could add more units, hard to keep up.
<AmeliaBR> Amelia: I think we should make sure there is clear behavior for units that don't have a reflected value.
<AmeliaBR> Dirk: Agree, but the first question is should we try to extend the list.
<AmeliaBR> Amelia: I don't think so. Too difficult to keep up with new units, and CSS Typed OM should eventually replace it anyway.
<AmeliaBR> Proposed RESOLVED: Do not extend the SVG_LENGTHTYPE_* and SVG_ANGLETYPE_* constants & enumeration for new CSS units.
<AmeliaBR> RESOLVED: Do not extend the SVG_LENGTHTYPE_* and SVG_ANGLETYPE_* constants & enumeration for new CSS units.
<AmeliaBR> Dirk: Can we add a link to CSS Typed OM in the spec?
<plh> --> https://www.w3.org/standards/history/css-typed-om-1 CSS Typed OM
<AmeliaBR> Amelia: A note is easy. Won't automatically apply for all SVG attributes, but will apply for those that have been upgraded to CSS properties.
<AmeliaBR> Dirk: Other attributes won't use CSS properties, anyway.
<AmeliaBR> Amelia: Not necessarily. We sometimes use CSS syntax for attribute values that aren't CSS properties.
<AmeliaBR> Dirk: Probably need to reconsider that. Not sure if browsers use them.
<AmeliaBR> Amelia: Yes, last I checked browsers are very wonky about new units (and calc) in SVG attributes, including ones that *do* map to CSS properties.
<AmeliaBR> Dirk: I'd prefer to have a clearer distinction between presentation attributes & parsing vs others.
<AmeliaBR> Amelia: Either way, we still need guidelines on what to do with the legacy DOM interfaces when new units are used.
<AmeliaBR> Dirk: Looking at the enum value, there is an "UNKNOWN" value, so that makes sense to use.
<AmeliaBR> Amelia: Yes, but not sure how that affects the rest of the interface & math methods. Can we still convert it to the px value or other units?
<AmeliaBR> Dirk: Need to check the existing spec & implementations. Probably make it able to convert to the simple number value, but not the rest.
<AmeliaBR> Tav: If it's a lot of work to *not* update the list, would it be easier to just update it?
<AmeliaBR> Dirk: Either way, with new units being proposed, we'd still need to define what happens.
<AmeliaBR> Amelia: I do agree that the focus should be to keep it as simple as possible. Unknown unit, but still make the simple user-unit value still returns a valid number.
<AmeliaBR> ... Either way, should be a separate issue to discuss the details.
<AmeliaBR> Proposed: If a length/angle was declared using a unit not in the enum, it should be exposed as type UNKNOWN.
<AmeliaBR> RESOLVED: If a length/angle was declared using a unit not in the enum, it should be exposed as type UNKNOWN.
<plh> Next meeting is August 13

@fsoder
Copy link

fsoder commented Jan 7, 2019

It looks like the edits [1] for SVGFECompositeElement (IDL constant for 'lighter') and SVGMarkerElement.orient ('auto-start-reverse') has yet to be made.

[1] Which, from my interpretation of the resolution in #424 (comment), should('ve) be(en) made.

@fsoder
Copy link

fsoder commented Jan 8, 2020

Still waiting on those edits (see #424 (comment))

moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 6, 2021
Spec: https://www.w3.org/TR/filter-effects-1/#attr-valuedef-operator-lighter

Lighter is defined in the compositing & blending spec as equivalent to porter-duff plus, https://www.w3.org/TR/compositing-1/#porterduffcompositingoperators_plus.

The 'lighter' composite mode already works in <canvas> (via globalCompositeOperation), and it's the same there.

Chrome implemented support in https://bugs.chromium.org/p/chromium/issues/detail?id=439037

Per w3c/svgwg#424 we should expose the new value in webidl and Chrome uses 7 too (https://codereview.chromium.org/779963002/patch/120001/130009)

Differential Revision: https://phabricator.services.mozilla.com/D100605

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1518099
gecko-commit: 12325181f43aee1257922717f8ae4d52e4e502d6
gecko-reviewers: jrmuizel, emilio
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jan 6, 2021
…zel,emilio

Spec: https://www.w3.org/TR/filter-effects-1/#attr-valuedef-operator-lighter

Lighter is defined in the compositing & blending spec as equivalent to porter-duff plus, https://www.w3.org/TR/compositing-1/#porterduffcompositingoperators_plus.

The 'lighter' composite mode already works in <canvas> (via globalCompositeOperation), and it's the same there.

Chrome implemented support in https://bugs.chromium.org/p/chromium/issues/detail?id=439037

Per w3c/svgwg#424 we should expose the new value in webidl and Chrome uses 7 too (https://codereview.chromium.org/779963002/patch/120001/130009)

Differential Revision: https://phabricator.services.mozilla.com/D100605
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 6, 2021
Spec: https://www.w3.org/TR/filter-effects-1/#attr-valuedef-operator-lighter

Lighter is defined in the compositing & blending spec as equivalent to porter-duff plus, https://www.w3.org/TR/compositing-1/#porterduffcompositingoperators_plus.

The 'lighter' composite mode already works in <canvas> (via globalCompositeOperation), and it's the same there.

Chrome implemented support in https://bugs.chromium.org/p/chromium/issues/detail?id=439037

Per w3c/svgwg#424 we should expose the new value in webidl and Chrome uses 7 too (https://codereview.chromium.org/779963002/patch/120001/130009)

Differential Revision: https://phabricator.services.mozilla.com/D100605

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1518099
gecko-commit: 12325181f43aee1257922717f8ae4d52e4e502d6
gecko-reviewers: jrmuizel, emilio
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 8, 2021
Per w3c/svgwg#424 we should expose the new value in webidl

Differential Revision: https://phabricator.services.mozilla.com/D101089

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1685543
gecko-commit: d7cb06b275cdb59f99aa6ff14e5f6de6ff200723
gecko-reviewers: emilio
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jan 8, 2021
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 8, 2021
Per w3c/svgwg#424 we should expose the new value in webidl

Differential Revision: https://phabricator.services.mozilla.com/D101089

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1685543
gecko-commit: d7cb06b275cdb59f99aa6ff14e5f6de6ff200723
gecko-reviewers: emilio
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Jan 11, 2021
…zel,emilio

Spec: https://www.w3.org/TR/filter-effects-1/#attr-valuedef-operator-lighter

Lighter is defined in the compositing & blending spec as equivalent to porter-duff plus, https://www.w3.org/TR/compositing-1/#porterduffcompositingoperators_plus.

The 'lighter' composite mode already works in <canvas> (via globalCompositeOperation), and it's the same there.

Chrome implemented support in https://bugs.chromium.org/p/chromium/issues/detail?id=439037

Per w3c/svgwg#424 we should expose the new value in webidl and Chrome uses 7 too (https://codereview.chromium.org/779963002/patch/120001/130009)

Differential Revision: https://phabricator.services.mozilla.com/D100605

UltraBlame original commit: 12325181f43aee1257922717f8ae4d52e4e502d6
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Jan 11, 2021
Per w3c/svgwg#424 we should expose the new value in webidl

Differential Revision: https://phabricator.services.mozilla.com/D101089

UltraBlame original commit: d7cb06b275cdb59f99aa6ff14e5f6de6ff200723
sidvishnoi pushed a commit to sidvishnoi/gecko-webmonetization that referenced this issue Jan 12, 2021
…zel,emilio

Spec: https://www.w3.org/TR/filter-effects-1/#attr-valuedef-operator-lighter

Lighter is defined in the compositing & blending spec as equivalent to porter-duff plus, https://www.w3.org/TR/compositing-1/#porterduffcompositingoperators_plus.

The 'lighter' composite mode already works in <canvas> (via globalCompositeOperation), and it's the same there.

Chrome implemented support in https://bugs.chromium.org/p/chromium/issues/detail?id=439037

Per w3c/svgwg#424 we should expose the new value in webidl and Chrome uses 7 too (https://codereview.chromium.org/779963002/patch/120001/130009)

Differential Revision: https://phabricator.services.mozilla.com/D100605
sidvishnoi pushed a commit to sidvishnoi/gecko-webmonetization that referenced this issue Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants