Bug 1246918 - Fix carets missing after scrolling down in selection mo…

…de on Fennec. r=roc

Fennec enables sCaretsExtendedVisibility which uses
Appearance::NormalNotShown instead of Appearance::None to keep actionbar
shown during scrolling. This breaks selection mode update when the
positions of the carets are not changed after scrolling.

To fix this, we need to implement appearance recovering for selection
mode scrolling like we did for cursor mode in bug 1212732, and make
UpdateCaretsForSelectionMode() respects UpdateCaretsHint.

MozReview-Commit-ID: LkfUIGKHL0h

layout/base/AccessibleCaretManager.cpp

@@ -202,7 +202,7 @@ AccessibleCaretManager::UpdateCarets(UpdateCaretsHint aHint)
     UpdateCaretsForCursorMode(aHint);
     break;
   case CaretMode::Selection:
-    UpdateCaretsForSelectionMode();
+    UpdateCaretsForSelectionMode(aHint);
     break;
   }
 }
@@ -316,7 +316,7 @@ AccessibleCaretManager::UpdateCaretsForCursorMode(UpdateCaretsHint aHint)
 }
 
 void
-AccessibleCaretManager::UpdateCaretsForSelectionMode()
+AccessibleCaretManager::UpdateCaretsForSelectionMode(UpdateCaretsHint aHint)
 {
   AC_LOG("%s: selection: %p", __FUNCTION__, GetSelection());
 
@@ -332,8 +332,8 @@ AccessibleCaretManager::UpdateCaretsForSelectionMode()
     return;
   }
 
-  auto updateSingleCaret = [](AccessibleCaret* aCaret, nsIFrame* aFrame,
-                              int32_t aOffset) -> PositionChangedResult
+  auto updateSingleCaret = [aHint](AccessibleCaret* aCaret, nsIFrame* aFrame,
+                                   int32_t aOffset) -> PositionChangedResult
   {
     PositionChangedResult result = aCaret->SetPosition(aFrame, aOffset);
     aCaret->SetSelectionBarEnabled(sSelectionBarEnabled);
@@ -344,7 +344,16 @@ AccessibleCaretManager::UpdateCaretsForSelectionMode()
         break;
 
       case PositionChangedResult::Changed:
-        aCaret->SetAppearance(Appearance::Normal);
+        switch (aHint) {
+          case UpdateCaretsHint::Default:
+            aCaret->SetAppearance(Appearance::Normal);
+            break;
+
+          case UpdateCaretsHint::RespectOldAppearance:
+            // Do nothing to preserve the appearance of the caret set by the
+            // caller.
+            break;
+        }
         break;
 
       case PositionChangedResult::Invisible:
@@ -365,7 +374,11 @@ AccessibleCaretManager::UpdateCaretsForSelectionMode()
     FlushLayout();
   }
 
-  UpdateCaretsForTilt();
+  if (aHint == UpdateCaretsHint::Default) {
+    // Only check for tilt carets with default update hint. Otherwise we might
+    // override the appearance set by the caller.
+    UpdateCaretsForTilt();
+  }
 
   if (!mActiveCaret) {
     DispatchCaretStateChangedEvent(CaretChangedReason::Updateposition);
@@ -560,9 +573,8 @@ AccessibleCaretManager::OnScrollStart()
 {
   AC_LOG("%s", __FUNCTION__);
 
-  if (GetCaretMode() == CaretMode::Cursor) {
-    mFirstCaretAppearanceOnScrollStart = mFirstCaret->GetAppearance();
-  }
+  mFirstCaretAppearanceOnScrollStart = mFirstCaret->GetAppearance();
+  mSecondCaretAppearanceOnScrollStart = mSecondCaret->GetAppearance();
 
   // Hide the carets. (Extended visibility makes them "NormalNotShown").
   if (sCaretsExtendedVisibility) {
@@ -579,11 +591,18 @@ AccessibleCaretManager::OnScrollEnd()
     return;
   }
 
+  mFirstCaret->SetAppearance(mFirstCaretAppearanceOnScrollStart);
+  mSecondCaret->SetAppearance(mSecondCaretAppearanceOnScrollStart);
+
+  // Flush layout to make the carets intersection correct since we turn the
+  // appearance of the carets from None or NormalNotShown into something
+  // visible.
+  FlushLayout();
+
   if (GetCaretMode() == CaretMode::Cursor) {
-    mFirstCaret->SetAppearance(mFirstCaretAppearanceOnScrollStart);
     if (!mFirstCaret->IsLogicallyVisible()) {
-      // If the caret is hide (Appearance::None) due to timeout or blur, no need
-      // to update it.
+      // If the caret is hidden (Appearance::None) due to timeout or blur, no
+      // need to update it.
       return;
     }
   }

layout/base/AccessibleCaretManager.h

@@ -135,7 +135,7 @@ class AccessibleCaretManager
   void DoNotShowCarets();
 
   void UpdateCaretsForCursorMode(UpdateCaretsHint aHint);
-  void UpdateCaretsForSelectionMode();
+  void UpdateCaretsForSelectionMode(UpdateCaretsHint aHint);
 
   // Provide haptic / touch feedback, primarily for select on longpress.
   void ProvideHapticFeedback();
@@ -234,10 +234,12 @@ class AccessibleCaretManager
   // The caret mode since last update carets.
   CaretMode mLastUpdateCaretMode = CaretMode::None;
 
-  // Store the appearance of the first caret when calling OnScrollStart so that
-  // it can be restored in OnScrollEnd.
+  // Store the appearance of the carets when calling OnScrollStart() so that it
+  // can be restored in OnScrollEnd().
   AccessibleCaret::Appearance mFirstCaretAppearanceOnScrollStart =
                                  AccessibleCaret::Appearance::None;
+  AccessibleCaret::Appearance mSecondCaretAppearanceOnScrollStart =
+                                 AccessibleCaret::Appearance::None;
 
   static const int32_t kAutoScrollTimerDelay = 30;
 
@@ -258,7 +260,7 @@ class AccessibleCaretManager
   static bool sCaretShownWhenLongTappingOnEmptyContent;
 
   // Android specific visibility extensions correct compatibility issues
-  // with caret-drag and ActionBar visibility during page scroll.
+  // with ActionBar visibility during page scroll.
   static bool sCaretsExtendedVisibility;
 
   // By default, javascript content selection changes closes AccessibleCarets and

layout/base/gtest/TestAccessibleCaretManager.cpp

@@ -60,6 +60,7 @@ class AccessibleCaretManagerTester : public ::testing::Test
     using AccessibleCaretManager::UpdateCarets;
     using AccessibleCaretManager::HideCarets;
     using AccessibleCaretManager::sCaretShownWhenLongTappingOnEmptyContent;
+    using AccessibleCaretManager::sCaretsExtendedVisibility;
 
     MockAccessibleCaretManager()
       : AccessibleCaretManager(nullptr)
@@ -344,27 +345,33 @@ TEST_F(AccessibleCaretManagerTester, TestScrollInSelectionMode)
 
     // Initially, first caret is out of scrollport, and second caret is visible.
     EXPECT_CALL(mManager.FirstCaret(), SetPosition(_, _))
-      .WillRepeatedly(Return(PositionChangedResult::Invisible));
-    EXPECT_CALL(mManager.SecondCaret(), SetPosition(_, _))
-      .WillRepeatedly(Return(PositionChangedResult::Changed));
+      .WillOnce(Return(PositionChangedResult::Invisible));
 
     EXPECT_CALL(mManager, DispatchCaretStateChangedEvent(
-                  CaretChangedReason::Updateposition)).Times(1);
+                  CaretChangedReason::Updateposition));
     EXPECT_CALL(check, Call("updatecarets"));
 
     EXPECT_CALL(mManager, DispatchCaretStateChangedEvent(
-                  CaretChangedReason::Visibilitychange)).Times(1);
-    EXPECT_CALL(check, Call("scrollstart"));
+                  CaretChangedReason::Visibilitychange));
+    EXPECT_CALL(check, Call("scrollstart1"));
 
     // After scroll ended, first caret is visible and second caret is out of
     // scroll port.
-    EXPECT_CALL(mManager.FirstCaret(), SetPosition(_, _))
-      .WillRepeatedly(Return(PositionChangedResult::Changed));
     EXPECT_CALL(mManager.SecondCaret(), SetPosition(_, _))
