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

[iOS] Fix FullscreenModel::UpdateDisabledCounterForContentHeight().

This CL updates UpdateDisabledCounterForContentHeight()'s implementation
in two ways:
- Accounts for the case when the FullscreenProvider is disabled and the
  scroll view may have been resized to a shorter height despite the
  content being fully visible on screen.
- Updates to a <= check so that fullscreen is disabled when the content
  fits the scroll view exactly.

Bug: 914347
Change-Id: I5d2c6e3ce09d25552de0f1071785d29078162377
Reviewed-on: https://chromium-review.googlesource.com/c/1423092
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: default avatarJustin Cohen <justincohen@chromium.org>
Reviewed-by: default avatarKurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: default avatarEugene But <eugenebut@chromium.org>
Auto-Submit: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#625360}
parent a689341a
......@@ -126,8 +126,8 @@ void AssertURLIs(const GURL& expectedURL) {
assertWithMatcher:grey_scrollViewContentOffset(CGPointMake(0, yOffset))];
}
// Verifies that the toolbar properly appears/disappears when scrolling up/down
// on a PDF that is short in length and wide in width.
// Verifies that the toolbar is not hidden when scrolling a short pdf, as the
// entire document is visible without hiding the toolbar.
- (void)testSmallWidePDFScroll {
web::test::SetUpFileBasedHttpServer();
GURL URL = web::test::HttpServer::MakeUrl(
......@@ -149,16 +149,10 @@ void AssertURLIs(const GURL& expectedURL) {
performAction:grey_swipeFastInDirection(kGREYDirectionDown)];
[ChromeEarlGreyUI waitForToolbarVisible:YES];
if (base::ios::IsRunningOnIOS12OrLater()) {
// Test that the toolbar is still visible even after attempting to hide it
// on swipe up.
HideToolbarUsingUI();
[ChromeEarlGreyUI waitForToolbarVisible:YES];
} else {
// Test that the toolbar is no longer visible after a user swipes up.
HideToolbarUsingUI();
[ChromeEarlGreyUI waitForToolbarVisible:NO];
}
// Test that the toolbar is still visible even after attempting to hide it
// on swipe up.
HideToolbarUsingUI();
[ChromeEarlGreyUI waitForToolbarVisible:YES];
// Reenable synchronization.
if (@available(iOS 12, *)) {
......
......@@ -143,6 +143,10 @@ class FullscreenModel : public ChromeBroadcastObserverInterface {
void SetScrollViewIsDragging(bool dragging);
bool IsScrollViewDragging() const;
// Setter for whether the scroll view is resized for fullscreen events.
void SetResizesScrollView(bool resizes_scroll_view);
bool ResizesScrollView() const;
private:
// Returns how a scroll to the current |y_content_offset_| from |from_offset|
// should be handled.
......@@ -214,6 +218,8 @@ class FullscreenModel : public ChromeBroadcastObserverInterface {
bool dragging_ = false;
// Whether the in-progress scroll is being ignored.
bool ignoring_current_scroll_ = false;
// Whether the scroll view is resized for fullscreen events.
bool resizes_scroll_view_ = false;
// The number of FullscreenModelObserver callbacks currently being executed.
size_t observer_callback_count_ = 0;
......
......@@ -220,6 +220,14 @@ bool FullscreenModel::IsScrollViewDragging() const {
return dragging_;
}
void FullscreenModel::SetResizesScrollView(bool resizes_scroll_view) {
resizes_scroll_view_ = resizes_scroll_view;
}
bool FullscreenModel::ResizesScrollView() const {
return resizes_scroll_view_;
}
FullscreenModel::ScrollAction FullscreenModel::ActionForScrollFromOffset(
CGFloat from_offset) const {
// Update the base offset but don't recalculate progress if:
......@@ -264,7 +272,20 @@ void FullscreenModel::UpdateProgress() {
}
void FullscreenModel::UpdateDisabledCounterForContentHeight() {
bool disable = content_height_ < scroll_view_height_;
// The model should be disabled when the content fits.
CGFloat disabling_threshold = scroll_view_height_;
if (resizes_scroll_view_) {
// When the FullscreenProvider is disabled, the scroll view can sometimes be
// resized to account for the viewport insets after the page has been
// rendered, so account for the maximum toolbar insets in the threshold.
disabling_threshold += expanded_toolbar_height_ + bottom_toolbar_height_;
}
// Don't disable fullscreen if both heights have not been received.
bool areBothHeightsSet = !AreCGFloatsEqual(content_height_, 0.0) &&
!AreCGFloatsEqual(scroll_view_height_, 0.0);
bool disable = areBothHeightsSet && content_height_ <= disabling_threshold;
if (disabled_for_short_content_ == disable)
return;
disabled_for_short_content_ = disable;
......
......@@ -256,9 +256,10 @@ TEST_F(FullscreenModelTest, DisableForShortContent) {
ASSERT_TRUE(model().enabled());
// The model should be disabled when the rendered content height is less than
// the height of the scroll view.
model().SetContentHeight(model().GetScrollViewHeight() - 1.0);
model().SetContentHeight(model().GetScrollViewHeight());
EXPECT_FALSE(model().enabled());
// Reset the height to kContentHeight and verify that the model is re-enabled.
model().SetContentHeight(model().GetScrollViewHeight() + 1.0);
model().SetContentHeight(model().GetScrollViewHeight() + 2 * kToolbarHeight +
1.0);
EXPECT_TRUE(model().enabled());
}
......@@ -13,6 +13,8 @@
#import "ios/chrome/browser/ui/fullscreen/fullscreen_web_view_proxy_observer.h"
#import "ios/chrome/browser/ui/fullscreen/scoped_fullscreen_disabler.h"
#include "ios/chrome/browser/ui/util/ui_util.h"
#include "ios/public/provider/chrome/browser/chrome_browser_provider.h"
#import "ios/public/provider/chrome/browser/ui/fullscreen_provider.h"
#import "ios/web/public/navigation_item.h"
#import "ios/web/public/navigation_manager.h"
#include "ios/web/public/ssl_status.h"
......@@ -115,6 +117,11 @@ void FullscreenWebStateObserver::DidFinishNavigation(
id<CRWWebViewProxy> web_view_proxy = web_state->GetWebViewProxy();
web_view_proxy.shouldUseViewContentInset = use_content_inset;
model_->SetResizesScrollView(!use_content_inset &&
!ios::GetChromeBrowserProvider()
->GetFullscreenProvider()
->IsInitialized());
// On iOS 12, resetting WKScrollView.contentInset at this point in the load
// will push the content down by the top inset. On iOS 11, however, this does
// not occur. Manually push the content below the toolbars the first time a
......
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