Bug 45228

I confirm David Chambers' report: window.scrollBy() doesn't respect the scaling factor when zoomed in/out (full zoom, not text-only zoom). Example code: var former_offset = document.body.scrollTop window.scrollBy(0, 300) if (former_offset + 300 != document.body.scrollTop) { // Bad. This is the case when zoomed in/out. } However, the behaviour of window.scrollTo() is fine. Thus it can be used as a workaround: window.scrollTo(0, 300 + document.body.scrollTop) This is equivalent to the right behaviour of: window.scrollBy(0, 300) Tested in webkit r83584

I do not think it is a JavaScriptCore issue.

Created attachment 117906 [details] Proposed Fix for the issue When the page is zoomed in/Out, the relative scroll distance must take the zoom factor into account.

Comment on attachment 117906 [details] Proposed Fix for the issue View in context: https://bugs.webkit.org/attachment.cgi?id=117906&action=review > Source/WebCore/page/DOMWindow.cpp:1442 > + int zoomedX = static_cast<int>(x * m_frame->pageZoomFactor() * m_frame->frameScaleFactor()); > + int zoomedY = static_cast<int>(y * m_frame->pageZoomFactor() * m_frame->frameScaleFactor()); > + view->scrollBy(IntSize(zoomedX, zoomedY)); Are you sure that Safari etc doesn't already add the pageZoomFactor in their client? and thus this may break their code?

I have taken the reference from window.scrollTo() API which does add the pageZoomFactor .

Comment on attachment 117906 [details] Proposed Fix for the issue View in context: https://bugs.webkit.org/attachment.cgi?id=117906&action=review > Source/WebCore/page/DOMWindow.cpp:-1441 > FrameView* view = m_frame->view(); > if (!view) > return; > - > - view->scrollBy(IntSize(x, y)); but I dont see pageZoomFactor included in this old code here?

>>but I dont see pageZoomFactor included in this old code here? Yes pageZoomFactor was not not applied and hence the issue with window.scrollBy() So If you look at the function scrollTo, Source/WebCore/page/DOMWindow.cpp: void DOMWindow::scrollTo(int x, int y) const { ..... ...... int zoomedX = static_cast<int>(x * m_frame->pageZoomFactor() * m_frame->frameScaleFactor()); int zoomedY = static_cast<int>(y * m_frame->pageZoomFactor() * m_frame->frameScaleFactor()); view->setScrollPosition(IntPoint(zoomedX, zoomedY)); } The relative scroll distance is multiplied with the zoom factor, same should be done for the API window.scrollBy

Comment on attachment 117906 [details] Proposed Fix for the issue Attachment 117906 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10736470 New failing tests: svg/custom/linking-uri-01-b.svg

(In reply to comment #8) > (From update of attachment 117906 [details]) > Attachment 117906 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/10736470 > > New failing tests: > svg/custom/linking-uri-01-b.svg Just FYI this is a false alarm that chromium-ews is throwing.

> Just FYI this is a false alarm that chromium-ews is throwing. My apologies for the false alarm. It should be silenced now.

Comment on attachment 117906 [details] Proposed Fix for the issue View in context: https://bugs.webkit.org/attachment.cgi?id=117906&action=review > LayoutTests/fast/dom/zoom-scroll-page-test.html:8 > +<script src="resources/animation-test-helpers.js" type="text/javascript" charset="utf-8"></script> > +<script type="text/javascript" charset="utf-8"> im pretty sure that it is unusual to declare the type and charset in our tests. > LayoutTests/fast/dom/zoom-scroll-page-test.html:21 > + if(offset >= 199) so what if it is 400? then it should fail right?

(In reply to comment #11) > LayoutTests/fast/dom/zoom-scroll-page-test.html:21 > + if(offset >= 199) >so what if it is 400? then it should fail right? In LayoutTests/fast/dom/zoom-scroll-page-test.html:11 The value of x is 200, and window.scrollBy(0,x) should scroll the window by 200 px. However the window.pageYoffset is 199.This is possibly because of some rounding issue somewhere else. Applying the zoom factor will not alter the value of x to 400 and offset to 400px.

Created attachment 118111 [details] Updated Patch

(In reply to comment #13) > Created an attachment (id=118111) [details] > Updated Patch Please test your change with page scaling in addition to page zooming! Thanks! :-)

Created attachment 118248 [details] Marked as patch Just marking the attachment as patch

Comment on attachment 118248 [details] Marked as patch Please test this with page scaling before attempting to land this change.

I added document.body.style.zoom = 2.0; and then tried with and without my patch and the window.scrollBy works fine.

Just to be clear what do you mean by Page Scaling?

Created attachment 119045 [details] Added the Test for scrolling when Scale factor is applied The proposed fix works fine when scale factor is applied.

Comment on attachment 119045 [details] Added the Test for scrolling when Scale factor is applied View in context: https://bugs.webkit.org/attachment.cgi?id=119045&action=review I would also have a test that changes both zoom and scale factor at once. > LayoutTests/fast/dom/scroll-scaled-page-test.html:7 > +<script> Hoist the script up into <head> > LayoutTests/fast/dom/scroll-scaled-page-test.html:10 > + if (window.layoutTestController) { fast & http tests are preferred to use the test harness code in LayoutTests/fast/js/resources/. You should include js-test-pre.js before your test script and js-test-post.js after your test script, and utilize the test functions. See http://trac.webkit.org/browser/trunk/LayoutTests/fast/xmlhttprequest/xmlhttprequest-responsetype-sync-request.html for an example test that uses this harness code. > LayoutTests/fast/dom/scroll-scaled-page-test.html:19 > + window.scrollBy(0, x) missing semi-colon (it's "legit" but we should keep it clean) > LayoutTests/fast/dom/scroll-scaled-page-test.html:25 > + resultString += " PASS - " + "The window scrolls by " + offset + "px when the scale factor is applied" ; Lots of whitespace in front of the semi-colon > LayoutTests/fast/dom/scroll-scaled-page-test.html:34 > +<body onload="testScroll()";> runaway semi-colon > LayoutTests/fast/dom/scroll-scaled-page-test.html:36 > +</body> No </html>? > LayoutTests/fast/dom/zoom-scroll-page-test.html:7 > +<script> hoist up > LayoutTests/fast/dom/zoom-scroll-page-test.html:16 > + window.scrollBy(0, x) semi-colon > LayoutTests/fast/dom/zoom-scroll-page-test.html:31 > +<body onload="testScroll()";> runaway semi-colon > LayoutTests/fast/dom/zoom-scroll-page-test.html:34 > + </html> > Source/WebCore/ChangeLog:11 > + The relative scroll distance must take the zoom factor into account. It's better to put the description up higher, under the bug title & url.

Comment on attachment 119045 [details] Added the Test for scrolling when Scale factor is applied View in context: https://bugs.webkit.org/attachment.cgi?id=119045&action=review The change looks OK to me with some comments. r- mostly because of the test is too fuzzy and the comments to handle. > LayoutTests/fast/dom/scroll-scaled-page-test.html:1 > +<html> Please add a doctype here: <!DOCTYPE html>. The rule of thumb is to add it when you don't need to explicitly test quirks mode. >> LayoutTests/fast/dom/scroll-scaled-page-test.html:10 >> + if (window.layoutTestController) { > > fast & http tests are preferred to use the test harness code in LayoutTests/fast/js/resources/. You should include js-test-pre.js before your test script and js-test-post.js after your test script, and utilize the test functions. See http://trac.webkit.org/browser/trunk/LayoutTests/fast/xmlhttprequest/xmlhttprequest-responsetype-sync-request.html for an example test that uses this harness code. Just to add on that, this is a preference not a requirement. Here the test is simple enough that I wouldn't be shocked if it did not use it. > LayoutTests/fast/dom/scroll-scaled-page-test.html:24 > + if(offset >= 199) I would be way better to test explicitly what you are expecting. As Kenneth pointed out, 400px would make the test pass, though I am not sure it is what you expect here. > LayoutTests/fast/dom/scroll-scaled-page-test.html:30 > + layoutTestController.notifyDone(); You don't need to call layoutTestController.waitUntilDone / notifyDone. This is used to prevent DRT from dumping the results after dispatching the 'load' event as it normally does. > LayoutTests/fast/dom/zoom-scroll-page-test.html:1 > +<html> Ditto doctype. > LayoutTests/fast/dom/zoom-scroll-page-test.html:3 > + <style type="text/css"> No need to put the type here. > LayoutTests/fast/dom/zoom-scroll-page-test.html:12 > + layoutTestController.waitUntilDone(); Same comment about waitUntilDone. >> Source/WebCore/ChangeLog:11 >> + The relative scroll distance must take the zoom factor into account. > > It's better to put the description up higher, under the bug title & url. There is no consensus on where to put your description. It is left to the contributor's taste, not the reviewer's. > Source/WebCore/ChangeLog:14 > + (WebCore::DOMWindow::scrollBy): It would be nice to put somewhere in your ChangeLog what you are explaining in the bug about being consistent with scrollTo. That would help reviewers understand why it is a good change. > Source/WebCore/page/DOMWindow.cpp:1441 > + int zoomedX = static_cast<int>(x * m_frame->pageZoomFactor() * m_frame->frameScaleFactor()); > + int zoomedY = static_cast<int>(y * m_frame->pageZoomFactor() * m_frame->frameScaleFactor()); You are consistent with scrollTo which is great but it would be better to explicitly state which type of float -> int conversion you are expecting (rounding, flooring, ...). The best would be to see what other browsers are doing here and try to match them (including some test cases to differentiate between the different conversion).

Created attachment 133901 [details] Patch

Comment on attachment 133901 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133901&action=review > Source/WebCore/page/DOMWindow.cpp:1353 > - view->scrollBy(IntSize(x, y)); > + IntPoint layoutPos(view->mapFromCSSToLayoutUnits(x), view->mapFromCSSToLayoutUnits(y)); > + view->setScrollPosition(layoutPos); This seems wrong. x,y is a delta, but you're using it as an absolute offset.

Created attachment 133927 [details] Patch

Created attachment 133930 [details] Patch (Revised based on smfr's review. Fixed borken logic. Updated test logic to catch the underlying error as well.)

Comment on attachment 133930 [details] Patch (Revised based on smfr's review. Fixed borken logic. Updated test logic to catch the underlying error as well.) View in context: https://bugs.webkit.org/attachment.cgi?id=133930&action=review > Source/WebCore/ChangeLog:8 > + window.scrollBy() scrolls incorrectly when zoomed in/out > + https://bugs.webkit.org/show_bug.cgi?id=45228 > + > + Reviewed by NOBODY (OOPS!). > + > + Test: fast/dom/zoom-scroll-page-test.html This needs some text to say what you did to fix the bug.

Created attachment 134280 [details] Patch

Comment on attachment 134280 [details] Patch Clearing flags on attachment: 134280 Committed r112395: <http://trac.webkit.org/changeset/112395>

All reviewed patches have been landed. Closing bug.