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

Can WebKitCSSMatrix be an alias of DOMMatrix? #19

Closed
foolip opened this issue Nov 30, 2015 · 43 comments
Closed

Can WebKitCSSMatrix be an alias of DOMMatrix? #19

foolip opened this issue Nov 30, 2015 · 43 comments
Assignees

Comments

@foolip
Copy link
Member

foolip commented Nov 30, 2015

@miketaylr, can you help me figure out what's blocking making WebKitCSSMatrix an alias of DOMMatrix, so that window.WebKitCSSMatrix and window.DOMMatrix are the same constructor?

Looking at the current spec, it seems like the only things are Constructor(DOMString transformList) and Constructor(WebKitCSSMatrix other), the other overrides are just making sure that the returned object is a WebKitCSSMatrix.

If there is nothing else, maybe we can have those constructors added to https://drafts.fxtf.org/geometry/#dommatrix ?

@miketaylr
Copy link
Member

This is something I had originally hoped was possible. Here are the 2 main differences, other than constructors you mention:

  1. DOMMatrix.scale(scale, scaleX, scaleY) and WebKitCSSMatrix.scale(scaleX, scaleY, scaleZ) have different method signatures.

(Maybe it's better to define WebKitCSSMatrix.scale() as DOMMatrix.scale3d(1, scaleX, scaleY, scaleZ)? Right now it's defined in terms of DOMMatrix.scaleNonUniform().)

  1. DOMMatrix.rotate(angle, originX, originY) and WebKitCSSMatrix.rotate(rotX, rotY, rotZ) have different method signatures. I'm not sure if it's possible to implement WebKitCSSMatrix.rotate in terms of DOMMatrix.rotate or DOMMatrix.rotateAxisAngle. Maybe @zcorpan has an idea.

@zcorpan zcorpan self-assigned this Dec 1, 2015
@foolip
Copy link
Member Author

foolip commented Dec 1, 2015

I see that Firefox has shipped DOMMatrix already, but not Chrome, Edge or Safari. What do you think about simply changing DOMMatrix to match WebKitCSSMatrix? Since only Gecko has DOMMatrix so far, I don't suppose too much could depend on it, right?

@zcorpan
Copy link
Member

zcorpan commented Dec 1, 2015

@rocallahan

@rocallahan
Copy link

I don't really know, but I assume we can change it.

@foolip
Copy link
Member Author

foolip commented Dec 2, 2015

@zcorpan @dirkschulze @cabanier, you're the editors of https://drafts.fxtf.org/geometry/Overview.html#DOMMatrix

Do you see a problem with making scale and rotate behave like WebKitCSSMatrix, and perhaps adding separate methods if the existing semantics are better for some use cases?

@zcorpan
Copy link
Member

zcorpan commented Dec 7, 2015

There is also SVGMatrix which DOMMatrix is supposed to supersede and be compatible with, and SVGMatrix is widely supported. I don't know which of SVGMatrix and WebKitCSSMatrix are more widely used, but I suppose it is not possible to be compatible with both...

@zcorpan
Copy link
Member

zcorpan commented Dec 7, 2015

Can't WebKitCSSMatrix inherit from DOMMatrix, and shadow scale and rotate? WebKitCSSMatrix can only be constructed, it's not returned from some other API?

@foolip
Copy link
Member Author

foolip commented Dec 7, 2015

A quick glance at Blink's IDL for SVGMatrix and WebKitCSSMatrix suggests that they could be merged. SVGMatrix has a few methods with the same names but fewer arguments, and a few extra methods, but I see no obvious conflicts in semantics.

There is the trouble that SVGMatrix is a tear-off, but that seems to equally problematic if trying to merge it into DOMMatrix.

@foolip
Copy link
Member Author

foolip commented Dec 7, 2015

As for inheriting and shadowing, that is what the spec currently does, but then you have to shadow everything so that it returns a WebKitCSSMatrix instead of a DOMMatrix. Just making all of the matrix types aliases ought to simplify things, if it actually works out.

@zcorpan
Copy link
Member

zcorpan commented Dec 7, 2015

