455403 - -moz-transform translate functions result in incorrect origin

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Keith Schwarz [:kschwarz]]

Webkit's spec states that when working with the -moz-transform property, the transform origin should always be constant throughout the transform.  That is, if the origin is set to the middle of the element's box, any transformation uses the middle of the element's box as the transform origin.

-moz-transform works by caching the net transformation matrix obtained by accumulating all of the transforms together into one giant matrix, then applying that matrix to the elements.  For example, given matrices A, B, and C, to be applied in that order, we should accumulate those together into the matrix CBA.

However, this leads to problems when we mix translations into the mix.  When the translations are introduced into the middle of the equation, the invariant that the transform must be applied to the same origin of the box no longer holds.  For example, suppose we have -moz-transform: translate(100px, 100px) rotate(45deg).  This _should_ translate the object over 100px down and right, then rotate it 45deg around its center.  However, this currently translates the element 100px down and right, then rotates it 45 degrees around the location that the element originally was.

The problem can best be stated using matrix math.  Suppose T is our translation matrix and R is the rotate matrix.  Then the net matrix for this operation is RT; that is, translate, then rotate.  However, the origin for the rotation is wrong - we need to move the origin by the same amount we translated!  Thus, what we want to do is translate, then move the origin up when working with the rotation.  This is equivalent to a change of basis operation that moves back from the translated space to the origin, applies the rotation, then translates back.  In matrix terms, this is

(T)(R)(T-1)(T)

But (T-1)(T) = identity, so this simplifies down to

TR

Meaning that we need to apply all translations _after_ we apply the other transformations.  I'm currently working on a patch to fix this; it should be ready soon.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This seems pretty odd.  I'd have expected the order of the transforms to be significant for mixing translates with other transforms, just like it is for, say, mixing skew and rotate.

Comment 2

[Keith Schwarz [:kschwarz]]

