Fixed layout tests to be scroll order agnostic

Fixed layout tests to be scroll order agnostic

This is needed as a prerequisite to making "visual viewport scrolls first"
the default. Scrolling the page before setting scale means that the layout
viewport is the only one that could scroll.

BUG=443724
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201135

[bokan, 2015-08-18 19:35:15]

bokan@chromium.org changed reviewers:
+ majidvp@chromium.org

[bokan, 2015-08-18 19:35:25]

Quickie, ptal.

[majidvp, 2015-08-18 20:19:18]

https://codereview.chromium.org/1301523006/diff/1/LayoutTests/fast/dom/zoom-s...
File LayoutTests/fast/dom/zoom-scroll-page-test.html (right):

https://codereview.chromium.org/1301523006/diff/1/LayoutTests/fast/dom/zoom-s...
LayoutTests/fast/dom/zoom-scroll-page-test.html:29:
internals.setPageScaleFactor(scaleFactor);
Don't scrollTo, scrollBy, and pageYOffset all operate on visual viewport
coordinate? So I would imagine that the order should not matter here?

https://codereview.chromium.org/1301523006/diff/1/LayoutTests/fast/events/tou...
File LayoutTests/fast/events/touch/touch-scaled-scrolled.html (right):

https://codereview.chromium.org/1301523006/diff/1/LayoutTests/fast/events/tou...
LayoutTests/fast/events/touch/touch-scaled-scrolled.html:16:
window.scrollTo(100,100);
Why move this here? client{X,Y} should be independent of the scroll position
whether it is virtual or visual viewport that is scrolling?

I suppose it may be even better to call scrollTo before and after setting the
page scale to exercised both paths.

[bokan, 2015-08-19 17:10:45]

https://codereview.chromium.org/1301523006/diff/1/LayoutTests/fast/dom/zoom-s...
File LayoutTests/fast/dom/zoom-scroll-page-test.html (right):

https://codereview.chromium.org/1301523006/diff/1/LayoutTests/fast/dom/zoom-s...
LayoutTests/fast/dom/zoom-scroll-page-test.html:29:
internals.setPageScaleFactor(scaleFactor);
On 2015/08/18 20:19:18, majidvp wrote:
> Don't scrollTo, scrollBy, and pageYOffset all operate on visual viewport
> coordinate? So I would imagine that the order should not matter here?

Yup, you're right. This confused me for a bit so I dug into it. It turns out to
be a floating point precision issue. When we scroll "layout first", the layout
viewport has enough content that it consumes the whole scroll. Scrolling "visual
first" splits the scroll between the viewports. Because of the way we handle
fractional scroll values, this means there's two places with floating point
imprecision so our error is increased (we truncate to ints twice). I think the
right solution here is to bump the "nearness" value in shouldBeCloseTo from
3->4.

https://codereview.chromium.org/1301523006/diff/1/LayoutTests/fast/events/tou...
File LayoutTests/fast/events/touch/touch-scaled-scrolled.html (right):

https://codereview.chromium.org/1301523006/diff/1/LayoutTests/fast/events/tou...
LayoutTests/fast/events/touch/touch-scaled-scrolled.html:16:
window.scrollTo(100,100);
On 2015/08/18 20:19:18, majidvp wrote:
> Why move this here? client{X,Y} should be independent of the scroll position
> whether it is virtual or visual viewport that is scrolling?
> 
> I suppose it may be even better to call scrollTo before and after setting the
> page scale to exercised both paths.

clientX/Y is relative to the layout viewport, meaning it depends on the visual
viewport's offset within the layout viewport. So this test depends on which
viewport we scroll first because the layout viewport has enough content that
it'll swallow the whole delta if scrolled first (so visual will have a (0, 0)
offset). Scrolling the visual viewport first will move it to a non-zero offset
so the client coordinates will differ. Does that make sense?

[majidvp, 2015-08-19 18:53:52]

On 2015/08/19 17:10:45, bokan wrote:
>
https://codereview.chromium.org/1301523006/diff/1/LayoutTests/fast/dom/zoom-s...
> File LayoutTests/fast/dom/zoom-scroll-page-test.html (right):
> 
>
https://codereview.chromium.org/1301523006/diff/1/LayoutTests/fast/dom/zoom-s...
> LayoutTests/fast/dom/zoom-scroll-page-test.html:29:
> internals.setPageScaleFactor(scaleFactor);
> On 2015/08/18 20:19:18, majidvp wrote:
> > Don't scrollTo, scrollBy, and pageYOffset all operate on visual viewport
> > coordinate? So I would imagine that the order should not matter here?
> 
> Yup, you're right. This confused me for a bit so I dug into it. It turns out
to
> be a floating point precision issue. When we scroll "layout first", the layout
> viewport has enough content that it consumes the whole scroll. Scrolling
"visual
> first" splits the scroll between the viewports. Because of the way we handle
> fractional scroll values, this means there's two places with floating point
> imprecision so our error is increased (we truncate to ints twice). I think the
> right solution here is to bump the "nearness" value in shouldBeCloseTo from
> 3->4.
> 
>
https://codereview.chromium.org/1301523006/diff/1/LayoutTests/fast/events/tou...
> File LayoutTests/fast/events/touch/touch-scaled-scrolled.html (right):
> 
>
https://codereview.chromium.org/1301523006/diff/1/LayoutTests/fast/events/tou...
> LayoutTests/fast/events/touch/touch-scaled-scrolled.html:16:
> window.scrollTo(100,100);
> On 2015/08/18 20:19:18, majidvp wrote:
> > Why move this here? client{X,Y} should be independent of the scroll position
> > whether it is virtual or visual viewport that is scrolling?
> > 
> > I suppose it may be even better to call scrollTo before and after setting
the
> > page scale to exercised both paths.
> 
> clientX/Y is relative to the layout viewport, meaning it depends on the visual
> viewport's offset within the layout viewport. So this test depends on which
> viewport we scroll first because the layout viewport has enough content that
> it'll swallow the whole delta if scrolled first (so visual will have a (0, 0)
> offset). Scrolling the visual viewport first will move it to a non-zero offset
> so the client coordinates will differ. Does that make sense?

This makes sense. I was incorrect about clientX/Y being relative to visual.

lgtm with following minor nit.  I suggesting adding a comment that explains 
why we want to scroll the page before scaling it. It is a bit subtle and easy
to miss.

[bokan, 2015-08-25 14:08:36]

The CQ bit was checked by bokan@chromium.org

[bokan, 2015-08-25 14:08:36]

The patchset sent to the CQ was uploaded after l-g-t-m from majidvp@chromium.org
Link to the patchset: https://codereview.chromium.org/1301523006/#ps40001
(title: "Added comment")

[commit-bot: I haz the power, 2015-08-25 14:08:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1301523006/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1301523006/40001

[commit-bot: I haz the power, 2015-08-25 15:19:47]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201135