Bug 42815 – getBoundingClientRect Broken for SVG Elements

• Component: SVG
• Status: RESOLVED FIXED

[Dan Wilson]

Bug copied from chromium issue tracker, because this is a webkit issue.

I've verified the issue on Webkit nightly (r63854), Chromium nightly (53312) as well as Safari 5.0. (Firefox 3.6.6 and Opera 10.6 work correctly.)

What follows is from http://code.google.com/p/chromium/issues/detail?id=47998

----

Chrome Version       : 5.0.375.86
URLs (if applicable) : https://developer.mozilla.org/presentations/xtech2005/svg-canvas/SVGDemo.xml
Other browsers tested:
Add OK or FAIL after other browsers where you have tested this issue:
Safari 4:
  Firefox 3.x: PASS
IE 7:
IE 8:

What steps will reproduce the problem?
1. visit URL
2. In Console, enter: document.getElementsByTagNameNS("http://www.w3.org/2000/svg", "rect")[0].getBoundingClientRect()


What is the expected result?
The correct bounding rectangle.

What happens instead?
All dimensions are zero.

Please provide any additional information below. Attach a screenshot if
possible.

As far as I can tell, the method, getBoundingClientRect, is broken for all SVG elements in chrome/webkit. It works in Firefox.

[Nikolas Zimmermann]

getBoundingClientRect() operates on RenderBoxModelObjects, see:

