Created attachment 96567 [details] evemu recording illustrating the unintended right-click Currently, libinput applies the logic that a two-finger tap means a right-click. Let's view this in more detail. There are in fact two kinds of two-finger taps. Kind A: Finger 0 placed. Finger 1 placed. Finger 1 removed. Finger 0 removed. Kind B: Finger 0 placed. Finger 1 placed. Finger 0 removed. Finger 1 removed. Currently, both kinds of the event sequences are registered as two-finger taps if the time period with both fingers present is short enough. Note that the last event does not matter for either kind, as it happens after libinput has already registered the tap. I would argue that it is the wrong thing to do, on the following grounds. People have two use cases for two-finger taps. Use case A: Initially, no fingers are on the touchpad. A user already has the pointer positioned correctly and wants to right-click just where it is. So one just taps with two fingers. The whole sequence of events is short, and sometimes it ends up as Kind A, sometimes as Kind B. Use case B: A user already has a finger on the touchpad, positioning the cursor. It takes some time. Then he/she wants to right-click. So the user places and quickly removes the second finger. Result: event sequence of Kind A, with a relatively long time (and quite possibly some movements) between the first and the second event. And now consider use case C, where a right-click is not wanted: A user is currently positioning the cursor, with the intention to left-click using a hardware clickpad button and move the cursor no further. The actions of removing the finger that moved the cursor and pressing the button with the other finger happen concurrently. Result: sometimes a sequence of events where two fingers are never on the touchpad simultaneously, sometimes a sequence of Kind B, with a relatively long time (and quite possibly some movements) between the first and the second event. The bug is that use case C is sometimes registered as a sequnce of a right-click and a left-click, instead of only the left-click. The attached recording illustrates the problem at its end. Proposal: if a finger has been on the touchpad for a sufficiently long time or has moved a long distance, make its removal ineligible for registering the two-finger tap.
(In reply to comment #0) > Proposal: if a finger has been on the touchpad for a sufficiently long time > or has moved a long distance, make its removal ineligible for registering > the two-finger tap. I certainly like that idea. One question though: do you see this when using the software button or generally on the touchpad area?
That's a trick question. All or almost all wrong events were related to the software buttons. However, that's because all or almost all wrong events (not only related to this bug) were related to the use of two hands on the touchpad, and because the only use case for two hands that is relevant for me is related to software buttons.
hmm, actually, I have to ask: did you try the latest libinput branch for this? fc51daff73960d96d5500c54d1223f4ffc47a6c1 was supposed to have fixed these things.
No, I am not on the latest git. Will retest later today and report.
Well, the patch does eliminate the most common case. However, I cannot consider it fully fixed. I will attach a (completely bogus, because nobody who doesn't know about this bug will do this sequence of touches) recording that does not contain any physical button clicks but leads to the unwanted right-click being registered.
Created attachment 101370 [details] Another recording illustrating the unsanted right click Well, I was wrong about the "nobody will do this" part. Decided to click the submit button (using a physical button), then caught myself thinking about something else with my finger on the software button, and with the context menu popped up because of this bug.
Even when I intend to click using the hardware button, the popup menu sometimes shows up for a brief moment.
I see the issue now. The tapping code simply hooks onto the time between finger releases, but it doesn't do it per finger. In your recording, you have a sequence of: first finger down first finger move/hold second finger down first finger up ... And because the tapping code sees the second finger down and first finger up as a down+up within the tap time, it triggers the event. That is definitely a bug.
Created attachment 101416 [details] [review] 0001-touchpad-disable-tapping-for-fingers-exceeding-the-t.patch This is a tentative patch, I've updated the state machine but still need to double-check everything. It needs some other patch but to test it alone, just change the msleep(foo) to usleep(foo * 1000) or ignore the build failure in test/touchpad.c Hans: change here is to add a 3-state machine to each touch: IDLE, TOUCH, DEAD. A touch goes into TOUCH first, whenever it goes over the threshold/timeout it goes DEAD. And we only send tap-generated button events if the state isn't DEAD. The state diagram in draw.io was updated, please double-check for me as well if that makes sense. It's missing the -> DEAD state transition on phys button presses, the rest should be there.
I have not looked at the code and have not tested it yet, but have looked at the tests. They make sense. Also it is good that the existing touchpad_2fg_tap test releases fingers in the same order as it places them on the touchpad. Maybe it is a good idea to add one more test like this for more complete coverage of the use cases mentioned in this bug: START_TEST(touchpad_2fg_tap_reordered) { struct litest_device *dev = litest_current_device(); struct libinput *li = dev->libinput; litest_drain_events(dev->libinput); litest_touch_down(dev, 0, 50, 50); litest_touch_down(dev, 1, 70, 70); litest_touch_up(dev, 1); litest_touch_up(dev, 0); libinput_dispatch(li); assert_button_event(li, BTN_RIGHT, LIBINPUT_BUTTON_STATE_PRESSED); usleep(300000); /* tap-n-drag timeout */ assert_button_event(li, BTN_RIGHT, LIBINPUT_BUTTON_STATE_RELEASED); litest_assert_empty_queue(li); } END_TEST
Yep, I've got exactly that test in my tree already, though I named it _inverted :) thanks though.
hi Peter, (In reply to comment #9) > Created attachment 101416 [details] [review] [review] > 0001-touchpad-disable-tapping-for-fingers-exceeding-the-t.patch > > This is a tentative patch, I've updated the state machine but still need to > double-check everything. It needs some other patch but to test it alone, > just change the msleep(foo) to usleep(foo * 1000) or ignore the build > failure in test/touchpad.c I've reviewed the patch and it looks good to me. > Hans: change here is to add a 3-state machine to each touch: IDLE, TOUCH, > DEAD. A touch goes into TOUCH first, whenever it goes over the > threshold/timeout it goes DEAD. And we only send tap-generated button events > if the state isn't DEAD. The state diagram in draw.io was updated, please > double-check for me as well if that makes sense. It's missing the -> DEAD > state transition on phys button presses, the rest should be there. I don't have access to the touchpad-tap-state-machine.svg equivalent on draw.io, only to the touchpad-softbutton-state-machine.svg diagram. If you can send me a link to the touchpad-tap-state-machine diagram I'll galdly take a look. Regards, Hans
Testing confirms that the bug is fixed, and nothing else became obviously broken.
Created attachment 101457 [details] Triple-tap that confused libinput Spoke too soon. I was able to confuse libinput's state machine by placing three fingers on a touchpad (which tranks two fingers but reports BTN_TOOL_TRIPLETAP) at the same time. Result: unwanted right-click, even though I have never removed any of these three fingers. While technically this is a different bug, let's attach the trace here.
Created attachment 101459 [details] Another instance of triple-tap, more clear In the attached trace, the event at 0.149926 illustrates the situation much better. E: 0.125195 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- E: 0.149926 0003 0039 -001 # EV_ABS / ABS_MT_TRACKING_ID -1 E: 0.149926 0001 014d 0000 # EV_KEY / BTN_TOOL_DOUBLETAP 0 E: 0.149926 0001 014e 0001 # EV_KEY / BTN_TOOL_TRIPLETAP 1 E: 0.149926 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- I.e., while reporting a triple-tap, the touchpad removes one of the tracking IDs, even though the finger is still on the touchpad. This gets counted as a touch-up and leads to the mis-recognition of the two-finger short-tap gesture.
(In reply to comment #12) > I don't have access to the touchpad-tap-state-machine.svg equivalent on > draw.io, only to the touchpad-softbutton-state-machine.svg diagram. If you > can send me a link to the touchpad-tap-state-machine diagram I'll galdly > take a look. I've enabled edit access for you, the URL is in the ...tap.c file at the top: https://drive.google.com/file/d/0B1NwWmji69noYTdMcU1kTUZuUVE/edit?usp=sharing Should work, but double-check to be sure please. (In reply to comment #15) > Created attachment 101459 [details] > Another instance of triple-tap, more clear > > In the attached trace, the event at 0.149926 illustrates the situation much > better. > > E: 0.125195 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- > E: 0.149926 0003 0039 -001 # EV_ABS / ABS_MT_TRACKING_ID -1 > E: 0.149926 0001 014d 0000 # EV_KEY / BTN_TOOL_DOUBLETAP 0 > E: 0.149926 0001 014e 0001 # EV_KEY / BTN_TOOL_TRIPLETAP 1 > E: 0.149926 0000 0000 0000 # ------------ SYN_REPORT (0) ---------- > > I.e., while reporting a triple-tap, the touchpad removes one of the tracking > IDs, even though the finger is still on the touchpad. This gets counted as a > touch-up and leads to the mis-recognition of the two-finger short-tap > gesture. Good point, I forgot about BTN_TOOL_TRIPLETAP, that's simply not handled in this version of the patch.
(In reply to comment #15) > I.e., while reporting a triple-tap, the touchpad removes one of the tracking > IDs, even though the finger is still on the touchpad. This gets counted as a > touch-up and leads to the mis-recognition of the two-finger short-tap > gesture. sorry, I should've been more verbose here: we currently don't handle BTN_TOOL_TRIPLETAP on touchpads that provide ABS_MT_SLOT (and thus real touchpoints). Hans is looking into that, but right now the tapping code merely sees a touch up from the event sequence above - it doesn't see the third finger at all. I've filed bug 80368 to keep track of that, if you don't mind I'd like to keep to the timing issue only. If the tripletap clicks still happen when bug 80368 is fixed, please file a separate bug for that.
commit 8fa5d0bf51083756e1a7ead11afae490a09542ab Author: Peter Hutterer <peter.hutterer@who-t.net> Date: Fri Jun 20 14:16:13 2014 +1000 touchpad: disable tapping for fingers exceeding the timeout/motion threshold
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.