Closed Bug 719054 Opened 13 years ago Closed 12 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla16

People

(Reporter: ayg, Assigned: ayg)

References

(Depends on 1 open bug)

Details

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

Attachments

(1 file, 1 obsolete file)

Patch v2, rebased
18.97 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
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?
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 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
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
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? Try: https://tbpl.mozilla.org/?tree=Try&rev=b131a2283228
Attachment #596692 - Attachment is obsolete: true
Attachment #636709 - Flags: review?(dbaron)
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
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 774169
Whiteboard: [DocArea=CSS]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: