Touchpad is sensitive over physical button area

[Alberto Milone]

Binary package hint: xfree86-driver-synaptics

My Synaptics touchpad has physical buttons below the bottom edge of its surface. As a result, when I try to perform click by pressing the bottom edge of the touchpad I get both a click and a movement. This often causes the click to take place after the movement thus causing me to click on the wrong icon, button, etc.

The driver should allow the deactivation of the area over the physical buttons.

[Alberto Milone (albertomilone)]

As you can see in the attached patch, I have implemented the support for a "MovementBottomEdge" option which, if set through either xorg.conf, synclient or xinput, allows users to blacklist the area of the touchpad below the value set in this option.

For example to set the new property to "4100" through Xinput you will have to type:
xinput set-int-prop "SynPS/2 Synaptics TouchPad" "Synaptics Movement Bottom Edge" 32 4100

to disable it:
xinput set-int-prop "SynPS/2 Synaptics TouchPad" "Synaptics Movement Bottom Edge" 32 0

NOTE: you will have to replace "SynPS/2 Synaptics TouchPad" with the name of your touchpad.

The "MovementBottomEdge" option is set to 0 hence disabled by default, therefore no one will notice a change in the behaviour of their touchpad unless they manually set this option.

[Alberto Milone (albertomilone)]

SRU request:

The debdiff and the package can be found in bug 365943. This patch won't cause regressions (as the option is disabled by default) and will be very useful to the OEM team.

Please accept it in jaunty-proposed.

[Martin Pitt (pitti)]

This looks okay for an SRU, since it is disabled by default. However, before we include this into a stable release, it should be forwarded upstream first, to avoid potential configuration migration if upstream decides to implement this differently.

[Alberto Milone (albertomilone)]

Created an attachment (id=25586)
Patch that adds the MovementBottomEdge property

USECASE:
My Synaptics touchpad has physical buttons below the bottom edge of its surface. As a result, when I try to perform a click by pressing the bottom edge of the touchpad I get both a click and a movement. This often causes the click to take place after the movement thus causing me to click on the wrong icon, button, etc.

PROPOSED SOLUTION:
The driver should allow the deactivation of movements in the area over the physical buttons.

EXPLANATION:
As you can see in the attached patch, I have implemented the support for a "MovementBottomEdge" option which, if set through either xorg.conf, synclient or xinput, allows users to blacklist the area of the touchpad below the value set in this option.

For example to set the new property to "4100" through Xinput you will have to type:
xinput set-int-prop "SynPS/2 Synaptics TouchPad" "Synaptics Movement Bottom Edge" 32 4100

to disable it:
xinput set-int-prop "SynPS/2 Synaptics TouchPad" "Synaptics Movement Bottom Edge" 32 0

The "MovementBottomEdge" property is set to 0 (hence disabled) by default, therefore no one will notice a change in the behaviour of their touchpad unless they manually set this option.

Please consider accepting my patch and let me know if you would like me to change it or rewrite it in a way that you deem more appropriate.

[Peter Hutterer (peter-hutterer)]

Please attach a photo of that touchpad, I'd like to see the physical layout.
Also, what laptop brand and models are affected by this?

[Alberto Milone (albertomilone)]

If you click on the Gallery you will see the photos of the Dell mini 10 laptop:
http://www.dell.com/content/products/productdetails.aspx/laptop-inspiron-10?c=us&cs=19&l=en&s=dhs

[Alberto Milone (albertomilone)]

To be more specific, the model is Dell Mini 10 v.

Any updates, comments, etc. on this?

[Peter Hutterer (peter-hutterer)]

CC'ing henrik and christoph for extra input.

As mentioned on IRC, I'm hesitant to put in properties in for broken hardware. Properties need to be maintained and we have too many already IMO.

Please attach the evtest output when you press each button once.

[Henrik Rydberg (rydberg)]

I am not completely opposed to the idea, but I have a feeling it can be implemented simpler with existing logic. There are already parameters to restrict the movement area to an arbitrary rectangular part of the trackpad. Any chance it could be used to solve your problem?

Henrik

[Alberto Milone (albertomilone)]

Created an attachment (id=26722)
evtest output for the left button

[Alberto Milone (albertomilone)]

Created an attachment (id=26723)
evtest output for the right button

[Alberto Milone (albertomilone)]

@Henrik
If you're referring to "BottomEdge", that's different from the physical bottom edge and seems to influence only horizontal edge scrolling (instead of movement in general).

If you're referring to something else, can you tell me what those parameters are, please?

@Peter
I have attached the output as you requested.

[Henrik Rydberg (rydberg)]

> If you're referring to "BottomEdge", that's different from the physical bottom
> edge and seems to influence only horizontal edge scrolling (instead of movement
> in general).

Yes, it is currently affecting edge scrolling, but if it could also affect movement
in general, would it not solve your problem?

[Alberto Milone (albertomilone)]

What if a user/vendor wants to enable and use horizontal edge scrolling (so that scrolling works in an area above the blacklisted one) and disable the area with the buttons (with MovementBottomEdge) at the same time?

In my opinion this is another use case to take into consideration.

What do you think?

[Peter Hutterer (peter-hutterer)]

I don't think we can use the edges for this feature. The edges have too much
semantic behaviour that we shouldn't break.

A move that starts inside and crosses an edge does not generate scroll events.
If edge scrolling is disabled, the edge settings are effectively ignored. Plus
there is the circular scrolling and corner-coasting interaction that also
complicates things.

This bug seems to affect all Dell Minis, so it's not just a single machine's
problem.

The patch you submitted before is a good start, but I don't want this to be
just on the bottom edge. Sooner or later it will affect other edges too so we
might as well get it right now.

I propose a "Synaptics Area" optional property that allows for all 4
directions to be limited to a given coordinate value.

The semantics get a bit interesting though:
- if a finger moves from the valid to the blacklisted area, does it continue
  in this area or are all coordinates clipped?
- if a finger starts a move in the blacklisted area, does it generate events?
- If a finger taps in the blacklisted area, does it generate events?

[Alberto Milone (albertomilone)]

(In reply to comment #11)
> This bug seems to affect all Dell Minis, so it's not just a single machine's
> problem.
>
> The patch you submitted before is a good start, but I don't want this to be
> just on the bottom edge. Sooner or later it will affect other edges too so we
> might as well get it right now.
>
> I propose a "Synaptics Area" optional property that allows for all 4
> directions to be limited to a given coordinate value.
>

I think your idea is more future-proof than mine and I look forward to working on it.

> The semantics get a bit interesting though:
> - if a finger moves from the valid to the blacklisted area, does it continue
> in this area or are all coordinates clipped?

I think the coordinates should be clipped for the reasons listed below.

> - if a finger starts a move in the blacklisted area, does it generate events?

Yes but, in my opinion, the driver should ignore them as the movement could be caused by (as in the case which my patch covers) an attempt to click or to perform some other action that specific area was meant for by manufacturer.

> - If a finger taps in the blacklisted area, does it generate events?
>

I'm not sure about this. It could make sense to disable finger taps (and finger taps in the corners?) too as we don't know what the blacklisted area is intended to do.

Of course better ideas are welcome.

[Peter Hutterer (peter-hutterer)]

clipping IMO doesn't work. given that your valid range goes from 0 - N, any coordinate beyond N would result in N. so you still get the fake motion event for the buttons.

ignoring events is more interesting but leads to jumps as the last coordinate when you move back into the valid area. not sure what the usability impact of this is.

[Alberto Milone (albertomilone)]

(In reply to comment #13)
> clipping IMO doesn't work. given that your valid range goes from 0 - N, any
> coordinate beyond N would result in N. so you still get the fake motion event
> for the buttons.
>
> ignoring events is more interesting but leads to jumps as the last coordinate
> when you move back into the valid area. not sure what the usability impact of
> this is.
>

What I meant to say is that, for example, if the event takes place in a point below the bottom edge defined by the Synaptics Area property, we can do something similar to what my current patch does:

1) The driver updates hw->y to its real value only if the event didn't take place in the blacklisted area.

2) The driver updates hw->x only if the y event didn't take place in the blacklisted area.

I didn't notice cursor jumps when testing the case that you suggested. Furthermore you would get a jump only if hw->y and hw->x were updated when touching the blacklisted area.

[Richard Hughes (richard-hughes)]

As soon as you've got anything to test, I've got the Dell Mini 10 hardware with F11/rawhide. It's driving me crazy too ;)

[Peter Hutterer (peter-hutterer)]

(In reply to comment #14)
> What I meant to say is that, for example, if the event takes place in a point
> below the bottom edge defined by the Synaptics Area property, we can do
> something similar to what my current patch does:
>
> 1) The driver updates hw->y to its real value only if the event didn't take
> place in the blacklisted area.
>
> 2) The driver updates hw->x only if the y event didn't take place in the
> blacklisted area.
>
> I didn't notice cursor jumps when testing the case that you suggested.
> Furthermore you would get a jump only if hw->y and hw->x were updated when
> touching the blacklisted area.

the scenario I was thinking of is that you move into the right bottom corner, then along in the blacklisted area to the left bottom corner and then up into the allowed area. This would generate a jump the length of the touchpad's width.

We can work around that though. Since by default the touchpad is in relative mode anyway, the only thing that should be capped here are the actual posts of the motion event, not the internal state. So at any point in time, the movement history contains the actual values - including the blacklisted area.

For your patch, this means that you hook in before posting the event and reset dx/dy to zero for blacklisted areas. I think that should do the job.
This simple diff shows what I mean:

iff --git a/src/synaptics.c b/src/synaptics.c
index a40d160..40ee7f2 100644
--- a/src/synaptics.c
+++ b/src/synaptics.c
@@ -2230,6 +2230,9 @@ HandleState(LocalDevicePtr local, struct SynapticsHwState *hw)
            buttons |= tap_mask;
     }

+ /* FIXME: HACK: */
+ if (edge) dx = dy = 0;
+
     /* Post events */
     if (dx || dy)
        xf86PostMotionEvent(local->dev, 0, 0, 2, dx, dy);

If the finger triggers an edge, no motion event is posted (i.e. the edges form the blacklisted area).

[Alberto Milone (albertomilone)]

(In reply to comment #16)
>
> the scenario I was thinking of is that you move into the right bottom corner,
> then along in the blacklisted area to the left bottom corner and then up into
> the allowed area. This would generate a jump the length of the touchpad's
> width.
>
> We can work around that though. Since by default the touchpad is in relative
> mode anyway, the only thing that should be capped here are the actual posts of
> the motion event, not the internal state. So at any point in time, the movement
> history contains the actual values - including the blacklisted area.
>
> For your patch, this means that you hook in before posting the event and reset
> dx/dy to zero for blacklisted areas. I think that should do the job.
>
> If the finger triggers an edge, no motion event is posted (i.e. the edges form
> the blacklisted area).
>

Ok, I see what you mean now.

I think I can start working on a prototype now.

[Alberto Milone (albertomilone)]

Created an attachment (id=27410)
First draft of the patch

The attached patch is a just first draft and I haven't touched the man page yet.

HOW IT WORKS:
When values other than 0 are assigned to the "Synaptics Area" property, taps, scrolling and movements are allowed only in the "Synaptics Area".

For example, if I set the bottom edge of the area to 3700:
xinput set-int-prop $YOUR_DEVICE_ID "Synaptics Area" 32 0 0 0 3700

the active area will be a rectangle which has the top, left and right physical edges and the bottom edge of the area (3700) as its limits. Of course it is also possible to use custom values for all the 4 edges of the area so as to create an area whose borders don't match with the physical ones.

NOTE: The line which begins with "if ((dx || dy) && (is_inside_active_area" is necessary only if we disable taps, in order to prevent the cursor from jumping. Maybe there's a better way to disable taps (?).

Let me know what you think.

[Peter Hutterer (peter-hutterer)]

thanks for the patch! here's a few comments:
- style: no spaces after negations. for example, if (! inside_active_area) should be if (!inside_active_area)

- don't use 0 as the "no range" marker. there might be touchpads (one day) that have negative ranges. In the server we use valid ranges instead, i.e. (min >= max) designates an unset range.

- one could argue that if the range is user-configured anyway we don't need to query for it (eventcomm.c).

- second-to-last hunk is whitespace only hunk, please remove

I'm a bit confused abut the comment above the is_inside_active_area(.. HIST(0)) part. can you paraphrase this please?

[Alberto Milone (albertomilone)]

Created an attachment (id=27494)
Second draft of the patch

(In reply to comment #19)
> thanks for the patch! here's a few comments:
> - style: no spaces after negations. for example, if (! inside_active_area)
> should be if (!inside_active_area)
>
Ok, fixed.

> - don't use 0 as the "no range" marker. there might be touchpads (one day) that
> have negative ranges. In the server we use valid ranges instead, i.e. (min >=
> max) designates an unset range.
>
Ok, now if the edges of the area match the physical ones (priv->minx, etc.) the property is unset.

> - one could argue that if the range is user-configured anyway we don't need to
> query for it (eventcomm.c).
>
Good point. I got rid of it

> - second-to-last hunk is whitespace only hunk, please remove
>
There's also a curly brace there.

>
> I'm a bit confused abut the comment above the is_inside_active_area(.. HIST(0))
> part. can you paraphrase this please?
>
That approach was wrong and I've come up with a better (and simpler) solution i.e. I set priv->tap_button to 0 in HandleTapProcessing(). Now the cursor doesn't jump and taps are disabled in the area.

Furthermore I have added an entry in the man page and added support for the new property in synclient.

The new patch seems to work for me without problems. More testing is welcome though.

Instructions for testing:
The physical edges are the default values of the area (the following are Dell Mini 10V's):
1472 5472 1408 4448

Set the bottom edge of the area to 4000 (as maybe 3700 was too much for the Dell mini) with the following command:
xinput set-int-prop $YOUR_DEVICE_ID "Synaptics Area" 32 1472 5472 1408 4000

Both scrolling and tapping are disabled outside of the active area and the cursor won't move when you perform a click.

What do you think?

[Peter Hutterer (peter-hutterer)]

(In reply to comment #20)
> > - don't use 0 as the "no range" marker. there might be touchpads (one day) that
> > have negative ranges. In the server we use valid ranges instead, i.e. (min >=
> > max) designates an unset range.
> >
> Ok, now if the edges of the area match the physical ones (priv->minx, etc.) the
> property is unset.

Still not quite sure about this. The reason is that the kernel doesn't
actually guaranteed a min/max range so it can be confusing during debugging
whether a value is supposed to be sent now or not.
I suggest to add another variable to the private rec that contains flags if an
area is set for a given edge. (Have a look at
afb60a0b2497c5d08cbd1739fa8ae6585c428881 for an example).

The initial value of the property can be empty (which means the property
handler must allow a prop->size 0 to unset). Again, this is an explicit signal
that no edges are set. The question is however how we then signal through the
property if only one edge is actually set.

Any suggestions?

> > I'm a bit confused abut the comment above the is_inside_active_area(.. HIST(0))
> > part. can you paraphrase this please?
> >
> That approach was wrong and I've come up with a better (and simpler) solution
> i.e. I set priv->tap_button to 0 in HandleTapProcessing(). Now the cursor
> doesn't jump and taps are disabled in the area.

wouldn't it be easier to only call HandleTapProcessing conditional to
!inside_active_area? or does that mess with the state?

> Furthermore I have added an entry in the man page and added support for the new
> property in synclient.

the man page entry is a bit brief, please explain what the options actually
do (and the property as well).

[Alberto Milone (albertomilone)]

(In reply to comment #21)
> (In reply to comment #20)
> > > - don't use 0 as the "no range" marker. there might be touchpads (one day) that
> > > have negative ranges. In the server we use valid ranges instead, i.e. (min >=
> > > max) designates an unset range.
> > >
> > Ok, now if the edges of the area match the physical ones (priv->minx, etc.) the
> > property is unset.
>
> Still not quite sure about this. The reason is that the kernel doesn't
> actually guaranteed a min/max range so it can be confusing during debugging
> whether a value is supposed to be sent now or not.
> I suggest to add another variable to the private rec that contains flags if an
> area is set for a given edge. (Have a look at
> afb60a0b2497c5d08cbd1739fa8ae6585c428881 for an example).
>
> The initial value of the property can be empty (which means the property
> handler must allow a prop->size 0 to unset). Again, this is an explicit signal
> that no edges are set. The question is however how we then signal through the
> property if only one edge is actually set.
>
> Any suggestions?
>

Why don't we just modify what I wrote in the 1st draft?

Let's consider is_inside_active_area():
In the 1st draft I tested priv->synpara.area_left_edge > 0 but I could simply test that priv->synpara.area_left_edge != 0 (etc.) so as to allow negative ranges:

if ((priv->synpara.area_left_edge != 0) && (x < priv->synpara.area_left_edge))

etc.

And of course I would have to modify SetProperty() (properties.c) accordingly:

if ((((area[0] != 0) && (area[1] != 0)) && (area[0] > area[1]) ) || (((area[2] != 0) && (area[3] != 0)) && (area[2] > area[3])))

This way things should work as you suggested and we won't need additional variables in the private rec.

What do you think?

> wouldn't it be easier to only call HandleTapProcessing conditional to
> !inside_active_area? or does that mess with the state?
>
That's the first thing that I tried but it made the touchpad unusable.

> the man page entry is a bit brief, please explain what the options actually
> do (and the property as well).
>
Sure, I wasn't sure about how verbose the man page could be.

[Alberto Milone (albertomilone)]

Created an attachment (id=27726)
Third draft of the patch

The attached patch contains the latest changes described in the previous comment.

Let me know what you think.

[Henrik Rydberg (rydberg)]

(In reply to comment #23)
> Created an attachment (id=27726) [details]
> Third draft of the patch
>
> The attached patch contains the latest changes described in the previous
> comment.
>
> Let me know what you think.
>

The code is clear and constitutes a good patch. I did not read all the details of the discussion, so I might have missed some important point, but the area parameters constitutes the physical area of the device and could be filled with default values based on the reported driver dimensions?

[Peter Hutterer (peter-hutterer)]

one minor fix: please shift the hunk encompassing the scroll.up while and the other loops by 4 spaces. It'll make the patch appear larger, but now that we've reviewed it several times that shouldn't be an issue.

Attach the final patch as git-formatted patch please, makes it easier to apply.

Henrik: see comment #21 for the explanation why min/max is not really suitable.

[Alberto Milone (albertomilone)]

Created an attachment (id=27758)
Final version of the patch

The attached patch is git-formatted.

I indented that block of code as Peter suggested, furthermore I have updated the comment about is_inside_active_area() so that it matches the current behaviour.

Let me know if there's something else that I should do.

[Peter Hutterer (peter-hutterer)]

Pushed as 7179a0eb11a842d9d5a420f5702a411b0dc217a2. Thanks for your work.

[Alberto Milone (albertomilone)]

The fix is now in Karmic.

[Soeren Sonnenburg (bugreports-nn7)]

I just git checkout/compiled and must say that this patch is not yet working correctly :(

I have a macbook pro here and this machine also does not have a visible mouse button, but only one below the touchpad.

When I use my thumb to click at the very bottom of the pad and then my index finger to move the mouse cursor the pad will simply say: Two fingers detected I will do twofinger scrolling. This happens even with AreaBottomEdge defined leaving enough room for my thumb.

I just checked how OSX handles this and indeed as soon as I click below some AreaBottomEdge (say vertically 650-800) the finger is not counted - so here my index finger works nice for moving the mouse.

So I would propose to do the same here: Either substract the number of fingers by one or ignore finger presses in that area (whatever is easier).

[Peter Hutterer (peter-hutterer)]

Agreed, this seems to be the better behaviour. Can you help us out with a
patch?

[Alberto Milone (albertomilone)]

(In reply to comment #28)
> I just git checkout/compiled and must say that this patch is not yet working
> correctly :(
>
> I have a macbook pro here and this machine also does not have a visible mouse
> button, but only one below the touchpad.
>
> When I use my thumb to click at the very bottom of the pad and then my index
> finger to move the mouse cursor the pad will simply say: Two fingers detected I
> will do twofinger scrolling. This happens even with AreaBottomEdge defined
> leaving enough room for my thumb.
>
> I just checked how OSX handles this and indeed as soon as I click below some
> AreaBottomEdge (say vertically 650-800) the finger is not counted - so here my
> index finger works nice for moving the mouse.
>
> So I would propose to do the same here: Either substract the number of fingers
> by one or ignore finger presses in that area (whatever is easier).
>

Ok, it looks like I neglected multi-touch touchpads.

What happens in OSX when you use you thumb to click and then put two fingers outside of the AreaBottomEdge (i.e. its equivalent in OSX) and move them? Do you get two finger scrolling? What happens with 3 fingers (if it makes any difference)?

[Soeren Sonnenburg (bugreports-nn7)]

when I hold thumb and in addition use two fingers I get two finger scrolling.

so number_of_fingers-1 :)

[Soeren Sonnenburg (bugreports-nn7)]

it just strikes me that one can do two(or three)-finger clicking in the bottom region and then use a finger above to scroll.

so I guess the correct handling would be

corrected_number_of_fingers= total - number_of_fingers_in_bottom_region

[JC Hulce (soaringsky)]

Thank you for taking the time to report this bug. This issue has been fixed in newer versions of Ubuntu, and Jaunty is EOL, so I am closing this bug task.

[Peter Hutterer (peter-hutterer)]

closing this again, was pushed as outlined in comment #27 and that behaviour has been unchanged for 6 years now. if you still want the extra finger counting please open a new bug but I should point out that it's unlikely to be implement for synaptics. libinput has that feature already, probably better to use that then.