element.scrollIntoView only scrolls the layout viewport

element.scrollIntoView only scrolls the layout viewport

window.scroll properties being relative to visual viewport breaks some pages
under pinch-zoom. The plan is to have all APIs reflect the layout viewport.

Note that this CL also affects element.scrollIntoViewIfNeeded.

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

[ymalik, 2015-10-30 03:43:12]

Description was changed from

==========
element.scrollIntoView only scrolls the layout viewport

window.scroll properties being relative to visual viewport breaks some pages
under pinch-zoom. The plan is to have all APIs reflect the layout viewport.

Note that this CL also affects element.scrollIntoViewIfNeeded.

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

to

==========
element.scrollIntoView only scrolls the layout viewport

window.scroll properties being relative to visual viewport breaks some pages
under pinch-zoom. The plan is to have all APIs reflect the layout viewport.

Note that this CL also affects element.scrollIntoViewIfNeeded.

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

[ymalik, 2015-10-30 03:43:12]

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

[bokan, 2015-10-30 16:08:55]

lgtm % comments.

https://codereview.chromium.org/1424313003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-into-view-scrolls-layout-viewport.html
(right):

https://codereview.chromium.org/1424313003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-into-view-scrolls-layout-viewport.html:13:
<script language="JavaScript" type="text/javascript">
nit: I think it's idiomatic now to leave the language and type attributes off.

https://codereview.chromium.org/1424313003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-into-view-scrolls-layout-viewport.html:33:
internals.settings.setInertVisualViewport(false);
I don't think there's a need to test the "off" case. You'll just end up removing
it in a week or two...

https://codereview.chromium.org/1424313003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-into-view-scrolls-layout-viewport.html:43:
internals.settings.setInertVisualViewport(true);
I would add some layout viewport scrolling and put the box out of the initial
viewport.  That way we're making sure there is an actual scroll into view
happening (you can confirm the layout viewport scroll too) but it's not
affecting the visual viewport.

https://codereview.chromium.org/1424313003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/1424313003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.h:428: void
scrollRectToVisible(const LayoutRect&, const ScrollAlignment& alignX, const
ScrollAlignment& alignY, ScrollType = ProgrammaticScroll, bool
scrollVisualViewport = true);
I think this param should be called something like makeVisibleInVisualViewport
since scrollVisualViewport could plausibly be interpreted that it'll still
scroll the layout viewport such that the rect is visible in the visual viewport.
Please also add a comment describing what this param will do.

[ymalik, 2015-11-01 00:02:24]

https://codereview.chromium.org/1424313003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-into-view-scrolls-layout-viewport.html
(right):

https://codereview.chromium.org/1424313003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-into-view-scrolls-layout-viewport.html:13:
<script language="JavaScript" type="text/javascript">
On 2015/10/30 16:08:55, bokan wrote:
> nit: I think it's idiomatic now to leave the language and type attributes off.

Done.

https://codereview.chromium.org/1424313003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-into-view-scrolls-layout-viewport.html:33:
internals.settings.setInertVisualViewport(false);
On 2015/10/30 16:08:55, bokan wrote:
> I don't think there's a need to test the "off" case. You'll just end up
removing
> it in a week or two...

Acknowledged.

https://codereview.chromium.org/1424313003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-into-view-scrolls-layout-viewport.html:43:
internals.settings.setInertVisualViewport(true);
On 2015/10/30 16:08:55, bokan wrote:
> I would add some layout viewport scrolling and put the box out of the initial
> viewport.  That way we're making sure there is an actual scroll into view
> happening (you can confirm the layout viewport scroll too) but it's not
> affecting the visual viewport.

Definitely a better test. Thanks.

https://codereview.chromium.org/1424313003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/1424313003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.h:428: void
scrollRectToVisible(const LayoutRect&, const ScrollAlignment& alignX, const
ScrollAlignment& alignY, ScrollType = ProgrammaticScroll, bool
scrollVisualViewport = true);
On 2015/10/30 16:08:55, bokan wrote:
> I think this param should be called something like makeVisibleInVisualViewport
> since scrollVisualViewport could plausibly be interpreted that it'll still
> scroll the layout viewport such that the rect is visible in the visual
viewport.
> Please also add a comment describing what this param will do.

Done.

[ymalik, 2015-11-01 00:03:52]

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

[ymalik, 2015-11-01 00:03:53]

+skobes for WebKit/Source/core

[skobes, 2015-11-02 02:46:48]

lgtm

[ymalik, 2015-11-02 15:04:08]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2015-11-02 15:04:08]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org
Link to the patchset: https://codereview.chromium.org/1424313003/#ps20001
(title: "review feedback")

[commit-bot: I haz the power, 2015-11-02 15:04:23]

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

[commit-bot: I haz the power, 2015-11-02 16:11:59]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-11-02 16:12:52]

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