Last Comment Bug 719054 - matrix() and matrix3d() with length units should be parse errors
: matrix() and matrix3d() with length units should be parse errors
Status: RESOLVED FIXED
[DocArea=CSS]
: dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: x86 Linux
: -- minor (vote)
: mozilla16
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on: 774475 774169
Blocks: 745523
  Show dependency treegraph
 
Reported: 2012-01-18 08:19 PST by :Aryeh Gregor (away until August 15)
Modified: 2014-12-28 05:32 PST (History)
3 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (18.92 KB, patch)
2012-02-13 08:31 PST, :Aryeh Gregor (away until August 15)
no flags Details | Diff | Splinter Review
Patch v2, rebased (18.97 KB, patch)
2012-06-26 07:40 PDT, :Aryeh Gregor (away until August 15)
dbaron: review+
Details | Diff | Splinter Review

Description :Aryeh Gregor (away until August 15) 2012-01-18 08:19:23 PST
The CSS Transform specs give the syntax of matrix() and matrix3d() as taking <number>s only:

matrix(<number>, <number>, <number>, <number>, <number>, <number>)
http://dev.w3.org/csswg/css3-transforms/#transform-functions

matrix3d(<number>, <number>, <number>, <number>, <number>, <number>, <number>, <number>, <number>, <number>, <number>, <number>, <number>, <number>, <number>, <number>)
http://dev.w3.org/csswg/css3-3d-transforms/#transform-functions

Firefox 12.0a1 (2012-01-17) incorrectly accepts <length> as well as <number> for the translation parts of these matrices, like matrix(1, 0, 0, 1, 10px, 10px).  Simple test-case:

data:text/html,<!doctype html>
<div style=-moz-transform:matrix(1,0,0,1,10px,10px)></div>
<script>
document.body.textContent=
document.querySelector("div").style.MozTransform
</script>

This should output the empty string.  It doesn't in Firefox, but does in all other browsers I tested, per spec.  This causes Firefox to fail some of the CSS Transforms tests I'm submitting to the W3C.

(Relatedly, the computed value of transform should not include "px", but I'm not testing that yet because it's not totally clear in the spec: https://www.w3.org/Bugs/Public/show_bug.cgi?id=15431)
Comment 1 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-01-18 09:48:08 PST
We should change this in the same release that we unprefix.  (Though I think maybe we should try to convince other browsers to change.)
Comment 2 Matt Woodrow (:mattwoodrow) 2012-01-18 19:48:32 PST
This was definitely an intentional choice to provide flexibility for authors, consistency with the translate() functions, and consistency with our existing matrix() implementation.

I'd prefer to see this allowed in the spec than remove it from our code.
Comment 3 :Aryeh Gregor (away until August 15) 2012-01-19 08:16:49 PST
If other engines aren't interested in supporting it, better that there be fewer features that are interoperably supported than more features that aren't.  People should be able to write a page that works in Gecko and have it work in other browsers too.  Having to manually convert lengths or percentages to pixels, or use translate() instead of matrix(), is a lot less annoying to authors than having pages work differently in different browsers.

Is there evidence that authors use this feature?  Are there good use-cases?  I'd think most authors would use translate() and so on, not matrix().  If you're hard-core enough to use matrix(), you're probably able to work around the fact that it only accepts pixels without too much trouble.  If the feature doesn't have clear value, it's going to be a lot easier for Gecko to just drop it than to persuade all the other implementations to add it.

Anyway, I filed a spec bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=15628

See also bug 719446.
Comment 4 :Aryeh Gregor (away until August 15) 2012-02-08 08:37:39 PST
The spec bug was resolved WONTFIX.
Comment 5 :Aryeh Gregor (away until August 15) 2012-02-09 07:37:05 PST
(In reply to David Baron [:dbaron] (busy through 12 Feb) from comment #1)
> We should change this in the same release that we unprefix.

Why not now?
Comment 6 :Aryeh Gregor (away until August 15) 2012-02-13 08:31:13 PST
Created attachment 596692 [details] [diff] [review]
Patch v1

Not requesting review for now because of comment 1, but it's still worth having a patch ready.  Honestly, we should have unprefixed like a year ago anyway . . .
Comment 7 Mozilla RelEng Bot 2012-02-13 08:36:49 PST
Autoland Patchset:
	Patches: 596692
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=9e63d34df962
Try run started, revision 9e63d34df962. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=9e63d34df962
Comment 8 Mozilla RelEng Bot 2012-02-13 15:00:30 PST
Try run for 9e63d34df962 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=9e63d34df962
Results (out of 211 total builds):
    success: 179
    warnings: 18
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-9e63d34df962
Comment 9 :Aryeh Gregor (away until August 15) 2012-06-26 07:40:58 PDT
Created attachment 636709 [details] [diff] [review]
Patch v2, rebased

Applies cleanly again.  We're looking to unprefix transforms soon, so I think this should be reviewable -- or should we hold off till 17?

Try: https://tbpl.mozilla.org/?tree=Try&rev=b131a2283228
Comment 10 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-07-10 16:30:00 PDT
Comment on attachment 636709 [details] [diff] [review]
Patch v2, rebased

>diff --git a/layout/reftests/forms/progress/transformations-ref.html b/layout/reftests/forms/progress/transformations-ref.html
>--- a/layout/reftests/forms/progress/transformations-ref.html
>+++ b/layout/reftests/forms/progress/transformations-ref.html
>@@ -1,14 +1,14 @@
> <!DOCTYPE html>
> <html>
>   <link rel='stylesheet' type='text/css' href='style.css'>
>   <style>
>     body > div:nth-child(1) { -moz-transform: matrix(1, -0.2, 0, 1, 0, 0); }
>-    body > div:nth-child(2) { -moz-transform: matrix(1, 0, 0.6, 1, 15em, 0); }
>+    body > div:nth-child(2) { -moz-transform: matrix(1, 0, 0.6, 1, 240, 0); }

I'd suggest making this a matrix(1, 0, 0.6, 1, 0, 0) combined with a translateX(15em), in the correct order (whichever that is).

>diff --git a/layout/reftests/forms/progress/transformations.html b/layout/reftests/forms/progress/transformations.html

Same here.

And I do want this to land for 16 along with the unprefixing.  Thanks for doing this.


r=dbaron
Comment 11 :Aryeh Gregor (away until August 15) 2012-07-11 00:52:44 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e3c67b2d95d
Comment 12 Ed Morley [:emorley] 2012-07-11 09:30:43 PDT
https://hg.mozilla.org/mozilla-central/rev/2e3c67b2d95d

Note You need to log in before you can comment on or make changes to this bug.