w3c / svgwg

SVG Working Group specifications
Other
695 stars 131 forks source link

Values in SVGZoomAndPan and SVGUnitTypes should be accessible to javascript #291

Closed longsonr closed 4 years ago

longsonr commented 7 years ago

Please: The SVG 2 specification

Makes it impossible to write var x = ZoomAndPan.SVG_ZOOMANDPAN_UNKNOWN as ZoomAndPan is [NoInterfaceObject].

Similarly with SVGUnitTypes.

This is not what either Chrome or Firefox do (both support accessing the ZoomAndPan interface values directly). In fact Firefox has tests that contain SVGUnitTypes so it's certainly likely that there's also usage of this in the wild.

I propose changing the ZoomAndPan interface to

interface SVGZoomAndPan {
};

SVGZoomAndPan implements SVGZoomAndPanValues;

Then having this as SVGZoomAndPanValues

[NoInterfaceObject]
interface SVGZoomAndPanValues {

  // Zoom and Pan Types
  const unsigned short SVG_ZOOMANDPAN_UNKNOWN = 0;
  const unsigned short SVG_ZOOMANDPAN_DISABLE = 1;
  const unsigned short SVG_ZOOMANDPAN_MAGNIFY = 2;

  [SetterThrows]
  attribute unsigned short zoomAndPan;
};

And all existing users of ZoomAndPan i.e. SVGSVGElement and SVGViewElement would then implement SVGZoomAndPanValues

This would mean that an SVGSVGElement would not be an SVGZoomAndPan but you would be able to access its contents.

Similarly for SVGUnitTypes.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1241899 and https://bugzilla.mozilla.org/show_bug.cgi?id=1241898 for the Firefox implementation which is landing now. This will align Firefox with Chrome's implementation.

nikosandronikos commented 7 years ago

That seems reasonable to me - and has the approval of some other WG members I see.

For posterity, the reason for the extra [NoInterfaceObject] SVGZoomAndPanValues interface, is due to a desire to separate mixins and regular interfaces.

foolip commented 7 years ago

Chrome actually does not expose the SVGZoomAndPan interface, Chrome 28 was the last version to do so. Safari 10 does also not have it, but I can confirm that Edge 14 and Firefox 51 do.

SVGZoomAndPan.prototype is undefined both Edge and Firefox, does WebIDL say something to make it so? I can't find it, but it may be in there somewhere.

SVGUnitTypes is different in that it has only constants, and it seems universally exposed. How about just dropping x implements SVGUnitTypes everywhere?

longsonr commented 7 years ago

We'd have x implements SVGUnitTypeValue which would be a [NoInterfaceObject] and contain the constants. SVGUnitTypes would then contain nothing but also implement SVGUnitTypeValues. https://bugzilla.mozilla.org/show_bug.cgi?id=1241898 has more details.

foolip commented 7 years ago

Context: I was pointed here by @fsoder in https://codereview.chromium.org/2415413002/

Here's a test for whether x implements SVGUnitTypes is already supported: https://software.hixie.ch/utilities/js/live-dom-viewer/saved/4581

Edge 14 and Firefox 51 have the constants everywhere. Chrome 55 and Safari 10 don't have them.

A search for "SVG_UNITTYPE" in httparchive:har.2016_09_15_chrome_requests_bodies finds 66 results. 64 of them are from api-maps.yandex.ru and the other two are https://boxy-svg.com/api/frontend/modules.js and http://www.javascripture.com/javascripture.js. All of the are using the SVGUnitTypes object, and so don't depend on x implements SVGUnitTypes.

@longsonr, given the lack of interop and seemingly low usage in the wild, would you be willing to drop x implements SVGUnitTypes from Gecko? To me it doesn't seem to add anything if SVGUnitTypes itself is exposed.

longsonr commented 7 years ago

@foolip I think we'll land https://bugzilla.mozilla.org/show_bug.cgi?id=1241898 as is and then think about removing the implements later. I.e. one step at a time. Feel free to add another bug in bugzilla to track that idea though.

nikosandronikos commented 7 years ago

Feel free to add another bug in bugzilla to track that idea though.

And file a separate issue here too please.

