572317 - -moz-transform depends on the window width -- bad rounding

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jonathan Protzenko [:protz]]

Testcase here:
http://jonathan.protzenko.free.fr/bugtransform/testcase.html

Basically, I have two squares perfectly stacked.

    ___
   | ^ |
   |___|
    ___
   |   |
   |___|

To the top one, I apply:

-moz-transform-origin: bottom left;
-moz-transform: rotate(-90deg) translate(-100px, 0);

The rotate gives:
 ___
| < |
|___|
     ___
    |   |
    |___|

And finally, the translate gives:

 ___  ___
| < ||   |
|___||___|


(The < shows the orientation of the top cube).

The issue is that the final layout depends on the viewport's width. Try to resize the main window pixel by pixel, and you will see that the position of the left cube is off-by-one half of the time.

Google Chrome does not have this issue, and always renders it properly.

Tested in 3.7a5.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Clearer testcase

Hmm.  So the issue appears when the left margin is nonintegral.  I believe the issue is that when we paint we snap the non-transformed div to screen pixels but don't snap the transformed one (because snapping there makes no sense).

Chrome "doesn't have this issue" because it never allows non-integer margins, even if you explicitly specify one (which leads it to have other bugs in terms of layout, like "auto" margins not actually centering correctly).  This testcase doesn't depend on the window width and shows the behavior clearly.

Over to graphics, since the layout here is in fact correct; the frame coordinates just happen to fall on non-integer pixel positions).

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We should be snapping the transformed div, actually. gfxContext::UserToDevicePixelSnapped should be modified so that if rotations by multiples of 90 degrees don't prevent snapping.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Like so

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 451759 [details] [diff] [review]
fix

>-    // rectangle is no longer axis-aligned after transforming, so we can't snap
>-    if (p1.x != p4.x ||
>-        p2.x != p3.x ||
>-        p1.y != p3.y ||
>-        p2.y != p4.y)
>-        return PR_FALSE;
>+    // Check that the rectangle is axis-aligned. For an axis-aligned rectangle,
>+    // two opposite corners define the entire rectangle. So check if
>+    // the axis-aligned rectangle with opposite corners p1 and p3
>+    // define an axis-aligned rectangle whose other corners are p2 and p4.
>+    if ((p2 == gfxPoint(p1.x, p3.y) && p4 == gfxPoint(p3.x, p1.y)) ||
>+        (p4 == gfxPoint(p1.x, p3.y) && p2 == gfxPoint(p3.x, p1.y))) {
>+        p1.Round();
>+        p3.Round();
> 

Clever

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: v2

Actually we can be even more clever and avoid computing p4.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/3a56b762ea04