Skip to content

Commit a4e9f70

Browse files
HarryCuttsHarryCutts
committed
CursorInputMapper: fix simultaneous button and move handling
Previously, if we received movement (REL_X and REL_Y) as well as button changes (BTN_*) from a mouse in the same evdev frame (i.e. not separated by a SYN_REPORT), we'd add the relative movements to the DOWN or UP event (as well as the BUTTON_PRESS or BUTTON_RELEASE events, which I'm pretty sure was an additional bug), without adding a HOVER_MOVE or MOVE event. This is invalid, and would cause the dispatcher to crash. As a side effect of fixing this, we no longer emit extra MOVE or HOVER_MOVE events for frames which only contain button changes. Test: $ atest --host inputflinger_tests Test: $ atest CtsInputTestCases Test: $ atest CtsHardwareTestCases_cts_tests:android.hardware.input.cts.tests.VirtualMouseTest Test: $ atest CtsHardwareTestCases_cts_tests:android.hardware.input.cts.tests.VirtualDeviceMirrorDisplayTest Test: enable the verifier and use a mouse for a while (including clicking and dragging) Bug: 405820964 Flag: EXEMPT bug fix Change-Id: Iabadc3be9594c5b56c26aaeae6082254527060de
1 parent 509f629 commit a4e9f70

File tree

2 files changed

+114
-51
lines changed

2 files changed

+114
-51
lines changed

‎services/inputflinger/reader/mapper/CursorInputMapper.cpp‎

