Closed Bug 958160 Opened 6 years ago Closed 4 years ago

getBoundingClientRect() is not minimal for rotated SVG elements

Categories

(Core :: SVG, defect)

26 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: gavin, Assigned: twointofive)

References

Details

(Keywords: regression, testcase)

Attachments

(3 files, 1 obsolete file)

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.
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.
Component: Untriaged → SVG
Product: Firefox → Core
OS: Windows 7 → All
Hardware: x86_64 → All
See Also: → 755947
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
IE11, Chrome31 and Noghtly29.0a1 get same results on Windows8.1 and Windows7....
IE11 and Chrome are buggy too.

bug 614732 (changeset a439c947dc99) is the cause.
Blocks: 614732
Status: UNCONFIRMED → NEW
Ever confirmed: true
Thanks for the regression range Gavin.
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).
Attached patch 958160.diff (obsolete) — Splinter Review
Assignee: nobody → twointofive
Attachment #8636112 - Flags: review?(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
Attachment #8636112 - Flags: review?(longsonr) → review+
Attached file 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.
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).
bug 1092125 landed. Do you want to land this as-is Tom or are you still planning to add more tests?
Flags: needinfo?(twointofive)
I'll add some tests but I won't get to it until next week.  Thanks for the reminder!
Attached patch Patch v2.diffSplinter Review
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.
Attachment #8636112 - Attachment is obsolete: true
Flags: needinfo?(twointofive)
Attachment #8664341 - Flags: review?(longsonr)
Attachment #8664341 - Flags: review?(longsonr) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cf2aec9e74c5
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.