Closed Bug 719054 Opened 10 years ago Closed 9 years ago

matrix() and matrix3d() with length units should be parse errors


(Core :: CSS Parsing and Computation, defect)

Not set





(Reporter: ayg, Assigned: ayg)


(Depends on 1 open bug)


(Keywords: dev-doc-complete, Whiteboard: [DocArea=CSS])


(1 file, 1 obsolete file)

The CSS Transform specs give the syntax of matrix() and matrix3d() as taking <number>s only:

matrix(<number>, <number>, <number>, <number>, <number>, <number>)

matrix3d(<number>, <number>, <number>, <number>, <number>, <number>, <number>, <number>, <number>, <number>, <number>, <number>, <number>, <number>, <number>, <number>)

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>

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:
We should change this in the same release that we unprefix.  (Though I think maybe we should try to convince other browsers to change.)
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.
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:

See also bug 719446.
The spec bug was resolved WONTFIX.
(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?
Attached patch Patch v1 (obsolete) — Splinter Review
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 . . .
Assignee: nobody → ayg
Whiteboard: [autoland]
Whiteboard: [autoland] → [autoland-in-queue]
Autoland Patchset:
	Patches: 596692
	Branch: mozilla-central => try
Try run started, revision 9e63d34df962. To cancel or monitor the job, see:
Try run for 9e63d34df962 is complete.
Detailed breakdown of the results available here:
Results (out of 211 total builds):
    success: 179
    warnings: 18
    failure: 14
Builds (or logs if builds failed) available at:
Whiteboard: [autoland-in-queue]
Applies cleanly again.  We're looking to unprefix transforms soon, so I think this should be reviewable -- or should we hold off till 17?

Attachment #596692 - Attachment is obsolete: true
Attachment #636709 - Flags: review?(dbaron)
Keywords: dev-doc-needed
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.

Attachment #636709 - Flags: review?(dbaron) → review+
Flags: in-testsuite+
Target Milestone: --- → mozilla16
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 774169
Depends on: 774475
Whiteboard: [DocArea=CSS]
You need to log in before you can comment on or make changes to this bug.