958160 - getBoundingClientRect() is not minimal for rotated SVG elements

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Gavin Kistner]

Attachment: getBoundingClientRect-on-rotated-elements.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/33.0.1750.22 Safari/537.36

Steps to reproduce:

Opened this web page: http://phrogz.net/svg/getBoundingClientRect-on-rotated-elements.html


Actual results:

The bounding rect for the circle at right as calculated by getBoundingClientRect() does not fit the circle.

This is a regression according to Robert Longson's comment here:
http://stackoverflow.com/a/10632233/405017

and is counter to the WONTFIX status of related bug 755947.


Expected results:

The bounding rect should touch the top/bottom/left/right sides of the circle.

Comment 1

[Robert Longson [:longsonr]]

There's a tool here to find a regression window if you want to help further Gavin: http://mozilla.github.io/mozregression/ if not someone else will probably find the window anyway at some point.

Comment 2

[Gavin Kistner]

Last good revision: fb81c9a433e4 (2012-02-10)
First bad revision: d71dab82fff4 (2012-02-11)
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fb81c9a433e4&tochange=d71dab82fff4

... attempting to bisect inbound builds
Getting inbound builds between fb81c9a433e4 and d71dab82fff4
Oh noes, no (more) inbound revisions :(
do you want to bisect further by fetching the repository and building? (y or n) n

Comment 3

[Alice0775 White]

IE11, Chrome31 and Noghtly29.0a1 get same results on Windows8.1 and Windows7....

Comment 4

[Robert Longson [:longsonr]]

IE11 and Chrome are buggy too.

bug 614732 (changeset a439c947dc99) is the cause.

Comment 5

[Robert Longson [:longsonr]]

Thanks for the regression range Gavin.

Comment 6

[Robert Longson [:longsonr]]

Following bug 614732 nsSVGUtils::TransformFrameRectToOuterSVG takes the mRect which is now in that frame's user space i.e. local co-ordinate system and converts the points of that to the outer SVG co-ordinate system. 

So we're calculating the bounding box (in one co-ordinate system) of a bounding box (in another co-ordinate system).

Comment 7

[Tom Klein]

Attachment: 958160.diff

Comment 8

[Robert Longson [:longsonr]]

Comment on attachment 8636112 [details] [diff] [review]
958160.diff

>+  gfxMatrix canvasTM = GetCanvasTM();
>+  if (canvasTM.PreservesAxisAlignedRectangles()) {
>+    return nsSVGUtils::TransformFrameRectToOuterSVG(
>+             mRect, canvasTM, PresContext());
>+  }
>+  else {

No else after return (i.e. lose the "} else {") 
FWIW: if you did need an else it should have been on the same line as the }

>+    // To get tight bounds we need to compute directly in outer SVG coordinates
>+    uint32_t flags = nsSVGUtils::eBBoxIncludeFill |
>+                     nsSVGUtils::eBBoxIncludeStroke |
>+                     nsSVGUtils::eBBoxIncludeMarkers;
>+    gfxRect extent = GetBBoxContribution(ToMatrix(canvasTM), flags).ToThebesRect();

wrap above lines to 80 characters somewhere.

r=longsonr with nits addressed

Comment 9

[Tom Klein]

Attachment: boundsTesting.html

I just realized this is going to expose the line corners computation issue noted in bug 1092125, comment 3 as shown in the attached testcase.

The bounds returned by getBoundingClientRect() are in green.

The black line with the aqua endpoint marker is actually broken with or without the patch - it's one of the few cases where we already send (non-identity) transforms to GetBBoxContribution.

So I guess I should wait to land this until I see what happens with bug 1092125, and in any case I should add a couple more bounds testcases.

Comment 10

[Tom Klein]

Sorry, I should probably add a little more explanation of what's happening with the marker example:

The untransformed marker line is at 45 degrees, and in that case the current line corners code gives corners that all lie on the axis line that runs the length of the line (two of those "corners" are on that axis at the bounds corners of the line (they lead to the correct bounds rectangle), the other two are also on that axis but interior to the bounds rectangle and don't contribute anything to the bounds computation).

But in the marker case we need the transformed bounds, so we transform those "corners" -45 degrees to four points that lie on a horizontal line, and then take the bounds of those four points, which results in a "rectangle" with zero height (and width larger than it should be).

Comment 11

[Robert Longson [:longsonr]]

bug 1092125 landed. Do you want to land this as-is Tom or are you still planning to add more tests?

Comment 12

[Tom Klein]

I'll add some tests but I won't get to it until next week.  Thanks for the reminder!

Comment 13

[Tom Klein]

Attachment: Patch v2.diff

The only changes in this version are in the tests.

The circle test from the first version of the patch failed the try run on Mac**:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8740bd2f0929
so I just removed that test since I think the two new line tests added in this version cover the functionality we want tested for this bug anyway.

Long story, but all three tests (the old circle test and the two new tests in this patch) were included in the try run, so we should be ready to land as long as the new tests pass review.

** Maybe the CG failure is because we're calling CGPathGetBoundingBox in PathCG::GetBounds
https://hg.mozilla.org/mozilla-central/file/d590b9601ba8/gfx/2d/PathCG.cpp#l326
which includes the control points of the bezier curve used to describe the circle (when we rotate by 45 one of the control points sticks out of the circle bounding box I guess), instead of calling CGPathGetPathBoundingBox which doesn't include the control points.  Compare
https://developer.apple.com/library/mac/documentation/GraphicsImaging/Reference/CGPath/#//apple_ref/doc/uid/TP30000959-CH1g-439779 and
https://stackoverflow.com/questions/26313553/strange-behaviour-of-cgpathgetboundingbox
I don't have a mac to test but I can create a bug and test on try if that sounds right.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf2aec9e74c5

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cf2aec9e74c5