OK, yeah. Sorry for not paying proper attention; I should be sleeping. :-)

I thought WebKitCSSMatrix#rotate with one argument would only scale X, but I see now it scales both X and Y in that case.

If they are indeed compatible then I don't mind changing DOMMatrix.

@foolip
Copy link
Member Author

foolip commented Dec 7, 2015

I think the most consequential differences between DOMMatrix and WebKitCSSMatrix are the ones pointed out in #19 (comment)

There's also the fact that multiply should take another object, a setMatrixValue method and perhaps other minor things. Happy to go through it it in detail if you agree that merging seems worthwhile.

@zcorpan
Copy link
Member

zcorpan commented Jan 20, 2016

multiply and setMatrixValue look like they are compatible, though?

@zcorpan
Copy link
Member

zcorpan commented Jan 20, 2016

From a quick github search it looks like most invocations of the constructor either use no argument or a string -- not another WebKitCSSMatrix instance.

@foolip
Copy link
Member Author

foolip commented Jan 20, 2016

It's WebKitCSSMatrix multiply(WebKitCSSMatrix other) vs. DOMMatrix multiply(optional DOMMatrixInit other). So it's the optional keyword. I guess that DOMMatrix looks like a DOMMatrixInit dictionary and that's why it works?

When I commented I think setMatrixValue was only in one of the specs, or perhaps I just failed to find it in one of them. They indeed look compatible.

@zcorpan
Copy link
Member

zcorpan commented Jan 20, 2016

Yep. Moving from required to optional shouldn't be a problem for Web compat.

The other thing is whether we should drop some other methods in the Geometry spec, like scaleNonUniform (which is like webkit-scale).

@foolip
Copy link
Member Author

foolip commented Jan 20, 2016

Yeah, dropping anything that becomes redundant sounds like a good idea, assuming it's not shipped and used by web content.

zcorpan added a commit to w3c/fxtf-drafts that referenced this issue Jan 22, 2016
Change the arguments from (angle, originX, originY) to (rotX, rotY,
rotZ), all optional.

Part of whatwg/compat#19
zcorpan added a commit to w3c/fxtf-drafts that referenced this issue Jan 22, 2016
Drop the scale() and scaleSelf() methods, and rename the
scaleNonUniform()/scaleNonUniformSelf() methods to scale() and
scaleSelf(), respectively. Make scaleX optional, with default value
1, and scaleY use the value of scaleX if omitted.

Part of whatwg/compat#19
zcorpan added a commit to w3c/fxtf-drafts that referenced this issue Jan 22, 2016
Make all arguments optional for DOMMatrix/DOMMatrixReadOnly methods,
except for setMatrixValue() which throws in WebKit anyway (because
"undefined" fails to parse). I made everything optional to be
consistent; making the minimal set of arguments optional would
be self-inconsistent and confusing (e.g. skewX vs. skewXSelf).

Part of whatwg/compat#19
zcorpan added a commit to w3c/fxtf-drafts that referenced this issue Jan 22, 2016
Change fromString() static method into overloaded constructor,
since Web content depends on it for WebKitCSSMatrix constructor.
However, I could not find (github search) content that depends on
WebKitCSSMatrix(other), so leave as fromMatrix() static method.

Also add a no-argument constructor. I think this was accidentally
left out when switching to static methods.

Part of whatwg/compat#19
zcorpan added a commit to w3c/fxtf-drafts that referenced this issue Jan 22, 2016
Alias WebKitCSSMatrix to DOMMatrix in the same way as SVGMatrix.

Part of whatwg/compat#19
@zcorpan
Copy link
Member

zcorpan commented Jan 22, 2016

OK so I've made a series of changes to Geometry to address this. Commits:

w3c/fxtf-drafts@430bc88
w3c/fxtf-drafts@99e3212
w3c/fxtf-drafts@8493a9c
w3c/fxtf-drafts@3c43462
w3c/fxtf-drafts@feba855
w3c/fxtf-drafts@a9fe6a7

Pls review :-)

@zcorpan zcorpan assigned miketaylr and unassigned zcorpan Jan 22, 2016
@miketaylr
Copy link
Member