foolip commented 7 years ago

The questions of NoInterfaceObject and what implements what are connected, so for the spec discussion I couldn't formulate an orthogonal or even follow-up issue. Concretely, my suggested fixes for SVGZoomAndPan and SVGUnitTypes would be:

longsonr commented 7 years ago

Making SVGZoomAndPan [NoInterfaceObject] will mean that you can't use its constants directly

Dropping SVGUnitTypes means that you can only use its constants directly.

That would seem to be massively confusing and inconsistent to me.

foolip commented 7 years ago

Making SVGZoomAndPan [NoInterfaceObject] will mean that you can't use its constants directly

The constants would be on SVGSVGElement and SVGViewElement together with the zoomAndPan attribute itself, doesn't that suffice?

Dropping SVGUnitTypes means that you can only use its constants directly.

The suggestion is to keep it, so that the kind of code found in https://github.com/w3c/svgwg/issues/291#issuecomment-254442089 keeps working.

nikosandronikos commented 7 years ago

Leave SVGZoomAndPan as a mixin interface, with NoInterfaceObject and a bunch of interfaces implementing it.

This would be a breaking change from SVG 1.1 SE. How confident are you that it would have a negligible impact?

foolip commented 7 years ago

The multiple inheritance of SVG 1.1 was never possible to implement in JavaScript bindings, I assume that's why implementations took different paths. Per https://github.com/w3c/svgwg/issues/291#issuecomment-254204205 Chrome and Safari don't have a window.SVGZoomAndPan, which is a good start.

A search for "SVGZoomAndPan" in httparchive:har.2016_09_15_chrome_requests_bodies finds only 64 results. None of the access anything on SVGZoomAndPan, AFAICT it's all one of two things:

Finding literally no real usage in httparchive is pretty unusual even for obscure things, so I'd say this is a safe change.

longsonr commented 7 years ago

The changes as originally outlined have now landed in Firefox nightlies.

foolip commented 7 years ago

Landing Make SVGUnitTypes [NoInterfaceObject] given that it is currently exposed everywhere doesn't seem like a good idea, even if it's what the spec currently says. See https://github.com/w3c/svgwg/issues/291#issuecomment-254442089 for pages in the wild that read the constants on SVGUnitTypes. @bzbarsky @heycam

longsonr commented 7 years ago

@foolip That's not what's happened. Read the bugs carefully and what's actually been proposed here.

bzbarsky commented 7 years ago

