From ad21a109a1a570f096f3c5c9a46c9eb46228874c Mon Sep 17 00:00:00 2001 From: Ting-Yu Lin Date: Thu, 11 Feb 2016 16:22:57 +0800 Subject: [PATCH] Bug 1246918 - Fix carets missing after scrolling down in selection mode 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 | 43 ++++-- layout/base/AccessibleCaretManager.h | 10 +- .../base/gtest/TestAccessibleCaretManager.cpp | 140 ++++++++++++++++-- mobile/android/app/mobile.js | 4 +- 4 files changed, 166 insertions(+), 31 deletions(-) diff --git a/layout/base/AccessibleCaretManager.cpp b/layout/base/AccessibleCaretManager.cpp index 23c66368a60cf..debfd03f04c0e 100644 --- a/layout/base/AccessibleCaretManager.cpp +++ b/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; } } diff --git a/layout/base/AccessibleCaretManager.h b/layout/base/AccessibleCaretManager.h index 4394b91f77f8f..4989d4202a006 100644 --- a/layout/base/AccessibleCaretManager.h +++ b/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 diff --git a/layout/base/gtest/TestAccessibleCaretManager.cpp b/layout/base/gtest/TestAccessibleCaretManager.cpp index 5b43c559a9aa2..ef2b905f1cec8 100644 --- a/layout/base/gtest/TestAccessibleCaretManager.cpp +++ b/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 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 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) { diff --git a/mobile/android/app/mobile.js b/mobile/android/app/mobile.js index 9740bce9a298c..1a8101a25e97f 100644 --- a/mobile/android/app/mobile.js +++ b/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