Thanks @zcorpan -- I had a quick glance but will take a closer look on Monday (flying home in a few hours). I assume we still want to do #34 (comment)?

@zcorpan
Copy link
Member

zcorpan commented Jan 22, 2016

Yeah, IMO at least

@foolip
Copy link
Member Author

foolip commented Jan 26, 2016

Anything I need to look at? Next step is to actually make WebKitCSSMatrix an alias of DOMMatrix?

@miketaylr
Copy link
Member

w3c/fxtf-drafts@3c43462

I also don't know about any content that relies on new WebKitCSSMatrix(other)... the only thing I've been able to find (on github and in my small web-JS corpus) is https://github.com/WebKit/webkit/blob/67985c34ffc405f69995e8a35f9c38618625c403/LayoutTests/platform/ios-simulator/ios/css/resources/construct-WebKitCSSMatrix.js, which references rdar://problem/6481690.

@zcorpan
Copy link
Member

zcorpan commented Jan 27, 2016

OK looked in httparchive now as well:

SELECT COUNT(*) AS num, REGEXP_EXTRACT(body, r'(new\s+WebKitCSSMatrix\s*\(\s*[^\(\)]+(?:\([^\)]*\)[^\)]*)?\))') AS match
FROM [httparchive:runs.2014_08_15_requests_body]
GROUP BY match
ORDER BY num DESC

results-20160127-070311.csv.zip

All of these pass in webkitTransform computed style, or a variable that needs further inspection to see what it is.

SELECT page, url, REGEXP_EXTRACT(body, r'(new\s+WebKitCSSMatrix\s*\([a-zA-Z]+\))') AS match
FROM [httparchive:runs.2014_08_15_requests_body]
WHERE REGEXP_MATCH(body, r'(new\s+WebKitCSSMatrix\s*\([a-zA-Z]+\))')

22 rows, all inspected manually; those that weren't 404 now were passing in webkitTransform computed style.

So no instances that pass in another WebKitCSSMatrix AFAICT.

@foolip
Copy link
Member Author

foolip commented Jan 27, 2016

FWIW, Blink doesn't have a WebKitCSSMatrix constructor that takes another WebKitCSSMatrix, just Constructor(optional DOMString cssValue). Why does the spec have one?

@zcorpan
Copy link
Member

zcorpan commented Jan 27, 2016

cc @grorg

@miketaylr
Copy link
Member

Why does the spec have one?

That's... a good question. I can't recall why I added it, now that I'm looking over WebKit's implementation. So probably just a mistake that can be removed.

@miketaylr
Copy link
Member

How does the following end up working (in Chrome)?

m = new WebKitCSSMatrix();
m2 = m.rotate(10, 20, 30);
m3 = new WebKitCSSMatrix(m2)
m3.a == m2.a.toPrecision(6)

