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

RESOLVED FIXED in mozilla16

Status

()

Core
CSS Parsing and Computation
--
minor
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

(Depends on: 1 bug, {dev-doc-complete})

Trunk
mozilla16
x86
Linux
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DocArea=CSS])

Attachments

(1 attachment, 1 obsolete attachment)

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)
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: https://www.w3.org/Bugs/Public/show_bug.cgi?id=15628

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?
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 . . .
Assignee: nobody → ayg
Whiteboard: [autoland]

Updated

5 years ago
Whiteboard: [autoland] → [autoland-in-queue]

Comment 7

5 years ago
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

5 years ago
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

Updated

5 years ago
Whiteboard: [autoland-in-queue]

Updated

5 years ago
Blocks: 745523
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
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.


r=dbaron
Attachment #636709 - Flags: review?(dbaron) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e3c67b2d95d
Flags: in-testsuite+
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/2e3c67b2d95d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 774169
Depends on: 774475
Whiteboard: [DocArea=CSS]
Updated:
https://developer.mozilla.org/en-US/docs/Web/CSS/transform-function#matrix%28%29
https://developer.mozilla.org/en-US/docs/Web/CSS/transform
And
https://developer.mozilla.org/en-US/Firefox/Releases/16#CSS
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.