aw: Ensure synchronization of PopupWindow handle visibility/position

Setting the visibility and position of a PopupWindow does not guarantee
that the new visibility/position will take effect in the same frame.
This leads to cases where the PopupWindow touch handles can appear out
of position for a single frame.

Prevent this by delaying visibility updates by one frame when the handles
transition from hidden to visible.

Also avoid issues where Android rejects one-dimensional selection
rect regions, as well as issues where selection bounds queried from
inside the selection event callback were stale.

BUG=490800

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

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

5 files changed, 98 insertions, 30 deletions

diff --git a/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java b/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
index 43f223c..1e33a95 100644
--- a/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
+++ b/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
@@ -2176,6 +2176,10 @@ public class ContentViewCore implements
     @CalledByNative
     private void onSelectionEvent(
             int eventType, int xAnchor, int yAnchor, int left, int top, int right, int bottom) {
+        // Ensure the provided selection coordinates form a non-empty rect, as required by
+        // the selection action mode.
+        if (left == right) ++right;
+        if (top == bottom) ++bottom;
         switch (eventType) {
             case SelectionEventType.SELECTION_SHOWN:
                 mSelectionRect.set(left, top, right, bottom);
@@ -2184,11 +2188,7 @@ public class ContentViewCore implements
                 // TODO(cjhopman): Remove this when there is a better signal that long press caused
                 // a selection. See http://crbug.com/150151.
                 mContainerView.performHapticFeedback(HapticFeedbackConstants.LONG_PRESS);
-                // There may already be an active selection, e.g., if the user
-                // longpresses different text while an existing selection is
-                // active. In such case, we need to update the selection rect.
                 showSelectActionMode(true);
-                invalidateActionModeContentRect();
                 break;
 
             case SelectionEventType.SELECTION_MOVED:
diff --git a/content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java b/content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java
index 720534c..2b86bd8 100644
--- a/content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java
+++ b/content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java
@@ -68,6 +68,13 @@ public class PopupTouchHandleDrawable extends View {
     private boolean mVisible;
     private boolean mTemporarilyHidden;
 
+    // There are no guarantees that the side effects of setting the position of
+    // the PopupWindow and the visibility of its content View will be realized
+    // in the same frame. Thus, to ensure the PopupWindow is seen in the right
+    // location, when the PopupWindow reappears we delay the visibility update
+    // by one frame after setting the position.
+    private boolean mDelayVisibilityUpdateWAR;
+
     // Deferred runnable to avoid invalidating outside of frame dispatch,
     // in turn avoiding issues with sync barrier insertion.
     private Runnable mInvalidationRunnable;
@@ -205,8 +212,20 @@ public class PopupTouchHandleDrawable extends View {
     }
 
     private void updateVisibility() {
-        boolean visible = mVisible && !mTemporarilyHidden;
-        setVisibility(visible ? VISIBLE : INVISIBLE);
+        int newVisibility = mVisible && !mTemporarilyHidden ? VISIBLE : INVISIBLE;
+
+        // When regaining visibility, delay the visibility update by one frame
+        // to ensure the PopupWindow has first been positioned properly.
+        if (newVisibility == VISIBLE && getVisibility() != VISIBLE) {
+            if (!mDelayVisibilityUpdateWAR) {
+                mDelayVisibilityUpdateWAR = true;
+                scheduleInvalidate();
+                return;
+            }
+        }
+        mDelayVisibilityUpdateWAR = false;
+
+        setVisibility(newVisibility);
     }
 
     private void updateAlpha() {
@@ -225,8 +244,8 @@ public class PopupTouchHandleDrawable extends View {
 
     private void doInvalidate() {
         if (!mContainer.isShowing()) return;
-        updatePosition();
         updateVisibility();
+        updatePosition();
         invalidate();
     }
 
diff --git a/ui/touch_selection/touch_selection_controller.cc b/ui/touch_selection/touch_selection_controller.cc
index 34cc30a..3dbaaa0 100644
--- a/ui/touch_selection/touch_selection_controller.cc
+++ b/ui/touch_selection/touch_selection_controller.cc
@@ -389,16 +389,13 @@ void TouchSelectionController::OnInsertionChanged() {
   if (!activate_insertion_automatically_)
     return;
 
-  const bool was_active = active_status_ == INSERTION_ACTIVE;
-  const gfx::PointF position = GetStartPosition();
-  if (!was_active)
-    ActivateInsertion();
-  else
-    client_->OnSelectionEvent(INSERTION_MOVED);
+  const bool activated = ActivateInsertionIfNecessary();
+
+  const TouchHandle::AnimationStyle animation = GetAnimationStyle(!activated);
+  insertion_handle_->SetVisible(GetStartVisible(), animation);
+  insertion_handle_->SetPosition(GetStartPosition());
 
-  insertion_handle_->SetVisible(GetStartVisible(),
-                                GetAnimationStyle(was_active));
-  insertion_handle_->SetPosition(position);
+  client_->OnSelectionEvent(activated ? INSERTION_SHOWN : INSERTION_MOVED);
 }
 
 void TouchSelectionController::OnSelectionChanged() {
@@ -407,32 +404,31 @@ void TouchSelectionController::OnSelectionChanged() {
   if (!activate_selection_automatically_)
     return;
 
-  const bool was_active = active_status_ == SELECTION_ACTIVE;
-  if (!was_active || response_pending_input_event_ == LONG_PRESS)
-    ActivateSelection();
-  else
-    client_->OnSelectionEvent(SELECTION_MOVED);
+  const bool activated = ActivateSelectionIfNecessary();
 
-  const TouchHandle::AnimationStyle animation = GetAnimationStyle(was_active);
+  const TouchHandle::AnimationStyle animation = GetAnimationStyle(!activated);
   start_selection_handle_->SetVisible(GetStartVisible(), animation);
   end_selection_handle_->SetVisible(GetEndVisible(), animation);
-
   start_selection_handle_->SetPosition(GetStartPosition());
   end_selection_handle_->SetPosition(GetEndPosition());
+
+  client_->OnSelectionEvent(activated ? SELECTION_SHOWN : SELECTION_MOVED);
 }
 
-void TouchSelectionController::ActivateInsertion() {
+bool TouchSelectionController::ActivateInsertionIfNecessary() {
   DCHECK_NE(SELECTION_ACTIVE, active_status_);
 
-  if (!insertion_handle_)
+  if (!insertion_handle_) {
     insertion_handle_.reset(
         new TouchHandle(this, TouchHandleOrientation::CENTER));
+  }
 
   if (active_status_ == INACTIVE) {
     active_status_ = INSERTION_ACTIVE;
     insertion_handle_->SetEnabled(true);
-    client_->OnSelectionEvent(INSERTION_SHOWN);
+    return true;
   }
+  return false;
 }
 
 void TouchSelectionController::DeactivateInsertion() {
@@ -444,7 +440,7 @@ void TouchSelectionController::DeactivateInsertion() {
   client_->OnSelectionEvent(INSERTION_CLEARED);
 }
 
-void TouchSelectionController::ActivateSelection() {
+bool TouchSelectionController::ActivateSelectionIfNecessary() {
   DCHECK_NE(INSERTION_ACTIVE, active_status_);
 
   if (!start_selection_handle_) {
@@ -474,8 +470,9 @@ void TouchSelectionController::ActivateSelection() {
     selection_handle_dragged_ = false;
     selection_start_time_ = base::TimeTicks::Now();
     response_pending_input_event_ = INPUT_EVENT_TYPE_NONE;
-    client_->OnSelectionEvent(SELECTION_SHOWN);
+    return true;
   }
+  return false;
 }
 
 void TouchSelectionController::DeactivateSelection() {
diff --git a/ui/touch_selection/touch_selection_controller.h b/ui/touch_selection/touch_selection_controller.h
index 7ec25d1..4161e77 100644
--- a/ui/touch_selection/touch_selection_controller.h
+++ b/ui/touch_selection/touch_selection_controller.h
@@ -129,9 +129,11 @@ class UI_TOUCH_SELECTION_EXPORT TouchSelectionController
   void OnInsertionChanged();
   void OnSelectionChanged();
 
-  void ActivateInsertion();
+  // Returns true if insertion mode was newly (re)activated.
+  bool ActivateInsertionIfNecessary();
   void DeactivateInsertion();
-  void ActivateSelection();
+  // Returns true if selection mode was newly (re)activated.
+  bool ActivateSelectionIfNecessary();
   void DeactivateSelection();
   void ForceNextUpdateIfInactive();
 
diff --git a/ui/touch_selection/touch_selection_controller_unittest.cc b/ui/touch_selection/touch_selection_controller_unittest.cc
index 8125c50..48e65a9 100644
--- a/ui/touch_selection/touch_selection_controller_unittest.cc
+++ b/ui/touch_selection/touch_selection_controller_unittest.cc
@@ -101,6 +101,7 @@ class TouchSelectionControllerTest : public testing::Test,
     events_.push_back(event);
     last_event_start_ = controller_->GetStartPosition();
     last_event_end_ = controller_->GetEndPosition();
+    last_event_bounds_rect_ = controller_->GetRectBetweenBounds();
   }
 
   scoped_ptr<TouchHandleDrawable> CreateDrawable() override {
@@ -193,6 +194,9 @@ class TouchSelectionControllerTest : public testing::Test,
   const gfx::PointF& GetLastSelectionEnd() const { return selection_end_; }
   const gfx::PointF& GetLastEventStart() const { return last_event_start_; }
   const gfx::PointF& GetLastEventEnd() const { return last_event_end_; }
+  const gfx::RectF& GetLastEventBoundsRect() const {
+    return last_event_bounds_rect_;
+  }
 
   std::vector<SelectionEventType> GetAndResetEvents() {
     std::vector<SelectionEventType> events;
@@ -208,6 +212,7 @@ class TouchSelectionControllerTest : public testing::Test,
   gfx::PointF caret_position_;
   gfx::PointF selection_start_;
   gfx::PointF selection_end_;
+  gfx::RectF last_event_bounds_rect_;
   std::vector<SelectionEventType> events_;
   bool caret_moved_;
   bool selection_moved_;
@@ -1046,4 +1051,49 @@ TEST_F(TouchSelectionControllerTest, HandlesShowOnLongPressInsideRect) {
   EXPECT_THAT(GetAndResetEvents(), ElementsAre(SELECTION_SHOWN));
 }
 
+TEST_F(TouchSelectionControllerTest, RectBetweenBounds) {
+  gfx::RectF start_rect(5, 5, 0, 10);
+  gfx::RectF end_rect(50, 5, 0, 10);
+  bool visible = true;
+
+  EXPECT_EQ(gfx::RectF(), controller().GetRectBetweenBounds());
+
+  OnLongPressEvent();
+  ChangeSelection(start_rect, visible, end_rect, visible);
+  ASSERT_THAT(GetAndResetEvents(), ElementsAre(SELECTION_SHOWN));
+  EXPECT_EQ(gfx::RectF(5, 5, 45, 10), controller().GetRectBetweenBounds());
+
+  // The result of |GetRectBetweenBounds| should be available within the
+  // |OnSelectionEvent| callback, as stored by |GetLastEventBoundsRect()|.
+  EXPECT_EQ(GetLastEventBoundsRect(), controller().GetRectBetweenBounds());
+
+  start_rect.Offset(1, 0);
+  ChangeSelection(start_rect, visible, end_rect, visible);
+  ASSERT_THAT(GetAndResetEvents(), ElementsAre(SELECTION_MOVED));
+  EXPECT_EQ(gfx::RectF(6, 5, 44, 10), controller().GetRectBetweenBounds());
+  EXPECT_EQ(GetLastEventBoundsRect(), controller().GetRectBetweenBounds());
+
+  // If only one bound is visible, the selection bounding rect should reflect
+  // only the visible bound.
+  ChangeSelection(start_rect, visible, end_rect, false);
+  ASSERT_THAT(GetAndResetEvents(), ElementsAre(SELECTION_MOVED));
+  EXPECT_EQ(start_rect, controller().GetRectBetweenBounds());
+  EXPECT_EQ(GetLastEventBoundsRect(), controller().GetRectBetweenBounds());
+
+  ChangeSelection(start_rect, false, end_rect, visible);
+  ASSERT_THAT(GetAndResetEvents(), ElementsAre(SELECTION_MOVED));
+  EXPECT_EQ(end_rect, controller().GetRectBetweenBounds());
+  EXPECT_EQ(GetLastEventBoundsRect(), controller().GetRectBetweenBounds());
+
+  // If both bounds are visible, the full bounding rect should be returned.
+  ChangeSelection(start_rect, false, end_rect, false);
+  ASSERT_THAT(GetAndResetEvents(), ElementsAre(SELECTION_MOVED));
+  EXPECT_EQ(gfx::RectF(6, 5, 44, 10), controller().GetRectBetweenBounds());
+  EXPECT_EQ(GetLastEventBoundsRect(), controller().GetRectBetweenBounds());
+
+  ClearSelection();
+  ASSERT_THAT(GetAndResetEvents(), ElementsAre(SELECTION_CLEARED));
+  EXPECT_EQ(gfx::RectF(), controller().GetRectBetweenBounds());
+}
+
 }  // namespace ui