Closed Bug 717722 Opened 8 years ago Closed 4 years ago

implement (WebKit)CSSMatrix()

Categories

(Core :: DOM: CSS Object Model, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: gal, Assigned: wchen)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 1 obsolete file)

It seems popular with web authors.
What compatibility gains do we get from implementing it?  Can you point at some popular sites?  Is this a priority on mobile devices or generally?
Dean Jackson posted a proposed draft spec:
http://lists.w3.org/Archives/Public/public-fx/2012JanMar/0007.html

Seems like a reasonable feature.
Is CSSMatrix the same thing as Matrix4x4?

If Matrix4x4 is intended to be The Matrix To Rule Them All, then yeah that sounds like a good thing.
Same functionality --- more, actually --- but it's not quite the same ... WebkitCSSMatrix::multiply returns the new matrix, whereas Matrix4x4::multiply modifies the matrix in-place. But Matrix4x4 offers multipliedBy to return a new matrix.

Should we ask Dean to make Matrix4x4 more of a drop-in replacement for WebkitCSSMatrix?
Something like |typedef Matrix4x4 WebkitCSSMatrix;| seems like a desirable goal to me.

Similarly, I would also love to have a reasonable solution to bug 683051, a matrix type that makes sense in canvas APIs.  Having a Matrix3x3 where |typedef Matrix3x3 SVGMatrix|;, or even |typedef Matrix4x4 SVGMatrix|, would solve that problem well.
Blocks: 811421
No longer blocks: 811421
Blocks: 815374
Several authors have written Javascript polyfills for WebKitCSSMatrix so I'm guessing this could be a useful/desired feature in FF. Maybe some of the JS classes could work as a starting point for implementing it in Firefox?

I posted a demo on codepen (linked below) of a JS class I wrote. It's received a decent amount of attention, and makes me think that web authors might want this.

codepen demo: http://cdpn.io/mvqab (only works in Chrome/Safari, since it's comparing WebKitCSSMatrix)
github repo: https://github.com/jonbrennecke/CSSMatrix
Yes, we ship DOMMatrix already.  This bug is about WebKitCSSMatrix.
This bug predates DOMMatrix, are you considering also having WebKitCSSMatrix? I've added a use counter in Blink to find out if we can remove it once DOMMatrix ships.
> are you considering also having WebKitCSSMatrix?

If we discover that it's necessary for compat, yes.  I'm hoping it won't be.
This sounds like a good candidate for WebCompat telemetry. 
I added it to the list https://wiki.mozilla.org/Compatibility/Telemetry#CSS
The Blink use counter is https://www.chromestatus.com/metrics/feature/timeline/popularity/630

It's still a long way from the stable channel but already at ~0.04%, which is very discouraging.

If it's impossible to remove, then making it an alias of DOMMatrix seems like the most promising path forward.
I see usage is up to 0.1% on the chromestatus board. :/

Doing some testing of a shim for Bug 907674 (which has like 7 dupes on webcompat.com), mapping WebKitCSSMatrix to DOMMatrix is pretty straightforward (and seems to do the job). However, it gets messy when you look at how it's actually used on ehow (and probably everywhere else on the web):

> // var e = document.defaultView.getComputedStyle(foo, null),
> // t = new WebKitCSSMatrix(e.webkitTransform);

Seems like a very common pattern: <https://github.com/search?l=javascript&q=new+WebKitCSSMatrix%28window.getComputedStyle%28el%2C+null%29.webkitTransform%29&type=Code&utf8=%E2%9C%93>

