591718 - getBoundingClientRect needs to take transforms into account

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect, P3)

Description

[Asa Dotzler [:asa]]

(from email to evangelism@moz)

I played around with rotation lately and created this
driving game using jquery:

http://marcfuehnen.com/car/ (watch in webkit first ...)

To handle my collision detection I need to rely on correct
offset values after the car has been rotated.

Everything's fine in webkit browsers (you can try it by
bumping into the 'no way' blocks) but in mozilla i guess
there is a problem with the offset values.

I did this experiment:

http://marcfuehnen.com/car/offset.htm

If you choose player and 'Get Offset' you will see that little
red dot marking the rotation's origin, and it's a different value
than shown in webkit browsers.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Need a testcase without huge gobs of JS gunk that has browser-specific codepaths...  Without that, hard to tell whether it's our issue or an issue in the gunk.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Also, looks like the old parser dropped all tags that were kids of <select> completely, while the new one drops the tags but lets their text kids be kids of the <select>?

If we do address this on the layout side, I'll need to know exactly what I can expect the new content model of <select> to look like.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Please ignore comment 2; it was meant for a different bug.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Looks like the relevant function ("offset") is:

function (a) {
    var b = this[0];
    if (!b || !b.ownerDocument) {
        return null;
    }
    if (a) {
        return this.each(function (e) {c.offset.setOffset(this, a, e);});
    }
    if (b === b.ownerDocument.body) {
        return c.offset.bodyOffset(b);
    }
    var d = b.getBoundingClientRect(), f = b.ownerDocument;
    b = f.body;
    f = f.documentElement;
    return {top: d.top + 
       (self.pageYOffset || c.support.boxModel && f.scrollTop || b.scrollTop) -
       (f.clientTop ||  b.clientTop || 0),
     left: d.left + 
     (self.pageXOffset || c.support.boxModel && f.scrollLeft || b.scrollLeft) -
     (f.clientLeft || b.clientLeft || 0) };
}

In Gecko, the getBoundingClientRect.top of the rotated rectangle is 244.5.  In webkit it's 250.

The relevant CSS declarations are:

  position:absolute;
  top: 200px;
  left: 225px;
  width: 48px;
  height: 24px;
  -moz-transform: rotate(45deg);
  -moz-transform-origin: left bottom;

and the containing block's bounding client rect is already different in the two browsers: ours has a top at 44.5 px while webkit's is at 43px (which makes sense, since that offset depends on the exact font metrics of the text used for "Content", and the default font seems to be different).

Past that, it looks like we set getBoundingClientRect().top to be 200px + 44.5px, for a total of 244.5px.  Webkit sets it to 200px + 24px - 16.97px + 43px for a total of 250.03, which they round to 250.  In other words, their bounding client rect puts the top at the position of the top-left corner of the rect _after_ the rotation, while ours is at the untransformed position.

I suppose getBoundingClientRect should take transforms into account, right?

Comment 5

[ezmilhouse]

"In other words, their
bounding client rect puts the top at the position of the top-left corner of the
rect _after_ the rotation, while ours is at the untransformed position."

Any suggestions how to fix that mathematically?

Comment 6

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

I'm actually not sure. This is un(der)specified. getBoundingClientRect() and getClientRects() expect the element to decompose into axis-aligned rectangles, so maybe it's better to not take transforms into account.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Not sure I follow the question in comment 5.  Fix what where?  In a web page, in Gecko, in Webkit?

roc, can we get Anne and the webkit guys to talk to us about this ASAP?  We haven't shipped transforms before, and it would be good to not change this behavior once we do so...

Comment 8

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

We've been shipping transforms since Firefox 3.5 / Mozilla 1.9.1.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Oh, ok.  I guess the recent thing was transitioning transforms....

In that case, I think we should just bring this up in whatever wg deals with this stuff nowadays and get it sorted out.  Maybe what we really need here is a new API to replace getBoundingClientRect(); asking for the "position" of a transformed rectangle isn't really a sensical operation...

