Bug 45228 - window.scrollBy() scrolls incorrectly when zoomed in/out
Summary: window.scrollBy() scrolls incorrectly when zoomed in/out
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.6
: P4 Minor
Assignee: Eugene Girard
URL:
Keywords:
Depends on:
Blocks: 68075
  Show dependency treegraph
 
Reported: 2010-09-04 10:05 PDT by David Chambers
Modified: 2012-03-28 07:38 PDT (History)
12 users (show)

See Also:


Attachments
Demo of incorrect window.scrollBy() behaviour (2.49 KB, text/html)
2010-09-04 10:05 PDT, David Chambers
no flags Details
Proposed Fix for the issue (3.75 KB, patch)
2011-12-05 11:20 PST, Sneha Bhat
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Updated Patch (3.63 KB, text/plain)
2011-12-06 14:23 PST, Sneha Bhat
no flags Details
Marked as patch (3.63 KB, patch)
2011-12-07 11:52 PST, Sneha Bhat
fsamuel: commit-queue-
Details | Formatted Diff | Diff
Added the Test for scrolling when Scale factor is applied (5.76 KB, patch)
2011-12-13 11:13 PST, Sneha Bhat
no flags Details | Formatted Diff | Diff
Patch (6.73 KB, patch)
2012-03-26 15:00 PDT, Eugene Girard
no flags Details | Formatted Diff | Diff
Patch (6.81 KB, patch)
2012-03-26 16:38 PDT, Eugene Girard
no flags Details | Formatted Diff | Diff
Patch (Revised based on smfr's review. Fixed borken logic. Updated test logic to catch the underlying error as well.) (6.76 KB, patch)
2012-03-26 16:48 PDT, Eugene Girard
no flags Details | Formatted Diff | Diff
Patch (6.90 KB, patch)
2012-03-28 06:14 PDT, Eugene Girard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Chambers 2010-09-04 10:05:17 PDT
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.
Comment 1 Martin Bouladour 2011-04-12 19:02:52 PDT
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
Comment 2 Antonio Gomes 2011-10-06 07:07:32 PDT
I do not think it is a JavaScriptCore issue.
Comment 3 Sneha Bhat 2011-12-05 11:20:54 PST
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 4 Kenneth Rohde Christiansen 2011-12-05 12:20:29 PST
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?
Comment 5 Sneha Bhat 2011-12-05 13:26:40 PST
I have taken the reference from window.scrollTo() API which does add the pageZoomFactor .
Comment 6 Kenneth Rohde Christiansen 2011-12-05 13:27:51 PST
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?
Comment 7 Sneha Bhat 2011-12-05 14:15:17 PST
>>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 8 WebKit Review Bot 2011-12-05 16:16:31 PST
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
Comment 9 Jarred Nicholls 2011-12-05 17:06:50 PST
(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.
Comment 10 Adam Barth 2011-12-05 17:18:31 PST
> Just FYI this is a false alarm that chromium-ews is throwing.

My apologies for the false alarm.  It should be silenced now.
Comment 11 Kenneth Rohde Christiansen 2011-12-06 01:17:13 PST
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?
Comment 12 Sneha Bhat 2011-12-06 14:22:19 PST
(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.
Comment 13 Sneha Bhat 2011-12-06 14:23:23 PST
Created attachment 118111 [details]
Updated Patch
Comment 14 Fady Samuel 2011-12-06 16:46:51 PST
(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! :-)
Comment 15 Sneha Bhat 2011-12-07 11:52:35 PST
Created attachment 118248 [details]
Marked as patch

Just marking the attachment as patch
Comment 16 Fady Samuel 2011-12-07 11:55:53 PST
Comment on attachment 118248 [details]
Marked as patch

Please test this with page scaling before attempting to land this change.
Comment 17 Sneha Bhat 2011-12-07 17:19:06 PST
 I added document.body.style.zoom = 2.0; and then tried with and without my patch and the window.scrollBy works fine.
Comment 18 Sneha Bhat 2011-12-08 12:15:08 PST
Just to be clear what do you mean by Page Scaling?
Comment 19 Sneha Bhat 2011-12-13 11:13:55 PST
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 20 Jarred Nicholls 2011-12-23 18:50:20 PST
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 21 Julien Chaffraix 2011-12-28 02:03:37 PST
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).
Comment 22 Eugene Girard 2012-03-26 15:00:45 PDT
Created attachment 133901 [details]
Patch
Comment 23 Simon Fraser (smfr) 2012-03-26 15:08:04 PDT
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.
Comment 24 Eugene Girard 2012-03-26 16:38:41 PDT
Created attachment 133927 [details]
Patch
Comment 25 Eugene Girard 2012-03-26 16:48:33 PDT
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 26 Simon Fraser (smfr) 2012-03-27 21:42:15 PDT
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.
Comment 27 Eugene Girard 2012-03-28 06:14:47 PDT
Created attachment 134280 [details]
Patch
Comment 28 WebKit Review Bot 2012-03-28 07:38:13 PDT
Comment on attachment 134280 [details]
Patch

Clearing flags on attachment: 134280

Committed r112395: <http://trac.webkit.org/changeset/112395>
Comment 29 WebKit Review Bot 2012-03-28 07:38:20 PDT
All reviewed patches have been landed.  Closing bug.