Bug 45228 – window.scrollBy() scrolls incorrectly when zoomed in/out

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[David Chambers]

Created attachment 66584 [details]
Demo of incorrect window.scrollBy() behaviour

window.scrollBy(0, 100) does not scroll by 100px if the browser's "zoom" is greater than or less than 100%. When zoomed in, the window is scrolled by less than 100px. When zoomed out, the window is scrolled by more than 100px. This suggests that window.scrollBy() does not respect the scaling factor being applied to the page.

Firefox handles these situations correctly.

[Martin Bouladour]

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

[Antonio Gomes]

I do not think it is a JavaScriptCore issue.

[Sneha Bhat]

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.

[Kenneth Rohde Christiansen]

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?

[Sneha Bhat]

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

[Kenneth Rohde Christiansen]

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?

[Sneha Bhat]

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

[WebKit Review Bot]

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

[Jarred Nicholls]

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

[Adam Barth]

> Just FYI this is a false alarm that chromium-ews is throwing.

My apologies for the false alarm.  It should be silenced now.

[Kenneth Rohde Christiansen]

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?

[Sneha Bhat]

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

[Sneha Bhat]

Created attachment 118111 [details]
Updated Patch

[Fady Samuel]

(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! :-)

[Sneha Bhat]

Created attachment 118248 [details]
Marked as patch

Just marking the attachment as patch

[Fady Samuel]

Comment on attachment 118248 [details]
Marked as patch

Please test this with page scaling before attempting to land this change.

[Sneha Bhat]

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

[Sneha Bhat]

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

[Sneha Bhat]

Created attachment 119045 [details]
Added the Test for scrolling when Scale factor is applied

The proposed fix works fine when scale factor is applied.

[Jarred Nicholls]

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.

[Julien Chaffraix]

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

[Eugene Girard]

Created attachment 133901 [details]
Patch

[Simon Fraser (smfr)]

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.

[Eugene Girard]

Created attachment 133927 [details]
Patch

[Eugene Girard]

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

[Simon Fraser (smfr)]

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.

[Eugene Girard]

Created attachment 134280 [details]
Patch

[WebKit Review Bot]

Comment on attachment 134280 [details]
Patch

Clearing flags on attachment: 134280

Committed r112395: <http://trac.webkit.org/changeset/112395>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.