Bug 68081

Summary: Page Scale affects window.scrollTo
Product: WebKit Reporter: John Knottenbelt <jknotten>
Component: New BugsAssignee: John Knottenbelt <jknotten>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, bdakin, fsamuel, gustavo.noronha, gustavo, hclam, pnormand, simon.fraser, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 68075    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

John Knottenbelt
Reported 2011-09-14 08:25:32 PDT
Page Scale affects window.scrollTo
Attachments
Patch (5.03 KB, patch)
2011-09-14 08:26 PDT, John Knottenbelt 		no flags
DetailsFormatted DiffDiff
Patch (7.08 KB, patch)
2011-09-19 09:14 PDT, Hin-Chung Lam 		no flags
DetailsFormatted DiffDiff
Patch (26.66 KB, patch)
2011-09-20 08:23 PDT, Hin-Chung Lam 		no flags
DetailsFormatted DiffDiff
Patch for landing (30.21 KB, patch)
2011-09-22 04:21 PDT, Hin-Chung Lam 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
John Knottenbelt
Comment 1 2011-09-14 08:26:13 PDT
Created attachment 107334 [details] Patch
John Knottenbelt
Comment 2 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!
Simon Fraser (smfr)
Comment 3 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.
Hin-Chung Lam
Comment 4 2011-09-19 09:14:52 PDT
Created attachment 107870 [details] Patch
Simon Fraser (smfr)
Comment 5 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)
Hin-Chung Lam
Comment 6 2011-09-20 08:23:47 PDT
Created attachment 108004 [details] Patch
Hin-Chung Lam
Comment 7 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.
WebKit Review Bot
Comment 8 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
Simon Fraser (smfr)
Comment 9 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?
Collabora GTK+ EWS bot
Comment 10 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
Hin-Chung Lam
Comment 11 2011-09-21 04:00:56 PDT
Committed r95608: <http://trac.webkit.org/changeset/95608>
Philippe Normand
Comment 12 2011-09-21 04:30:34 PDT
Can I know why this landed meanwhile it failed to pass EWS builds on 2 ports?
Hin-Chung Lam
Comment 13 2011-09-21 04:34:25 PDT
Whoops.. Sorry! I overlooked.. I'll fix those build failures now.
Philippe Normand
Comment 14 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.
Hin-Chung Lam
Comment 15 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.
Hin-Chung Lam
Comment 16 2011-09-22 04:21:36 PDT
Created attachment 108310 [details] Patch for landing
WebKit Review Bot
Comment 17 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>
WebKit Review Bot
Comment 18 2011-09-22 06:30:15 PDT
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.