I am exposing this bug using an SVG test case that I have at hand, but the code path that handles HTML elements makes the same kind of truncations. Inside Element::getBoundingClientRect, the element's enclosing bounding box, scaled by the renderer's effective zoom factor (which is a float), is truncated to int, early on, then it enters a series of calculations. At the end of the function, the result is unscaled by multiplying with the inverse of the zoom factor, and truncated to int, yet again. This leads to unnecessary off-by-one errors (or even greater errors at very large . I am opening this bug as the continuation of the work on getBoundingClientRect. See the bugs 42815, 46775.
Created attachment 71524 [details] Test case (layout test) This is the test case, to be run from LayoutTests/svg/zoom/page/ The current (incorrect) output, after one zoom-in operation, is the below. (The results vary from zoom level to zoom level.) Zoom in, then reload. You should not see any loss of precision. On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". PASS svg1.left is 0 PASS svg1.top is 0 PASS svg1.width is 150 FAIL svg1.height should be 50. Was 51. PASS svg1.right is 150 FAIL svg1.bottom should be 50. Was 51. PASS rect1.left is 0 PASS rect1.top is 0 FAIL rect1.width should be 100. Was 101. FAIL rect1.height should be 50. Was 51. FAIL rect1.right should be 100. Was 101. FAIL rect1.bottom should be 50. Was 51. PASS image1.left is 100 PASS image1.top is 0 PASS image1.width is 50 FAIL image1.height should be 50. Was 51. PASS image1.right is 150 FAIL image1.bottom should be 50. Was 51. PASS successfullyParsed is true TEST COMPLETE
Created attachment 71553 [details] New async testcases I modified Cosmins testcases, to make them work using the existing testPageZoom.js framework. These tests are dumping text, no render tree dumps, I enabled an option to dump pixel test results as well. NOTE: I generated the results w/o Cosmins patch on bug 46775. That's why they still contain FAIL messages. I'm only uploading them here to share with with Cosmin.
Created attachment 72538 [details] Fix and layout tests In response to Niko's comment from bug 48640: > 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. I replaced adjustIntRect... with adjustFloatRect... from the very beginning, but that turned out not to be sufficient. For example, assuming the coordinate values 20, 30, 40, 50, zoomed by the factor 1.2f (whose more precise approximation is 1.20000005), the results of scaling 20 and 40 up and down are exact, but the results of scaling 30 and 50 up and down are not. See the following sample code: #include <iostream> #include <iomanip> float scale = 1.2f; // 1.20000005 float f24 = 24.0f; float f36 = 36.0f; float f48 = 48.0f; float f60 = 60.0f; float f20 = f24 / scale; // 20.0 (exact) float f30 = f36 / scale; // 29.9999981 float f40 = f48 / scale; // 40.0 (exact) float f50 = f60 / scale; // 49.9999962 int main() { std::cout << std::setprecision(9) << f20 << std::endl << std::setprecision(9) << f30 << std::endl << std::setprecision(9) << f40 << std::endl << std::setprecision(9) << f50 << std::endl; } For this reason, in spite of deferring the float-to-int conversion, an epsilon is still required in enclosingIntRect. I essentially made enclosingIntRect act as in roundForImprecise. If the reviewer does not agree with this modification to enclosingIntRect, I will perhaps add another function called enclosingIntRectRoundedForImprecise, in the next iteration of this bugfix. And an observation: the epsilon value 0.01 used in enclosingIntRect is still too small, but making it larger won't help. The zooming tests pass with zoomCount=2, but they start failing if zoomCount grows larger than 2. Coordinates like svg1.top are altered from 250 to 248, even for zoomCount=3.
Changing the bug title: "getBoundingClientRect: Do not truncate the coordinates to integers"
*** Bug 48640 has been marked as a duplicate of this bug. ***
Created attachment 72622 [details] Fix and layout test In response to Niko's comment in bug 48640: > 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? Done. I am still puzzled by the discrepancy between zoomCount=2 and zoomCount=3. With zoomCount=1,2 the results are well-behaved, within a very small distance from the expected value (with delta=1.0e-5 -- very decent!). On the other hand, with zoomCount=3,4,5... the errors grow wilder, suddenly, and even delta=0.5 is not good enough for some of the coordinates. I wonder if some precision is being lost accidentally, somewhere inside WebKit.
Comment on attachment 72538 [details] Fix and layout tests Oops, forgot to mark the previous patch obsolete.
Comment on attachment 72622 [details] Fix and layout test View in context: https://bugs.webkit.org/attachment.cgi?id=72622&action=review This looks great, especially the tests, though some nitpicks: > WebCore/rendering/RenderObject.h:1006 > +inline float adjustFloatForAbsoluteZoom(float value, RenderObject* renderer) > { > - rect.setX(adjustForAbsoluteZoom(rect.x(), renderer)); > - rect.setY(adjustForAbsoluteZoom(rect.y(), renderer)); > - rect.setWidth(adjustForAbsoluteZoom(rect.width(), renderer)); > - rect.setHeight(adjustForAbsoluteZoom(rect.height(), renderer)); > + return adjustFloatForAbsoluteZoom(value, renderer->style()); > } This helper function seems useless. > WebCore/rendering/RenderObject.h:1030 > + rect.setX(adjustFloatForAbsoluteZoom(rect.x(), renderer)); > + rect.setY(adjustFloatForAbsoluteZoom(rect.y(), renderer)); > + rect.setWidth(adjustFloatForAbsoluteZoom(rect.width(), renderer)); > + rect.setHeight(adjustFloatForAbsoluteZoom(rect.height(), renderer)); Just cache the RenderStyle* style = renderer->style() here, and pass it instead of renderer to adjustFloatForAbsoluteZoom. > WebCore/rendering/style/RenderStyle.h:1322 > + float zoomFactor = style->effectiveZoom(); > + return value / zoomFactor; No need for this local variable zoomFactor.
Created attachment 72658 [details] Fix and layout test (In reply to comment #8) > > WebCore/rendering/RenderObject.h:1006 > > +inline float adjustFloatForAbsoluteZoom(float value, RenderObject* renderer) > > [snip] > > This helper function seems useless. I agree. I just followed the pattern used in adjustForAbsoluteZoom(int,RenderObject*). So I assume you consider adjustForAbsoluteZoom equally useless, and all the calls from dom/Element.cpp and html/HTMLImageElement.cpp should be directed to adjustForAbsoluteZoom(int,RenderStyle*)? (I'm only asking; I am by no means suggesting that I should do that here...)
Comment on attachment 72658 [details] Fix and layout test Oops, forgot to update WebCore/ChangeLog
Created attachment 72662 [details] Fix and layout test
(In reply to comment #11) > Created an attachment (id=72662) [details] > Fix and layout test Looks great! Sorry for the delayed response...
Comment on attachment 72662 [details] Fix and layout test Rejecting patch 72662 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2 Last 500 characters of output: ....................................... fast/dom/Attr .... fast/dom/CSSStyleDeclaration ..... fast/dom/DOMException .... fast/dom/DOMImplementation ..... fast/dom/Document ................ fast/dom/Document/CaretRangeFromPoint .... fast/dom/Element ........... fast/dom/Element/getBoundingClientRect.html -> failed Exiting early after 1 failures. 7028 tests run. 116.24s total testing time 7027 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 3 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/6088008
Created attachment 74085 [details] Fix and layout test > fast/dom/Element/getBoundingClientRect.html -> failed Oops... Fixed. > Looks great! Sorry for the delayed response... Thanks (and no worries). One more review and cq+, please?
Comment on attachment 74085 [details] Fix and layout test View in context: https://bugs.webkit.org/attachment.cgi?id=74085&action=review > LayoutTests/platform/chromium-win/fast/dom/Element/getBoundingClientRect-expected.txt:9 > +FAIL testRect.top.toFixed(2) should be 42.00. Was 44.00. Why do we have FAIL messages now? Did I overlook something?
(In reply to comment #15) > Why do we have FAIL messages now? Did I overlook something? See bug 46315. Here are the relevant snippets from LayoutTests/ChangeLog: http://trac.webkit.org/changeset/68091 * platform/chromium-win/fast/dom/Element/getBoundingClientRect-expected.txt: This test is sensitive to font metrics. They appear correct visually, just positioned slightly different vertically. * platform/chromium-win/fast/dom/Range/getBoundingClientRect-expected.txt: Same as above.
(In reply to comment #16) > (In reply to comment #15) > > Why do we have FAIL messages now? Did I overlook something? > > See bug 46315. > > Here are the relevant snippets from LayoutTests/ChangeLog: > > http://trac.webkit.org/changeset/68091 > * platform/chromium-win/fast/dom/Element/getBoundingClientRect-expected.txt: This test is sensitive to font metrics. They appear correct visually, just positioned slightly different vertically. > * platform/chromium-win/fast/dom/Range/getBoundingClientRect-expected.txt: Same as above. So why don't you change the expectations in the test file, so it says PASS? I must have overlooked that the test contained FAIL messages initially...
Created attachment 74204 [details] Fix and layout tests > So why don't you change the expectations in the test file, so it says PASS? I spent some time trying to figure out how to reconcile Chromium vs. non-Chromium WebKit results, until (silly me) it eventually occurred to me that the place to put font-dependent (therefore platform-dependent) coordinates is the expectation file, not the source HTML. So I added SKIP besides the usual PASS/FAIL and one of the tests, and I removed shouldBe() calls altogether in the other one. In general, IMO, the shouldBe calls should be placed (no pun intended) only where there's a good reason to expect a certain number, or a certain formula. Everything else should be checked via expectation files.
(In reply to comment #18) > Created an attachment (id=74204) [details] > Fix and layout tests > > > So why don't you change the expectations in the test file, so it says PASS? > > I spent some time trying to figure out how to reconcile Chromium vs. non-Chromium WebKit results, until (silly me) it eventually occurred to me that the place to put font-dependent (therefore platform-dependent) coordinates is the expectation file, not the source HTML. > So I added SKIP besides the usual PASS/FAIL and one of the tests, and I removed shouldBe() calls altogether in the other one. > > In general, IMO, the shouldBe calls should be placed (no pun intended) only where there's a good reason to expect a certain number, or a certain formula. Everything else should be checked via expectation files. Hrm, I don't (In reply to comment #18) > Created an attachment (id=74204) [details] > Fix and layout tests > > > So why don't you change the expectations in the test file, so it says PASS? > > I spent some time trying to figure out how to reconcile Chromium vs. non-Chromium WebKit results, until (silly me) it eventually occurred to me that the place to put font-dependent (therefore platform-dependent) coordinates is the expectation file, not the source HTML. > So I added SKIP besides the usual PASS/FAIL and one of the tests, and I removed shouldBe() calls altogether in the other one. > > In general, IMO, the shouldBe calls should be placed (no pun intended) only where there's a good reason to expect a certain number, or a certain formula. Everything else should be checked via expectation files. Hm, you are also zooming non-text stuff like <div> or <svg> - I still don't see why the numeric values, after zooming eg. once or twice shouldn't be deterministic and non-font dependant? Hah, I just realized that fast/dom/Element/getBoundingClientRect, doesn't zoom SVGs, I'm sorry overlooked this fact! So it's definately font dependant. Sorry for the confusion :-) Ok, new suggestion: Use the "Ahem" font, that should give you exactly the same results on all platforms (hopefully). Text will appear as black blocks (rects), all of the same width/height and advance. Can you give that another try? I'd love to avoid SKIP ... messages, or not checking something.
Comment on attachment 72662 [details] Fix and layout test Cleared Nikolas Zimmermann's review+ from obsolete attachment 72662 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Created attachment 74353 [details] Fix and layout tests (In reply to comment #19) > Ok, new suggestion: Use the "Ahem" font, that should give you exactly the same results on all platforms (hopefully). Text will appear as black blocks (rects), all of the same width/height and advance. > Can you give that another try? I did, it worked, thanks for the suggestion. One thing that I found convenient is that I no longer need two -expected files for each of the two test cases. But I still see a virtue in not having the font-dependent coordinates hardcoded in the HTML source: in the moment when I loaded the HTML in the browser, my first instinct was, oh! now there are FAILures! In fact, everything was fine. It was the browser just showing the regular font (thus producing the errors), while the tests are now run using the Ahem font. > I'd love to avoid SKIP ... messages, or not checking something. But they are still being checked, using the -expected file. One of the following two things should be done: either leave the SKIPs in there and use the expectation file to check the actual coordinates; or insert a code snippet that only allows the two tests to run in testshell, so that they don't look alarming and misleadingly incorrect in the browser. In my latest patch, I did the former, because I think that loading the HTML tests in the browser is useful. However, if you still want me to do the latter, please tell me, and I'll resubmit.
(In reply to comment #21) > Created an attachment (id=74353) [details] > Fix and layout tests > > (In reply to comment #19) > > Ok, new suggestion: Use the "Ahem" font, that should give you exactly the same results on all platforms (hopefully). Text will appear as black blocks (rects), all of the same width/height and advance. > > Can you give that another try? > > I did, it worked, thanks for the suggestion. One thing that I found convenient is that I no longer need two -expected files for each of the two test cases. > > But I still see a virtue in not having the font-dependent coordinates hardcoded in the HTML source: in the moment when I loaded the HTML in the browser, my first instinct was, oh! now there are FAILures! > In fact, everything was fine. It was the browser just showing the regular font (thus producing the errors), while the tests are now run using the Ahem font. Sure, these tests are really intended to be run in DRT only, it doesn't make much sense to test these zooming things in the browser itself. If you want to be sure you can add a: if (!window.layoutTestController) { alert('please run in drt'); return; } on top of the test. > > > I'd love to avoid SKIP ... messages, or not checking something. > > But they are still being checked, using the -expected file. Yeah, but errors are not immediately obvious, unlike where you expect to see 'PASS' messages and now get 'FAIL' messages. > > One of the following two things should be done: either leave the SKIPs in there and use the expectation file to check the actual coordinates; or insert a code snippet that only allows the two tests to run in testshell, so that they don't look alarming and misleadingly incorrect in the browser. In my latest patch, I did the former, because I think that loading the HTML tests in the browser is useful. However, if you still want me to do the latter, please tell me, and I'll resubmit. It would be great, if you would warn the user if anyone tries to execute the tests in a non-DRT environment (eg. in Safari) and replace all SKIP usages by shouldBe(...). Then you also don't need to dump the actual results in the expecaction files. Thanks for you hard work!
Created attachment 74442 [details] Fix and layout tests (In reply to comment #22) > It would be great, if you would warn the user if anyone tries to execute the tests in a non-DRT environment (eg. in Safari) and replace all SKIP usages by shouldBe(...). Then you also don't need to dump the actual results in the expecaction files. Done. I now realized that possible failures may accidentally end up under the radar, if they occur at a time when tests are being rebaselined. So it's a good thing that you insisted. > Thanks for you hard work! This one wasn't really that hard, but thank you for your appreciation :-)
Comment on attachment 74442 [details] Fix and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=74442&action=review Patch is still perfect, I"m tempted to r+ it, but there are still some minor issues with the LayoutTests: > LayoutTests/css3/zoom-coords.xhtml:-87 > -shouldBe('text1.top >= 75', 'true'); > +shouldBe('text1.top >= 250', 'true'); > shouldBe('text1.width > 0', 'true'); > shouldBe('text1.height > 0', 'true'); > shouldBe('text1.right > 100', 'true'); > -shouldBe('text1.bottom > 75', 'true'); There's no way you could check for exact coordinate matches here?? > LayoutTests/css3/zoom-coords.xhtml:136 > -shouldBe('text2.left == 800', 'true'); > -shouldBe('text2.top >= 400', 'true'); > +shouldBe('text2.left == 175', 'true'); > +shouldBe('text2.top >= 100', 'true'); Ditto. > LayoutTests/css3/zoom-coords.xhtml:141 > shouldBe('text2.width > 0', 'true'); > shouldBe('text2.height > 0', 'true'); > -shouldBe('text2.right > 800', 'true'); > -shouldBe('text2.bottom > 400', 'true'); > +shouldBe('text2.right > 175', 'true'); > +shouldBe('text2.bottom > 100', 'true'); > +debug(""); Ditto. > LayoutTests/css3/zoom-coords.xhtml:170 > +shouldBe('text3.left == 1100', 'true'); > +shouldBe('text3.top >= 500', 'true'); > +shouldBe('text3.width > 0', 'true'); > +shouldBe('text3.height > 0', 'true'); > +shouldBe('text3.right > 1100', 'true'); > +shouldBe('text3.bottom > 500', 'true'); Ditto. > LayoutTests/fast/dom/Element/getBoundingClientRect-expected.txt:8 > +PASS rect.left.toFixed(3) is "8.000" > +PASS rect.top.toFixed(3) is "40.000" > +PASS rect.width.toFixed(3) is "300.000" > +PASS rect.height.toFixed(3) is "100.000" > +PASS rect.right is rect.left + rect.width > +PASS rect.bottom is rect.top + rect.height > + Excellent! > LayoutTests/svg/zoom/page/zoom-getBoundingClientRect-expected.txt:6 > +PASS parseFloat(r1.left.toFixed(2)) is 20 You could have used: shouldBeEqualToString("r1.left.toFixed(2)", "20.00") This is cleaner, then using parseFloat on a toFixed returned string. > LayoutTests/svg/zoom/page/zoom-getBoundingClientRect.xhtml:11 > +</svg><svg xmlns="http://www.w3.org/2000/svg" version="1.1" Can you add a newline between </svg> and <svg>. > LayoutTests/svg/zoom/page/zoom-getBoundingClientRect.xhtml:15 > +</svg><svg xmlns="http://www.w3.org/2000/svg" version="1.1" Ditto. > LayoutTests/svg/zoom/page/zoom-getBoundingClientRect.xhtml:34 > + shouldBe('parseFloat(r1.left.toFixed(2))', '20'); > + shouldBe('parseFloat(r1.top.toFixed(2))', '30'); As stated above, shouldBeEqualToString should be sufficient. > LayoutTests/svg/zoom/page/zoom-zoom-coords-expected.txt:86 > +PASS parseFloat(text2.top.toFixed(2)) >= 100 is true > +PASS parseFloat(text2.width.toFixed(2)) > 0 is true > +PASS parseFloat(text2.height.toFixed(2)) > 0 is true > +PASS parseFloat(text2.right.toFixed(2)) > 175 is true > +PASS parseFloat(text2.bottom.toFixed(2)) > 100 is true > + No way to use equality here? > LayoutTests/svg/zoom/page/zoom-zoom-coords-expected.txt:110 > +PASS parseFloat(text3.top.toFixed(2)) >= 500 is true > +PASS parseFloat(text3.width.toFixed(2)) > 0 is true > +PASS parseFloat(text3.height.toFixed(2)) > 0 is true > +PASS parseFloat(text3.right.toFixed(2)) > 1100 is true > +PASS parseFloat(text3.bottom.toFixed(2)) > 500 is true Ditto. > LayoutTests/svg/zoom/page/zoom-zoom-coords.xhtml:22 > +</svg><svg id="svg2" xmlns="http://www.w3.org/2000/svg" Please add a newline between </svg> and <svg>. > LayoutTests/svg/zoom/page/zoom-zoom-coords.xhtml:28 > +</svg><svg id="svg3" xmlns="http://www.w3.org/2000/svg" Ditto.
Created attachment 74599 [details] Fix and layout tests (In reply to comment #24) Ok, Niko, I did everything that you asked, except for the following: > > LayoutTests/svg/zoom/page/zoom-zoom-coords.xhtml:22 > > +</svg><svg id="svg2" xmlns="http://www.w3.org/2000/svg" > > Please add a newline between </svg> and <svg>. > > > LayoutTests/svg/zoom/page/zoom-zoom-coords.xhtml:28 > > +</svg><svg id="svg3" xmlns="http://www.w3.org/2000/svg" > > Ditto. If I insert whitespace between </svg><svg>, the renderer will insert a small space between the two SVG images, wreacking havoc in all those well-behaved, easy-to-follow (multiples of 50, etc.) coordinates that I carefully crafted. I'd rather not have that little space, and the small cost of having two contiguous </svg><svg> tags inside the test case is IMO significantly outweighed by its benefit.
(In reply to comment #25) > Created an attachment (id=74599) [details] > Fix and layout tests > > (In reply to comment #24) > Ok, Niko, > I did everything that you asked, except for the following: > > > > LayoutTests/svg/zoom/page/zoom-zoom-coords.xhtml:22 > > > +</svg><svg id="svg2" xmlns="http://www.w3.org/2000/svg" > > > > Please add a newline between </svg> and <svg>. > > > > > LayoutTests/svg/zoom/page/zoom-zoom-coords.xhtml:28 > > > +</svg><svg id="svg3" xmlns="http://www.w3.org/2000/svg" > > > > Ditto. > > If I insert whitespace between </svg><svg>, the renderer will insert a small space between the two SVG images, wreacking havoc in all those well-behaved, easy-to-follow (multiples of 50, etc.) coordinates that I carefully crafted. I'd rather not have that little space, and the small cost of having two contiguous </svg><svg> tags inside the test case is IMO significantly outweighed by its benefit. Great, will review now.
Comment on attachment 74599 [details] Fix and layout tests Wonderful patch, thanks for your great patience with this patch!
(In reply to comment #27) > (From update of attachment 74599 [details]) > Wonderful patch, thanks for your great patience with this patch! And I thank you for your thorough review :-)
The commit-queue encountered the following flaky tests while processing attachment 74599 [details]: fast/workers/storage/use-same-database-in-page-and-workers.html compositing/iframes/overlapped-nested-iframes.html Please file bugs against the tests. These tests were authored by dumi@chromium.org and simon.fraser@apple.com. The commit-queue is continuing to process your patch.
The commit-queue encountered the following flaky tests while processing attachment 74599 [details]: compositing/iframes/overlapped-nested-iframes.html fast/workers/storage/use-same-database-in-page-and-workers.html Please file bugs against the tests. These tests were authored by dumi@chromium.org and simon.fraser@apple.com. The commit-queue is continuing to process your patch.
Comment on attachment 74599 [details] Fix and layout tests Clearing flags on attachment: 74599 Committed r72826: <http://trac.webkit.org/changeset/72826>
All reviewed patches have been landed. Closing bug.