(And I'm not sure how one can sanely do hit-testing on transformed elements using it; clearly the testcase in comment 0 is leaving out some key aspect of what's actually going on in the game.)

Comment 10

[ezmilhouse]

I don't know too many use cases where you might be interested in getting the "real" offset of a rectangle after it's been rotated, but in my case it got critical.

I guess when people start using css transforms more often they will also run into this problem. I don't know what's the best solution - the way webkit does it or the way mozilla does it, but I would like to get the same results in both browsers.

Therefore I wrote a little function that gives you the webkit results also in mozilla:

/*
 * MOZILLA OFFSET CORRECTION AFTER ROTATION
 * 
 * offset_x 	| rotation's turning point's offset (x)
 * offset_y 	| rotation's turning point's offset (y)
 * w			| width of rectangle
 * h			| height of rectangle
 * x			| relative distance of turning point to left side of rectangle as in -moz-transform-origin:12px 12px;
 * y			| relative distance of turning point to top side of rectangle as in -moz-transform-origin:12px 12px;
 * degree		| recatangle's rotation (in degree) as in -moz-transform:rotate(60deg);
 * round		| round results true/false
 * 
 * @return: array of all four corner coordinates of the rotated rectangle, starting with the real (webkit-like) offset 
 * (x= box[0][0], y= box[0][1]) and going on clockwise.
 * 
 */

function getOffsetAfterRotation(offset_x, offset_y, w, h, x, y, degree, round){
	
	var rad = -degree * Math.PI * 2.0 / 360.0;
	
	// move rectangle to (0,0) position
	var box = [[0,0],[0,h],[w,h],[w,0]].map(function(p){
		return [p[0]-x,p[1]-y]});

	// rotate rectangle
	function rotate(point){
		return [point[0]*Math.cos(rad) + point[1]*Math.sin(rad),
		        -point[0]*Math.sin(rad) + point[1]*Math.cos(rad)]
	}
	
	box = box.map(rotate);
	
	// return rectangle to turning point position
	box = box.map(function(p){
		return [p[0]+offset_x, p[1]+offset_y]});
	
	if(round){
		return box.map(function(p){return [Math.round(p[0]),
		                                   Math.round(p[1])]});
	} else{
		return box;
	}

}

See it in action here:
http://marcfuehnen.com/hacks/mozilla_offset_correction_after_rotation/

Maybe it helps ...

Thx for your time ...

Comment 11

[Boris Zbarsky [:bzbarsky]]

> the "real" offset

Which offset do you mean by that?  Offset from where to where?

> but I would like to get the same results in both browsers.

Of course.  I did start a thread on www-style about this, fwiw.

Comment 12

[ezmilhouse]

"real offset" => by real I mean the visible top left corner of a rotated rectangle, the one that you get with $('#rect').offset() - in moz it's always the same, even after rotation - in webkit it moves on with the rotation, my function gives you the webkit values as a workaround for moz.

Comment 13

[Luka Marčetić]

Confirmed in Firefox/3.5.16.
After doing a scale transformation, getBoundingClientRect().left returned an old value (pre-transform).

Comment 14

[Luka Marčetić]

(In reply to comment #13)
> Confirmed in Firefox/3.5.16.
> After doing a scale transformation, getBoundingClientRect().left returned an
> old value (pre-transform).

I should add I'm using GNU/Linux on a x86-64 platform.

Comment 15

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Also from Firebugs perspective our highlighter needs to be able to match the same
shape and position of any element under it and there is currently no way to do
that for hierarchical css transforms because each ancestor can have a different
transform origin. In this situation the offset correction included here would not work.

e.g. If you have a div rotated by 20% that contains a child div that is rotated by 30% then both rotations need to be taken into account and also the transform origins of all parent elements.

Comment 17

[4esn0k]

i implement solution to get get the MouseEvent coordinates for an element(in local coordinate system of element) that has CSS3 2d Transforms

i have to implement fix for getBoundingClientRect too
see gist.github.com/1145197 for solution
live demo:
http://jsfiddle.net/YLCd8/2/

solutin is based on iterating on all parents and multiplication of all transformation matrices

Comment 19

[Andrew Udvare]

Attachment: IE10 screenshot

IE 10 also adds translate values.

Comment 20

[Atul Varma [:atul]]

Attachment: Simple HTML test case for this bug.

I stumbled upon this bug independently and wrote this simple "test case" which runs as expected on Webkit and Opera (possibly IE9 too, but I don't have it on hand) but doesn't on Firefox. I hope it's somehow useful.

Comment 21

[Paul Rouget [:paul]]

We need this for the devtools.

Comment 22

[Atul Varma [:atul]]

I just tried my test case out on IE9 and it works there too, so Firefox is the only major browser that it doesn't work on. That's not to say that Firefox is wrong, of course, but it sure would be easy for Web developers if this function worked the same way across every browser.

Comment 23

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

roc posted http://lists.w3.org/Archives/Public/www-style/2011Nov/0629.html

Comment 24

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

Attachment: cleanup frame transform computation code

Comment 25

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

Attachment: part 2: Add support for taking transforms into account in GetAllInFlowRects(Union)

Comment 26

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

Attachment: part 3: take transforms into account in getClientRects/getBoundingClientRect

Includes tests.

Comment 27

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

Try shows some test failures:
1257 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/base/tests/test_bug677878.html | Test timed out.
3538 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_selection_expanding.html | The contents of fixedDiv1 aren't selected (div1-fixedDiv1, all boxes are overflow: visible;): Selected String: "aaa aaaaaaa aaaaaaa\naaaaaaa aaaaaaa aaaaaaa aaaaaaa
(and other failures in test_selection_expanding.html)
16497 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_mousecapture.xul | setCapture on body retargets to root node mousemove event fired - got false, expected true

Comment 28

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

Attachment: Part 1 v2

nsIFrame::GetTransformMatrix had a bug that meant it wouldn't stop correctly when aStopAtAncestor was this frame's parent. Fixing that fixed most of the test failures.

Comment 29

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

Attachment: Part 3 v2

Added more tests to test_clientRects.html.

Fixed test_bug677878.html which relied on getBoundingClientRect not taking transforms into account.

Comment 30

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

These patches pass debug tests on try.

Comment 31

[Mats Palmgren (inactive)]

Comment on attachment 576976 [details] [diff] [review]
Part 1 v2

A few nits - fix as you see fit.  r=mats

>layout/base/nsLayoutUtils.cpp
>+GetTransformToAncestor( ...
>+    ctm = ctm*parent->GetTransformMatrix(aAncestor, &parent);

add spaces around "*"

>layout/base/nsLayoutUtils.h
>+   * Transform aRect relative to aAncestor down to the coordinate system of aFrame.

line has > 80 cols

>+   * Transform aRect relative to aFrame up to the coordinate system of aAncestor.

line has > 80 cols

>layout/forms/nsComboboxControlFrame.cpp
>     nsIFrame* parent = nsnull;
>-    gfx3DMatrix ctm = frame->GetTransformMatrix(&parent);
>+    gfx3DMatrix ctm = frame->GetTransformMatrix(nsnull, &parent);

while you are here, remove the initialization of 'parent'

>layout/generic/nsFrame.cpp
>+nsIFrame::GetTransformMatrix(nsIFrame* aStopAtAncestor, nsIFrame **aOutAncestor)

I would prefer 'nsIFrame** aOutAncestor'

>layout/generic/nsIFrame.h
>+   * @param aStopAtAncestor don't look further than aStopAtAncestor.

please say explicitly that it looks at all ancestors when null

>+  virtual gfx3DMatrix GetTransformMatrix(nsIFrame* aStopAtAncestor, nsIFrame **aOutAncestor);

I would prefer 'nsIFrame** aOutAncestor'
line has > 80 cols

>layout/svg/base/src/nsSVGForeignObjectFrame.cpp
>+nsSVGForeignObjectFrame::GetTransformMatrix(nsIFrame* aAncestor, nsIFrame **aOutAncestor)

I would prefer 'nsIFrame** aOutAncestor'
line has > 80 cols

>layout/svg/base/src/nsSVGForeignObjectFrame.h
>+  virtual gfx3DMatrix GetTransformMatrix(nsIFrame* aAncestor, nsIFrame **aOutAncestor);

I would prefer 'nsIFrame** aOutAncestor'
line has > 80 cols

Comment 32

[Mats Palmgren (inactive)]

Comment on attachment 576866 [details] [diff] [review]
part 2: Add support for taking transforms into account in GetAllInFlowRects(Union)

> layout/base/nsLayoutUtils.cpp
> nsIFrame* nsLayoutUtils::GetContainingBlockForClientRect(nsIFrame* aFrame)
> {
>-  // get the nearest enclosing SVG foreign object frame or the root frame

https://developer.mozilla.org/en/DOM/element.getClientRects
", unless the element is inside an SVG foreignobject element, in which case the top-left is relative to the nearest foreignobject ancestor and in the coordinate system of that foreignobject"
needs to be removed, and add a note about this bug in older versions?

Comment 33

[Mats Palmgren (inactive)]

Comment on attachment 576977 [details] [diff] [review]
Part 3 v2

> dom/tests/mochitest/general/test_clientRects.html

this file has DOS line endings

Comment 34

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

I'm planning to land this after the Dec 20 branch to ensure we get maximum coverage, since this could possibly break sites.

Comment 35

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

Fixed all nits.

https://hg.mozilla.org/integration/mozilla-inbound/rev/10ea92a6a850
https://hg.mozilla.org/integration/mozilla-inbound/rev/f793f9cfa72c
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc7c7734ec7c

Comment 36

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

(In reply to Mats Palmgren [:mats] from comment #32)
> https://developer.mozilla.org/en/DOM/element.getClientRects
> ", unless the element is inside an SVG foreignobject element, in which case
> the top-left is relative to the nearest foreignobject ancestor and in the
> coordinate system of that foreignobject"
> needs to be removed, and add a note about this bug in older versions?

I removed the text, but didn't add a note about the bug. I expect no-one will actually run into it.

Comment 37

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d07146e9c62e because the wily mobile/xul/chrome/tests/browser_scrollbar.js didn't like it.

Comment 38

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

The vertical scrollbar can be transformed by _updateScrollbars in browser.js. Simplest fix is hopefully to check for a translate() transform on the scrollbar and include any X-translation in the expected getBoundingClientRect value.

Try push with hypothetical test fix:
https://tbpl.mozilla.org/?tree=Try&rev=812bee53c58d

Comment 39

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

Hmm, running browser-chrome alone didn't seem to work at all (lots of "Browser undefined" errors on many tests). Retrying with a bigger set of Android tests:
https://tbpl.mozilla.org/?tree=Try&rev=02d99bc76219

Comment 40

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

Attachment: Part 4: fix mobile browser test to handle getBoundingClientRect including the effect of transforms

Comment 41

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/61ee01b6a46d
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8519c842f6e
https://hg.mozilla.org/integration/mozilla-inbound/rev/baa4388f9cea
https://hg.mozilla.org/integration/mozilla-inbound/rev/24bbd63d12d2

Comment 42

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/61ee01b6a46d
https://hg.mozilla.org/mozilla-central/rev/a8519c842f6e
https://hg.mozilla.org/mozilla-central/rev/baa4388f9cea
https://hg.mozilla.org/mozilla-central/rev/24bbd63d12d2

Comment 44

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/DOM/element.getBoundingClientRect

Mentioned on Firefox 12 for developers.