Specifying multiple transforms loses precision in the translate part

RESOLVED FIXED in mozilla17

Status

()

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

People

(Reporter: ayg, Assigned: froydnj)

Tracking

Trunk
mozilla17
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Test-case:

data:text/html,<!doctype html>
<div style="-moz-transform:
translateX(-10px)
scaleX(1000)
translateX(0.01px)
scaleX(0.001)
"></div>
<script>
document.body.textContent =
getComputedStyle(document.querySelector("div"))
.MozTransform
</script>

Mathematically, this should be the identity matrix.  IE9 is "matrix(1, 0, 0, 1, -2.23517e-007, 0)", and Chrome 17 dev is "matrix(1, 0, 0, 1, -0.00000022351741790771484, 0)", both of which are reasonable.  Firefox 12.0a1 (2012-01-17), however, is "matrix(1, 0, 0, 1, 6.66667px, 0px)", which is over six pixels off.  (Opera Next 12.00 alpha is even worse: "matrix(1, 0, 0, 1, -10, 0)".)

It's reasonable if there are errors that are small fractions of a pixel at the end of the day, but they shouldn't accumulate like this.  I hit this sort of rounding error while fuzz-testing with composing various random-ish matrices; I suspect it won't be uncommon for it to add up to a few pixels in real pages, if people compose a reasonably long chain of matrices.  Can we use higher precision for intermediate computations?

This is causing Gecko test failures in my CSS transform tests, where I set a tolerance of 0.05px for getComputedStyle() and Gecko slips above that in some cases: http://hg.csswg.org/test/file/e617571dec78/contributors/aryehgregor/incoming
We're presumably storing the lengths in nscoord units, which are accurate to 1/60px.  So we can represent 0.016666px and we can represent 0px, but nothing in between.  Hence the observed 6.66667px of error, after multiplying by 1000.

This isn't a matter of intermediate computations; the actual computed value storage is almost certainly in nscoord.

I agree that for the transform case we should perhaps reconsider this....
If you compute "translateX(-10px) scaleX(1000) translateX(0.01px) scaleX(0.001)" exactly, it works out to the identity matrix.  So it's not bad for this purpose if the computed value is stored imprecisely -- the identity matrix can be stored exactly!  But the result doesn't work out to the identity in Gecko in the first place, which appears to be because the transformations are converted to imprecise matrices *before* they're composed, rather than after.

I'm not objecting if the computed matrix is *also* stored more precisely, though.
The computed value is a list of transformations, not a single matrix.  Otherwise transitions of transforms would not work right, for example.
Hmm, right.  So it's only the *resolved* value that's a single matrix.  It's confusing that authors don't have direct access to computed values, and the way of getting resolved values is called getComputedStyle().

In that case, yes, I think it makes sense to store the computed values here with more precision.  As it stands, it's easy to write reasonable-looking transforms that behave completely differently in Gecko vs. IE/WebKit due to rounding errors.
Storing floats seems like a good idea. It would reduce float<->int conversions too.
(Assignee)

Comment 6

5 years ago
Created attachment 603350 [details] [diff] [review]
handling pixels and numbers directly in ProcessTranslatePart

Simply handling pixels and numbers directly in ProcessTranslatePart so that we don't effectively do float((nscoord)(pixel * aAppUnitsPerPixel))/aAppUnitsPerPixel is enough to give us the same answers as Chrome here.

I assume just using GetFloatValue directly isn't the Way Things Are Done, though; should there be some sort of wrapper in nsPresContext for this?
Attachment #603350 - Flags: feedback?(bzbarsky)
Comment on attachment 603350 [details] [diff] [review]
handling pixels and numbers directly in ProcessTranslatePart

Oh, so this _was_ a case of intermediate computations, sort of.

Is aAppUnitsPerMatrixUnit guaranteed to be the same as application-wide number of app units in a CSS px?  Worth checking with someone who really knows this code...  In either case, the whole "percent" thing there is pointless, right?  And if you do this then CalcLength in that file can go away.