(this might be some WebIDL magic I don't know about)

@zcorpan
Copy link
Member

zcorpan commented Jan 27, 2016

It toStrings to a <transform-list>

@zcorpan
Copy link
Member

zcorpan commented Jan 27, 2016

In Geometry the WebIDL magic is stringifier + prose

@miketaylr
Copy link
Member

Ah, there we go. So yes, that constructor is just a spec (editor) bug.

@miketaylr
Copy link
Member

@zcorpan here is some more feedback from wchen:

https://bugzilla.mozilla.org/show_bug.cgi?id=1241575#c6

Some quirks I noticed while working on this (and may need to be spec'ed):
- Chrome will accept "none", "inherit" and "initial". Safari will only accept "none". This patch will only accept "none" and treat it as the a 2d identity matrix (Chrome and Safari don't keep track of the is2D flag).
- Chrome and Safari will accept an empty string as a transform list but will syntax error on a string consisting of only whitespace. Empty string is treated as the identity matrix. This patch does the same.
- Chrome accepts relative length values. Safari accepts only absolute length values. DOMMatrix spec says to syntax error if values are not absolute length. This patch only accepts absolute length values.
- Calculated values are accepted in Chrome and accepted (but buggy and crashy) in Safari. This patch treats calculated values as a syntax error.

As for the "parse transform lists as a transform property" problem (issue #34), what do you think makes more sense -- leaving the DOMMatrix spec as-is or speccing the WebKitCSSMatrix alias to only take transform property syntax? (Seems kinda weird to have aliases with different behavior...).

Or possibly a note that for historical reasons the WebKitCSSMatrix constructor is allowed to only use transform property syntax.

@miketaylr
Copy link
Member

And then in theory we can just delete this whole thing from the compat spec, once the small issues are worked out?

(and file bugs for Gecko to update its DOMMatrix/DOMMatrixReadOnly implementation).

@zcorpan
Copy link
Member

zcorpan commented Jan 27, 2016

As for the "parse transform lists as a transform property" problem (issue #34), what do you think makes more sense -- leaving the DOMMatrix spec as-is

Yes. See #34 (comment)

Or possibly a note that for historical reasons the WebKitCSSMatrix constructor is allowed to only use transform property syntax.

That doesn't make much sense IMO. People shouldn't use the constructor at all; they should use DOMMatrix instead.

And then in theory we can just delete this whole thing from the compat spec, once the small issues are worked out?

Yep. Please file any bugs on the Geometry spec (link in bottom right corner of the spec).

@miketaylr
Copy link
Member

As for the "parse transform lists as a transform property" problem (issue #34), what do you think makes more sense -- leaving the DOMMatrix spec as-is

Yes. See #34 (comment)

Oh, right. That reply got buried in my GitHub issues email folder. Agreed DOMMatrix shouldn't change that.

That doesn't make much sense IMO. People shouldn't use the constructor at all; they should use DOMMatrix instead.

Yes, nobody should be using WebKitCSSMatrix for new content (though that's probably not true today as only one engine supports (an older version of) DOMMatrix.

My concern is more about documenting how to parse WebKitCSSMatrix(transformList) for implementations that don't yet support the DOMMatrix alias (which is all of them) or a backwards compatible SVG transform attribute -> CSS transform property syntax. But you're right, that doesn't make sense in the DOMMatrix spec.

One option is to update the compat spec to reflect the current state of what Gecko has (or will soon have, when 1241575 is fixed), and add a note pointing future implementers to DOMMatrix instead. And then nuke the whole thing at some point in the future (maybe that future is immediately after the spec updates are in git history).

@miketaylr
Copy link
Member

Given #19 (comment), I think we can close this. 🎈

@foolip
Copy link
Member Author

foolip commented Jan 28, 2016

@grorg
Copy link

grorg commented Jan 29, 2016

Thanks for this. I'll file a bug for WebKit to follow along (and implement DOMMatrix as well).

https://bugs.webkit.org/show_bug.cgi?id=153675

@miketaylr
Copy link
Member

Here's one issue I missed (around rotateAxisAngle): https://bugzilla.mozilla.org/show_bug.cgi?id=1245242. I'll file an issue on DOMMatrix but wanted to record it here.

(As I said in the bug, there doesn't seem to be a huge usage of rotateAxisAngle out in the wild, from what I can tell -- but we should aim for compatibility)

@miketaylr
Copy link
Member

@zcorpan help my matrix-fried brain out. According to the Geometry spec's definition of DOMMatrixReadOnly.rotateAxisAngleSelf, should m and m3 produce the same result? If so, it's just a Gecko bug.

m = new DOMMatrix()
m2 = m.rotateAxisAngle(0.707, 0.707, 0.707, 45)
m3 = new WebKitCSSMatrix('rotate3d(0.707, 0.707, 0.707, 45deg)')

@zcorpan
Copy link
Member

zcorpan commented Feb 4, 2016

I think they should be the same.

@miketaylr
Copy link
Member

Yep, was a Gecko bug -- false alarm. The fix should be in tomorrow's nightly.

🚒

foolip added a commit to foolip/browser-compat-data that referenced this issue Aug 27, 2020
The support data here comes mainly from the following:
mdn#1617
mdn#3644
mdn#3743
mdn#4902
mdn#4936

With the removal of the GeometryInterfaces flag from BCD, lint errors
made it apparent that a lot of the versions were wrong, based on
Chromium commits while the flag was still not enabled.

In short, all of this shipped together in M61:
https://storage.googleapis.com/chromium-find-releases-static/065.html#06592e080fd1cf4a188265ed4f9bcf826952671a

The only exceptions are scaleNonUniform which shipped in M73:
https://storage.googleapis.com/chromium-find-releases-static/57f.html#57f350e070536c7870cad53b3edf74f58cc51d50

And scaleNonUniformSelf isn't in the spec and was never in Chromium:
w3c/fxtf-drafts#214
whatwg/compat#19
foolip added a commit to foolip/browser-compat-data that referenced this issue Aug 27, 2020
First, remove the GeometryInterfaces flag, removed over 2 years ago:
https://chromium.googlesource.com/chromium/src/+/3e51c4becac41fe61dbd02a68bd875cc4a340457

Lint errors made it apparent that a lot of the versions were wrong.

The support data here comes mainly from the following:
mdn#1617
mdn#3644
mdn#3743
mdn#4902
mdn#4936

The root mistake was looking at commits which added/updated these APIs
while it was still behind a flag, and some BCD entries using those
releases without documenting it as behind a flag.

In short, all of this shipped together in M61:
https://storage.googleapis.com/chromium-find-releases-static/065.html#06592e080fd1cf4a188265ed4f9bcf826952671a

The only exceptions are scaleNonUniform which shipped in M73:
https://storage.googleapis.com/chromium-find-releases-static/57f.html#57f350e070536c7870cad53b3edf74f58cc51d50

And scaleNonUniformSelf isn't in the spec and was never in Chromium:
w3c/fxtf-drafts#214
whatwg/compat#19

The data for opera, opera_android, samsung_internet and webview_android
was mirrored.
queengooborg pushed a commit to mdn/browser-compat-data that referenced this issue Aug 27, 2020
)

First, remove the GeometryInterfaces flag, removed over 2 years ago:
https://chromium.googlesource.com/chromium/src/+/3e51c4becac41fe61dbd02a68bd875cc4a340457

Lint errors made it apparent that a lot of the versions were wrong.

The support data here comes mainly from the following:
#1617
#3644
#3743
#4902
#4936

The root mistake was looking at commits which added/updated these APIs
while it was still behind a flag, and some BCD entries using those
releases without documenting it as behind a flag.

In short, all of this shipped together in M61:
https://storage.googleapis.com/chromium-find-releases-static/065.html#06592e080fd1cf4a188265ed4f9bcf826952671a

The only exceptions are scaleNonUniform which shipped in M73:
https://storage.googleapis.com/chromium-find-releases-static/57f.html#57f350e070536c7870cad53b3edf74f58cc51d50

And scaleNonUniformSelf isn't in the spec and was never in Chromium:
w3c/fxtf-drafts#214
whatwg/compat#19

The data for opera, opera_android, samsung_internet and webview_android
was mirrored.
foolip added a commit to foolip/browser-compat-data that referenced this issue Aug 27, 2020
First, remove the GeometryInterfaces flag, removed over 2 years ago:
https://chromium.googlesource.com/chromium/src/+/3e51c4becac41fe61dbd02a68bd875cc4a340457

Lint errors made it apparent that a lot of the versions were wrong.

The support data here comes mainly from the following:
mdn#1617
mdn#3644
mdn#3743
mdn#4902
mdn#4936

The root mistake was looking at commits which added/updated these APIs
while it was still behind a flag, and some BCD entries using those
releases without documenting it as behind a flag.

In short, all of this shipped together in M61:
https://storage.googleapis.com/chromium-find-releases-static/065.html#06592e080fd1cf4a188265ed4f9bcf826952671a

The only exceptions are scaleNonUniform which shipped in M73:
https://storage.googleapis.com/chromium-find-releases-static/57f.html#57f350e070536c7870cad53b3edf74f58cc51d50

And scaleNonUniformSelf isn't in the spec and was never in Chromium:
w3c/fxtf-drafts#214
whatwg/compat#19

The data for edge, opera, opera_android, samsung_internet and webview_android
was mirrored.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants