Commit e1e99584 authored by Kurt Horimoto's avatar Kurt Horimoto Committed by Commit Bot

[iOS] Fix PageDisplayState before applying scroll offset.

The referenced bug occurs due to ordering issues of our viewport inset
updates relative to PageDisplayState application.  When reloading the
page using overscroll, the PageDisplayState is stored.  Since the page
is scrolled to the top, the contentOffset is -contentInset.top (-92 in
portrait on iPhone X).  Upon rotating the device, the following events
occur:
1) CRWWebController's |-orientationDidChange| kicks off the scroll
   restoration process for the y offset of -92.  This is asynchronous
   because it first executes JavaScript in order to extract the
   viewport tag.
2) BVC's |-traitCollectionDidChange:| updates the toolbar heights for
   the new horizontal size class, which in turn updates the scroll
   view's contentInset.  For landscape iPhone X, this produces a top
   contentInset of 50 (i.e. the "scrolled to top" y content offset is
   -50).
3) The asynchronous viewport meta tag data extraction finishes and
   CRWWebController restores the -92 contentOffset, pushing the page
   below the toolbar.

This CL updates PageScrollState semantics such that it stores the
offset from the scrolled-to-top-left position, which allows the
application of PageScrollStates contentInset-agnostic.

Bug: 923856
Change-Id: I468970abbdbd200e4ac836d5664a985f0c8e3e71
Reviewed-on: https://chromium-review.googlesource.com/c/1432377Reviewed-by: default avatarEugene But <eugenebut@chromium.org>
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Auto-Submit: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#625828}
parent 8ee899db
...@@ -9,7 +9,12 @@ ...@@ -9,7 +9,12 @@
namespace web { namespace web {
// Class used to represent the scrolling offset of a webview. // Class used to represent the scrolling offset of a webview. The offsets
// stored are offsets from the scrolled-to-top-left resting position. That
// means an |offset_y| value of 0 corresponds to a UIScrollView.contentOffset.y
// value of -UIScrollView.contentInset.top.
// TODO(crbug.com/925073): Update this class to store contentInset and
// contentOffset directly.
class PageScrollState { class PageScrollState {
public: public:
// Default constructor. Initializes scroll offsets to NAN. // Default constructor. Initializes scroll offsets to NAN.
...@@ -21,7 +26,7 @@ class PageScrollState { ...@@ -21,7 +26,7 @@ class PageScrollState {
// The scroll offset is valid if its x and y values are both non-NAN. // The scroll offset is valid if its x and y values are both non-NAN.
bool IsValid() const; bool IsValid() const;
// Accessors for scroll offsets and zoom scale. // Accessors for scroll offsets.
double offset_x() const { return offset_x_; } double offset_x() const { return offset_x_; }
void set_offset_x(double offset_x) { offset_x_ = offset_x; } void set_offset_x(double offset_x) { offset_x_ = offset_x; }
double offset_y() const { return offset_y_; } double offset_y() const { return offset_y_; }
......
...@@ -3596,8 +3596,11 @@ registerLoadRequestForURL:(const GURL&)requestURL ...@@ -3596,8 +3596,11 @@ registerLoadRequestForURL:(const GURL&)requestURL
} }
} else if (_webView) { } else if (_webView) {
CGPoint scrollOffset = [self scrollPosition]; CGPoint scrollOffset = [self scrollPosition];
displayState.scroll_state().set_offset_x(std::floor(scrollOffset.x)); UIEdgeInsets contentInset = self.webScrollView.contentInset;
displayState.scroll_state().set_offset_y(std::floor(scrollOffset.y)); displayState.scroll_state().set_offset_x(
std::floor(scrollOffset.x + contentInset.left));
displayState.scroll_state().set_offset_y(
std::floor(scrollOffset.y + contentInset.top));
UIScrollView* scrollView = self.webScrollView; UIScrollView* scrollView = self.webScrollView;
displayState.zoom_state().set_minimum_zoom_scale( displayState.zoom_state().set_minimum_zoom_scale(
scrollView.minimumZoomScale); scrollView.minimumZoomScale);
...@@ -3766,8 +3769,9 @@ registerLoadRequestForURL:(const GURL&)requestURL ...@@ -3766,8 +3769,9 @@ registerLoadRequestForURL:(const GURL&)requestURL
- (void)applyWebViewScrollOffsetFromScrollState: - (void)applyWebViewScrollOffsetFromScrollState:
(const web::PageScrollState&)scrollState { (const web::PageScrollState&)scrollState {
DCHECK(scrollState.IsValid()); DCHECK(scrollState.IsValid());
CGPoint scrollOffset = UIEdgeInsets contentInset = self.webScrollView.contentInset;
CGPointMake(scrollState.offset_x(), scrollState.offset_y()); CGPoint scrollOffset = CGPointMake(scrollState.offset_x() - contentInset.left,
scrollState.offset_y() - contentInset.top);
if (_loadPhase == web::PAGE_LOADED) { if (_loadPhase == web::PAGE_LOADED) {
// If the page is loaded, update the scroll immediately. // If the page is loaded, update the scroll immediately.
[self.webScrollView setContentOffset:scrollOffset]; [self.webScrollView setContentOffset:scrollOffset];
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment