Closed Bug 591718 Opened 14 years ago Closed 12 years ago

getBoundingClientRect needs to take transforms into account

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla12
Tracking Status
firefox11 --- wontfix
blocking2.0 --- -

People

(Reporter: asa, Assigned: roc)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(6 files, 2 obsolete files)

(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.
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.
Keywords: qablocker, qawanted
blocking2.0: --- → ?
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.
Please ignore comment 2; it was meant for a different bug.
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?
Summary: Offset not accurate after rotation with -moz-transform → getBoundingClientRect needs to take transforms into account
Component: Style System (CSS) → DOM
QA Contact: style-system → general
"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?
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.
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...
We've been shipping transforms since Firefox 3.5 / Mozilla 1.9.1.
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.)
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 ...
> 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.
"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.
blocking2.0: ? → -
Confirmed in Firefox/3.5.16.
After doing a scale transformation, getBoundingClientRect().left returned an old value (pre-transform).
(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.
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.
Blocks: 669887
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
Attached image IE10 screenshot
IE 10 also adds translate values.
Component: DOM → DOM: CSS Object Model
QA Contact: general → style-system
OS: Windows 7 → All
Priority: -- → P3
Hardware: x86 → All
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.
We need this for the devtools.
Blocks: 663852, 663830
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.
Assignee: nobody → roc
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
Attached patch Part 1 v2Splinter Review
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.
Attachment #576865 - Attachment is obsolete: true
Attachment #576976 - Flags: review?(matspal)
Attachment #576865 - Flags: review?(matspal)
Attached patch Part 3 v2Splinter Review
Added more tests to test_clientRects.html.

Fixed test_bug677878.html which relied on getBoundingClientRect not taking transforms into account.
Attachment #576867 - Attachment is obsolete: true
Attachment #576977 - Flags: review?(matspal)
Attachment #576867 - Flags: review?(matspal)
These patches pass debug tests on try.
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
Attachment #576976 - Flags: review?(matspal) → review+
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?
Attachment #576866 - Flags: review?(matspal) → review+
Comment on attachment 576977 [details] [diff] [review]
Part 3 v2

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

this file has DOS line endings
Attachment #576977 - Flags: review?(matspal) → review+
I'm planning to land this after the Dec 20 branch to ensure we get maximum coverage, since this could possibly break sites.
(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.
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.
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
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
Attachment #584703 - Flags: review?(mark.finkle) → review+
Blocks: 722003
Documentation updated:

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

Mentioned on Firefox 12 for developers.
You need to log in before you can comment on or make changes to this bug.