-      .WillRepeatedly(Return(PositionChangedResult::Invisible));
+      .WillOnce(Return(PositionChangedResult::Invisible));
 
     EXPECT_CALL(mManager, DispatchCaretStateChangedEvent(
-                  CaretChangedReason::Updateposition)).Times(1);
+                  CaretChangedReason::Updateposition));
+    EXPECT_CALL(check, Call("scrollend1"));
+
+    EXPECT_CALL(mManager, DispatchCaretStateChangedEvent(
+                  CaretChangedReason::Visibilitychange));
+    EXPECT_CALL(check, Call("scrollstart2"));
+
+    // After the scroll ended, both carets are visible.
+    EXPECT_CALL(mManager, DispatchCaretStateChangedEvent(
+                  CaretChangedReason::Updateposition));
+    EXPECT_CALL(check, Call("scrollend2"));
   }
 
   mManager.UpdateCarets();
@@ -375,11 +382,119 @@ TEST_F(AccessibleCaretManagerTester, TestScrollInSelectionMode)
   mManager.OnScrollStart();
   EXPECT_EQ(FirstCaretAppearance(), Appearance::None);
   EXPECT_EQ(SecondCaretAppearance(), Appearance::None);
-  check.Call("scrollstart");
+  check.Call("scrollstart1");
+
+  mManager.OnReflow();
+  EXPECT_EQ(FirstCaretAppearance(), Appearance::None);
+  EXPECT_EQ(SecondCaretAppearance(), Appearance::None);
+
+  mManager.OnScrollEnd();
+  EXPECT_EQ(FirstCaretAppearance(), Appearance::Normal);
+  EXPECT_EQ(SecondCaretAppearance(), Appearance::NormalNotShown);
+  check.Call("scrollend1");
+
+  mManager.OnScrollStart();
+  EXPECT_EQ(FirstCaretAppearance(), Appearance::None);
+  EXPECT_EQ(SecondCaretAppearance(), Appearance::None);
+  check.Call("scrollstart2");
+
+  mManager.OnReflow();
+  EXPECT_EQ(FirstCaretAppearance(), Appearance::None);
+  EXPECT_EQ(SecondCaretAppearance(), Appearance::None);
+
+  mManager.OnScrollEnd();
+  EXPECT_EQ(FirstCaretAppearance(), Appearance::Normal);
+  EXPECT_EQ(SecondCaretAppearance(), Appearance::Normal);
+  check.Call("scrollend2");
+}
+
+TEST_F(AccessibleCaretManagerTester,
+       TestScrollInSelectionModeWithExtendedVisibility)
+{
+  EXPECT_CALL(mManager, GetCaretMode())
+    .WillRepeatedly(Return(CaretMode::Selection));
+
+  MockFunction<void(std::string aCheckPointName)> check;
+  {
+    InSequence dummy;
+
+    // Initially, first caret is out of scrollport, and second caret is visible.
+    EXPECT_CALL(mManager.FirstCaret(), SetPosition(_, _))
+      .WillOnce(Return(PositionChangedResult::Invisible));
+
+    EXPECT_CALL(mManager, DispatchCaretStateChangedEvent(
+                  CaretChangedReason::Updateposition));
+    EXPECT_CALL(check, Call("updatecarets"));
+
+    EXPECT_CALL(mManager, DispatchCaretStateChangedEvent(
+                  CaretChangedReason::Visibilitychange));
+    EXPECT_CALL(check, Call("scrollstart1"));
+
+    EXPECT_CALL(mManager, DispatchCaretStateChangedEvent(
+                  CaretChangedReason::Updateposition));
+    EXPECT_CALL(check, Call("reflow1"));
+
+    // After scroll ended, first caret is visible and second caret is out of
+    // scroll port.
+    EXPECT_CALL(mManager.SecondCaret(), SetPosition(_, _))
+      .WillOnce(Return(PositionChangedResult::Invisible));
+
+    EXPECT_CALL(mManager, DispatchCaretStateChangedEvent(
+                  CaretChangedReason::Updateposition));
+    EXPECT_CALL(check, Call("scrollend1"));
+
+    EXPECT_CALL(mManager, DispatchCaretStateChangedEvent(
+                  CaretChangedReason::Visibilitychange));
+    EXPECT_CALL(check, Call("scrollstart2"));
+
+    EXPECT_CALL(mManager, DispatchCaretStateChangedEvent(
+                  CaretChangedReason::Updateposition));
+    EXPECT_CALL(check, Call("reflow2"));
+
+    // After the scroll ended, both carets are visible.
+    EXPECT_CALL(mManager, DispatchCaretStateChangedEvent(
+                  CaretChangedReason::Updateposition));
+    EXPECT_CALL(check, Call("scrollend2"));
+  }
+
+  AutoRestore<bool> savePref(
+    MockAccessibleCaretManager::sCaretsExtendedVisibility);
+  MockAccessibleCaretManager::sCaretsExtendedVisibility = true;
+
+  mManager.UpdateCarets();
+  EXPECT_EQ(FirstCaretAppearance(), Appearance::NormalNotShown);
+  EXPECT_EQ(SecondCaretAppearance(), Appearance::Normal);
+  check.Call("updatecarets");
+
+  mManager.OnScrollStart();
+  EXPECT_EQ(FirstCaretAppearance(), Appearance::NormalNotShown);
+  EXPECT_EQ(SecondCaretAppearance(), Appearance::NormalNotShown);
+  check.Call("scrollstart1");
+
+  mManager.OnReflow();
+  EXPECT_EQ(FirstCaretAppearance(), Appearance::NormalNotShown);
+  EXPECT_EQ(SecondCaretAppearance(), Appearance::NormalNotShown);
+  check.Call("reflow1");
 
   mManager.OnScrollEnd();
   EXPECT_EQ(FirstCaretAppearance(), Appearance::Normal);
   EXPECT_EQ(SecondCaretAppearance(), Appearance::NormalNotShown);
+  check.Call("scrollend1");
+
+  mManager.OnScrollStart();
+  EXPECT_EQ(FirstCaretAppearance(), Appearance::NormalNotShown);
+  EXPECT_EQ(SecondCaretAppearance(), Appearance::NormalNotShown);
+  check.Call("scrollstart2");
+
+  mManager.OnReflow();
+  EXPECT_EQ(FirstCaretAppearance(), Appearance::NormalNotShown);
+  EXPECT_EQ(SecondCaretAppearance(), Appearance::NormalNotShown);
+  check.Call("reflow2");
+
+  mManager.OnScrollEnd();
+  EXPECT_EQ(FirstCaretAppearance(), Appearance::Normal);
+  EXPECT_EQ(SecondCaretAppearance(), Appearance::Normal);
+  check.Call("scrollend2");
 }
 
 TEST_F(AccessibleCaretManagerTester, TestScrollInCursorModeWhenLogicallyVisible)
