Fix Transition Layout bug when oscillating around Peeked state.

When the currentState is Peeked and a transition starts, if the user
switched direction (from Revealed to Hidden and from Hidden to Revealed
again), the layout switcher's collection view would start a new
transition when one already existed:
`'NSInternalInconsistencyException', reason: 'the collection is already
in the middle of an interactive transition'`

Create a bool property |layoutBeingInteractedWith|, which is YES when
the user is scrubbing a layout transition. The layout transition
progress is only updated if this property is YES.

Create a bool property |gesturesEnabled|, which is set to NO when a pan
gesture ends and set to YES when a gesture begins and
|layoutInTransition| is NO. This is to prevent a gesture from being
recognised if it starts while the layout is already transitioning but
changes or ends after the layout transition is over. If that were to
happen, the viewRevealngVerticalPanHandler would fail to notify the
animatees that a view reveal would take place.

Bug: 1130037, 1127604
Change-Id: Ifc5eb40843673ab3ce663ed23a3b2f8cf18d9564
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2421741Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Commit-Queue: Roberto Moura <mouraroberto@google.com>
Cr-Commit-Position: refs/heads/master@{#812189}

ios/chrome/browser/ui/gestures/layout_switcher.h

@@ -17,8 +17,11 @@ enum class LayoutSwitcherState {
 
 // Notifies of a transition of layout to the specified state. Called when the
 // view revealing vertical pan handler starts a transition of layout. The
-// conformer should prepare its layout for a transition to |nextState|.
-- (void)willTransitionToLayout:(LayoutSwitcherState)nextState;
+// conformer should prepare its layout for a transition to |nextState|, that
+// should execute the specified completion block on completion.
+- (void)willTransitionToLayout:(LayoutSwitcherState)nextState
+                    completion:
+                        (void (^)(BOOL completed, BOOL finished))completion;
 
 // Notifies of a change in the progress of the transition of layout currently in
 // progress.

ios/chrome/browser/ui/gestures/view_revealing_vertical_pan_handler.h

@@ -15,12 +15,6 @@
 // TODO(crbug.com/1123512): Add support for going straight from a Hidden state
 // to a revealed state (and vice-versa) if the gesture's translation and
 // velocity are enough to trigger such transition.
-// TODO(crbug.com/1130037): When the currentState is Peeked and a transition
-// starts, if the user switches direction (from Revealed to Hidden and from
-// Hidden to Revealed again), the layout switcher's collection view starts a new
-// transition when one already exists: `'NSInternalInconsistencyException',
-// reason: 'the collection is already in the middle of an interactive
-// transition'`
 @interface ViewRevealingVerticalPanHandler : NSObject
 
 // |peekedHeight| is the height of the view when peeked (partially revealed).

ios/chrome/browser/ui/gestures/view_revealing_vertical_pan_handler.mm

@@ -44,8 +44,17 @@ const CGFloat kAnimationDuration = 0.25f;
 @property(nonatomic, assign) CGFloat progressWhenInterrupted;
 // Set of UI elements which are animated during view reveal transitions.
 @property(nonatomic, strong) NSHashTable<id<ViewRevealingAnimatee>>* animatees;
-// Whether the revealed view is undergoing a transition of layout.
+// Whether the revealed view is undergoing a transition of layout. Set to YES
+// when the transition layout is created and set to NO inside the transition's
+// completion block.
 @property(nonatomic, assign) BOOL layoutInTransition;
+// Whether the layout transition is being interactively scrubbed by the user.
+// Set to YES when the transition layout is created and set to NO when the pan
+// gesture ends.
+@property(nonatomic, assign) BOOL layoutBeingInteractedWith;
+// Whether new pan gestures should be handled. Set to NO when a pan gesture ends
+// and set to YES when a pan gesture starts while layoutInTransition is NO.
+@property(nonatomic, assign) BOOL gesturesEnabled;
 @end
 
 @implementation ViewRevealingVerticalPanHandler
@@ -67,6 +76,14 @@ const CGFloat kAnimationDuration = 0.25f;
 }
 
 - (void)handlePanGesture:(UIPanGestureRecognizer*)gesture {
+  // Avoid handling a pan gesture if it started before the layout transition was
+  // finished.
+  if (!self.layoutInTransition &&
+      gesture.state == UIGestureRecognizerStateBegan) {
+    self.gesturesEnabled = YES;
+  }
+  if (!self.gesturesEnabled)
+    return;
   CGFloat translationY = [gesture translationInView:gesture.view.superview].y;
 
   if (gesture.state == UIGestureRecognizerStateBegan) {
@@ -143,19 +160,37 @@ const CGFloat kAnimationDuration = 0.25f;
 // Creates a transition layout in the revealed view if going from Peeked to
 // Revealed state or vice-versa.
 - (void)createLayoutTransitionIfNeeded {
+  if (self.layoutInTransition) {
+    // Cancel the current layout transition.
+    [self.layoutSwitcherProvider.layoutSwitcher
+        didUpdateTransitionLayoutProgress:0];
+    [self.layoutSwitcherProvider.layoutSwitcher
+        didTransitionToLayoutSuccessfully:NO];
+    self.layoutBeingInteractedWith = NO;
+    return;
+  }
+
   if (self.currentState == ViewRevealState::Peeked &&
       self.nextState == ViewRevealState::Revealed) {
-    [self.layoutSwitcherProvider.layoutSwitcher
-        willTransitionToLayout:LayoutSwitcherState::Full];
-    self.layoutInTransition = YES;
+    [self willTransitionToLayout:LayoutSwitcherState::Full];
   } else if (self.currentState == ViewRevealState::Revealed &&
              self.nextState == ViewRevealState::Peeked) {
-    [self.layoutSwitcherProvider.layoutSwitcher
-        willTransitionToLayout:LayoutSwitcherState::Horizontal];
-    self.layoutInTransition = YES;
+    [self willTransitionToLayout:LayoutSwitcherState::Horizontal];
   }
 }
 
+// Notifies the layout switcher that a layout transition should happen.
+- (void)willTransitionToLayout:(LayoutSwitcherState)nextState {
+  auto completion = ^(BOOL completed, BOOL finished) {
+    self.layoutInTransition = NO;
+  };
+  [self.layoutSwitcherProvider.layoutSwitcher
+      willTransitionToLayout:nextState
+                  completion:completion];
+  self.layoutInTransition = YES;
+  self.layoutBeingInteractedWith = YES;
+}
+
 // Initiates a transition if there isn't already one running
 - (void)animateTransitionIfNeeded {
   if (self.animator.isRunning) {
@@ -250,7 +285,7 @@ const CGFloat kAnimationDuration = 0.25f;
   progress += self.progressWhenInterrupted;
   progress = base::ClampToRange<CGFloat>(progress, 0, 1);
   self.animator.fractionComplete = progress;
-  if (self.layoutInTransition) {
+  if (self.layoutBeingInteractedWith) {
     [self.layoutSwitcherProvider.layoutSwitcher
         didUpdateTransitionLayoutProgress:progress];
   }
@@ -270,7 +305,8 @@ const CGFloat kAnimationDuration = 0.25f;
     // the state is Peeked), the current animation should be stopped and a new
     // one created.
     if (translation > 0) {
-      if (self.nextState != ViewRevealState::Revealed) {
+      if (self.nextState != ViewRevealState::Revealed &&
+          !self.layoutInTransition) {
         self.nextState = ViewRevealState::Revealed;
         [self createAnimatorIfNeeded];
       }
@@ -292,10 +328,12 @@ const CGFloat kAnimationDuration = 0.25f;
                                                   Velocity:velocity]);
 
   [self.animator continueAnimationWithTimingParameters:nil durationFactor:1];
-  if (self.layoutInTransition) {
+
+  if (self.layoutBeingInteractedWith) {
     [self.layoutSwitcherProvider.layoutSwitcher
         didTransitionToLayoutSuccessfully:!self.animator.reversed];
-    self.layoutInTransition = NO;
+    self.gesturesEnabled = NO;
+    self.layoutBeingInteractedWith = NO;
   }
 }
 

ios/chrome/browser/ui/gestures/view_revealing_vertical_pan_handler_unittest.mm

@@ -8,6 +8,8 @@
 
 #include "base/check_op.h"
 #import "base/test/ios/wait_util.h"
+#import "ios/chrome/browser/ui/gestures/layout_switcher.h"
+#import "ios/chrome/browser/ui/gestures/layout_switcher_provider.h"
 #import "ios/chrome/browser/ui/gestures/view_revealing_animatee.h"
 #include "testing/platform_test.h"
 #import "third_party/ocmock/OCMock/OCMock.h"
@@ -22,6 +24,13 @@
 @end
 
 @implementation FakeAnimatee
+- (instancetype)init {
+  self = [super init];
+  if (self) {
+    self.state = ViewRevealState::Hidden;
+  }
+  return self;
+}
 - (void)willAnimateViewReveal:(ViewRevealState)viewRevealState {
 }
 - (void)animateViewReveal:(ViewRevealState)viewRevealState {
@@ -31,6 +40,57 @@
 }
 @end
 
+// A fake layout switcher provider.
+@interface FakeLayoutSwitcherProvider : NSObject <LayoutSwitcherProvider>
+@end
+
+@implementation FakeLayoutSwitcherProvider
+@synthesize layoutSwitcher = _layoutSwitcher;
+
+- (instancetype)initWithLayoutSwitcher:(id<LayoutSwitcher>)layoutSwitcher {
+  self = [super init];
+  if (self) {
+    _layoutSwitcher = layoutSwitcher;
+  }
+  return self;
+}
+@end
+
+// A fake layout switcher with observable layout properties.
+@interface FakeLayoutSwitcher : NSObject <LayoutSwitcher>
+@property(nonatomic, assign) LayoutSwitcherState state;
+@property(nonatomic, assign) LayoutSwitcherState nextState;
+@property(nonatomic, copy) void (^transitionCompletionBlock)
+    (BOOL completed, BOOL finished);
+@end
+
+@implementation FakeLayoutSwitcher
+- (instancetype)init {
+  self = [super init];
+  if (self) {
+    self.state = LayoutSwitcherState::Horizontal;
+  }
+  return self;
+}
+
+- (void)willTransitionToLayout:(LayoutSwitcherState)nextState
+                    completion:
+                        (void (^)(BOOL completed, BOOL finished))completion {
+  self.nextState = nextState;
+  self.transitionCompletionBlock = completion;
+}
+
+- (void)didUpdateTransitionLayoutProgress:(CGFloat)progress {
+}
+
+- (void)didTransitionToLayoutSuccessfully:(BOOL)success {
+  if (success) {
+    self.state = self.nextState;
+  }
+  self.transitionCompletionBlock(YES, success);
+}
+@end
+
 // A fake gesture recognizer that allows the translation and velocity to be set.
 @interface FakeGestureRecognizer : UIPanGestureRecognizer
 @property(nonatomic, assign) CGPoint translation;
@@ -105,6 +165,14 @@ TEST_F(ViewRevealingVerticalPanHandlerTest, DetectPan) {
            revealedCoverHeight:kBVCHeightTabGrid
                 baseViewHeight:kBaseViewHeight];
 
+  // Create a fake layout switcher and a provider.
+  FakeLayoutSwitcher* fake_layout_switcher = [[FakeLayoutSwitcher alloc] init];
+  FakeLayoutSwitcherProvider* fake_layout_switcher_provider =
+      [[FakeLayoutSwitcherProvider alloc]
+          initWithLayoutSwitcher:fake_layout_switcher];
+  pan_handler.layoutSwitcherProvider = fake_layout_switcher_provider;
+  EXPECT_EQ(LayoutSwitcherState::Horizontal, fake_layout_switcher.state);
+
   // Create a fake animatee.
   FakeAnimatee* fake_animatee = [[FakeAnimatee alloc] init];
   [pan_handler addAnimatee:fake_animatee];
@@ -114,13 +182,17 @@ TEST_F(ViewRevealingVerticalPanHandlerTest, DetectPan) {
   SimulatePanGesture(pan_handler, kThumbStripHeight * kRevealThreshold);
   EXPECT_EQ(ViewRevealState::Peeked, fake_animatee.state);
 
-  // Simulate a pan gesture from Peeked state to Revealed state.
+  // Simulate a pan gesture from Peeked state to Revealed state. The layout
+  // should transition to full state.
   SimulatePanGesture(pan_handler, remaining_height * kRevealThreshold);
   EXPECT_EQ(ViewRevealState::Revealed, fake_animatee.state);
+  EXPECT_EQ(LayoutSwitcherState::Full, fake_layout_switcher.state);
 
-  // Simulate a pan gesture from Revealed state to Peeked state.
+  // Simulate a pan gesture from Revealed state to Peeked state. The layout
+  // should transition back to horizontal state.
   SimulatePanGesture(pan_handler, -(remaining_height * kRevealThreshold));
   EXPECT_EQ(ViewRevealState::Peeked, fake_animatee.state);
+  EXPECT_EQ(LayoutSwitcherState::Horizontal, fake_layout_switcher.state);
 
   // Simulate a pan gesture from Peeked state to Hidden state.
   SimulatePanGesture(pan_handler, -(kThumbStripHeight * kRevealThreshold));

ios/chrome/browser/ui/tab_grid/grid/grid_view_controller.mm

@@ -692,8 +692,11 @@ NSIndexPath* CreateIndexPath(NSInteger index) {
 
 #pragma mark - LayoutSwitcher
 
-- (void)willTransitionToLayout:(LayoutSwitcherState)nextState {
+- (void)willTransitionToLayout:(LayoutSwitcherState)nextState
+                    completion:
+                        (void (^)(BOOL completed, BOOL finished))completion {
   auto completionBlock = ^(BOOL completed, BOOL finished) {
+    completion(completed, finished);
     self.collectionView.scrollEnabled = YES;
   };
   switch (nextState) {