Bug 48640 – Add layout test that check getBoundingClientRect w/ zooming

• Component: SVG
• Status: RESOLVED DUPLICATE

[Cosmin Truta]

I initially submitted this in the original bug, 42815, but that is closed. I need to readjust my workflow, and open a new bug for every submitted patch. This seems to be the way preferred by WebKit.

[Cosmin Truta]

This is part of the work on bug 48110.

[Cosmin Truta]

Created attachment 72341 [details]
Zooming tests for getBoundingClientRect

Here is the comment originally entered in bug 42815:

"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."

[Nikolas Zimmermann]

Comment on attachment 72341 [details]
Zooming tests for getBoundingClientRect

View in context: https://bugs.webkit.org/attachment.cgi?id=72341&action=review

Hi Cosmin,

good job, though in the wrong direction I fear. I was confused what you mean with rounding problems. I thought you wanted to solve it in the code, rather than in the tests :-)

> LayoutTests/svg/custom/getBoundingClientRect.xhtml:7
> +<div style="width:500px;height=100px;">

This can't work :-) s/=/:/

> LayoutTests/svg/zoom/page/zoom-zoom-coords.xhtml:53
> +    shouldBeApprox('div1.left', '0', '1');

The last argument doesn't need to be a string, but it's okay, you don't need to change it everywhere.

> LayoutTests/svg/zoom/page/zoom-zoom-coords.xhtml:54
> +    shouldBeApprox('div1.top', '0', '1');

I don't get the whole story behind shouldBeApprox.
CSSOM defines getBoundingClientRect to return a ClientRect() object, containing 'float' coordinates.
See http://dev.w3.org/csswg/cssom-view/#clientrect.



        FloatRect localRect;
        if (svgElement->boundingBox(localRect))
            quads.append(renderer()->localToAbsoluteQuad(localRect));

localToAbsoluteQuad gives you a FloatRect, stored in localRect.
The problem is the "    IntRect result = quads[0].enclosingBoundingBox();" usage here.
When using boundingBox() instead of enclosingBoundingBox() you'd get a FloatRect as result, which is really what ought to be used here.

Instead of using "adjustIntRectForAbsoluteZoom(IntRect& rect, RenderObject* renderer)" a adjustFloatRectForAbsoluteZoom should be added to RenderObject.h (there's already a FloatQuad version and a FloatPoint version).
That should fix the real issue.

Of course when testing, you might get results like 50.000001 or sth else when zooming (due the rounding error accumulation).
You can solve this by using shouldBe("div1.width.toFixed(2)", "50.00").... (toFixed is the key here)

Does this help to solve the underlying problem?

[Cosmin Truta]

(In reply to comment #3)
> I was confused what you mean with rounding problems. I thought you wanted to solve it in the code, rather than in the tests :-)

I did that as an interim workaround, because developing the actual fix was taking too long, and I wasn't even sure that I can do it properly. Even now, after submitting the patch for bug 48110, I can say it's a good solution, but not that it's the best solution. I had to modify enclosingIntRect(), in a way that you might or might not find acceptable ;-)

Here is the trouble I'm running into:
float scale = 1.2f;  // 1.20000005
float orig_x = 30.0f;  // 30.0
float scaled_x = orig_x * scale;  // 36.0
float adjusted_x = scaled_x / scale;  // 29.9999981

Here, the original enclosingIntRect() is taking floatf(29.9999981), which is 29, and I'm expecting 30. There is no way to recover this loss, except by making the compromise of introducing an epsilon inside enclosingIntRect (or inside an other new function, named enclosingIntRectRoundedForImprecise, if you don't agree with modifying enclosingIntRect).

See my patch, and my detailed comments, in bug 48110.

[Nikolas Zimmermann]

(In reply to comment #4)
> (In reply to comment #3)
> > I was confused what you mean with rounding problems. I thought you wanted to solve it in the code, rather than in the tests :-)
> 
> I did that as an interim workaround, because developing the actual fix was taking too long, and I wasn't even sure that I can do it properly. Even now, after submitting the patch for bug 48110, I can say it's a good solution, but not that it's the best solution. I had to modify enclosingIntRect(), in a way that you might or might not find acceptable ;-)

Hmm, as I wrote in my last comment, the usage of enclosingIntRect is the actual problem. We shouldn't do that at all, but give back a FloatRect, according to the CSS OM API?

[Cosmin Truta]

The entire work will be contained in 48110.

*** This bug has been marked as a duplicate of bug 48110 ***