Make window.scroll properties relative to the layout viewport by default.

Make window.scroll properties relative to the layout viewport by default.

This CL sets the inert-visual-viewport flag to true by default. The following
APIs in the CSSOM spec are affected: scrollTo, scrollBy, innerWidth,
innerHeight, scrollX, scrollY, element.scrollTop, element.scrollLeft,
element.scrollIntoView.

Intent-to-ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/mt1uciFuvWQ/ecQi0f8kCQAJ

BUG=489206
Committed: https://crrev.com/f75faf53de4a5c59269143b6edbfc6dc40c7fb33
Cr-Commit-Position: refs/heads/master@{#358117}

[ymalik, 2015-11-05 00:39:18]

Description was changed from

==========
Make window.scroll properties relative to the layout viewport by default.

This CL sets the inert-visual-viewport flag to true by default. The following
APIs in the CSSOM spec are affected: scrollTo, scrollBy, innerWidth,
innerHeight, scrollX, scrollY, element.scrollTop, element.scrollLeft,
element.scrollIntoView.

Intent-to-ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/mt1uciFuvWQ/ecQi0f8k...

BUG=489206
==========

to

==========
Make window.scroll properties relative to the layout viewport by default.

This CL sets the inert-visual-viewport flag to true by default. The following
APIs in the CSSOM spec are affected: scrollTo, scrollBy, innerWidth,
innerHeight, scrollX, scrollY, element.scrollTop, element.scrollLeft,
element.scrollIntoView.

Intent-to-ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/mt1uciFuvWQ/ecQi0f8k...

BUG=489206
==========

[ymalik, 2015-11-05 00:39:18]

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

[ymalik, 2015-11-05 00:42:01]

@bokan 

Note that you've already LGTM-ed the layout tests here:
https://codereview.chromium.org/1415513002/#ps200001

[ymalik, 2015-11-05 00:43:41]

ymalik@chromium.org changed reviewers:
+ aelias@chromium.org

[ymalik, 2015-11-05 00:43:41]

+aelias for content/public/android/javatests

Please see the inline comment explaining the reason for the change.

[ymalik, 2015-11-05 00:49:08]

https://codereview.chromium.org/1416283006/diff/1/content/public/android/java...
File
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
(right):

https://codereview.chromium.org/1416283006/diff/1/content/public/android/java...
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:50:
+ "<br/><p><span id=\"plain_text\">This is Plain Text One</span></p>"
@aelias
Making scroll properties relative to the layout viewport impacts
element.scrollIntoView(), where we no longer scroll the visual viewport. The
test on the "plain_text" element expects that the element is scrolled into view.
Doing this simply ensures that the element is in the viewport.

[aelias_OOO_until_Jul13, 2015-11-05 01:08:29]

https://codereview.chromium.org/1416283006/diff/1/content/public/android/java...
File
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
(right):

https://codereview.chromium.org/1416283006/diff/1/content/public/android/java...
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:50:
+ "<br/><p><span id=\"plain_text\">This is Plain Text One</span></p>"
On 2015/11/05 at 00:49:08, ymalik1 wrote:
> @aelias
> Making scroll properties relative to the layout viewport impacts
element.scrollIntoView(), where we no longer scroll the visual viewport. The
test on the "plain_text" element expects that the element is scrolled into view.
Doing this simply ensures that the element is in the viewport.

Would it work to remove the "initial-scale=2.0, maximum-scale=2.0" above
instead?

[ymalik, 2015-11-05 02:12:06]

https://codereview.chromium.org/1416283006/diff/1/content/public/android/java...
File
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
(right):

https://codereview.chromium.org/1416283006/diff/1/content/public/android/java...
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:50:
+ "<br/><p><span id=\"plain_text\">This is Plain Text One</span></p>"
On 2015/11/05 01:08:29, aelias wrote:
> On 2015/11/05 at 00:49:08, ymalik1 wrote:
> > @aelias
> > Making scroll properties relative to the layout viewport impacts
> element.scrollIntoView(), where we no longer scroll the visual viewport. The
> test on the "plain_text" element expects that the element is scrolled into
view.
> Doing this simply ensures that the element is in the viewport.
> 
> Would it work to remove the "initial-scale=2.0, maximum-scale=2.0" above
> instead?

Yes that works as well. Do you prefer that? I assumed that these test cases
passing under pinch-zoom was more important than our dependence on the order of
the html elements.

[aelias_OOO_until_Jul13, 2015-11-05 03:26:24]

Yes, I think I prefer it.  These tests shouldn't have anything to do with pinch
zoom, I'm guessing that viewport tag was copied from another test.  I also just
smoke tested locally that text editing scroll changes work OK even pinch zoomed
and with inert-virtual-viewport, so it seems that the breakage here is just an
artifact of how the tests are written.

lgtm to land with that change.

[bokan, 2015-11-05 14:43:52]

https://codereview.chromium.org/1416283006/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/repaint/fixed-right-bottom-in-page-scale.html
(right):

https://codereview.chromium.org/1416283006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/repaint/fixed-right-bottom-in-page-scale.html:33:
<div style="position: fixed; bottom: 400px; right: 500px; z-index: 1">
This was because the scrollTo doesn't scroll the visual viewport any more so it
doesn't bring the box into view. Could you change the window.scrollTo to an
eventSender scroll instead that does scroll the visual viewport. That way the
test still works exactly as before.

https://codereview.chromium.org/1416283006/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html
(right):

https://codereview.chromium.org/1416283006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html:9:
<script language="JavaScript" type="text/javascript">
Just <script> is fine.

https://codereview.chromium.org/1416283006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html:42:
eventSender.mouseScrollBy(0, -10);
Maybe I'm missing something but how does this work? Shouldn't the first scroll
be at the visual viewport since we scroll visual first now?

https://codereview.chromium.org/1416283006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html:51:
finishJSTest();
Could you add a return here, it seems we're hitting this check twice.

[ymalik, 2015-11-05 15:57:02]

https://codereview.chromium.org/1416283006/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/repaint/fixed-right-bottom-in-page-scale.html
(right):

https://codereview.chromium.org/1416283006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/repaint/fixed-right-bottom-in-page-scale.html:33:
<div style="position: fixed; bottom: 400px; right: 500px; z-index: 1">
On 2015/11/05 14:43:52, bokan wrote:
> This was because the scrollTo doesn't scroll the visual viewport any more so
it
> doesn't bring the box into view. Could you change the window.scrollTo to an
> eventSender scroll instead that does scroll the visual viewport. That way the
> test still works exactly as before.

Done.

https://codereview.chromium.org/1416283006/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html
(right):

https://codereview.chromium.org/1416283006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html:9:
<script language="JavaScript" type="text/javascript">
On 2015/11/05 14:43:52, bokan wrote:
> Just <script> is fine.

Done. Thanks.

https://codereview.chromium.org/1416283006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html:42:
eventSender.mouseScrollBy(0, -10);
On 2015/11/05 14:43:52, bokan wrote:
> Maybe I'm missing something but how does this work? Shouldn't the first scroll
> be at the visual viewport since we scroll visual first now?

The -10 here is the number of mouse wheel ticks. So we're actually scrolling 400
px, which does scroll the layout viewport. Changed to use continuousMouseScroll
to make that more clear.

https://codereview.chromium.org/1416283006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html:51:
finishJSTest();
On 2015/11/05 14:43:52, bokan wrote:
> Could you add a return here, it seems we're hitting this check twice.

If we were hitting it twice, it would reflect in the expected results. I think
calling finishJSTest on an async test just terminates it before the call to
requestAnimationFrame. But yes, it's more correct to return.

[bokan, 2015-11-05 16:35:48]

https://codereview.chromium.org/1416283006/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html
(right):

https://codereview.chromium.org/1416283006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html:42:
eventSender.mouseScrollBy(0, -10);
On 2015/11/05 15:57:01, ymalik1 wrote:
> On 2015/11/05 14:43:52, bokan wrote:
> > Maybe I'm missing something but how does this work? Shouldn't the first
scroll
> > be at the visual viewport since we scroll visual first now?
> 
> The -10 here is the number of mouse wheel ticks. So we're actually scrolling
400
> px, which does scroll the layout viewport. Changed to use
continuousMouseScroll
> to make that more clear.

Got it, that makes more sense, thanks.

https://codereview.chromium.org/1416283006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html:51:
finishJSTest();
On 2015/11/05 15:57:01, ymalik1 wrote:
> On 2015/11/05 14:43:52, bokan wrote:
> > Could you add a return here, it seems we're hitting this check twice.
> 
> If we were hitting it twice, it would reflect in the expected results. I think
> calling finishJSTest on an async test just terminates it before the call to
> requestAnimationFrame. But yes, it's more correct to return.

It is reflected in the expected results, there's two "numScrollsReceived is 1"
lines. Does this not fix it or did you forget to update the expectation file?

[ymalik, 2015-11-05 16:40:26]

https://codereview.chromium.org/1416283006/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html
(right):

https://codereview.chromium.org/1416283006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html:51:
finishJSTest();
On 2015/11/05 16:35:48, bokan wrote:
> On 2015/11/05 15:57:01, ymalik1 wrote:
> > On 2015/11/05 14:43:52, bokan wrote:
> > > Could you add a return here, it seems we're hitting this check twice.
> > 
> > If we were hitting it twice, it would reflect in the expected results. I
think
> > calling finishJSTest on an async test just terminates it before the call to
> > requestAnimationFrame. But yes, it's more correct to return.
> 
> It is reflected in the expected results, there's two "numScrollsReceived is 1"
> lines. Does this not fix it or did you forget to update the expectation file?

The first "numScrollsReceived is 1" is from the previous branch "numRAFCalls ==
1". That's when numScrollsReceived is first incremented by the call to onscroll
from the layout viewport scroll.

[bokan, 2015-11-05 17:02:47]

lgtm, thanks.

https://codereview.chromium.org/1416283006/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html
(right):

https://codereview.chromium.org/1416283006/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html:51:
finishJSTest();
On 2015/11/05 16:40:26, ymalik1 wrote:
> On 2015/11/05 16:35:48, bokan wrote:
> > On 2015/11/05 15:57:01, ymalik1 wrote:
> > > On 2015/11/05 14:43:52, bokan wrote:
> > > > Could you add a return here, it seems we're hitting this check twice.
> > > 
> > > If we were hitting it twice, it would reflect in the expected results. I
> think
> > > calling finishJSTest on an async test just terminates it before the call
to
> > > requestAnimationFrame. But yes, it's more correct to return.
> > 
> > It is reflected in the expected results, there's two "numScrollsReceived is
1"
> > lines. Does this not fix it or did you forget to update the expectation
file?
> 
> The first "numScrollsReceived is 1" is from the previous branch "numRAFCalls
==
> 1". That's when numScrollsReceived is first incremented by the call to
onscroll
> from the layout viewport scroll.

DOH! Brain fart on my part :/

[ymalik, 2015-11-05 17:49:10]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2015-11-05 17:49:11]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org
Link to the patchset: https://codereview.chromium.org/1416283006/#ps20001
(title: "worked on review comments")

[commit-bot: I haz the power, 2015-11-05 17:50:28]

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

[commit-bot: I haz the power, 2015-11-05 20:15:54]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-11-05 20:16:30]

Patchset 2 (id:??) landed as
https://crrev.com/f75faf53de4a5c59269143b6edbfc6dc40c7fb33
Cr-Commit-Position: refs/heads/master@{#358117}