@@ -537,7 +652,7 @@ TEST_F(AccessibleCaretManagerTester, TestScrollInCursorModeOnEmptyContent)
     EXPECT_CALL(mManager, DispatchCaretStateChangedEvent(
                    CaretChangedReason::Updateposition));
     EXPECT_CALL(check, Call("scrollend3"));
-}
+  }
 
   // Simulate a single tap on an empty content.
   mManager.UpdateCarets();
@@ -566,7 +681,6 @@ TEST_F(AccessibleCaretManagerTester, TestScrollInCursorModeOnEmptyContent)
   check.Call("scrollend3");
 }
 
-
 TEST_F(AccessibleCaretManagerTester,
        TestScrollInCursorModeOnEmptyContentWithSpecialPreference)
 {

mobile/android/app/mobile.js

@@ -955,8 +955,8 @@ pref("layout.accessiblecaret.timeout_ms", 0);
 pref("layout.accessiblecaret.use_long_tap_injector", false);
 
 // AccessibleCarets behaviour is extended to support Android specific
-// requirements during caret-drag, tapping into empty inputs, and to
-// hide carets while maintaining ActionBar visiblity during page scroll.
+// requirements to hide carets while maintaining ActionBar visiblity during page
+// scroll.
 pref("layout.accessiblecaret.extendedvisibility", true);
 
 // Selection change notifications generated by Javascript changes