Yeah, that commit has a .... really terrible commit message that has no bearing on what the commit actually does. :( That's rather unfortunate. You have to read the actual IDL changes to have any idea what it actually does.

foolip commented 7 years ago

Oh, so AFACIT that commit doesn't change observable behavior, is that right?

longsonr commented 7 years ago

@foolip

Sure it does, that's why this github issue exists.

Elements such as SVGMaskElement will not be SVGUnitType objects, although they will still allow access to the constants and members of that interface.

foolip commented 7 years ago

Oh, document.createElementNS('http://www.w3.org/2000/svg','mask') instanceof SVGUnitTypes was true in Gecko before, fascinating. That returns false in Chrome/Safari and throws in Edge.

How did that actually work, is it implements X itself that caused instanceof X to be true? Are there any other odd cases like that left?

bzbarsky commented 7 years ago

How did that actually work

Because Gecko implements https://heycam.github.io/webidl/#es-interface-hasinstance (as of today's draft; I wish we had something resembling stable links, because this part might get removed from the spec because no one except Gecko implements is).

But also, in IDL spec terms, the thing we are really after with the specific changes to SVGUnitTypes is separating out the concepts of "interfaces" and "mixins". We are trying to get to a point where things on the right-hand side of an "implements" statement are always [NoInterfaceObject] interfaces, at which point we can drop the ability to "implements" an interface and have explicit mixin syntax.

In other words, this part of the SVG spec's IDL blocks fixing https://www.w3.org/Bugs/Public/show_bug.cgi?id=26452

bzbarsky commented 7 years ago

Or more precisely, the current IDL in the SVG spec doesn't block it, but breaks backwards compat. @longsonr's proposal keeps the desirable property of not conflating mixins and interfaces, but restores backwards compat.

foolip commented 7 years ago

@bzbarsky, I guess "If V is a platform object that implements the interface for which O is the interface prototype object, return true" is the relevant bit here? That's interesting, I had no idea that was ever intended to work, or indeed that it did work in Gecko.

In any case, the goal of "things on the right-hand side of an "implements" statement are always [NoInterfaceObject] interfaces" is clearly a good one, and before this issue I assumed that it was already so, or otherwise a mistake.

As for compat, I think https://github.com/w3c/svgwg/issues/291#issuecomment-254930687 has a good chance of working out. For Gecko, the SVG_UNITTYPE* constants would go away in a few places, but per https://github.com/w3c/svgwg/issues/291#issuecomment-254442089 it ought be quite low risk.

bzbarsky commented 7 years ago

I don't have specific opinions on what the spec should say here, apart from the mixin business. Going to defer to @longsonr on that.

bzbarsky commented 7 years ago

@foolip Oh, and yes, your quoted piece of the IDL spec is what affects instanceof behavior.

longsonr commented 7 years ago

Happy to try having the constants only on SVGUnitTypes in a future release of Firefox provided other UAs are on board too.

foolip commented 7 years ago

No change would be required for Chrome and Safari. Who's the SVG contact for Edge to comment on this? @travisleithead, can you poke someone?

nikosandronikos commented 7 years ago

@atanassov or @boggydigital ?

boggydigital commented 7 years ago

LGTM

foolip commented 7 years ago

@boggydigital, to make it explicit, is the proposal in https://github.com/w3c/svgwg/issues/291#issuecomment-254930687 that LGTY, or what was originally proposed in this issue?

boggydigital commented 7 years ago

The latest one in https://github.com/w3c/svgwg/issues/291#issuecomment-254930687

foolip commented 7 years ago

OK, thanks @boggydigital!

@nikosandronikos, what does the "Needs WG input" label mean, is there anything more needed?

AmeliaBR commented 7 years ago

@foolip

"Needs WG input" means that something needs discussion, to decide what the spec should say. Which has been happening here nicely. (It makes me so glad for moving to GitHub: this is exactly how it is supposed to work!)

At this point, I think we can probably make a resolution on it at our call tomorrow (Thurs afternoon in North America). Are you able to prep a pull request of exactly what changes need to be made to the spec text?

foolip commented 7 years ago

I haven't edited this spec before, but it seemed simple enough: https://github.com/w3c/svgwg/issues/295

nikosandronikos commented 7 years ago

@longsonr As far as I can tell, you haven't commented on @foolip's suggestion regarding SVGZoomAndPan:

Leave SVGZoomAndPan as a mixin interface, with NoInterfaceObject and a bunch of interfaces implementing it.

My preference would be to accept @foolip's proposal for SVGUnitTypes and @longsonr's proposal for SVGZoomAndPan, but I'm happy to defer to the implementers on this one. I'd like to hear solid support for both aspects of the proposal before we resolve on it.

foolip commented 7 years ago

The reasons I've suggested keeping SVGZoomAndPan as-is in the spec:

longsonr commented 7 years ago

I don't see the point in making the constants in SVGZoomAndPan inaccessible to javascript so I expect Firefox will stay as it now is in that regard.

AmeliaBR commented 7 years ago

SVGZoomAndPan is pretty much useless due to the current lack of browser support for user-controlled zoom and pan, which explains the lack of usage. You'd need to search demos from the Adobe SVG Viewer days to find practical uses for it.

I'm more concerned about consistency. Why have some shared constants only available from a static interface, and others only available from the implementing interfaces?

foolip commented 7 years ago

Regarding consistency, the only way to achieve that would be to have the constants everywhere, but that wouldn't be consistent with the rest of the platform, and apparently not needed for web compat. By making SVGUnitTypes a normal interface, that becomes the only wart.

Needless to say, if one weren't constrained at all by web compat, then none of these things would be numerical constants at all.

AmeliaBR commented 7 years ago

We talked about the issue on our call today. Since we've got 3 different implementers giving the OK for SVGUnitTypes to be an ordinary interface with no mixin, we're okay with that.

@shepazu is going to look in to getting the W3C permissions sorted for @foolip in order to merge that pull request. We may need to temporarily add you to the GitHub SVG team, but I don't think that means you have to be formally assigned as a Google rep on the WG. If it gets too complicated, one of the WG members can make the edits.


For SVGZoomAndPan, we hope to get consensus from you all before making a change (if any).

Things to consider (my personal opinion):

As much as I'd love to see proper browser implementations of user zoom & pan control for graphics, I suspect that when that comes it will be via a new functionality that works for all web content, not just SVG, and that is compatible with existing pinch-zoom implementations. (And hopefully, it will be via a mechanism that doesn't "disable" zoom, it only defines which container isolates the zoom.)