Lines changed: 47 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -272,9 +272,6 @@ std::list<NotifyArgs> CursorInputMapper::sync(nsecs_t when, nsecs_t readTime) {
272272
pointerProperties.id = 0;
273273
pointerProperties.toolType = ToolType::MOUSE;
274274

275-
PointerCoords pointerCoords;
276-
pointerCoords.clear();
277-
278275
// A negative value represents inverted scrolling direction.
279276
// Applies only if the source is a mouse.
280277
const bool isMouse =
@@ -291,18 +288,6 @@ std::list<NotifyArgs> CursorInputMapper::sync(nsecs_t when, nsecs_t readTime) {
291288

292289
float xCursorPosition = AMOTION_EVENT_INVALID_CURSOR_POSITION;
293290
float yCursorPosition = AMOTION_EVENT_INVALID_CURSOR_POSITION;
294-
if (mSource == AINPUT_SOURCE_MOUSE) {
295-
pointerCoords.setAxisValue(AMOTION_EVENT_AXIS_RELATIVE_X, deltaX);
296-
pointerCoords.setAxisValue(AMOTION_EVENT_AXIS_RELATIVE_Y, deltaY);
297-
} else {
298-
// Pointer capture and navigation modes
299-
pointerCoords.setAxisValue(AMOTION_EVENT_AXIS_X, deltaX);
300-
pointerCoords.setAxisValue(AMOTION_EVENT_AXIS_Y, deltaY);
301-
pointerCoords.setAxisValue(AMOTION_EVENT_AXIS_RELATIVE_X, deltaX);
302-
pointerCoords.setAxisValue(AMOTION_EVENT_AXIS_RELATIVE_Y, deltaY);
303-
}
304-
305-
pointerCoords.setAxisValue(AMOTION_EVENT_AXIS_PRESSURE, down ? 1.0f : 0.0f);
306291

307292
// Moving an external trackball or mouse should wake the device.
308293
// We don't do this for internal cursor devices to prevent them from waking up
@@ -313,23 +298,48 @@ std::list<NotifyArgs> CursorInputMapper::sync(nsecs_t when, nsecs_t readTime) {
313298
policyFlags |= POLICY_FLAG_WAKE;
314299
}
315300

301+
const int32_t metaState = getContext()->getGlobalMetaState();
302+
// Send an event for the cursor motion.
303+
// TODO(b/407018146): we currently need to send a zero-delta HOVER_MOVE before scrolls so that
304+
// InputDispatcher generates a HOVER_ENTER event if necessary. (The VirtualMouseTest in CTS
305+
// depends on this.) Fix this issue then remove `|| scrolled` here.
306+
if (moved || scrolled) {
307+
int32_t action = wasDown || mSource != AINPUT_SOURCE_MOUSE
308+
? AMOTION_EVENT_ACTION_MOVE
309+
: AMOTION_EVENT_ACTION_HOVER_MOVE;
310+
311+
PointerCoords moveCoords;
312+
moveCoords.clear();
313+
moveCoords.setAxisValue(AMOTION_EVENT_AXIS_RELATIVE_X, deltaX);
314+
moveCoords.setAxisValue(AMOTION_EVENT_AXIS_RELATIVE_Y, deltaY);
315+
if (mSource != AINPUT_SOURCE_MOUSE) {
316+
// Pointer capture and navigation modes
317+
moveCoords.setAxisValue(AMOTION_EVENT_AXIS_X, deltaX);
318+
moveCoords.setAxisValue(AMOTION_EVENT_AXIS_Y, deltaY);
319+
}
320+
321+
moveCoords.setAxisValue(AMOTION_EVENT_AXIS_PRESSURE, wasDown ? 1.0f : 0.0f);
322+
323+
out.push_back(NotifyMotionArgs(getContext()->getNextId(), when, readTime, getDeviceId(),
324+
mSource, *mDisplayId, policyFlags, action, 0, 0, metaState,
325+
lastButtonState, MotionClassification::NONE, 1,
326+
&pointerProperties, &moveCoords, mXPrecision, mYPrecision,
327+
xCursorPosition, yCursorPosition, downTime,
328+
/*videoFrames=*/{}));
329+
}
330+
316331
// Synthesize key down from buttons if needed.
317332
out += synthesizeButtonKeys(getContext(), AKEY_EVENT_ACTION_DOWN, when, readTime, getDeviceId(),
318333
mSource, *mDisplayId, policyFlags, lastButtonState,
319334
currentButtonState);
320335

321-
// Send motion event.
322-
if (downChanged || moved || scrolled || buttonsChanged) {
323-
int32_t metaState = getContext()->getGlobalMetaState();
336+
// Send motion events for buttons and scrolling.
337+
if (downChanged || scrolled || buttonsChanged) {
324338
int32_t buttonState = lastButtonState;
325-
int32_t motionEventAction;
326-
if (downChanged) {
327-
motionEventAction = down ? AMOTION_EVENT_ACTION_DOWN : AMOTION_EVENT_ACTION_UP;
328-
} else if (down || (mSource != AINPUT_SOURCE_MOUSE)) {
329-
motionEventAction = AMOTION_EVENT_ACTION_MOVE;
330-
} else {
331-
motionEventAction = AMOTION_EVENT_ACTION_HOVER_MOVE;
332-
}
339+
340+
PointerCoords pointerCoords;
341+
pointerCoords.clear();
342+
pointerCoords.setAxisValue(AMOTION_EVENT_AXIS_PRESSURE, down ? 1.0f : 0.0f);
333343

334344
if (buttonsReleased) {
335345
BitSet32 released(buttonsReleased);
@@ -346,12 +356,16 @@ std::list<NotifyArgs> CursorInputMapper::sync(nsecs_t when, nsecs_t readTime) {
346356
}
347357
}
348358

349-
out.push_back(NotifyMotionArgs(getContext()->getNextId(), when, readTime, getDeviceId(),
350-
mSource, *mDisplayId, policyFlags, motionEventAction, 0, 0,
351-
metaState, currentButtonState, MotionClassification::NONE, 1,
352-
&pointerProperties, &pointerCoords, mXPrecision, mYPrecision,
353-
xCursorPosition, yCursorPosition, downTime,
354-
/*videoFrames=*/{}));
359+
if (downChanged) {
360+
int32_t action = down ? AMOTION_EVENT_ACTION_DOWN : AMOTION_EVENT_ACTION_UP;
361+
out.push_back(NotifyMotionArgs(getContext()->getNextId(), when, readTime, getDeviceId(),
362+
mSource, *mDisplayId, policyFlags, action, 0, 0,
363+
metaState, currentButtonState,
364+
MotionClassification::NONE, 1, &pointerProperties,
365+
&pointerCoords, mXPrecision, mYPrecision,
366+
xCursorPosition, yCursorPosition, downTime,
367+
/*videoFrames=*/{}));
368+
}
355369

356370
if (buttonsPressed) {
357371
BitSet32 pressed(buttonsPressed);
@@ -371,7 +385,7 @@ std::list<NotifyArgs> CursorInputMapper::sync(nsecs_t when, nsecs_t readTime) {
371385
ALOG_ASSERT(buttonState == currentButtonState);
372386

373387
// Send hover move after UP to tell the application that the mouse is hovering now.
374-
if (motionEventAction == AMOTION_EVENT_ACTION_UP && (mSource == AINPUT_SOURCE_MOUSE)) {
388+
if (downChanged && !down && (mSource == AINPUT_SOURCE_MOUSE)) {
375389
out.push_back(NotifyMotionArgs(getContext()->getNextId(), when, readTime, getDeviceId(),
376390
mSource, *mDisplayId, policyFlags,
377391
AMOTION_EVENT_ACTION_HOVER_MOVE, 0, 0, metaState,

‎services/inputflinger/tests/CursorInputMapper_test.cpp‎

Lines changed: 67 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,8 @@ constexpr auto HOVER_MOVE = AMOTION_EVENT_ACTION_HOVER_MOVE;
5757
constexpr auto INVALID_CURSOR_POSITION = AMOTION_EVENT_INVALID_CURSOR_POSITION;
5858
constexpr auto AXIS_X = AMOTION_EVENT_AXIS_X;
5959
constexpr auto AXIS_Y = AMOTION_EVENT_AXIS_Y;
60+
constexpr auto AXIS_RELATIVE_X = AMOTION_EVENT_AXIS_RELATIVE_X;
61+
constexpr auto AXIS_RELATIVE_Y = AMOTION_EVENT_AXIS_RELATIVE_Y;
6062
constexpr ui::LogicalDisplayId DISPLAY_ID = ui::LogicalDisplayId::DEFAULT;
6163
constexpr ui::LogicalDisplayId SECONDARY_DISPLAY_ID = ui::LogicalDisplayId{DISPLAY_ID.val() + 1};
6264
constexpr int32_t DISPLAY_WIDTH = 480;
@@ -261,6 +263,59 @@ TEST_F(CursorInputMapperUnitTest, HoverAndLeftButtonPress) {
261263
VariantWith<NotifyMotionArgs>(WithMotionAction(HOVER_MOVE))));
262264
}
263265

266+
TEST_F(CursorInputMapperUnitTest, MoveAndButtonChangeInSameFrame) {
267+
createMapper();
268+
std::list<NotifyArgs> args;
269+
270+
// Move the cursor and press the button
271+
args += process(EV_REL, REL_X, -10);
272+
args += process(EV_REL, REL_Y, 20);
273+
args += process(EV_KEY, BTN_LEFT, 1);
274+
args += process(EV_SYN, SYN_REPORT, 0);
275+
ASSERT_THAT(args,
276+
ElementsAre(VariantWith<NotifyMotionArgs>(
277+
AllOf(WithMotionAction(HOVER_MOVE), WithButtonState(0),
278+
WithNegativeAxis(AXIS_RELATIVE_X),
279+
WithPositiveAxis(AXIS_RELATIVE_Y), WithPressure(0.0f))),
280+
VariantWith<NotifyMotionArgs>(
281+
AllOf(WithMotionAction(ACTION_DOWN),
282+
WithButtonState(AMOTION_EVENT_BUTTON_PRIMARY),
283+
WithZeroAxis(AXIS_RELATIVE_X),
284+
WithZeroAxis(AXIS_RELATIVE_Y), WithPressure(1.0f))),
285+
VariantWith<NotifyMotionArgs>(
286+
AllOf(WithMotionAction(BUTTON_PRESS),
287+
WithActionButton(AMOTION_EVENT_BUTTON_PRIMARY),
288+
WithButtonState(AMOTION_EVENT_BUTTON_PRIMARY),
289+
WithZeroAxis(AXIS_RELATIVE_X),
290+
WithZeroAxis(AXIS_RELATIVE_Y), WithPressure(1.0f)))));
291+
292+
// Move some more and release the button
293+
args.clear();
294+
args += process(EV_REL, REL_X, 10);
295+
args += process(EV_REL, REL_Y, -5);
296+
args += process(EV_KEY, BTN_LEFT, 0);
297+
args += process(EV_SYN, SYN_REPORT, 0);
298+
ASSERT_THAT(args,
299+
ElementsAre(VariantWith<NotifyMotionArgs>(
300+
AllOf(WithMotionAction(ACTION_MOVE),
301+
WithButtonState(AMOTION_EVENT_BUTTON_PRIMARY),
302+
WithPositiveAxis(AXIS_RELATIVE_X),
303+
WithNegativeAxis(AXIS_RELATIVE_Y), WithPressure(1.0f))),
304+
VariantWith<NotifyMotionArgs>(
305+
AllOf(WithMotionAction(BUTTON_RELEASE),
306+
WithActionButton(AMOTION_EVENT_BUTTON_PRIMARY),
307+
WithButtonState(0), WithZeroAxis(AXIS_RELATIVE_X),
308+
WithZeroAxis(AXIS_RELATIVE_Y), WithPressure(0.0f))),
309+
VariantWith<NotifyMotionArgs>(
310+
AllOf(WithMotionAction(ACTION_UP), WithButtonState(0),
311+
WithZeroAxis(AXIS_RELATIVE_X),
312+
WithZeroAxis(AXIS_RELATIVE_Y), WithPressure(0.0f))),
313+
VariantWith<NotifyMotionArgs>(
314+
AllOf(WithMotionAction(HOVER_MOVE), WithButtonState(0),
315+
WithZeroAxis(AXIS_RELATIVE_X),
316+
WithZeroAxis(AXIS_RELATIVE_Y), WithPressure(0.0f)))));
317+
}
318+
264319
/**
265320
* Test that enabling mouse swap primary button will have the left click result in a
266321
* `SECONDARY_BUTTON` event and a right click will result in a `PRIMARY_BUTTON` event.
@@ -539,12 +594,15 @@ TEST_F(CursorInputMapperUnitTest, ProcessShouldHandleCombinedXYAndButtonUpdates)
539594
args += process(ARBITRARY_TIME, EV_KEY, BTN_MOUSE, 1);
540595
args += process(ARBITRARY_TIME, EV_SYN, SYN_REPORT, 0);
541596
EXPECT_THAT(args,
542-
ElementsAre(VariantWith<NotifyMotionArgs>(WithMotionAction(ACTION_DOWN)),
543-
VariantWith<NotifyMotionArgs>(WithMotionAction(BUTTON_PRESS))));
544-
EXPECT_THAT(args,
545-
Each(VariantWith<NotifyMotionArgs>(AllOf(WithPositiveAxis(AXIS_X),
546-
WithNegativeAxis(AXIS_Y),
547-
WithPressure(1.0f)))));
597+
ElementsAre(VariantWith<NotifyMotionArgs>(
598+
AllOf(WithMotionAction(ACTION_MOVE), WithPressure(0.0f),
599+
WithPositiveAxis(AXIS_X), WithNegativeAxis(AXIS_Y))),
600+
VariantWith<NotifyMotionArgs>(
601+
AllOf(WithMotionAction(ACTION_DOWN), WithPressure(1.0f),
602+
WithZeroAxis(AXIS_X), WithZeroAxis(AXIS_Y))),
603+
VariantWith<NotifyMotionArgs>(
604+
AllOf(WithMotionAction(BUTTON_PRESS), WithPressure(1.0f),
605+
WithZeroAxis(AXIS_X), WithZeroAxis(AXIS_Y)))));
548606
args.clear();
549607

550608
// Move X, Y a bit while pressed.
@@ -780,11 +838,9 @@ TEST_F(CursorInputMapperUnitTest, ProcessShouldHandleAllButtonsWithZeroCoords) {
780838
args += process(ARBITRARY_TIME, EV_KEY, BTN_RIGHT, 0);
781839
args += process(ARBITRARY_TIME, EV_SYN, SYN_REPORT, 0);
782840
EXPECT_THAT(args,
783-
ElementsAre(VariantWith<NotifyMotionArgs>(WithMotionAction(BUTTON_RELEASE)),
784-
VariantWith<NotifyMotionArgs>(WithMotionAction(ACTION_MOVE))));
785-
EXPECT_THAT(args,
786-
Each(VariantWith<NotifyMotionArgs>(
787-
AllOf(WithButtonState(AMOTION_EVENT_BUTTON_TERTIARY),
841+
ElementsAre(VariantWith<NotifyMotionArgs>(
842+
AllOf(WithMotionAction(BUTTON_RELEASE),
843+
WithButtonState(AMOTION_EVENT_BUTTON_TERTIARY),
788844
WithCoords(0.0f, 0.0f), WithPressure(1.0f)))));
789845
args.clear();
790846

@@ -817,10 +873,6 @@ TEST_P(CursorInputMapperButtonKeyTest, ProcessShouldHandleButtonKeyWithZeroCoord
817873
EXPECT_THAT(args,
818874
ElementsAre(VariantWith<NotifyKeyArgs>(AllOf(WithKeyAction(AKEY_EVENT_ACTION_DOWN),
819875
WithKeyCode(expectedKeyCode))),
820-
VariantWith<NotifyMotionArgs>(
821-
AllOf(WithMotionAction(HOVER_MOVE),
822-
WithButtonState(expectedButtonState),
823-
WithCoords(0.0f, 0.0f), WithPressure(0.0f))),
824876
VariantWith<NotifyMotionArgs>(
825877
AllOf(WithMotionAction(BUTTON_PRESS),
826878
WithButtonState(expectedButtonState),
@@ -833,9 +885,6 @@ TEST_P(CursorInputMapperButtonKeyTest, ProcessShouldHandleButtonKeyWithZeroCoord
833885
ElementsAre(VariantWith<NotifyMotionArgs>(
834886
AllOf(WithMotionAction(BUTTON_RELEASE), WithButtonState(0),
835887
WithCoords(0.0f, 0.0f), WithPressure(0.0f))),
836-
VariantWith<NotifyMotionArgs>(
837-
AllOf(WithMotionAction(HOVER_MOVE), WithButtonState(0),
838-
WithCoords(0.0f, 0.0f), WithPressure(0.0f))),
839888
VariantWith<NotifyKeyArgs>(AllOf(WithKeyAction(AKEY_EVENT_ACTION_UP),
840889
WithKeyCode(expectedKeyCode)))));
841890
}

0 commit comments

Comments
 (0)