PassRefPtr<ClientRect> Element::getBoundingClientRect() const
{
    document()->updateLayoutIgnorePendingStylesheets();
    RenderBoxModelObject* renderBoxModelObject = this->renderBoxModelObject();

This will always be zero for SVG elements. I was not aware of this method to retrieve the bounding box of a SVG element, as SVG provides it's own getBBox() function.

Just reading http://www.w3.org/TR/cssom-view/#the-getclientrects-and-getboundingclient, which says:

If the element does not have an associated CSS layout box and is in the http://www.w3.org/2000/svg namespace return a ClientRectList object containing a single ClientRect object that describes the bounding box of the element as defined by SVG specification. [SVG]

So it might be as easy as:
if (isSVGElement()) {
    SVGElement* svgElement = static_cast<SVGElement*>(this);
    if (svgElement->isStyledLocatable())
        return ClientRect::create(static_cast<SVGStyledLocatableElement*>(svgElement)->getBBox());
}

Just noticed that ClientRect stores a FloatRect internally, but only has an IntRect constructor, so after adding a ClientRect(const FloatRect&) constructor, the code above should work as is.

Someone interessted in implementing? I'm quite busy atm.

[Alexey Proskuryakov]

Duplicate of bug 27978?

[Cosmin Truta]

Created attachment 65517 [details]
Fix and layout test

(In reply to comment #1)
I wonder if it's necessary to create this separate code path for SVG inside Element::getBoundingClientRect, when it's simpler to just add box model object capabilities to the SVG render. I replaced the base class of RenderSVGModelObject with RenderPath (which is a subclass of RenderBoxModelObject), and everything just starts working.

[Dirk Schulze]

Comment on attachment 65517 [details]
Fix and layout test

RenderPath and the other SVG renderers (without SVGRoot and some more) are no elements that follow the boxing model, so it is wrong to inherit from RenderBox. It's better to follow Nikos suggestion.

[Cosmin Truta]

Ok, I followed Nikolas' suggestion, and it worked well except that the results aren't consistent with Firefox.
The output rectangle, as received from getBBox(), is the rectangle specified in the SVG: (0, 0, 100, 100). On the other hand, in Firefox, the output is the actual position within the window; e.g., on my desktop it's (104.6, -478.65, 1721, 1722).
It is necessary to translate the result from getBBox() to the actual screen coordinates, no?

[Cosmin Truta]

Created attachment 66167 [details]
Fix and layout test, take 2

As mentioned in the previous comment, it is not sufficient to just call getBBox(). It is also necessary to go through the rest of the function's calculations, in order to obtain the correct result. I am now stretching the rectangle in the layout test, to ensure that the necessary calculations are performed.
Upon this submission, the values displayed in Chromium/TestShell are consistent with those displayed in Firefox.

[Nikolas Zimmermann]

Comment on attachment 66167 [details]
Fix and layout test, take 2

Hi Cosmin,

the patch itself looks fine, but I want to see more tests, especially in the context of zooming & scrolling.
Have a look at svg/zoom/page and copy your testcase there, and add the logic that zooms the page several times.
Also grep for "overflow: scroll" in svg/custom, you can find some tests as template for you.

Thanks in advance,
Niko

[Cosmin Truta]

Created attachment 67155 [details]
Fix and layout test, take 3

Niko,
Here is the old patch with new tests, as you suggested. The test include an unstretched and a stretched rectangle, as well as two overflown rectangles (a hidden one, and a scrolled one).

I did not know how to test zooming. When I added tests for getBoundingClientRect to an existing SVG zooming test, I was still getting the initial rectangle coordinates, before zooming. The getBoundingClientRect function does work correctly, for example, when I run that test in the browser, I zoom then I click the refresh button, the coordinates are scaled as expected. However, when zooming using testPageZoom.js, without hitting the refresh button or doing other sorts of UI interaction, the rectangle coordinates are not scaled. If you wish, I can show you the example that I'm using.

And there is another thing: getBoundingClientRect works well on various SVG elements that I tried, except <text>. (In comparison, getBBox does work on <text> under Chromium, and both getBBox and getBoundingClientRect work well on <text> under Firefox.) Would it be possible to have my patch accepted, and allow me to work on the remaining issue, separately, so that I keep the code chunks separate and reasonably small?

Best regards,
Cosmin

[Nikolas Zimmermann]

(In reply to comment #8)
> Created an attachment (id=67155) [details]
> Fix and layout test, take 3
> 
> Niko,
> Here is the old patch with new tests, as you suggested. The test include an unstretched and a stretched rectangle, as well as two overflown rectangles (a hidden one, and a scrolled one).
Good job.
 
> I did not know how to test zooming. When I added tests for getBoundingClientRect to an existing SVG zooming test, I was still getting the initial rectangle coordinates, before zooming. The getBoundingClientRect function does work correctly, for example, when I run that test in the browser, I zoom then I click the refresh button, the coordinates are scaled as expected.
What do you mean by refreshing, reloading the page?

> However, when zooming using testPageZoom.js, without hitting the refresh button or doing other sorts of UI interaction, the rectangle coordinates are not scaled. If you wish, I can show you the example that I'm using.
Please attach the example here. I expected that you'd run into trouble with zooming :-)

> 
> And there is another thing: getBoundingClientRect works well on various SVG elements that I tried, except <text>. (In comparison, getBBox does work on <text> under Chromium, and both getBBox and getBoundingClientRect work well on <text> under Firefox.) Would it be possible to have my patch accepted, and allow me to work on the remaining issue, separately, so that I keep the code chunks separate and reasonably small?
If you're commited to work on the remaining issues, it's just fine with me!

[Nikolas Zimmermann]

Comment on attachment 67155 [details]
Fix and layout test, take 3

Good first patch. Looking forward to the fixes to get zooming + text working as well. r=me.

[Cosmin Truta]

Created attachment 67181 [details]
Test case: getBoundingClientRect does not react to the scripted zooming

(In reply to comment #9)
> What do you mean by refreshing, reloading the page?
Yes.

I know very little at this point, so I don't know exactly what is the expected behavior. Here is the first example, involving strictly XHTML (e.g. <div>). No matter how much I zoom, after zooming and refreshing the coordinates stay the same, or are off-by-one at worst. Is this the correct behavior?
2nd example is coming up next.

[Cosmin Truta]

Created attachment 67182 [details]
67181: Test case: getBoundingClientRect does not react to the scripted zooming, Part 2

... and here is the 2nd example, in which getBoundingClientRect is applied to an SVG element.

Under testPageZoom.js, nothing happens, the coordinates displayed remain unchanged. But after manual zooming and refresh, the coordinates are changing.
Which one (if any) of the two test cases have the correct behavior?

[Cosmin Truta]

Forgot to mention that Firefox acts the same way as Chromium does with the XHTML test case involving <div>, so, from that comparison, I am inclined to think that the behavior of the SVG test case is incorrect.

You can probably tell by now that I don't know that much CSS and JavaScript.

But there's another thing: Firefox also expects reloading the page to display the updated coordinates; zooming alone does not seem to be enough for Firefox, either.

[Cosmin Truta]

Created attachment 67185 [details]
Test case: getBoundingClientRect not working on <text>

This test case is currently failing. In order to make it pass, it is necessary to comment out the if-statement that checks txt.getBoundingClientRect() at line 20.

[WebKit Commit Bot]

Comment on attachment 67155 [details]
Fix and layout test, take 3

Clearing flags on attachment: 67155

Committed r67252: <http://trac.webkit.org/changeset/67252>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/67252 might have broken Qt Linux Release minimal

[Cosmin Truta]

Created attachment 68445 [details]
Test case: getBoundingClientRect does not work correctly with zooming

I am attaching a simpler test case, that I wrote using the "zoom" attribute.

The dimensions of <svg> elements are shown correctly, and are the same, regardless whether the page is zoomed (either zoomed in or zoomed out).

The dimensions of <rect> elements are shown incorrectly, and are changing, depending on how the page is being zoomed.

As of latest dev version, this is the output, unzoomed:

PASS svg1.width is 100
PASS svg1.height is 50
FAIL r1.width should be 100. Was 200.
FAIL r1.height should be 50. Was 100.

PASS svg2.width is 200
PASS svg2.height is 100
FAIL r2.width should be 200. Was 100.
FAIL r2.height should be 100. Was 50.

And this is the output, zoomed-in twice (in Chromium on Linux):

PASS svg1.width is 100
PASS svg1.height is 50
FAIL r1.width should be 100. Was 288.
FAIL r1.height should be 50. Was 144.

PASS svg2.width is 200
PASS svg2.height is 100
FAIL r2.width should be 200. Was 144.
FAIL r2.height should be 100. Was 72.

[Cosmin Truta]

Created attachment 69153 [details]
Fix and layout test for zoomed elements

getBoundingClientRect finally works correctly with zooming.

There are still off-by-one errors when zoomed arbitrarily. I'm pretty sure that's because the bounding rectangle is adjusted for absolute zoom *after* it is converted to IntRect. The roundoff errors are significant enough to cause off-by-one, and sometimes even off-by-two errors from time to time. The layout test that I included is okay, though, because zooming is done at well-behaved ratios (50% and 200%).

I placed the layout test in the css3/ directory, rather than in an svg/ subdirectory, because I discovered that the zoom attribute hasn't been tested with getBoundingC..., not even for HTML elements.

[Cosmin Truta]

The work on <text> will be continued under the bug 46775.

[Nikolas Zimmermann]

Good work Cosmin.
Note, that we've never tested the combination of the zoom property on SVG elements at all. I wasn't even aware that it actually works.

It's definately non-standard for SVG - what happens if you manually zoom into your standalone SVG file - does getBoundingClientRect still work? Does it already work?

We really need a svg specific test in svg/zoom/page. Can you come up with one?

[Cosmin Truta]

(In reply to comment #21)
> Good work Cosmin.
> Note, that we've never tested the combination of the zoom property on SVG elements at all. I wasn't even aware that it actually works.
> 

> It's definately non-standard for SVG - what happens if you manually zoom into your standalone SVG file - does getBoundingClientRect still work? Does it already work?

It wasn't working in the past, but it does works now with this patch :-)
I was referring to this when I mentioned the off-by-one errors that occur under arbitrary zooming.

> We really need a svg specific test in svg/zoom/page. Can you come up with one?

I tried, but I couldn't. Could you please give me a few more hints?

The results of getBounding... do not show up refreshed until the browser window is explicitly reloaded. The same is happening when running svg/zoom/resources/testPageZoom.js: the pixel dump does show up zoomed, but the text output acts as if zooming did not happen. (It's harder to tell that now, after patching the source, since the coordinates do indeed stay the same; but even before, with the bug inside, testPageZoom.js alone was not sufficient to display the wrong coordinates after zooming.)

You can see this effect by loading the layout test from this patch, in an unpatched WebKit. Zoom it manually, and see that the updated (wrong) coordinates don't appear until you hit the reload button.

So I can't test this in DumpRT, unless I'm able to do somehow trigger a page reload from JavaScript. But I don't know how (or if) I could do that.

[Cosmin Truta]

Thank you for showing me how to do zooming from JavaScript. That helped my problem... until I run into something else:

If I run the test multiple times, most of the time I'm getting the expected results. Some times, unfortunately, I'm getting off-by-one errors, other times some coordinates are off by a large margins (they seem to hold garbage values), and some other times those coordinates are plain zero.

Any idea how to resolve this flakiness in testing?

[Nikolas Zimmermann]

(In reply to comment #23)
> Thank you for showing me how to do zooming from JavaScript. That helped my problem... until I run into something else:
> 
> If I run the test multiple times, most of the time I'm getting the expected results. Some times, unfortunately, I'm getting off-by-one errors, other times some coordinates are off by a large margins (they seem to hold garbage values), and some other times those coordinates are plain zero.
That's not good at all, you could valgrind it! It almost sounds like memory corruption...

> 
> Any idea how to resolve this flakiness in testing?
Yeah, find the underlying issue :-)

[Cosmin Truta]

I resolved the flakiness by replacing the call to "../resources/testPageZoom.js" with a direct call to window.eventSender.zoomPageIn(). It turns out that the asynchronous nature of testPageZoom.js caused inconsistent results.

On the other hand, the off-by-one errors still remain. I opened the bug 48110 to continue the work over there. The fix for that bug will include a layout test that checks getBoundingClientRect with zooming.

[Cosmin Truta]

Created attachment 72199 [details]
Zooming tests for getBoundingClientRect

At this point, I seem to have trouble solving the precision issue (in bug 48110) completely. I can fix some of the roundoff errors, but not all of them.
Fortunately, I now know enough (I think) to write the zooming tests that were initially requested by Niko. I did this by following Niko's testing pattern (setting window.postZoomCallback), and by adding the functions areArraysApproxEqual(), isResultApproxCorrect() and shouldBeApprox() to js-test-pre.js.

[Cosmin Truta]

Comment on attachment 72199 [details]
Zooming tests for getBoundingClientRect

Setting flags to r- and cq-.
This will be addressed in bug 48640.