nikosandronikos commented 7 years ago

My take on exposing exposing SVGZoomAndPan as an interface is that I agree doing that will still have some level of inconsistency, but I think that's where authors will expect to find the constants, and that's the consistency that matters.

foolip commented 7 years ago

For SVGZoomAndPan, @fsoder said in https://codereview.chromium.org/2415413002/#msg22 that he'd prefer "nuke from orbit", so if there's any appetite for removing it entirely that might be worth a try. FWIW, there is a use counter for the zoomAndPan attribute in Chrome, and it's pretty low: https://www.chromestatus.com/metrics/feature/timeline/popularity/1102

In the meantime, exposing the interface would only increase the risk of eventual removal, and whatever authors may initially expect, in https://github.com/w3c/svgwg/issues/291#issuecomment-255047561 no content was found that actually tried to access the constants on the interface.

nikosandronikos commented 7 years ago

Re-opening, because PR #295 only resolved half the issue - we haven't resolved on whether SVGZoomAndPan will stay the same or not yet.

AmeliaBR commented 6 years ago

Does anyone have any outstanding issues on this, or can this be closed?

Teleconference minutes (reposting what Github-bot posted in the wrong issue):

The Working Group just discussed Values in SVGZoomAndPan and SVGUnitTypes should be accessible to javascript.

The full IRC log of that discussion <Chris__> topic: Values in SVGZoomAndPan and SVGUnitTypes should be accessible to javascript
<Chris__> github: https://github.com/w3c/svgwg/issues/291
<krit> AmeliaBR: we have interfaces that are defined as NoInterfaceObject but other object types which implement these infterfaces which have those properties on them
<krit> AmeliaBR: so you could access those constants but not as a static item
<krit> AmeliaBR: usually constants are property of a class
<krit> AmeliaBR: need to recall. it is from 2016
<krit> krit: those are mixing and get implemented by those classes. So they actually are part of those classes implementing them.
<krit> krit: not sure where the issue is?
<krit> AmeliaBR: those are legacy features and not used on the web. Keep them close to SVG 1.1
<krit> AmeliaBR: not sure what is required.
<krit> krit: are those interfaces implemented already?
<krit> krit: we were asked to re-add interfaces not because they are used but because they are implemented widely
<AmeliaBR> Here's the last update on implementation https://github.com/w3c/svgwg/issues/291#issuecomment-254204205
<krit> Chris__: so we have interfaces that are not used but implemented.
<krit> Chris__: seems like some ppl argue it shouldn't be exposed.
<AmeliaBR> SVGUnitTypes was implemented, and was re-introduced to the spec. SVGZoomAndPan wasn't well implemented cross-browser, and that's whay it wasn't changed at the time.
<krit> Chris__: Firefox seems to expose and implement it directly and does seem to have tests
<krit> AmeliaBR: there are 2 different causes.
<krit> AmeliaBR: no decision has been made and issue was reopened. Not implemented browsers anyways.
<krit> krit: they are not implemented? because I think they are.
<AmeliaBR> s/no decision/for SVGZoomAndPan/
<Chris__> https://bugzilla.mozilla.org/show_bug.cgi?id=1241898
<krit> Chris__: original comment was about aligning implementations so we have 2 implementations, Blink and Firefox
<AmeliaBR> s/for SVGZoomAndPan/for SVGZoomAndPan, no decision/
<krit> ericwilligers: we could still remove those interfaces if they are not sued
<krit> s/sued/used/
<krit> Chris__: yes we could
<krit> Chris__: not sure what is proposed is better
<krit> AmeliaBR: I don't think we are getting anywhere on this on the call. We should ping ppl if this is still an issue or if they are happy with changes that were made.
<krit> Chris__: agree. Will post on the issue and ask for comments.
<Chris__> github: https://github.com/w3c/svgwg/issues/269
foolip commented 6 years ago

