Skip to content

Commit 53a0e70

Browse files
HarryCuttsHarryCutts
committed
GestureConverter: fix inconsistencies from clicks during gestures
When button changes occurred during an ongoing fake-finger gesture (scrolling, pinching, or swiping), the converter was producing an inconsistent event stream, with DOWN events when pointers were already down, UP events when they were already up, and similarly for BUTTON_PRESS and BUTTON_RELEASE. To fix this, make pressed buttons and in-progress fake-finger gestures mutually exclusive from the perspective of a consumer of input events. If a button press occurs while a gesture is in progress, it is dropped. If a gesture starts while one or more buttons are down, we release them before starting the gesture. (In both cases, the corresponding button release events are also dropped, whenever they occur.) Gestures are given priority over button states because this issue is most likely to occur when the user is making a multi-finger swipe, presses down a bit too hard, and presses the touchpad's button by mistake. For the same reason, we don't send BUTTON_PRESS events for buttons that are still down after a gesture, as this will likely result in unintended clicks. Another approach I considered was to send BUTTON_PRESS and _RELEASE events when the button state changes, and then be smart about when we send DOWN and UP so that the stream is always in a DOWN state when either a button is pressed or there's a fake finger from a gesture present. This would give a more accurate representation of the actual input, but could result in some pretty unusual event sequences. For example, if a button is pressed during a three-finger swipe but doesn't lift until after the swipe ends, you'd have a period after the two POINTER_UPs at the end of the swipe but before the final UP (when the button lifts), during which you might even have MOVEs, and the classification would have to change at the last POINTER_UP, which isn't done anywhere else. So the number of event sequences that consumers have to support (or at least not behave badly when they receive) goes way up. This would also be more complex to implement. Bug: 372571823 Test: manually play around with various gestures while pressing and releasing the button, and check that the pointer doesn't lock up Test: replay the recording from b/372571823#comment3, and check the pointer doesn't lock up: $ adb shell uinput - < recording.evemu Test: patch change I59b886bfb632f0f26ee58c40f82f447f5ea59b41, then repeat the above steps with the input verifier enabled and check there are no crashes: $ adb shell 'stop && setprop log.tag.InputDispatcherVerifyEvents \ DEBUG && start' Test: $ atest --host \ frameworks/native/services/inputflinger/tests/GestureConverter_test.cpp Test: $ atest --test-filter='GestureAndButtonInterleavings*' \ --host inputflinger_tests (both of these test commands must be run due to b/391854943) Flag: EXEMPT bug fix Change-Id: I2c6f8f268fe804f187d972ab2170ea9b45374c64
1 parent bdb3bc4 commit 53a0e70

3 files changed

Lines changed: 181 additions & 8 deletions

File tree

‎services/inputflinger/reader/mapper/gestures/GestureConverter.cpp‎

Lines changed: 39 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,15 @@
1414
* limitations under the License.
1515
*/
1616

17+
#include "../Macros.h"
18+
1719
#include "gestures/GestureConverter.h"
1820

21+
#include <ios>
1922
#include <optional>
2023
#include <sstream>
2124

