1241898 - Why is SVGUnitTypes not [NoInterfaceObject]?

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Boris Zbarsky [:bzbarsky]]

It is in the spec.  Can we make it so?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Spec == https://svgwg.org/svg2-draft/idl.html

Comment 2

[Robert Longson [:longsonr]]

Attachment: patch

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/666bd7d68521
Make SVGUnitTypes [NoInterfaceObject]. r=peterv

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for reftest and mochitest failures:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3cb6a5ae7a09615c7fd8fa94324271cd4b968068

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=666bd7d685219795a95f7cacfb260f1b4a56981a
Reftest failure: https://treeherder.mozilla.org/logviewer.html#?job_id=34154859&repo=mozilla-inbound
Mochitest failure: https://treeherder.mozilla.org/logviewer.html#?job_id=34154777&repo=mozilla-inbound

Comment 5

[Robert Longson [:longsonr]]

Cameron, is it intended that SVG 2 no longer support

clip1.clipPathUnits.baseVal = SVGUnitTypes.SVG_UNIT_TYPE_USERSPACEONUSE;

or anything from

https://dxr.mozilla.org/mozilla-central/source/dom/svg/test/test_SVGUnitTypes.html?q=test_SVGUnitTypes.html&redirect_type=direct

Obviously I can remove the mochitest and rewrite the reftest to avoid such usage if that's the way forward.

Comment 6

[Cameron McCormack (:heycam)]

Hmm, so that was a change I made in the spec when fixing the IDL to remove multiple inheritance during the conversion to Web IDL.  For "mixin" interfaces like this, I made them [NoInterfaceObject].  But you are right that previously (if you interpreted the old IDL in a sensible way) this exposed the constants on SVGUnitTypes.  clip1.clippathUnits.baseVal = SVGClipPathElement.SVG_UNIT_TYPE_USERSPACEONUSE still works of course.

I see that all other browsers still expose SVGUnitTypes, so I'm wondering now if it's a mistake to remove this, at least without confirming first that nobody uses them.  (I don't see an entry for SVGUnitTypes in chromestatus.com so perhaps we should add a use counter ourselves.)  WDYT?

Comment 7

[Robert Longson [:longsonr]]

I think we should wontfix this and ensure that all IDL that has constants defined in is not [NoInterfaceObject].

Comment 8

[Robert Longson [:longsonr]]

https://lists.w3.org/Archives/Public/www-svg/2016Aug/0053.html

Comment 9

[Boris Zbarsky [:bzbarsky]]

If we want to keep the constants on SVGUnitTypes, I think the IDL here should look like this:

1)  An SVGUnitTypes interface that is empty but not [NoInterfaceObject].
2)  Some mixin, name not relevant, which is therefore [NoInterfaceObject]
    and is mixed into SVGUnitTypes and the other interfaces that SVGUnitTypes
    is currently mixed into.

The only observable difference from our current behavior, I believe, would be that "element instanceof SVGUnitTypes" no longer tests true for any elements.  But does it test true in any other browser?  http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4409 reports false in Chrome and Safari, and throws a "Function does not have a valid prototype object" exception in Edge.  For posterity, that testcase is:

  var el = document.createElementNS("http://www.w3.org/2000/svg", "linearGradient");
  w(el instanceof SVGUnitTypes);

This would remove one of the very few cases where we still have confusion between interfaces and mixins and pave the way towards having them be distinct concepts in IDL...

Comment 10

[Robert Longson [:longsonr]]

Attachment: both changes in one patch

That seems much more sensible. Can we get the SVG specification updated along these lines Cam?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Note that this leaves the open question of what SVGUnitTypes.prototype should be, but note that this is already not interoperable between Edge and Chrome/Safari.  I think my suggestion above would basically align us with the Chrome/Safari behavior.

Comment 12

[Robert Longson [:longsonr]]

Attachment: unittypes.txt

Comment 13

[Cameron McCormack (:heycam)]

I'm not actively involved in SVG spec work lately, but you can raise an issue here: https://github.com/w3c/svgwg/issues/new

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8790046 [details] [diff] [review]
unittypes.txt

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

r=me with that, and of course a DOM peer's review is required for the .webidl changes.  Proposing those specific IDL changes in the GitHub issue you raise would be good, thanks.

::: dom/svg/test/mochitest.ini
@@ -83,5 @@
>  [test_SVGStringList.xhtml]
>  [test_SVGStyleElement.xhtml]
>  [test_SVGTransformListAddition.xhtml]
>  [test_SVGTransformList.xhtml]
> -[test_SVGUnitTypes.html]

Rather than delete this test, can you just adjust it so that it checks that one of the unit type constants is visible on these elements?

Comment 15

[Robert Longson [:longsonr]]

Attachment: unittypes.txt

only the webidl changes need yuor review Peter

Comment 16

[Peter Van der Beken [:peterv]]

Comment on attachment 8799227 [details] [diff] [review]
unittypes.txt

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

Please don't forget to open an issue with the WG.

::: dom/webidl/moz.build
@@ +550,5 @@
>      'SVGURIReference.webidl',
>      'SVGUseElement.webidl',
>      'SVGViewElement.webidl',
>      'SVGZoomAndPan.webidl',
> +    'SVGZoomAndPanValues.webidl',

This doesn't belong here, right? But you'll deal with that when merging I guess :-).

Comment 17

[Robert Longson [:longsonr]]

https://github.com/w3c/svgwg/issues/291

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8cc44c79c30
Make SVGUnitTypes [NoInterfaceObject]. r=cam r=peterv

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for build bustage:

WebIDL.WebIDLError: error: Unresolved type '<unresolved scope>::SVGUnitTypesValues'., /builds/slave/m-in-lx-0000000000000000000000/build/src/dom/webidl/SVGUnitTypes.webidl line 16:24

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a8cc44c79c30946426314285c6f63c0f4d2b0295
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=38093736&repo=mozilla-inbound

WebIDL.WebIDLError: error: Unresolved type '<unresolved scope>::SVGUnitTypesValues'., /builds/slave/m-in-lx-0000000000000000000000/build/src/dom/webidl/SVGUnitTypes.webidl line 16:24

Comment 20

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4616f76f25ec
Backed out changeset a8cc44c79c30 for build bustage. r=backout DONTBUILD

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c73e584c4071
Make SVGUnitTypes [NoInterfaceObject]. r=cam r=peterv

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9adfea13296e
Fix SVGFilteElement typo in test_SVGUnitTypes.html.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c73e584c4071
https://hg.mozilla.org/mozilla-central/rev/9adfea13296e