For SVGUnitTypes, things appear to be the same as they were in https://github.com/w3c/svgwg/issues/291#issuecomment-254442089. I confirmed this in Chrome, Firefox and Safari, but Edge actually crashes when loading https://software.hixie.ch/utilities/js/live-dom-viewer/saved/4581. I think the spec is fine as-is there, and this is tested in https://wpt.fyi/svg/historical.html.

For SVGZoomAndPan, https://svgwg.org/svg2-draft/types.html#InterfaceSVGZoomAndPan has [NoInterfaceObject], which I also think is the right thing, since SVGZoomAndPan is the target of implements statements: SVGSVGElement implements SVGZoomAndPan and SVGViewElement implements SVGZoomAndPan. Also tested in https://wpt.fyi/svg/historical.html.

I didn't report the issue, but I'm satisfied.

travisleithead commented 6 years ago

Thanks @foolip (I also reported the crashing site, so we can fix it.)

longsonr commented 6 years ago

https://bugzilla.mozilla.org/show_bug.cgi?id=1435568 should make us align with Chrome for SVGUnitTypes i.e. individual elements won't implement SVGUnitTypes

css-meeting-bot commented 6 years ago

The Working Group just discussed end of meeting :).

The full IRC log of that discussion <Chris__> topic: end of meeting :)
<Chris__> rrsagent, make minutes
<RRSAgent> I have made the request to generate https://www.w3.org/2018/01/29-svg-minutes.html Chris__
<Chris__> rrsagent, make logs public
<RRSAgent> I have made the request, Chris__
<Chris__> rrsagent, make minutes
<RRSAgent> I have made the request to generate https://www.w3.org/2018/01/29-svg-minutes.html Chris__
<Chris> trackbot, start telcon
<RRSAgent> logging to https://www.w3.org/2018/02/05-svg-irc
<trackbot> RRSAgent, make logs public
<RRSAgent> I have made the request, trackbot
<trackbot> Meeting: SVG Working Group Teleconference
<trackbot> Date: 05 February 2018
<BogdanBrinza> We have three agenda+ items today (as announced in the mail to WG)
<BogdanBrinza> https://github.com/w3c/svgwg/issues/291
<Chris> github: https://github.com/w3c/svgwg/issues/291
<BogdanBrinza> Was discussed last time - is there anything outstanding on the topic?
<BogdanBrinza> Last week - not clear if there were new issues. New comments indicate that people are happy with proposal and spec changes and browser implementers started the work
<BogdanBrinza> resolution: close the issue
longsonr commented 6 years ago

What's happening with ZoomAndPan. Is it to be removed altogether? Keeping it and making it [nointerfaceobject] makes no sense as far as I can see.

AmeliaBR commented 6 years ago

SVGZoomAndPan, unlike SVGUnitTypes, is still used as mixin (it is implemented by SVGSVGElement ).

So there is still access to the named constants through any <svg> element instance.

dstorey commented 6 years ago

Are we happy to close this one now?

longsonr commented 6 years ago

I still think that making people write SVGUnitTypes.SVG_UNIT_TYPE_USERSPACEONUSE yet then saying you can't write SVGZoomAndPan.SVG_ZOOMANDPAN_DISABLE, you must write SVGSVGElement.SVG_ZOOMANDPAN_DISABLE is strangely inconsistent. At the moment Firefox allows SVGZoomAndPan.SVG_ZOOMANDPAN_DISABLE to try to avoid this confusion but also SVGSVGElement.SVG_ZOOMANDPAN_DISABLE to avoid breaking anyone who might write things the spec way.

Should SVGZoomAndPan be removed altogether though? Is it useful/functional anywhere?