(Defining getters/setters for webkitTransform and webkitTransition on CSSStyleDeclaration.prototype does eventually get the site to work).
We are going to support -webkit prefix in CSS only on some whitelisted sites. If this support improves some high-volume real-world sites, it would deserve to consider.
Masatoshi, see Bug 1107378 about some of the prefixes being rewritten for certain chinese Web sites.
Flags: needinfo?(VYV03354)
Yeah, I was saying about bug 1107378.
Flags: needinfo?(VYV03354)
(In reply to Mike Taylor [:miketaylr] from comment #14)
> I see usage is up to 0.1% on the chromestatus board. :/

And it hasn't even reached the stable channel yet... this isn't looking good at all.
Yeah, that's where the spec for this will live (still WIP, obviously. ^_^).
Assignee: nobody → wchen
Note here one open issue about possibly making WebKitCSSMatrix an alias of DOMMatrix: https://github.com/whatwg/compat/issues/19
Attached patch Implement WebKitCSSMatrix (obsolete) — Splinter Review
This patch trivially shadows the WebKitCSSMatrix methods to call into DOMMatrix methods. The only non-minor change is the addition of the WebKitCSSMatrix version of |rotate| because we don't have an equivalent in DOMMatrix.

Deviations from the spec (these are things we should fix in the spec):
- In the step for |rotate|, we first set rotX to 0 if undefined otherwise we may end up trying to rotate by undefined.
- |rotate| post multiplies the axis rotation matrices in the order of Z, Y then X since it looks like that is what WebKit is doing.
- WebKitCSSMatrix doesn't keep track of whether a matrix is 2D while DOMMatrix does so the |rotate| algorithm should set the is2D flag appropriately. In the patch I set it to false if we try and rotate about the X or Y axis by a non-zero angle.

(In reply to Mike Taylor [:miketaylr] from comment #23)
> Note here one open issue about possibly making WebKitCSSMatrix an alias of
> DOMMatrix: https://github.com/whatwg/compat/issues/19

If we are willing to break DOMMatrix in order to merge it with WebKitCSSMatrix then we may want to consider going one step further and just renaming DOMMatrix to WebKitCSSMatrix after the merge since doesn't seem like there is anything to gain from having an alias.
Attachment #8706741 - Flags: review?(amarchesini)
>+  double rotX = aRotX.WasPassed() ? aRotX.Value() : 0;

You should just give it a default value in the IDL, no?

Similar for scale(), where scaleX and scaleZ can just have default values in IDL.  scaleY will have to continue as-is.
Uploaded wrong version of the patch. I've also gone ahead and made bz's suggested IDL changes.

We should also update the IDL in the spec:
- For the scale method, scaleX and scaleZ should have default value of 1
- For the rotate method, rotX should have a default value of 0
- Remove language dealing with the arguments above being undefined.
Attachment #8706741 - Attachment is obsolete: true
Attachment #8706741 - Flags: review?(amarchesini)
Attachment #8706748 - Flags: review?(amarchesini)
Comment on attachment 8706748 [details] [diff] [review]
Implement WebKitCSSMatrix

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

::: dom/base/DOMMatrix.h
@@ +146,1 @@
>  {

no virtual DTOR ?

::: dom/base/WebKitCSSMatrix.cpp
@@ +11,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +static const double radPerDegree = 2.0 * M_PI / 360.0;

sRadPerDegree

@@ +21,5 @@
> +  return obj.forget();
> +}
> +
> +already_AddRefed<WebKitCSSMatrix>
> +WebKitCSSMatrix::Constructor(const GlobalObject& aGlobal, const nsAString& aTransformList, ErrorResult& aRv)

80chars max.

@@ +44,5 @@
> +
> +WebKitCSSMatrix*
> +WebKitCSSMatrix::SetMatrixValue(const nsAString& aTransformList, ErrorResult& aRv)
> +{
> +  DOMMatrix::SetMatrixValue(aTransformList, aRv);

if (NS_WARN_IF(aRv.Failed()) {
  return nullptr;
}

::: dom/base/WebKitCSSMatrix.h
@@ +25,5 @@
> +
> +  static already_AddRefed<WebKitCSSMatrix>
> +  Constructor(const GlobalObject& aGlobal, ErrorResult& aRv);
> +  static already_AddRefed<WebKitCSSMatrix>
> +  Constructor(const GlobalObject& aGlobal, const nsAString& aTransformList, ErrorResult& aRv);

80chars max here and in the rest of the file.
Attachment #8706748 - Flags: review?(amarchesini) → review+
This patch puts this feature behind the same pref that we use for other webkit prefixed features to address dbaron's comment to the intent to ship: https://groups.google.com/d/msg/mozilla.dev.platform/DmOgvnSuq74/98uYCAElAgAJ
Attachment #8708240 - Flags: review?(amarchesini)
Release Note Request (optional, but appreciated)
[Why is this notable]: WebKitCSSMatrix has been available for years in WebKit based browsers and has seen widespread usage on the web. This feature is
currently available in Chrome, Safari and Edge. Mike Taylor has been working on standardizing the feature and it is being implemented in Gecko to improve web compatibility.
[Suggested wording]: Added support for WebKitCSSMatrix to improve the mobile Web experience
[Links (documentation, blog post, etc)]:
*Link to standard*:
https://compat.spec.whatwg.org/#webkitcssmatrix-interface
https://compat.spec.whatwg.org/#dom-window-orientation

*Estimated or target release*: Firefox 46
relnote-firefox: --- → ?
Filed https://github.com/whatwg/compat/issues/30 for spec updates (will be working on this today, but leaving a link for posterity).
Attachment #8708240 - Flags: review?(amarchesini) → review+
https://hg.mozilla.org/mozilla-central/rev/1e963938e988
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Depends on: 1241786
Note: this will stay in aurora until everything is ready (it's behind the same layout.css.prefixes.webkit pref as the other webkit CSS stuff: https://bugzilla.mozilla.org/show_bug.cgi?id=1238827)
No longer depends on: 1241786
Depends on: 1244464
Removing this from the beta 46 release notes. This is back in the relnote:? pool but over in bug 1213126 now.
Added to Fx47 release notes.
We're still not shipping this pref in 47, at least according to the code currently on beta.

Could you remove it from the release notes?
Flags: needinfo?(rkothari)
(Specifically: this feature is guarded by the pref "layout.css.prefixes.webkit", which is not being enabled for Beta/Release builds until Firefox 48 at the earliest.  That enabling will happen in bug 1259345, once we're confident that it's not going to cause trouble once 48 moves to beta.)
OS: Mac OS X → All
Hardware: x86 → All
(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #36)
> We're still not shipping this pref in 47, at least according to the code
> currently on beta.
> 
> Could you remove it from the release notes?

Got it. Done.
Flags: needinfo?(rkothari)
You need to log in before you can comment on or make changes to this bug.