(In reply to comment #1)
> This seems pretty odd.  I'd have expected the order of the transforms to be
> significant for mixing translates with other transforms, just like it is for,
> say, mixing skew and rotate.

I agree - it seems like it makes more sense mathematically.  However, I can see why it might not be intuitive for developers - having the translation axes always in the original coordinate space does simplify some calculations.  Perhaps we should bring this up with the Webkit group?

Comment 3

[Keith Schwarz [:kschwarz]]

Attachment: Potential Patch #1

Fixes the issue by deferring translation transforms until after all other transforms have already been applied.  Because of subpixel rounding errors I'm hesitant to post this with attached reftests; if someone has an idea of how to add tests to prevent regressions and ensure correctness, I would be very appreciative.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Can't you test by comparing one -moz-transform+-moz-transform-origin to another?

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And did you test that this is actually what WebKit does, not just what the spec says?

Comment 6

[Keith Schwarz [:kschwarz]]

I just checked with a Webkit-based browser and it does seem to have this behavior.  Interestingly, doing a translate without a second parameter had it duplicate the first parameter, rather than defaulting to zero... I should go make a note of this on the appropriate bug...

Your testing idea sounds like it should work.  I'll try to get some reftests up when I get the chance.

Comment 7

[Dean Jackson]

> Interestingly, doing a translate without a second parameter had it
duplicate the first parameter, rather than defaulting to zero... I should go
make a note of this on the appropriate bug...

Are you sure? translate(10px) is equivalent to translate(10px, 0) on WebKit TOT.

IIRC, I fixed this bug since Safari 3.1 so you might need a nightly.

Comment 8

[Dean Jackson]

On transform origin:

You can think of it as two translation operations that occur each side of the real transform property. These translates are calculated before the real transform applies.

So, in your example above it is  O T R O' where O is the origin translate and O' is the inverse. You first translate the coordinate system by the origin, apply the real transform, and then undo the origin.

Does this make sense/help?

Sorry if the spec isn't so clear here. I'll look into it.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

To ask the same question in another way:

Say we have:

div { width: 100px; height: 100px;
      transform-origin: 0 0 /* doesn't matter as long as it's the same */ }

Is it always true that

<div style="transform: func1() func2()">
  stuff in here
</div>

is always the same as:

<div style="transform: func1()">
  <div style="transform: func2()">
    stuff in here
  </div>
</div>

?

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: examples testing that question

So if the invariant I stated in comment 9 is correct, then (1) and (3) should be the same, and (2) and (4) should be the same.  That's what WebKit does, but not what we do.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: examples (without translate) testing that question

Here we do follow the invariant I stated, though.  (Can't test in my WebKit, since it doesn't seem to support skewX.)

(Annoyingly, I had to use different skew syntax between Webkit and Gecko since the WebKit I have doesn't support skewX, and we don't seem to support two-parameter skew().)

Comment 12

[Keith Schwarz [:kschwarz]]

(In reply to comment #9)
> To ask the same question in another way:
> 
> Say we have:
> 
> div { width: 100px; height: 100px;
>       transform-origin: 0 0 /* doesn't matter as long as it's the same */ }
> 
> Is it always true that
> 
> <div style="transform: func1() func2()">
>   stuff in here
> </div>
> 
> is always the same as:
> 
> <div style="transform: func1()">
>   <div style="transform: func2()">
>     stuff in here
>   </div>
> </div>
> 
> ?

If that's the case, then I think all we need to do is apply the transforms in the inverse order than the initial patch (e.g. last transform first, rather than first transform first).  Does this seem correct?  If so, I can fix that pretty easily (and apologize for misinterpreting the spec the first time around.)

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 338749 [details] [diff] [review]
Potential Patch #1

I don't think this (separation of the affine and linear parts of the transformation) is what we want to do.

I'm hoping Dean can confirm, though.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #12)
> If that's the case, then I think all we need to do is apply the transforms in
> the inverse order than the initial patch (e.g. last transform first, rather
> than first transform first).  Does this seem correct?  If so, I can fix that
> pretty easily (and apologize for misinterpreting the spec the first time
> around.)

Except you're doing the right thing already for the skew+rotate cases.

I'm finding the matrix multiplication code a little hard to follow since nsStyleTransformMatrix::operator*= implements aOther * this rather than this * aOther, and thus operator* implements B * A.  Given that the operator *= does a copy at the end anyway, I wonder if it would be clearer to implement only operator*.  (That may depend on how efficient the compiler is when it's returning a struct.)

I'd also note that the SVG spec does explicitly confirm what I suggested in comment 9 in http://www.w3.org/TR/SVG11/coords.html#TransformAttribute .

Comment 15

[Dean Jackson]

Yes, comment #9 is correct. In both your attached examples (1) == (3) and (2) == (4).
This is the same behaviour as SVG.

I haven't looked at your code, but I wonder if you are multiplying the matrices in the right order. Like SVG, you want to left-multiply when you have a list of functions. This gives you the same result as nesting.

Also, skewX works on WebKit nightlies.

Comment 16

[Dean Jackson]

Oh, David says the same thing about matrix multiplication order in comment #14. Sorry!

Comment 17

[Keith Schwarz [:kschwarz]]

I think I've tracked down the source of the bug to two errors that masked each other and led to odd results:

1. I was multiplying the transformation matrices in the wrong order (e.g. right-to-left instead of left-to-right)
2. I was switching two of the matrix entries when converting from nsStyleTransformMatrix to gfxMatrix (I even commented this one) because of a typo at the top of gfxMatrix.h.

I'll get a new set of reftests posted soon, along with a revised potential patch.

Comment 18

[Keith Schwarz [:kschwarz]]

Attachment: Potential Patch #2

Fixes the issue by correcting the buggy matrix math and bad nsStyleTransformMatrix -> gfxMatrix conversions.  Corrects some reftests that relied on the backwards behavior, and added some reftests that should prevent a regression.  It might be worth testing the reftests on another system, since some of the other tests seem to be easily affected by subpixel rounding errors.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 339022 [details] [diff] [review]
Potential Patch #2

>    * Multiplies this matrix by another matrix.  The multiplication is
>-   * a post-multiplication, so A *= B updates this matrix to be BA.
>+   * a pre-multiplication (as opposed to gfxMatrix's post-multiplication),
>+   * so A *= B yields the matrix AB.

...the differences are also because the gfxMatrix and the cairo code think about vectors as rows (which I think is unusual; there's even a comment in cairo-matrix about it:
http://hg.mozilla.org/mozilla-central/annotate/50ffd2d5b96a/gfx/cairo/cairo/src/cairo-matrix.c#l287 )
whereas the SVG and transform specs and nsStyleTransformMatrix think about vectors as columns (which is what I'd seen before doing linear algebra).

I don't think saying that the difference is pre-multiplication vs. post-multiplication quite makes sense; I think that both multiplication operations are the post-multiplication; it's just the gfxMatrix matrices are the other way around because they're formatted to work with row vectors, and thus they have to be multiplied in the other order to achieve the same effect in terms of transformations.


Could you also fix the very first matrix drawing in gfxMatrix.h, since it's clearly wrong?  (Probably to put the tx and ty at the bottom like the drawing right below it, since that's the way things are done in gfxMatrix.)

Comment 20

[Keith Schwarz [:kschwarz]]

Attachment: Potential Patch #3

Updated patch that fixes the matrix diagram in gfxMatrix.h, clarifies what mathematical operation the nsStyleTransformMatrix uses, and fixes some comments comparing pre- and post-multiplication of matrices.

No functional code changes from the previous patch.

Comment 21

[Keith Schwarz [:kschwarz]]

Attachment: Potential Patch #3 (correction)

Ooops... Patch 3 had a minor comment error in it.  Fixed in this version.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 339113 [details] [diff] [review]
Potential Patch #3 (correction)

You didn't need to ask for review again.  (For me, at least, the distinction between review- and review+ is whether you need to ask for review again.)  However, given that you did, I'll make one comment:
  
>+   * Multiplies this matrix by another matrix. The multiplication is
>+   * a pre-multiplication, so the resulting matrix will apply the transforms
>+   * of the other matrix before applying the transforms from this matrix.

I find "before" and "after" with respect to transforms confusing, since transforming a point goes from the inside-out, but I often think about transforms as outside-in.

I'm also not sure that pre-multiplication and post-multiplication are particularly clear.  I'd say something more like "Multiplies this matrix by another matrix, in that order, so that after A *= B, the new value of A is the old AB.  However, since we're dealing with column vectors, this means B is applied to a point first (on the inside) and A is applied second (on the outside), since (AB)v is A(Bv)."

Maybe something similar for the operator* as well.

But if you prefer what you've written, that's fine too.

Comment 23

[Keith Schwarz [:kschwarz]]

Attachment: Potential Patch #4

Updated comments to be mathematically explicit about what the * and *= operators do.  Hopefully this makes it clear what the operations do.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Landed: http://hg.mozilla.org/mozilla-central/rev/aab6b12f4a2b

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Backed out.

The reftests hung; I have no idea why.

They would also have failed without landing some of the other patches first.

Comment 26

[Keith Schwarz [:kschwarz]]

Looking at the tinderbox log, it seems like the tests that are hanging the program are in the text-transform/ directory, not the transform/ directory, which houses the -moz-transform tests.  Are you sure this patch is what caused the problem?

Also, which patches need to land before this one?  Should we mark them as depends-on?

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #26)
> Looking at the tinderbox log, it seems like the tests that are hanging the
> program are in the text-transform/ directory, not the transform/ directory,
> which houses the -moz-transform tests.  Are you sure this patch is what caused
> the problem?

The last thing it reported was a pass on the very last test in the text-transform/ directory, so the next thing to run would be the first test in the transform/ directory, based on the master reftest.list.

Comment 28

[Keith Schwarz [:kschwarz]]

The reftest hang is caused by the skew(45deg) transform [using the version of skew that only accepts one parameter).  This causes the following errors occur and the reftest to stop responding:

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file ../../../../../mozilla-central/content/html/content/src/nsHTMLCanvasElement.cpp, line 396
JavaScript error: , line 0: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMHTMLCanvasElement.toDataURL]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://reftest/content/reftest.js :: DocumentLoaded :: line 469"  data: no]

I'm guessing that the problem is that a skew(45deg) transform results in a singular matrix

| 1 1 0 |
| 1 1 0 |
| 0 0 1 |

Which might do bad things to Thebes.  I'm currently checking to see what happens when applying the patch that fixes the two-parameter skew.  If that doesn't resolve it (which I doubt it will), I'll post a fix to this patch that swaps out the skew(45deg) for something else and then will file a separate bug.

Comment 29

[Keith Schwarz [:kschwarz]]

It looks like singular matrices with -moz-transform cause reftests to fail.  I'm filing as a separate bug and will change the patch so that the reftests don't have any singular transforms in them.

Comment 30

[Keith Schwarz [:kschwarz]]

Attachment: Potential Patch #5 [Checkin: Comment 31]

Fixes Potential Patch #4 to not use singular matrices in the reftests.  That bug has been filed as Bug 456163.  The patch currently works on my Linux and Windows builds, so hopefully it won't hurt the tree again.

Comment 31

[Serge Gautherie (:sgautherie)]

Comment on attachment 339552 [details] [diff] [review]
Potential Patch #5
[Checkin: Comment 31]

http://hg.mozilla.org/mozilla-central/rev/bee5256b90e1