New issue
Advanced search Search tips

Issue 600507 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Scroll top controls in from bottom of page causes viewport to get pushed up

Project Member Reported by bokan@chromium.org, Apr 4 2016

Issue description

Version: 51.0.2697.0
OS: Android

What steps will reproduce the problem?
(1) Disable threaded scrolling in about:flags
(2) On any scrollable page, zoom in and scroll all the way to the bottom of the page, hiding the top controls.
(3) Slowly start scrolling up, showing the top controls

What is the expected output?
The top controls should come in, the page should stay still relative to the top controls.

What do you see instead?
The page scrolls up relative to the top controls.
 

Comment 1 by bokan@chromium.org, Apr 4 2016

Labels: Hotlist-Polish
This is occurring because we don't anchor the viewport on main thread scrolls during top controls movement. The change in viewport size pushes up the visual viewport because the layout viewport shrinks.
Project Member

Comment 2 by bugdroid1@chromium.org, Apr 27 2016

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

commit 71cb5b185f274ee0f96b346ce8a515073e9e2975
Author: bokan <bokan@chromium.org>
Date: Wed Apr 27 03:45:22 2016

Make WebWidget::resize early out if the size doesn't change.

This was already done in most of WebWidget's descendants except for
render_widget_fullscreen_pepper and WebWidgetPopupImpl. It makes more sense to
check this inside the call rather than relying on the caller
(RenderWidget::Resize in this case) and this simplifies some changes I'm making
here in another CL: https://codereview.chromium.org/1844013002/

WebWidgetPopupImpl turned out to be caching the window rect itself. I removed
this cached variable and let it just query the RenderWidget's rect instead. In
doing this, I believe RenderWidget has a bug in how it handles setWindowRect.
It caches the given rect and uses that when queried until a Move ACK comes back
from the browser host at which point it starts returning the view_screen_rect_
again, but it never sets the view_screen_rect_. I now set the view_screen_rect_
to the pending_window_rect_ once we get the last Move ACK.

BUG= 600507 

Review URL: https://codereview.chromium.org/1894333002

Cr-Commit-Position: refs/heads/master@{#389984}

[modify] https://crrev.com/71cb5b185f274ee0f96b346ce8a515073e9e2975/content/renderer/render_widget.cc
[modify] https://crrev.com/71cb5b185f274ee0f96b346ce8a515073e9e2975/content/renderer/render_widget_fullscreen_pepper.cc
[modify] https://crrev.com/71cb5b185f274ee0f96b346ce8a515073e9e2975/third_party/WebKit/Source/web/WebPagePopupImpl.cpp
[modify] https://crrev.com/71cb5b185f274ee0f96b346ce8a515073e9e2975/third_party/WebKit/Source/web/WebPagePopupImpl.h

Project Member

Comment 3 by bugdroid1@chromium.org, Apr 27 2016

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

commit c63441cc9941c223e2f2d311085903c00da85938
Author: bokan <bokan@chromium.org>
Date: Wed Apr 27 15:49:12 2016

Fix main thread top controls scrolling to mirror CC.

There's a couple of issues I found where our behavior on the main thread is
divergent from the compositor. The major one is that we don't anchor the
viewport when top controls change the viewport bounds. At the document extents,
the changing bounds can cause the viewport's scroll offset to be clamped causing
unwanted scrolling. I've added an anchoring in didUpdateTopControls that should
match CC's anchoring down to the sub-pixel level. This also required some
rearranging of operations in applyViewportDeltas.

Now that we anchor the top control resizes on main thread as well, we can't have
a separate calls to change the "shrinks_blink_layout_size" flag and resize the
Blink size so I've added an overload of resize() in WebWidget that does both at
the same time.

In addition, there were other minor issues that I found but weren't causing an
obvoius noticable effect. CC's MaxScrollOffset method |ceil|s the top controls
adjustment so I made the main thread do this too. CC's anchoring logic still
scrolled the outer viewport first. This was changed a while back in viewport.cc
so I changed it here too.

BUG= 600507 
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[modify] https://crrev.com/c63441cc9941c223e2f2d311085903c00da85938/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/c63441cc9941c223e2f2d311085903c00da85938/content/renderer/render_view_impl.cc
[modify] https://crrev.com/c63441cc9941c223e2f2d311085903c00da85938/content/renderer/render_view_impl.h
[modify] https://crrev.com/c63441cc9941c223e2f2d311085903c00da85938/content/renderer/render_widget.cc
[modify] https://crrev.com/c63441cc9941c223e2f2d311085903c00da85938/content/renderer/render_widget.h
[modify] https://crrev.com/c63441cc9941c223e2f2d311085903c00da85938/third_party/WebKit/Source/core/frame/FrameView.h
[modify] https://crrev.com/c63441cc9941c223e2f2d311085903c00da85938/third_party/WebKit/Source/core/frame/VisualViewport.cpp
[modify] https://crrev.com/c63441cc9941c223e2f2d311085903c00da85938/third_party/WebKit/Source/web/ResizeViewportAnchor.cpp
[modify] https://crrev.com/c63441cc9941c223e2f2d311085903c00da85938/third_party/WebKit/Source/web/WebViewFrameWidget.cpp
[modify] https://crrev.com/c63441cc9941c223e2f2d311085903c00da85938/third_party/WebKit/Source/web/WebViewFrameWidget.h
[modify] https://crrev.com/c63441cc9941c223e2f2d311085903c00da85938/third_party/WebKit/Source/web/WebViewImpl.cpp
[modify] https://crrev.com/c63441cc9941c223e2f2d311085903c00da85938/third_party/WebKit/Source/web/WebViewImpl.h
[modify] https://crrev.com/c63441cc9941c223e2f2d311085903c00da85938/third_party/WebKit/Source/web/tests/TopControlsTest.cpp
[modify] https://crrev.com/c63441cc9941c223e2f2d311085903c00da85938/third_party/WebKit/Source/web/tests/VisualViewportTest.cpp
[modify] https://crrev.com/c63441cc9941c223e2f2d311085903c00da85938/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
[modify] https://crrev.com/c63441cc9941c223e2f2d311085903c00da85938/third_party/WebKit/public/web/WebView.h
[modify] https://crrev.com/c63441cc9941c223e2f2d311085903c00da85938/third_party/WebKit/public/web/WebWidget.h

Comment 4 by bokan@chromium.org, Apr 27 2016

I'm leaving this open since there's still a small issue that I punted from the above CL:

1) visit bokan.ca/clamp.html
2) pinch zoom in and scroll down (at least 100px) to hide the top controls
3) Without lifting your finger, start scrolling up so that top controls are revealed and then the page reaches the top. When the page is fully scrolled to the top, it will scroll in JS to the end of the document
4) Once scrolled to the end of the document, release your finger

What should happen: Nothing
What happens: The scroll offset is pushed up a little bit

I suspect this is happening because we're now doing the top controls scroll compensation/anchoring in two places: LayerTreeHostImpl::UpdateViewportContainerSizes and WebViewImpl::resize. 

Comment 5 by bokan@chromium.org, Jun 9 2016

Status: Fixed (was: Started)
Closing since the above no longer repros for me reliably.

Sign in to add a comment