25+
#include <android-base/logging.h>
2226
#include <android-base/stringprintf.h>
2327
#include <com_android_input_flags.h>
2428
#include <ftl/enum.h>
@@ -250,6 +254,18 @@ std::list<NotifyArgs> GestureConverter::handleButtonsChange(nsecs_t when, nsecs_
250254
const Gesture& gesture) {
251255
std::list<NotifyArgs> out = {};
252256

257+
if (mCurrentClassification != MotionClassification::NONE) {
258+
// Handling button changes during an ongoing gesture would be tricky, as we'd have to avoid
259+
// sending duplicate DOWN events or premature UP events (e.g. if the gesture ended but the
260+
// button was still down). It would also make handling touchpad events more difficult for
261+
// apps, which would have to handle cases where e.g. a scroll gesture ends (and therefore
262+
// the event lose the TWO_FINGER_SWIPE classification) but there isn't an UP because the
263+
// button's still down. It's unclear how one should even handle button changes during most
264+
// gestures, and they're probably accidental anyway. So, instead, just ignore them.
265+
LOG(INFO) << "Ignoring button change because a gesture is ongoing.";
266+
return out;
267+
}
268+
253269
PointerCoords coords;
254270
coords.clear();
255271
coords.setAxisValue(AMOTION_EVENT_AXIS_RELATIVE_X, 0);
@@ -312,6 +328,15 @@ std::list<NotifyArgs> GestureConverter::handleButtonsChange(nsecs_t when, nsecs_
312328
for (uint32_t button = 1; button <= GESTURES_BUTTON_FORWARD; button <<= 1) {
313329
if (buttonsReleased & button) {
314330
uint32_t actionButton = gesturesButtonToMotionEventButton(button);
331+
if (!(newButtonState & actionButton)) {
332+
// We must have received the ButtonsChange gesture that put this button down during
333+
// another gesture, and therefore dropped the BUTTON_PRESS action for it, or
334+
// released the button when another gesture began during its press. Drop the
335+
// BUTTON_RELEASE too to keep the stream consistent.
336+
LOG(INFO) << "Dropping release event for button 0x" << std::hex << actionButton
337+
<< " as it wasn't in the button state.";
338+
continue;
339+
}
315340
newButtonState &= ~actionButton;
316341
out.push_back(makeMotionArgs(when, readTime, AMOTION_EVENT_ACTION_BUTTON_RELEASE,
317342
actionButton, newButtonState, /* pointerCount= */ 1,
@@ -362,7 +387,7 @@ std::list<NotifyArgs> GestureConverter::handleScroll(nsecs_t when, nsecs_t readT
362387
std::list<NotifyArgs> out;
363388
PointerCoords& coords = mFakeFingerCoords[0];
364389
if (mCurrentClassification != MotionClassification::TWO_FINGER_SWIPE) {
365-
out += exitHover(when, readTime);
390+
out += prepareForFakeFingerGesture(when, readTime);
366391

367392
mCurrentClassification = MotionClassification::TWO_FINGER_SWIPE;
368393
coords.clear();
@@ -421,7 +446,7 @@ std::list<NotifyArgs> GestureConverter::handleFling(nsecs_t when, nsecs_t readTi
421446
std::list<NotifyArgs> out;
422447
mDownTime = when;
423448
mCurrentClassification = MotionClassification::TWO_FINGER_SWIPE;
424-
out += exitHover(when, readTime);
449+
out += prepareForFakeFingerGesture(when, readTime);
425450
out.push_back(makeMotionArgs(when, readTime, AMOTION_EVENT_ACTION_DOWN,
426451
/*actionButton=*/0, /*buttonState=*/0,
427452
/*pointerCount=*/1, &coords));
@@ -479,7 +504,7 @@ std::list<NotifyArgs> GestureConverter::endScroll(nsecs_t when, nsecs_t readTime
479504
// separate swipes with an appropriate lift event between them, so we don't have to worry
480505
// about the finger count changing mid-swipe.
481506

482-
out += exitHover(when, readTime);
507+
out += prepareForFakeFingerGesture(when, readTime);
483508

484509
mCurrentClassification = MotionClassification::MULTI_FINGER_SWIPE;
485510

@@ -567,9 +592,7 @@ std::list<NotifyArgs> GestureConverter::endScroll(nsecs_t when, nsecs_t readTime
567592
LOG_ALWAYS_FATAL_IF(gesture.details.pinch.zoom_state != GESTURES_ZOOM_START,
568593
"First pinch gesture does not have the START zoom state (%d instead).",
569594
gesture.details.pinch.zoom_state);
570-
std::list<NotifyArgs> out;
571-
572-
out += exitHover(when, readTime);
595+
std::list<NotifyArgs> out = prepareForFakeFingerGesture(when, readTime);
573596

574597
mCurrentClassification = MotionClassification::PINCH;
575598
mPinchFingerSeparation = INITIAL_PINCH_SEPARATION_PX;
@@ -644,6 +667,16 @@ std::list<NotifyArgs> GestureConverter::exitHover(nsecs_t when, nsecs_t readTime
644667
}
645668
}
646669

670+
std::list<NotifyArgs> GestureConverter::prepareForFakeFingerGesture(nsecs_t when,
671+
nsecs_t readTime) {
672+
std::list<NotifyArgs> out;
673+
if (isPointerDown(mButtonState)) {
674+
out += releaseAllButtons(when, readTime);
675+
}
676+
out += exitHover(when, readTime);
677+
return out;
678+
}
679+
647680
NotifyMotionArgs GestureConverter::makeHoverEvent(nsecs_t when, nsecs_t readTime, int32_t action) {
648681
PointerCoords coords;
649682
coords.clear();

‎services/inputflinger/reader/mapper/gestures/GestureConverter.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,8 @@ class GestureConverter {
9292
[[nodiscard]] std::list<NotifyArgs> enterHover(nsecs_t when, nsecs_t readTime);
9393
[[nodiscard]] std::list<NotifyArgs> exitHover(nsecs_t when, nsecs_t readTime);
9494

95+
[[nodiscard]] std::list<NotifyArgs> prepareForFakeFingerGesture(nsecs_t when, nsecs_t readTime);
96+
9597
NotifyMotionArgs makeHoverEvent(nsecs_t when, nsecs_t readTime, int32_t action);
9698

9799
NotifyMotionArgs makeMotionArgs(nsecs_t when, nsecs_t readTime, int32_t action,

‎services/inputflinger/tests/GestureConverter_test.cpp‎

Lines changed: 140 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,15 @@
1515
*/
1616

1717
#include <memory>
18+
#include <tuple>
1819

20+
#include <android-base/result-gmock.h>
21+
#include <android-base/result.h>
1922
#include <com_android_input_flags.h>
2023
#include <flag_macros.h>
2124
#include <gestures/GestureConverter.h>
2225
#include <gtest/gtest.h>
26+
#include <input/InputVerifier.h>
2327

2428
#include "FakeEventHub.h"
2529
#include "FakeInputReaderPolicy.h"
@@ -43,8 +47,12 @@ const auto TOUCHPAD_PALM_REJECTION =
4347
const auto TOUCHPAD_PALM_REJECTION_V2 =
4448
ACONFIG_FLAG(input_flags, enable_v2_touchpad_typing_palm_rejection);
4549

50+
constexpr stime_t ARBITRARY_GESTURE_TIME = 1.2;
51+
constexpr stime_t GESTURE_TIME = ARBITRARY_GESTURE_TIME;
52+
4653
} // namespace
4754

55+
using android::base::testing::Ok;
4856
using testing::AllOf;
4957
using testing::Each;
5058
using testing::ElementsAre;
@@ -55,9 +63,8 @@ class GestureConverterTest : public testing::Test {
5563
protected:
5664
static constexpr int32_t DEVICE_ID = END_RESERVED_ID + 1000;
5765
static constexpr int32_t EVENTHUB_ID = 1;
58-
static constexpr stime_t ARBITRARY_GESTURE_TIME = 1.2;
5966

60-
void SetUp() {
67+
GestureConverterTest() {
6168
mFakeEventHub = std::make_unique<FakeEventHub>();
6269
mFakePolicy = sp<FakeInputReaderPolicy>::make();
6370
mFakeListener = std::make_unique<TestInputListener>();
@@ -1698,4 +1705,135 @@ TEST_F_WITH_FLAGS(GestureConverterTest, KeypressCancelsHoverMove,
16981705
WithMotionAction(AMOTION_EVENT_ACTION_HOVER_MOVE))));
16991706
}
17001707

1708+
/**
1709+
* Tests that the event stream output by the converter remains consistent when converting sequences
1710+
* of Gestures interleaved with button presses in various ways. Takes tuples of three Gestures: one
1711+
* that starts the gesture sequence, one that continues it (which may or may not be used in a
1712+
* particular test case), and one that ends it.
1713+
*/
1714+
class GestureConverterConsistencyTest
1715+
: public GestureConverterTest,
1716+
public testing::WithParamInterface<std::tuple<Gesture, Gesture, Gesture>> {
1717+
protected:
1718+
GestureConverterConsistencyTest()
1719+
: GestureConverterTest(),
1720+
mParamStartGesture(std::get<0>(GetParam())),
1721+
mParamContinueGesture(std::get<1>(GetParam())),
1722+
mParamEndGesture(std::get<2>(GetParam())),
1723+
mDeviceContext(*mDevice, EVENTHUB_ID),
1724+
mConverter(*mReader->getContext(), mDeviceContext, DEVICE_ID),
1725+
mVerifier("Test verifier") {
1726+
mConverter.setDisplayId(ui::LogicalDisplayId::DEFAULT);
1727+
}
1728+
1729+
base::Result<void> processMotionArgs(NotifyMotionArgs arg) {
1730+
return mVerifier.processMovement(arg.deviceId, arg.source, arg.action,
1731+
arg.getPointerCount(), arg.pointerProperties.data(),
1732+
arg.pointerCoords.data(), arg.flags);
1733+
}
1734+
1735+
void verifyArgsFromGesture(const Gesture& gesture, size_t gestureIndex) {
1736+
std::list<NotifyArgs> args =
1737+
mConverter.handleGesture(ARBITRARY_TIME, READ_TIME, ARBITRARY_TIME, gesture);
1738+
for (const NotifyArgs& notifyArg : args) {
1739+
const NotifyMotionArgs& arg = std::get<NotifyMotionArgs>(notifyArg);
1740+
ASSERT_THAT(processMotionArgs(arg), Ok())
1741+
<< "when processing " << arg.dump() << "\nfrom gesture " << gestureIndex << ": "
1742+
<< gesture.String();
1743+
}
1744+
}
1745+
1746+
void verifyArgsFromGestures(const std::vector<Gesture>& gestures) {
1747+
for (size_t i = 0; i < gestures.size(); i++) {
1748+
ASSERT_NO_FATAL_FAILURE(verifyArgsFromGesture(gestures[i], i));
1749+
}
1750+
}
1751+
1752+
Gesture mParamStartGesture;
1753+
Gesture mParamContinueGesture;
1754+
Gesture mParamEndGesture;
1755+
1756+
InputDeviceContext mDeviceContext;
1757+
GestureConverter mConverter;
1758+
InputVerifier mVerifier;
1759+
};
1760+
1761+
TEST_P(GestureConverterConsistencyTest, ButtonChangesDuringGesture) {
1762+
verifyArgsFromGestures({
1763+
mParamStartGesture,
1764+
Gesture(kGestureButtonsChange, GESTURE_TIME, GESTURE_TIME,
1765+
/*down=*/GESTURES_BUTTON_LEFT, /*up=*/GESTURES_BUTTON_NONE, /*is_tap=*/false),
1766+
mParamContinueGesture,
1767+
Gesture(kGestureButtonsChange, GESTURE_TIME, GESTURE_TIME,
1768+
/*down=*/GESTURES_BUTTON_NONE, /*up=*/GESTURES_BUTTON_LEFT, /*is_tap=*/false),
1769+
mParamEndGesture,
1770+
});
1771+
}
1772+
1773+
TEST_P(GestureConverterConsistencyTest, ButtonDownDuringGestureAndUpAfterEnd) {
1774+
verifyArgsFromGestures({
1775+
mParamStartGesture,
1776+
Gesture(kGestureButtonsChange, GESTURE_TIME, GESTURE_TIME,
1777+
/*down=*/GESTURES_BUTTON_LEFT, /*up=*/GESTURES_BUTTON_NONE, /*is_tap=*/false),
1778+
mParamContinueGesture,
1779+
mParamEndGesture,
1780+
Gesture(kGestureButtonsChange, GESTURE_TIME, GESTURE_TIME,
1781+
/*down=*/GESTURES_BUTTON_NONE, /*up=*/GESTURES_BUTTON_LEFT, /*is_tap=*/false),
1782+
});
1783+
}
1784+
1785+
TEST_P(GestureConverterConsistencyTest, GestureStartAndEndDuringButtonDown) {
1786+
verifyArgsFromGestures({
1787+
Gesture(kGestureButtonsChange, GESTURE_TIME, GESTURE_TIME,
1788+
/*down=*/GESTURES_BUTTON_LEFT, /*up=*/GESTURES_BUTTON_NONE, /*is_tap=*/false),
1789+
mParamStartGesture,
1790+
mParamContinueGesture,
1791+
mParamEndGesture,
1792+
Gesture(kGestureButtonsChange, GESTURE_TIME, GESTURE_TIME,
1793+
/*down=*/GESTURES_BUTTON_NONE, /*up=*/GESTURES_BUTTON_LEFT, /*is_tap=*/false),
1794+
});
1795+
}
1796+
1797+
TEST_P(GestureConverterConsistencyTest, GestureStartsWhileButtonDownAndEndsAfterUp) {
1798+
verifyArgsFromGestures({
1799+
Gesture(kGestureButtonsChange, GESTURE_TIME, GESTURE_TIME,
1800+
/*down=*/GESTURES_BUTTON_LEFT, /*up=*/GESTURES_BUTTON_NONE, /*is_tap=*/false),
1801+
mParamStartGesture,
1802+
mParamContinueGesture,
1803+
Gesture(kGestureButtonsChange, GESTURE_TIME, GESTURE_TIME,
1804+
/*down=*/GESTURES_BUTTON_NONE, /*up=*/GESTURES_BUTTON_LEFT, /*is_tap=*/false),
1805+
mParamEndGesture,
1806+
});
1807+
}
1808+
1809+
TEST_P(GestureConverterConsistencyTest, TapToClickDuringGesture) {
1810+
verifyArgsFromGestures({
1811+
mParamStartGesture,
1812+
Gesture(kGestureButtonsChange, GESTURE_TIME, GESTURE_TIME,
1813+
/*down=*/GESTURES_BUTTON_LEFT, /*up=*/GESTURES_BUTTON_LEFT, /*is_tap=*/false),
1814+
mParamEndGesture,
1815+
});
1816+
}
1817+
1818+
INSTANTIATE_TEST_SUITE_P(
1819+
GestureAndButtonInterleavings, GestureConverterConsistencyTest,
1820+
testing::Values(
1821+
std::make_tuple(Gesture(kGestureScroll, GESTURE_TIME, GESTURE_TIME, 0, -10),
1822+
Gesture(kGestureScroll, GESTURE_TIME, GESTURE_TIME, 0, -5),
1823+
Gesture(kGestureFling, GESTURE_TIME, GESTURE_TIME, 1, 1,
1824+
GESTURES_FLING_START)),
1825+
std::make_tuple(Gesture(kGestureSwipe, GESTURE_TIME, GESTURE_TIME, 0, -10),
1826+
Gesture(kGestureSwipe, GESTURE_TIME, GESTURE_TIME, 0, 5),
1827+
Gesture(kGestureSwipeLift, GESTURE_TIME, GESTURE_TIME)),
1828+
std::make_tuple(Gesture(kGestureFourFingerSwipe, GESTURE_TIME, GESTURE_TIME, 0,
1829+
-10),
1830+
Gesture(kGestureFourFingerSwipe, GESTURE_TIME, GESTURE_TIME, 0, 5),
1831+
Gesture(kGestureFourFingerSwipeLift, GESTURE_TIME, GESTURE_TIME)),
1832+
std::make_tuple(Gesture(kGesturePinch, GESTURE_TIME, GESTURE_TIME,
1833+
/*dz=*/1, GESTURES_ZOOM_START),
1834+
Gesture(kGesturePinch, GESTURE_TIME, GESTURE_TIME,
1835+
/*dz=*/0.8, GESTURES_ZOOM_UPDATE),
1836+
Gesture(kGesturePinch, GESTURE_TIME, GESTURE_TIME,
1837+
/*dz=*/1, GESTURES_ZOOM_END))));
1838+
17011839
} // namespace android

0 commit comments

Comments
 (0)