Bug 1649191 - Remove the guarding statement about mismatching carets mode from AccessibleCaretManager APIs. r=botond

Steps to reproduce:
1. Open https://bugzilla.mozilla.org/home
   (Note the cursor is already blinking in the <input>
   "Enter a bug number or some search terms")
2. Pinch-zoom in.
3. Tap on the <input> "Enter a bug number or some search terms."
4. Pan/scroll the page, and the scrolling is very laggy.

Without this patch, AccessibleCaret disables APZ incorrectly via the
above operations. Here's an analysis.

In step 2, `OnScrollEnd()` called at the end of the pinch-zoom operation
is supposed to reset `mIsScrollStarted` to `false`, but `GetCaretMode()`
returns `CaretMode::Cursor` because the page already has a focus on
<input>. We are early-returned from `OnScrollEnd()` because
`mLastUpdateCaretMode` is still the default value `CaretMode::None`.

In step 3, tapping the <input> will call `UpdateCaretsForCursorMode()`,
setting `mIsCaretPositionChanged` to `true`. Then
`UpdateShouldDisableApz()` incorrectly sets `mShouldDisableApz` to
`true` because we still have `mIsScrollStarted=true`.

In step 4, the operation is laggy because APZ is disabled.

This patch fixed this bug by removing the guarding statement
`mLastUpdateCaretMode != GetCaretMode()` from three callback methods.

The statements were added in the very first patch introducing
`AccessibleCaretManager`. I don't recall why we needed them. (Perhaps to
avoid unnecessary updates notified from other PresShell?). Anyway, since
then, these callbacks have evolved to update carets only if any caret is
logically visible, so I don't see why we need these guards nowadays. By
doing so, `mIsScrollStarted` can be reset to `false` in `OnScrollEnd()`
in step 2.

Differential Revision: https://phabricator.services.mozilla.com/D99284
This commit is contained in:
Ting-Yu Lin 2020-12-09 23:02:34 +00:00
Родитель 2b305d3d49
Коммит 1e4e187165
2 изменённых файлов: 6 добавлений и 12 удалений

Просмотреть файл

@ -679,10 +679,6 @@ void AccessibleCaretManager::OnScrollStart() {
}
void AccessibleCaretManager::OnScrollEnd() {
if (mLastUpdateCaretMode != GetCaretMode()) {
return;
}
AutoRestore<bool> saveAllowFlushingLayout(mAllowFlushingLayout);
mAllowFlushingLayout = false;
@ -714,10 +710,6 @@ void AccessibleCaretManager::OnScrollEnd() {
}
void AccessibleCaretManager::OnScrollPositionChanged() {
if (mLastUpdateCaretMode != GetCaretMode()) {
return;
}
AutoRestore<bool> saveAllowFlushingLayout(mAllowFlushingLayout);
mAllowFlushingLayout = false;
@ -742,10 +734,6 @@ void AccessibleCaretManager::OnScrollPositionChanged() {
}
void AccessibleCaretManager::OnReflow() {
if (mLastUpdateCaretMode != GetCaretMode()) {
return;
}
AutoRestore<bool> saveAllowFlushingLayout(mAllowFlushingLayout);
mAllowFlushingLayout = false;

Просмотреть файл

@ -100,6 +100,7 @@ class AccessibleCaretManagerTester : public ::testing::Test {
}
bool IsTerminated() const override { return false; }
bool IsScrollStarted() const { return mIsScrollStarted; }
MOCK_CONST_METHOD0(GetCaretMode, CaretMode());
MOCK_METHOD1(DispatchCaretStateChangedEvent,
@ -716,6 +717,11 @@ MOZ_CAN_RUN_SCRIPT_FOR_DEFINITION {
EXPECT_CALL(check, Call("scrollend3"));
}
// Simulate a pinch-zoom operation before tapping on an empty content.
mManager.OnScrollStart();
mManager.OnScrollEnd();
EXPECT_EQ(mManager.IsScrollStarted(), false);
// Simulate a single tap on an empty content.
mManager.UpdateCarets();
EXPECT_EQ(FirstCaretAppearance(), Appearance::NormalNotShown);