New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 612305 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug

Blocking:
issue 558575



Sign in to add a comment

scroll anchoring during window resize

Project Member Reported by skobes@chromium.org, May 16 2016

Issue description

It would be super awesome if we could make scroll anchoring work for window resizes on desktop.

Unfortunately its adjustments are getting clobbered by ResizeViewportAnchor.

I don't really understand what ResizeViewportAnchor is doing but it seems to be related to placement of the visual viewport during top controls movements.  So hopefully we can tweak it to behave better when ScrollAnchor has scrolled the layout viewport during the resize.
 

Comment 1 by ojan@chromium.org, May 17 2016

We should disable ResizeViewportAnchor when scroll anchoring is enabled. Scroll anchoring should be strictly better than ResizeViewportAnchor and we should delete the latter once we ship scroll anchoring. Right?

Comment 2 by skobes@chromium.org, May 17 2016

Are you thinking of RotationViewportAnchor?

Comment 3 by ojan@chromium.org, May 17 2016

Doh. I am! 
Labels: Te-NeedsFurtherTriage

Comment 5 by skobes@chromium.org, May 19 2016

Labels: -Te-NeedsFurtherTriage
Owner: skobes@chromium.org
Status: Assigned (was: Unconfirmed)

Comment 6 by bokan@chromium.org, May 31 2016

RotationViewportAnchor should be redundant with layout scroll anchoring so I agree with removing it.

The ResizeViewportAnchor is for the case where we're zoomed in, fully scrolled and moving the top controls. The top controls will shrink/expand the layout and visual viewports by different amounts (since we're zoomed in) which will change the maximum scroll position of each. This anchor ensures that the location the user is seeing doesn't change because of these clamps.

We'll likely want to rethink how this should interact with layout scroll anchoring but I think it does need to stick around.
Project Member

Comment 7 by bugdroid1@chromium.org, Jul 28 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6913f420989be6dbaed45bd7e1b60042ff119d70

commit 6913f420989be6dbaed45bd7e1b60042ff119d70
Author: skobes <skobes@chromium.org>
Date: Thu Jul 28 18:36:33 2016

Fix scroll anchoring during viewport resize.

WebView::resizeWithTopControls does a synchronous layout.  Anchoring scrolls
performed during this layout were getting clobbered by ResizeViewportAnchor,
whose purpose is to scroll the viewports to preserve their combined position
(as reported by RootFrameViewport) after bounds-clamping to their new sizes.

After this change, ResizeViewportAnchor tracks the resize-induced scroll delta
while ignoring other scrolls.

BUG= 626258 ,  612305 

Review-Url: https://codereview.chromium.org/2184333002
Cr-Commit-Position: refs/heads/master@{#408439}

[modify] https://crrev.com/6913f420989be6dbaed45bd7e1b60042ff119d70/third_party/WebKit/Source/web/ResizeViewportAnchor.cpp
[modify] https://crrev.com/6913f420989be6dbaed45bd7e1b60042ff119d70/third_party/WebKit/Source/web/ResizeViewportAnchor.h
[modify] https://crrev.com/6913f420989be6dbaed45bd7e1b60042ff119d70/third_party/WebKit/Source/web/RotationViewportAnchor.cpp
[modify] https://crrev.com/6913f420989be6dbaed45bd7e1b60042ff119d70/third_party/WebKit/Source/web/RotationViewportAnchor.h
[delete] https://crrev.com/f79d8967f4c6bc91dfb93ffd4f7972a2e39fd05e/third_party/WebKit/Source/web/ViewportAnchor.h
[modify] https://crrev.com/6913f420989be6dbaed45bd7e1b60042ff119d70/third_party/WebKit/Source/web/WebViewImpl.cpp
[modify] https://crrev.com/6913f420989be6dbaed45bd7e1b60042ff119d70/third_party/WebKit/Source/web/WebViewImpl.h
[modify] https://crrev.com/6913f420989be6dbaed45bd7e1b60042ff119d70/third_party/WebKit/Source/web/tests/VisualViewportTest.cpp
[add] https://crrev.com/6913f420989be6dbaed45bd7e1b60042ff119d70/third_party/WebKit/Source/web/tests/data/icb-relative-content.html
[modify] https://crrev.com/6913f420989be6dbaed45bd7e1b60042ff119d70/third_party/WebKit/Source/web/web.gypi

Comment 8 by skobes@chromium.org, Jul 28 2016

This mostly works, but the 20-adjustment limit added in  issue 601906  becomes very noticeable.
Status: Fixed (was: Assigned)
The 20-adjustment limit was removed in r413010, so this is fixed.

Sign in to add a comment