Page Scale affects window.scrollTo

• Component: New Bugs
• Status: RESOLVED FIXED

[John Knottenbelt]

Page Scale affects window.scrollTo

[John Knottenbelt]

Created attachment 107334 [details]
Patch

[John Knottenbelt]

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

Created attachment 107870 [details]
Patch

[Simon Fraser (smfr)]

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]

Created attachment 108004 [details]
Patch

[Hin-Chung Lam]

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 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 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 on attachment 108004 [details]
Patch

Attachment 108004 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9764462

[Hin-Chung Lam]

Committed r95608: <http://trac.webkit.org/changeset/95608>

[Philippe Normand]

Can I know why this landed meanwhile it failed to pass EWS builds on 2 ports?

[Hin-Chung Lam]

Whoops.. Sorry! I overlooked.. I'll fix those build failures now.

[Philippe Normand]

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]

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

Created attachment 108310 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 108310 [details]
Patch for landing

Clearing flags on attachment: 108310

Committed r95715: <http://trac.webkit.org/changeset/95715>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.