WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
48110
getBoundingClientRect: Do not truncate the coordinates to integers
https://bugs.webkit.org/show_bug.cgi?id=48110
Summary
getBoundingClientRect: Do not truncate the coordinates to integers
Cosmin Truta
Reported
2010-10-21 21:38:13 PDT
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
.
Attachments
Test case (layout test)
(1.88 KB, application/xhtml+xml)
2010-10-21 21:44 PDT
,
Cosmin Truta
no flags
Details
New async testcases
(155.55 KB, patch)
2010-10-22 05:26 PDT
,
Nikolas Zimmermann
no flags
Details
Formatted Diff
Diff
Fix and layout tests
(35.71 KB, patch)
2010-11-01 12:13 PDT
,
Cosmin Truta
ctruta
: review-
Details
Formatted Diff
Diff
Fix and layout test
(41.41 KB, patch)
2010-11-01 20:27 PDT
,
Cosmin Truta
zimmermann
: review-
Details
Formatted Diff
Diff
Fix and layout test
(41.37 KB, patch)
2010-11-02 06:45 PDT
,
Cosmin Truta
ctruta
: review-
ctruta
: commit-queue-
Details
Formatted Diff
Diff
Fix and layout test
(41.33 KB, patch)
2010-11-02 07:11 PDT
,
Cosmin Truta
no flags
Details
Formatted Diff
Diff
Fix and layout test
(64.75 KB, patch)
2010-11-16 23:42 PST
,
Cosmin Truta
no flags
Details
Formatted Diff
Diff
Fix and layout tests
(78.27 KB, patch)
2010-11-17 22:26 PST
,
Cosmin Truta
no flags
Details
Formatted Diff
Diff
Fix and layout tests
(71.37 KB, patch)
2010-11-18 21:45 PST
,
Cosmin Truta
no flags
Details
Formatted Diff
Diff
Fix and layout tests
(69.91 KB, patch)
2010-11-19 16:50 PST
,
Cosmin Truta
zimmermann
: review-
Details
Formatted Diff
Diff
Fix and layout tests
(70.67 KB, patch)
2010-11-22 14:37 PST
,
Cosmin Truta
no flags
Details
Formatted Diff
Diff
Show Obsolete
(9)
View All
Add attachment
proposed patch, testcase, etc.
Cosmin Truta
Comment 1
2010-10-21 21:44:13 PDT
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
Nikolas Zimmermann
Comment 2
2010-10-22 05:26:28 PDT
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.
Cosmin Truta
Comment 3
2010-11-01 12:13:30 PDT
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.
Cosmin Truta
Comment 4
2010-11-01 20:05:29 PDT
Changing the bug title: "getBoundingClientRect: Do not truncate the coordinates to integers"
Cosmin Truta
Comment 5
2010-11-01 20:12:03 PDT
***
Bug 48640
has been marked as a duplicate of this bug. ***
Cosmin Truta
Comment 6
2010-11-01 20:27:20 PDT
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.
Cosmin Truta
Comment 7
2010-11-01 20:29:05 PDT
Comment on
attachment 72538
[details]
Fix and layout tests Oops, forgot to mark the previous patch obsolete.
Nikolas Zimmermann
Comment 8
2010-11-02 00:15:38 PDT
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.
Cosmin Truta
Comment 9
2010-11-02 06:45:16 PDT
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...)
Cosmin Truta
Comment 10
2010-11-02 07:04:50 PDT
Comment on
attachment 72658
[details]
Fix and layout test Oops, forgot to update WebCore/ChangeLog
Cosmin Truta
Comment 11
2010-11-02 07:11:44 PDT
Created
attachment 72662
[details]
Fix and layout test
Nikolas Zimmermann
Comment 12
2010-11-15 23:29:26 PST
(In reply to
comment #11
)
> Created an attachment (id=72662) [details] > Fix and layout test
Looks great! Sorry for the delayed response...
WebKit Commit Bot
Comment 13
2010-11-16 00:15:24 PST
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
Cosmin Truta
Comment 14
2010-11-16 23:42:44 PST
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?
Nikolas Zimmermann
Comment 15
2010-11-17 03:20:53 PST
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?
Cosmin Truta
Comment 16
2010-11-17 07:57:52 PST
(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.
Nikolas Zimmermann
Comment 17
2010-11-17 08:17:05 PST
(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...
Cosmin Truta
Comment 18
2010-11-17 22:26:38 PST
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.
Nikolas Zimmermann
Comment 19
2010-11-18 00:36:03 PST
(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.
Eric Seidel (no email)
Comment 20
2010-11-18 03:18:13 PST
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
.
Cosmin Truta
Comment 21
2010-11-18 21:45:37 PST
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.
Nikolas Zimmermann
Comment 22
2010-11-19 05:37:01 PST
(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!
Cosmin Truta
Comment 23
2010-11-19 16:50:48 PST
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 :-)
Nikolas Zimmermann
Comment 24
2010-11-19 19:32:50 PST
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.
Cosmin Truta
Comment 25
2010-11-22 14:37:43 PST
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.
Nikolas Zimmermann
Comment 26
2010-11-29 11:09:36 PST
(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.
Nikolas Zimmermann
Comment 27
2010-11-29 11:14:17 PST
Comment on
attachment 74599
[details]
Fix and layout tests Wonderful patch, thanks for your great patience with this patch!
Cosmin Truta
Comment 28
2010-11-29 11:46:21 PST
(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 :-)
WebKit Commit Bot
Comment 29
2010-11-29 12:21:39 PST
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.
WebKit Commit Bot
Comment 30
2010-11-29 13:07:47 PST
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.
WebKit Commit Bot
Comment 31
2010-11-29 13:39:59 PST
Comment on
attachment 74599
[details]
Fix and layout tests Clearing flags on attachment: 74599 Committed
r72826
: <
http://trac.webkit.org/changeset/72826
>
WebKit Commit Bot
Comment 32
2010-11-29 13:40:07 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug