Open Bug 732153 Opened 13 years ago Updated 2 years ago

getComputedStyle().transform should return the original list of transforms, not a matrix

Categories

(Core :: CSS Parsing and Computation, defect, P4)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

People

(Reporter: ayg, Unassigned)

References

Details

Attachments

(1 file)

Patch part 1 -- Fix unrelated bug
3.05 KB, patch
dbaron
: review+
lsblakk
: approval-mozilla-aurora-
Details | Diff | Splinter Review 
See here: <https://www.w3.org/Bugs/Public/show_bug.cgi?id=15797#c10>.  The Transforms editors think that the resolved value for 'transform' should match the computed value, so "translateX(10px) scale(2)" should be returned as such from getComputedStyle() instead of as "matrix(2, 0, 0, 2, 10, 0)".  Do we want to do this?  Apparently WebKit is interested in changing.  We have complete interop on the current behavior, but it doesn't match what's inherited -- in particular, it resolves percents, but they aren't resolved for inheritance.

(The spec hasn't been changed yet.  If you don't want it to change, now is a good time to say so.)
Severity: enhancement → normal
Priority: -- → P4
Blocks: 745523
I think this is actually pretty simple since we carry around the full list of transforms in computed style (and have since 3-D transforms landed, I think).  So the necessary changes should be entirely localized to nsComputedDOMStyle::DoGetTransform (in layout/style/nsComputedDOMStyle.cpp).

(Are you sure our behavior for percentages is correct, though?)

And presumably we already have a way to serialize this stuff in nsCSSValue.  The interesting question is whether the serialization you get is actually appropriate for computed style or whether there's a bunch of grunt-work needed to fix it up somehow.
(In reply to David Baron [:dbaron] from comment #1)
> think).  So the necessary changes should be entirely localized to
> nsComputedDOMStyle::DoGetTransform (in layout/style/nsComputedDOMStyle.cpp).

er, at least the base of the changes should be localized there, but it might require changes to called functions per my later paragraph
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Summary: getComputedStyle().MozTransform should return the original list of transforms, not a matrix → getComputedStyle().transform should return the original list of transforms, not a matrix
The test-case should be reasonably self-explanatory.  Current builds return "none" only when !HasTransform(), which is true not only if the computed transform is none, but also if 'backface-visibility: hidden' or 'transform-style: preserve-3d' is set.  This means that the computed transform of an element with one of these properties set is 'matrix(1, 0, 0, 1, 0, 0)' instead of 'none'.  I found this out the hard way when an initial pass at the patch crashed with null pointer dereference.  :)

So as far as the actual patch: nsCSSValueList::AppendToString would work, except that it will serialize lengths as-is, and we have to serialize all lengths as px.  Other methods in the class that have to do this look like they use SetValueToCoord, but this works with nsROCSSPrimitiveValue, not nsCSSValueList.  So I could do this by iterating over the nsCSSValueList, grabbing each function in turn, filtering its values in turn through SetValueToCoord, populating a new function with the result, and then using AppendToString.  But this seems awfully roundabout.  Is there a better way to do this, or is that how I should proceed?
Attachment #639353 - Flags: review?(dbaron)
Comment on attachment 639353 [details] [diff] [review]
Patch part 1 -- Fix unrelated bug

r=dbaron.  Sorry for the delay.

(Should we try to get this in to branches?  I think it's a pretty recent regression.)
Attachment #639353 - Flags: review?(dbaron) → review+
At a glance, looks like bug 531344, which was mozilla2.0b2.  So it should be quite ancient.  But I don't see the bug actually occur in 13, so maybe it is recent.  Simple test-case:

data:text/html,<!doctype html>
<div style="backface-visibility: hidden"></div>
<script>
document.body.textContent =
getComputedStyle(document.body.firstChild).MozTransform
</script>
</script>

This outputs "none" in 13, as it should, but "matrix(1, 0, 0, 1, 0, 0)" in a nightly, which is the bug.


Anyway, if you would like me to work on the actual bug here, I need you to answer this:

(In reply to :Aryeh Gregor from comment #3)
> So as far as the actual patch: nsCSSValueList::AppendToString would work,
> except that it will serialize lengths as-is, and we have to serialize all
> lengths as px.  Other methods in the class that have to do this look like
> they use SetValueToCoord, but this works with nsROCSSPrimitiveValue, not
> nsCSSValueList.  So I could do this by iterating over the nsCSSValueList,
> grabbing each function in turn, filtering its values in turn through
> SetValueToCoord, populating a new function with the result, and then using
> AppendToString.  But this seems awfully roundabout.  Is there a better way
> to do this, or is that how I should proceed?
https://hg.mozilla.org/integration/mozilla-inbound/rev/cda0a576a7a8

(I should have made that a separate bug so it could have its own resolution.)
Whiteboard: [Leave open]
(In reply to Aryeh Gregor from comment #5)
> This outputs "none" in 13, as it should, but "matrix(1, 0, 0, 1, 0, 0)" in a
> nightly, which is the bug.
"none" in Gecko 15, "matrix(1, 0, 0, 1, 0, 0)" in Gecko 16.
Comment on attachment 639353 [details] [diff] [review]
Patch part 1 -- Fix unrelated bug

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Unknown.  Something in Firefox 16.
User impact if declined: Probably not a lot, to be honest.  It's a regression, but a very small one: getComputedStyle() returns a different but equivalent-ish value in certain relatively marginal cases.  It's unlikely that any sites are depending on this, and we have no bug reports to suggest they are, but of course we can't say for sure.
Testing completed (on m-c, etc.): None yet.
Risk to taking this patch (and alternatives if risky): Patch is one line and quite unlikely to cause trouble, which is why I'm suggesting it even though the regression is very minor.
String or UUID changes made by this patch: None.

Basically, this is a low-risk fix for a very minor regression.  IIUC, that makes it fair game for Aurora, although not Beta (which didn't regress anyway according to Neil).
Attachment #639353 - Flags: approval-mozilla-aurora?
Oh -- d'oh!  Of course, my test uses unprefixed attributes.  This is a correct test:

data:text/html,<!doctype html>
<div style="-moz-transform-style:preserve-3d"></div><script>
document.body.textContent =
getComputedStyle(document.body.firstChild).MozTransform
</script>

That outputs "matrix(1, 0, 0, 1, 0, 0)" in 13.  Same with -moz-backface-visibility: hidden.  So if this is a regression, it's already very old.  The bugs you give landed in 10 and 13 respectively, and the latter was backported to 12 too.  Do we still want to backport it to Aurora anyway?
Assignee: ayg → nobody
Status: ASSIGNED → NEW
Whiteboard: [Leave open]
(In reply to :Aryeh Gregor from comment #11)
>Do we still want to backport it to Aurora
> anyway?

Haven't been approving this because of this outstanding question. David, can you address this?
Probably not worth backporting, then.
Comment on attachment 639353 [details] [diff] [review]
Patch part 1 -- Fix unrelated bug

Thanks David, minusing for Aurora in that case.
Attachment #639353 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Note, Bug #784095 was caused by this regression.
As I think you've found in that bug, this isn't a regression; we've always done this this way.  It's just the spec has changed since we implemented transforms originally.
Rotate and scale transforms being converted into matrix transforms is a very big deal to me, and to anyone who is basing JavaScript (most likely jQuery) animation on it. FireFox 16 needs to do this is a backwards-compatible way, by not modifying the transform property past what the user entered.

How my site is affected - http://support.mozilla.org/en-US/questions/941008
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: