Bug 68081 - Page Scale affects window.scrollTo
Summary: Page Scale affects window.scrollTo
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Knottenbelt
URL:
Keywords:
Depends on:
Blocks: 68075
  Show dependency treegraph
 
Reported: 2011-09-14 08:25 PDT by John Knottenbelt
Modified: 2011-09-22 06:30 PDT (History)
10 users (show)

See Also:


Attachments
Patch (5.03 KB, patch)
2011-09-14 08:26 PDT, John Knottenbelt
no flags Details | Formatted Diff | Diff
Patch (7.08 KB, patch)
2011-09-19 09:14 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (26.66 KB, patch)
2011-09-20 08:23 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch for landing (30.21 KB, patch)
2011-09-22 04:21 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Knottenbelt 2011-09-14 08:25:32 PDT
Page Scale affects window.scrollTo
Comment 1 John Knottenbelt 2011-09-14 08:26:13 PDT
Created attachment 107334 [details]
Patch
Comment 2 John Knottenbelt 2011-09-14 08:38:26 PDT
In this layout test there is a 3x2 grid of 100x100 coloured divs. The test case gives an expected behaviour of scrolling to page coordinates (100,100) - I expect to see just the bottom right two divs.

However, when I run this layout test on DumpRenderTree, the actual visible output is a scroll to page coordinates (50, 50) -- half the top row and half the left column. 

Please let me know if my expected output (as in the patch) is incorrect. 

Thanks!
Comment 3 Simon Fraser (smfr) 2011-09-14 09:05:52 PDT
Comment on attachment 107334 [details]
Patch

The image result needs to go into a platform-specific directory, because it shows scrollbars.

Please avoid red in successful pixel results.
Comment 4 Hin-Chung Lam 2011-09-19 09:14:52 PDT
Created attachment 107870 [details]
Patch
Comment 5 Simon Fraser (smfr) 2011-09-19 10:59:42 PDT
Comment on attachment 107870 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107870&action=review

> LayoutTests/fast/events/scale-and-scroll.html:20
> +          var scaleOffset = 0;

Would be nice to test with a non-zero offset.

> Source/WebCore/page/DOMWindow.cpp:1146
> +    return static_cast<int>(view->scrollX() / m_frame->pageZoomFactor() / m_frame->pageScaleFactor());

I'd slightly prefer scrollX / (pagezoomfactor * pagescalefactor)
Comment 6 Hin-Chung Lam 2011-09-20 08:23:47 PDT
Created attachment 108004 [details]
Patch
Comment 7 Hin-Chung Lam 2011-09-20 08:24:32 PDT
Actually the last patch didn't solve all the problem, namely iframes.

I've corrected the root problem and added some more test cases.
Comment 8 WebKit Review Bot 2011-09-20 09:44:08 PDT
Comment on attachment 108004 [details]
Patch

Attachment 108004 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9767300
Comment 9 Simon Fraser (smfr) 2011-09-20 10:09:38 PDT
Comment on attachment 108004 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108004&action=review

> Source/WebCore/ChangeLog:13
> +        2. Frame::frameScaleFactor() returns the scale factor of this frame with
> +           respect to the container. So for the main frame it'll return the scale
> +           factor of the page, inner frames will return 1.0

I wonder about behavioral changes as result of this. For example, was adjustForZoom() only ever called with the main frame's document before, or was there a bug?

> Source/WebCore/page/FrameView.cpp:530
> +    bool overrideHidden = m_frame->page() && m_frame->page()->mainFrame() == m_frame && m_frame->frameScaleFactor() > 1;

Does this still require the m_frame->page()->mainFrame() == m_frame check?
Comment 10 Collabora GTK+ EWS bot 2011-09-20 15:42:03 PDT
Comment on attachment 108004 [details]
Patch

Attachment 108004 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9764462
Comment 11 Hin-Chung Lam 2011-09-21 04:00:56 PDT
Committed r95608: <http://trac.webkit.org/changeset/95608>
Comment 12 Philippe Normand 2011-09-21 04:30:34 PDT
Can I know why this landed meanwhile it failed to pass EWS builds on 2 ports?
Comment 13 Hin-Chung Lam 2011-09-21 04:34:25 PDT
Whoops.. Sorry! I overlooked.. I'll fix those build failures now.
Comment 14 Philippe Normand 2011-09-21 04:43:44 PDT
I rolled it out before I saw your comment, sorry! I can help with GTK failures if needed. Please don't forget EWS next time, it's a great tool and it's sad to not take full advantage of it.
Comment 15 Hin-Chung Lam 2011-09-21 04:45:00 PDT
(In reply to comment #14)
> I rolled it out before I saw your comment, sorry! I can help with GTK failures if needed. Please don't forget EWS next time, it's a great tool and it's sad to not take full advantage of it.

Thanks for acting quick! Will be sure to use it next time, I'm fixing the patch now.
Comment 16 Hin-Chung Lam 2011-09-22 04:21:36 PDT
Created attachment 108310 [details]
Patch for landing
Comment 17 WebKit Review Bot 2011-09-22 06:30:08 PDT
Comment on attachment 108310 [details]
Patch for landing

Clearing flags on attachment: 108310

Committed r95715: <http://trac.webkit.org/changeset/95715>
Comment 18 WebKit Review Bot 2011-09-22 06:30:15 PDT
All reviewed patches have been landed.  Closing bug.