Preserve scroll view properties on -[CRWWebViewScrollViewProxy setScrollView:].

Change-Id: I3bbe59acc8e2ac8dc4a1e9d6cfc8d97d184cb026
BUG: 1023250
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1961285
Commit-Queue: Hiroshi Ichikawa <ichikawa@chromium.org>
Auto-Submit: Hiroshi Ichikawa <ichikawa@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#731847}

ios/web/common/features.h

@@ -50,6 +50,11 @@ extern const base::Feature kUseJSForErrorPage;
 // is requested by default.
 extern const base::Feature kUseDefaultUserAgentInWebClient;
 
+// When enabled, preserves properties of the UIScrollView using CRWPropertyStore
+// when the scroll view is recreated. When disabled, only preserve a small set
+// of properties using hard coded logic.
+extern const base::Feature kPreserveScrollViewProperties;
+
 // Use WKWebView.loading to update WebState::IsLoading.
 // TODO(crbug.com/1006012): Clean up this flag after experiment.
 bool UseWKWebViewLoading();

ios/web/common/features.mm

@@ -44,6 +44,9 @@ const base::Feature kUseJSForErrorPage{"UseJSForErrorPage",
 const base::Feature kUseDefaultUserAgentInWebClient{
     "UseDefaultUserAgentInWebClient", base::FEATURE_DISABLED_BY_DEFAULT};
 
+const base::Feature kPreserveScrollViewProperties{
+    "PreserveScrollViewProperties", base::FEATURE_DISABLED_BY_DEFAULT};
+
 bool UseWKWebViewLoading() {
   return base::FeatureList::IsEnabled(web::features::kUseWKWebViewLoading);
 }

ios/web/public/ui/crw_web_view_scroll_view_proxy.h

@@ -22,6 +22,32 @@
 // information look at the UIScrollView documentation.
 // TODO(crbug.com/546152): rename class to CRWContentViewScrollViewProxy.
 @interface CRWWebViewScrollViewProxy : NSObject
+
+// Used by the CRWWebViewProxy to set the UIScrollView to be managed.
+- (void)setScrollView:(UIScrollView*)scrollView;
+
+// Adds |observer| to subscribe to change notifications.
+- (void)addObserver:(id<CRWWebViewScrollViewProxyObserver>)observer;
+
+// Removes |observer| as a subscriber for change notifications.
+- (void)removeObserver:(id<CRWWebViewScrollViewProxyObserver>)observer;
+
+// Returns a scroll view proxy which can be accessed as UIScrollView.
+//
+// Note: This method is introduced because CRWWebViewScrollViewProxy cannot
+// simply be a subclass of UIScrollView. Its implementation relies on
+// -forwardInvocation: which only works for methods not implemented in the class
+// (including those implemented in its superclass). So -forwardInvocation:
+// cannot forward methods in UIScrollView if CRWWebViewScrollViewProxy is a
+// subclass of UIScrollView.
+- (UIScrollView*)asUIScrollView;
+
+@end
+
+// Methods/properties implemented by -forwardInvocation:. Declared in a category
+// to avoid compilation error saying they are not implemented.
+@interface CRWWebViewScrollViewProxy (ForwardedMethods)
+
 @property(nonatomic, assign) CGPoint contentOffset;
 @property(nonatomic, assign) UIEdgeInsets contentInset;
 @property(nonatomic, readonly, getter=isDecelerating) BOOL decelerating;
@@ -51,29 +77,6 @@
 - (void)removeGestureRecognizer:(UIGestureRecognizer*)gestureRecognizer;
 - (void)setContentOffset:(CGPoint)contentOffset animated:(BOOL)animated;
 
-// Used by the CRWWebViewProxy to set the UIScrollView to be managed.
-- (void)setScrollView:(UIScrollView*)scrollView;
-
-// Adds |observer| to subscribe to change notifications.
-- (void)addObserver:(id<CRWWebViewScrollViewProxyObserver>)observer;
-
-// Removes |observer| as a subscriber for change notifications.
-- (void)removeObserver:(id<CRWWebViewScrollViewProxyObserver>)observer;
-
-// Returns a scroll view proxy which can be accessed as UIScrollView.
-//
-// Note: This method is introduced because CRWWebViewScrollViewProxy cannot
-// simply be a subclass of UIScrollView. Its implementation relies on
-// -forwardInvocation: which only works for methods not implemented in the class
-// (including those implemented in its superclass). So -forwardInvocation:
-// cannot forward methods in UIScrollView if CRWWebViewScrollViewProxy is a
-// subclass of UIScrollView.
-//
-// TODO(crbug.com/1023250): Support KVO of this scroll view.
-// TODO(crbug.com/1023250): Restore properties of the scroll view when the
-// scroll view is reset.
-- (UIScrollView*)asUIScrollView;
-
 @end
 
 // A protocol to be implemented by objects to listen for changes to the

ios/web/web_state/ui/BUILD.gn

@@ -47,6 +47,8 @@ source_set("ui") {
   ]
 
   sources = [
+    "crw_properties_store.h",
+    "crw_properties_store.mm",
     "crw_swipe_recognizer_provider.h",
     "crw_touch_tracking_recognizer.h",
     "crw_touch_tracking_recognizer.mm",

ios/web/web_state/ui/crw_properties_store.h

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef IOS_WEB_WEB_STATE_UI_CRW_PROPERTIES_STORE_H_
+#define IOS_WEB_WEB_STATE_UI_CRW_PROPERTIES_STORE_H_
+
+#import <Foundation/Foundation.h>
+
+// An attribute of a property with an Objective-C object type to be preserved.
+typedef NS_ENUM(NSInteger, CRWStoredPropertyAttribute) {
+  // "strong" attribute.
+  CRWStoredPropertyAttributeStrong,
+  // "weak" attribute.
+  CRWStoredPropertyAttributeWeak,
+  // "copy" attribute.
+  CRWStoredPropertyAttributeCopy,
+};
+
+// An object which preserves properties of an underlying object when the
+// underlying object is reassigned.
+//
+// This is useful when:
+//   - A class is a proxy for an underlying object.
+//   - The underlying object can be nil or reassigned during the lifetime of the
+//     wrapper.
+//   - The proxy should preserve a subset of the properties of the underlying
+//     object when the underlying object is reassigned.
+//
+// A caller must call its "register" methods to register properties to be
+// preserved before using it.
+//
+// TODO(crbug.com/1023250): Add a unit test for this class.
+// TODO(crbug.com/1023250): Add more safety checks for this class.
+@interface CRWPropertiesStore : NSObject
+
+// Registers a property with an Objective-C object type to be preserved.
+// |getter| and |setter| are selectors of the getter/setter of the underlying
+// object.
+- (void)registerObjectPropertyWithGetter:(nonnull SEL)getter
+                                  setter:(nonnull SEL)setter
+                               attribute:(CRWStoredPropertyAttribute)attribute;
+
+// Registers a property with a type not an Objective-C object to be preserved.
+// |getter| and |setter| are selectors of the getter/setter of the underlying
+// object. |size| is the byte size of the property type.
+//
+// This should be used e.g., for scalar types (NSInteger, CGFloat, etc.) and C
+// structures (CGRect, CGPoint, etc.).
+- (void)registerNonObjectPropertyWithGetter:(nonnull SEL)getter
+                                     setter:(nonnull SEL)setter
+                                       size:(NSUInteger)size;
+
+// Saves the properties of |object| to the properties store. Should be called
+// against the old underlying object when the underlying object is reassigned.
+- (void)savePropertiesFromObject:(nonnull id)object;
+
+// Loads the properties from the properties store to |object|. Should be called
+// against the new underlying object when the underlying object is reassigned.
+- (void)loadPropertiesToObject:(nonnull id)object;
+
+// Clears values of all the properties in the properties store. This prevents
+// from retaining property values no longer needed. It does not reset
+// registration of properties.
+- (void)clearValues;
+
+// Forwards |invocation| to the properties store.
+//
+// If |invocation| is an invocation of a getter or setter of a registered
+// property, it gets or sets the property in the property store, which is later
+// restored in the underlying object, and returns YES. Otherwise does nothing
+// and returns NO.
+//
+// This should be called in -forwardInvocation: of the wrapper when the
+// underlying object is nil.
+- (BOOL)forwardInvocationToPropertiesStore:(nonnull NSInvocation*)invocation;
+
+@end
+
+#endif  // IOS_WEB_WEB_STATE_UI_CRW_PROPERTIES_STORE_H_

ios/web/web_state/ui/crw_web_view_scroll_view_proxy+internal.h

@@ -18,7 +18,11 @@
     CRBProtocolObservers<CRWWebViewScrollViewProxyObserver>* observers;
 
 // The underlying UIScrollView. It can change or become nil.
-@property(nonatomic, weak, readonly) UIScrollView* underlyingScrollView;
+//
+// This must be a strong reference to ensure that this proxy is aware of the
+// timing when this becomes nil. The proxy needs to save the properties of the
+// underlying scroll view when it happens.
+@property(nonatomic, readonly) UIScrollView* underlyingScrollView;
 
 @end
 

ios/web/web_state/ui/crw_web_view_scroll_view_proxy.mm

@@ -5,12 +5,13 @@
 #import "ios/web/web_state/ui/crw_web_view_scroll_view_proxy+internal.h"
 
 #import <objc/runtime.h>
-
 #include <memory>
 
 #include "base/auto_reset.h"
 #import "base/ios/crb_protocol_observers.h"
 #include "base/mac/foundation_util.h"
+#include "ios/web/common/features.h"
+#import "ios/web/web_state/ui/crw_properties_store.h"
 #import "ios/web/web_state/ui/crw_web_view_scroll_view_delegate_proxy.h"
 
 #if !defined(__has_feature) || !__has_feature(objc_arc)
@@ -78,7 +79,7 @@
 @property(nonatomic, strong)
     CRBProtocolObservers<CRWWebViewScrollViewProxyObserver>* observers;
 
-@property(nonatomic, weak) UIScrollView* underlyingScrollView;
+@property(nonatomic, strong) UIScrollView* underlyingScrollView;
 
 // This exists for compatibility with UIScrollView (see -asUIScrollView).
 @property(nonatomic, weak) id<UIScrollViewDelegate> delegate;
@@ -107,6 +108,10 @@
   NSMutableDictionary<NSString*,
                       NSMapTable<id, CRWKeyValueObserverForwarder*>*>*
       _keyValueObserverForwarders;
+
+  // A storage used to preserve values of the scroll view properties on
+  // resetting the underlying scroll view.
+  CRWPropertiesStore* _propertiesStore;
 }
 
 - (instancetype)init {
@@ -119,6 +124,47 @@
     _delegateProxy = [[CRWWebViewScrollViewDelegateProxy alloc]
         initWithScrollViewProxy:self];
     _keyValueObserverForwarders = [[NSMutableDictionary alloc] init];
+
+    if (base::FeatureList::IsEnabled(
+            web::features::kPreserveScrollViewProperties)) {
+      _propertiesStore = [[CRWPropertiesStore alloc] init];
+
+      // A list of properties preserved on resetting the underlying scroll view.
+      //
+      // The underlying scroll view can be nil or can be reassigned. Properties
+      // of the underlying scroll view are usually not preserved when the scroll
+      // view is reassigned. Properties listed here will be preserved i.e.:
+      //   - If the property is assigned while the underlying scroll view is
+      //   nil,
+      //     the assignment is applied when the underlying scroll view is
+      //     assigned.
+      //   - The property is preserved when the underlying scroll view is
+      //     reassigned.
+      //
+      // This list should contain all properties of UIScrollView and its
+      // ancestor classes (not limited to properties explicitly declared in
+      // CRWWebViewScrollViewProxy) which:
+      //   - is a readwrite property
+      //   - AND is supposed to be modified directly, considering it's a scroll
+      //     view of a web view. e.g., |frame| and |subviews| do not meet this
+      //     condition because they are managed by the web view.
+      //
+      // TODO(crbug.com/1023250): Make this list comprehensive.
+      [_propertiesStore
+          registerNonObjectPropertyWithGetter:@selector
+          (isDirectionalLockEnabled)
+                                       setter:@selector
+                                       (setDirectionalLockEnabled:)
+                                         size:sizeof(BOOL)];
+      [_propertiesStore
+          registerObjectPropertyWithGetter:@selector(tintColor)
+                                    setter:@selector(setTintColor:)
+                                 attribute:CRWStoredPropertyAttributeStrong];
+      [_propertiesStore
+          registerObjectPropertyWithGetter:@selector(backgroundColor)
+                                    setter:@selector(setBackgroundColor:)
+                                 attribute:CRWStoredPropertyAttributeCopy];
+    }
   }
   return self;
 }
@@ -146,16 +192,36 @@
 - (void)setScrollView:(UIScrollView*)scrollView {
   if (self.underlyingScrollView == scrollView)
     return;
+
+  // Clean up the delegate/observers of the old scroll view, and save its
+  // properties for later restoration.
   [self.underlyingScrollView setDelegate:nil];
   [self stopObservingScrollView:self.underlyingScrollView];
+  if (base::FeatureList::IsEnabled(
+          web::features::kPreserveScrollViewProperties) &&
+      self.underlyingScrollView) {
+    [_propertiesStore savePropertiesFromObject:self.underlyingScrollView];
+  }
+
+  // Set up the delegate/observers of the new scroll view, and restore its
+  // properties.
   DCHECK(!scrollView.delegate);
   scrollView.delegate = self.delegateProxy;
   [self startObservingScrollView:scrollView];
+  if (base::FeatureList::IsEnabled(
+          web::features::kPreserveScrollViewProperties) &&
+      scrollView) {
+    [_propertiesStore loadPropertiesToObject:scrollView];
+    // Clear the stored values of the properties. This prevents from keeping
+    // retaining old property values.
+    [_propertiesStore clearValues];
+  }
+
   self.underlyingScrollView = scrollView;
+
   if (_storedClipsToBounds) {
     scrollView.clipsToBounds = *_storedClipsToBounds;
   }
-
   // Assigns |contentInsetAdjustmentBehavior| which was set before setting the
   // scroll view.
   if (_storedContentInsetAdjustmentBehavior) {
@@ -352,9 +418,33 @@
 
 - (void)forwardInvocation:(NSInvocation*)invocation {
   // Called when this proxy is accessed through -asUIScrollView and the method
-  // is not implemented in this class. self.underlyingScrollView may be nil, but
-  // it is safe. nil can respond to any invocation (and does nothing).
-  [invocation invokeWithTarget:self.underlyingScrollView];
+  // is not implemented in this class.
+  if (self.underlyingScrollView) {
+    // Forwards the invocation to the undelrying scroll view.
+    [invocation invokeWithTarget:self.underlyingScrollView];
+  } else {
+    BOOL handled = NO;
+    if (base::FeatureList::IsEnabled(
+            web::features::kPreserveScrollViewProperties)) {
+      // Forwards the invocation to the property store. If it is an invocation
+      // of a getter or setter of a preserved property, it gets or sets the
+      // property in the property store, which is later restored in the
+      // underlying scroll view.
+      handled =
+          [_propertiesStore forwardInvocationToPropertiesStore:invocation];
+    }
+
+    // If it is not an invocation of a getter or setter of a preserved property,
+    // do nothing and return a zero value. This must be done explicitly because
+    // it looks not guaranteed to return a zero value if it does nothing here or
+    // call [invocation invokeWithTarget:nil].
+    if (!handled && invocation.methodSignature.methodReturnLength > 0) {
+      // NSMutableData is initialized with zero bytes.
+      NSMutableData* zeroData = [NSMutableData
+          dataWithLength:invocation.methodSignature.methodReturnLength];
+      [invocation setReturnValue:zeroData.mutableBytes];
+    }
+  }
 }
 
 #pragma mark - NSObject

ios/web/web_state/ui/crw_web_view_scroll_view_proxy_unittest.mm

@@ -7,6 +7,8 @@
 #import <UIKit/UIKit.h>
 
 #include "base/compiler_specific.h"
+#include "base/test/scoped_feature_list.h"
+#include "ios/web/common/features.h"
 #import "ios/web/web_state/ui/crw_web_view_scroll_view_delegate_proxy.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/platform_test.h"
@@ -572,4 +574,99 @@ TEST_F(CRWWebViewScrollViewProxyTest, RemoveKVObserver) {
   EXPECT_OCMOCK_VERIFY(static_cast<id>(observer));
 }
 
+// Verifies that properties registered to |propertiesStore| are preserved if:
+//   - the setter is called when the underlying scroll view is nil
+//   - the getter is called after the underlying scroll view is still nil
+TEST_F(CRWWebViewScrollViewProxyTest,
+       PreservePropertiesWhileUnderlyingScrollViewIsNil) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      web::features::kPreserveScrollViewProperties);
+
+  // Recreate CRWWebViewScrollViewProxy with the updated feature flags.
+  web_view_scroll_view_proxy_ = [[CRWWebViewScrollViewProxy alloc] init];
+
+  [web_view_scroll_view_proxy_ setScrollView:nil];
+
+  // A preserved property with a primitive type.
+  [web_view_scroll_view_proxy_ asUIScrollView].directionalLockEnabled = YES;
+  EXPECT_TRUE(
+      [web_view_scroll_view_proxy_ asUIScrollView].directionalLockEnabled);
+  [web_view_scroll_view_proxy_ asUIScrollView].directionalLockEnabled = NO;
+  EXPECT_FALSE(
+      [web_view_scroll_view_proxy_ asUIScrollView].directionalLockEnabled);
+
+  // A preserved property with an object type.
+  [web_view_scroll_view_proxy_ asUIScrollView].tintColor = UIColor.redColor;
+  EXPECT_EQ(UIColor.redColor,
+            [web_view_scroll_view_proxy_ asUIScrollView].tintColor);
+}
+
+// Verifies that properties registered to |propertiesStore| are preserved if:
+//   - the setter is called when the underlying scroll view is nil
+//   - the getter is called after the underlying scroll view is assigned
+TEST_F(CRWWebViewScrollViewProxyTest,
+       PreservePropertiesWhenUnderlyingScrollViewIsNewlyAssigned) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      web::features::kPreserveScrollViewProperties);
+
+  // Recreate CRWWebViewScrollViewProxy with the updated feature flags.
+  web_view_scroll_view_proxy_ = [[CRWWebViewScrollViewProxy alloc] init];
+
+  [web_view_scroll_view_proxy_ setScrollView:nil];
+
+  [web_view_scroll_view_proxy_ asUIScrollView].directionalLockEnabled = YES;
+  [web_view_scroll_view_proxy_ asUIScrollView].tintColor = UIColor.redColor;
+
+  UIScrollView* underlying_scroll_view = [[UIScrollView alloc] init];
+  [web_view_scroll_view_proxy_ setScrollView:underlying_scroll_view];
+
+  // The properties are restored on the underlying scroll view.
+  EXPECT_TRUE(underlying_scroll_view.directionalLockEnabled);
+  EXPECT_EQ(UIColor.redColor, underlying_scroll_view.tintColor);
+
+  // The same property values are available via the scroll view proxy as well.
+  EXPECT_TRUE(
+      [web_view_scroll_view_proxy_ asUIScrollView].directionalLockEnabled);
+  EXPECT_EQ(UIColor.redColor,
+            [web_view_scroll_view_proxy_ asUIScrollView].tintColor);
+
+  [web_view_scroll_view_proxy_ setScrollView:nil];
+}
+
+// Verifies that properties registered to |propertiesStore| are preserved if:
+//   - the setter is called when the underlying scroll view is non-nil
+//   - the getter is called after the underlying scroll view is reassigned
+TEST_F(CRWWebViewScrollViewProxyTest,
+       PreservePropertiesWhenUnderlyingScrollViewIsReassigned) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      web::features::kPreserveScrollViewProperties);
+
+  // Recreate CRWWebViewScrollViewProxy with the updated feature flags.
+  web_view_scroll_view_proxy_ = [[CRWWebViewScrollViewProxy alloc] init];
+
+  UIScrollView* underlying_scroll_view1 = [[UIScrollView alloc] init];
+  [web_view_scroll_view_proxy_ setScrollView:underlying_scroll_view1];
+
+  [web_view_scroll_view_proxy_ asUIScrollView].directionalLockEnabled = YES;
+  [web_view_scroll_view_proxy_ asUIScrollView].tintColor = UIColor.redColor;
+
+  UIScrollView* underlying_scroll_view2 = [[UIScrollView alloc] init];
+  [web_view_scroll_view_proxy_ setScrollView:underlying_scroll_view2];
+
+  // The properties are restored on the underlying scroll view.
+  EXPECT_TRUE(underlying_scroll_view2.directionalLockEnabled);
+  EXPECT_EQ(UIColor.redColor, underlying_scroll_view2.tintColor);
+
+  // The same property values are available via the scroll view proxy as well.
+  EXPECT_TRUE(
+      [web_view_scroll_view_proxy_ asUIScrollView].directionalLockEnabled);
+  EXPECT_EQ(UIColor.redColor,
+            [web_view_scroll_view_proxy_ asUIScrollView].tintColor);
+
+  [web_view_scroll_view_proxy_ setScrollView:nil];
+}
+
 }  // namespace