That said, this will also give differences in behavior when translate() is used vs when, say, relative positioning is used.  That may be ok....
Attachment #603350 - Flags: feedback?(bzbarsky) → feedback?(matt.woodrow)
(In reply to Boris Zbarsky (:bz) from comment #7)
> That said, this will also give differences in behavior when translate() is
> used vs when, say, relative positioning is used.  That may be ok....

What's an example?
<div style="position: relative; left: -10px">
  <div style="-moz-transform: scaleX(1000)">
    <div style="position: relative; left: 0.01px;">
      <div style="-moz-transform: scaleX(0.001)">

or so
Ah, right.
Comment on attachment 603350 [details] [diff] [review]
handling pixels and numbers directly in ProcessTranslatePart

Review of attachment 603350 [details] [diff] [review]:
-----------------------------------------------------------------

Unfortunately aAppUnitsPerMatrixUnit isn't always fixed, it's affected by zooming. Though the callers of ReadTransforms seem really inconsistent here, the a lot are fixed to nsPresContext::AppUnitsPerCSSPixel(), others are checking AppUnitsPerDevPixel(), this might be worth checking as a follow-up.

For this I think we can just multiply by the ratio between aAppUnitsPerMatrixUnit and nsPresContext::AppUnitsPerCSSPixel().

As bz suggested, the percent piece here will always be 0.

Idea looks good though!
Attachment #603350 - Flags: feedback?(matt.woodrow) → feedback+
(Assignee)

Comment 12

5 years ago
Created attachment 604394 [details] [diff] [review]
handling pixels and numbers directly in ProcessTranslatePart

Here's a cleaned up version implementing Matt's suggestions and also ditching CalcLength.
Attachment #603350 - Attachment is obsolete: true
Attachment #604394 - Flags: review?(bzbarsky)
Attachment #604394 - Flags: feedback+
(Assignee)

Updated

5 years ago
Whiteboard: [autoland-try:-b do -p all -u all -t none]

Updated

5 years ago
Whiteboard: [autoland-try:-b do -p all -u all -t none] → [autoland-in-queue]

Comment 13

5 years ago
Autoland Patchset:
	Patches: 604394
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=fe93ca799c91
Try run started, revision fe93ca799c91. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=fe93ca799c91

Comment 14

5 years ago
Try run for fe93ca799c91 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=fe93ca799c91
Results (out of 216 total builds):
    exception: 7
    success: 168
    warnings: 40
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-fe93ca799c91

Updated

5 years ago
Whiteboard: [autoland-in-queue]
Comment on attachment 604394 [details] [diff] [review]
handling pixels and numbers directly in ProcessTranslatePart

>+++ b/layout/style/nsStyleTransformMatrix.cpp
>+    return aValue.GetFloatValue() *
>+           (aAppUnitsPerMatrixUnit / float(nsPresContext::AppUnitsPerCSSPixel()));

I believe the division is backwards.  The old code ended up doing:

  NSAppUnitsToFloatPixels(
    nsPresContext::CSSPixelsToAppUnits(aValue.GetFloatValue()),
    aAppUnitsPerMatrixUnit
  )

That corresponds to multiplying the result of GetFloatValue() by
nsPresContext::AppUnitsPerCSSPixel and then dividing by aAppUnitsPerMatrixUnit,
no?

r=me with that fixed and ideally a test added that would have caused the backwards division.
Attachment #604394 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 16

5 years ago
(In reply to Boris Zbarsky (:bz) from comment #15)
> Comment on attachment 604394 [details] [diff] [review]
> handling pixels and numbers directly in ProcessTranslatePart
> 
> >+++ b/layout/style/nsStyleTransformMatrix.cpp
> >+    return aValue.GetFloatValue() *
> >+           (aAppUnitsPerMatrixUnit / float(nsPresContext::AppUnitsPerCSSPixel()));
> 
> I believe the division is backwards.

I may have misunderstood Matt's explanation; I also failed to actually think about what the code was doing. :)

I will try to put together a testcase.
(Assignee)

Comment 17

5 years ago
I haven't been able to generate testcases, so was going to check in the patch.  However, this try run:

https://tbpl.mozilla.org/?tree=Try&rev=292acc29933e

showed reftest failures with the patch modified as per bz's review in comment 15.  I haven't investigated, though I do wonder if there's some subtle rounding behavior at work here.
(Assignee)

Comment 18

5 years ago
Created attachment 646114 [details] [diff] [review]
handling pixels and numbers directly in ProcessTranslatePart

Updated per bz's review.
Attachment #604394 - Attachment is obsolete: true
Attachment #646114 - Flags: review+
(Assignee)

Comment 19

5 years ago
Another try run showed all green for reftests:

https://tbpl.mozilla.org/?tree=Try&rev=b07993954399

so I'm not sure what was wrong with the run in comment 17.  Maybe an intermittent orange?

Checked in:

http://hg.mozilla.org/integration/mozilla-inbound/rev/363045309cb0
Assignee: nobody → nfroyd
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/363045309cb0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.