ozone: Handle on-screen stylus support consistently

ozone: Handle on-screen stylus support consistently

With this change the TabletEventConverter is adapted to handle on screen stylus
on ChromeOS as per the existing behavior of the Android active stylus support
(for example the S-Pen.):

- Mouse events are generated for motion when the pen is hovering on the
  screen and if any buttons on the pen are clicked
- Touch events are generated when the pen touches the screen, with the
  stylus pressure being passed in as the touch event force.

TEST=On an active stylus equipped system run monitorEvents in a browser and
observe that touch events are generated when the stylus is tapped to the screen
and moved around. Mouse events are generated when the device is merely hovering
in proximity to the screen.
BUG=448467

[robert.bradford, 2015-01-16 18:54:21]

Patchset #2 (id:20001) has been deleted

[robert.bradford, 2015-01-16 19:01:21]

robert.bradford@intel.com changed reviewers:
+ rbyers@chromium.org, spang@chromium.org

[robert.bradford, 2015-01-16 19:01:21]

Obviously still need unit tests but interactive testing shows this working well.

Michael, I hope this shows that the changes needed to support the on screen
stylus in the TabletEventConverterEvdev class is quite small esp when
considering that the buttons on the stylus should work like a mouse and the
behavior with the non-contact hovering.

Thanks

[Rick Byers, 2015-01-19 20:40:06]

rbyers@chromium.org changed reviewers:
- rbyers@chromium.org

[Rick Byers, 2015-01-19 20:44:20]

I don't have any ozone-specific context, so moving myself from reviewer to cc. 
But in general this is consistent with the approach we want to take with stylus
approach in chromium.  We should be consistent across platforms, and we already
have a long history on Android - let's not invent something new for ChromeOS.

Please reference bug 448467 (I added more context/links there).

https://codereview.chromium.org/847663004/diff/1/ui/events/ozone/evdev/tablet...
File ui/events/ozone/evdev/tablet_event_converter_evdev.cc (right):

https://codereview.chromium.org/847663004/diff/1/ui/events/ozone/evdev/tablet...
ui/events/ozone/evdev/tablet_event_converter_evdev.cc:212: /* touch_id */ 1,
What if there is both real touchscreen and stylus contacts present at once?  A
hard-coded touch_id may be problematic in that case.

[spang, 2015-01-19 20:53:04]

On 2015/01/19 20:44:20, Rick Byers wrote:
> I don't have any ozone-specific context, so moving myself from reviewer to cc.

> But in general this is consistent with the approach we want to take with
stylus
> approach in chromium.  We should be consistent across platforms, and we
already
> have a long history on Android - let's not invent something new for ChromeOS.
> 
> Please reference bug 448467 (I added more context/links there).
> 
>
https://codereview.chromium.org/847663004/diff/1/ui/events/ozone/evdev/tablet...
> File ui/events/ozone/evdev/tablet_event_converter_evdev.cc (right):
> 
>
https://codereview.chromium.org/847663004/diff/1/ui/events/ozone/evdev/tablet...
> ui/events/ozone/evdev/tablet_event_converter_evdev.cc:212: /* touch_id */ 1,
> What if there is both real touchscreen and stylus contacts present at once?  A
> hard-coded touch_id may be problematic in that case.

I need more context here.

If the SPen is for an Android tablet, what value is there in adding support to
ChromeOS? How are you testing it?

[robert.bradford, 2015-01-20 16:30:56]

On 2015/01/19 20:53:04, spang wrote:
> On 2015/01/19 20:44:20, Rick Byers wrote:
> > I don't have any ozone-specific context, so moving myself from reviewer to
cc.
> 
> > But in general this is consistent with the approach we want to take with
> stylus
> > approach in chromium.  We should be consistent across platforms, and we
> already
> > have a long history on Android - let's not invent something new for
ChromeOS.
> > 
> > Please reference bug 448467 (I added more context/links there).
> > 
> >
>
https://codereview.chromium.org/847663004/diff/1/ui/events/ozone/evdev/tablet...
> > File ui/events/ozone/evdev/tablet_event_converter_evdev.cc (right):
> > 
> >
>
https://codereview.chromium.org/847663004/diff/1/ui/events/ozone/evdev/tablet...
> > ui/events/ozone/evdev/tablet_event_converter_evdev.cc:212: /* touch_id */ 1,
> > What if there is both real touchscreen and stylus contacts present at once? 
A
> > hard-coded touch_id may be problematic in that case.
> 
> I need more context here.
> 
> If the SPen is for an Android tablet, what value is there in adding support to
> ChromeOS? How are you testing it?

Apologies for being unclear. The hardware i'm using is a 2 in 1 convertible
laptop similar to the Lenovo Thinkpad Helix. The pen appears on the I2C bus
(like the touch screen) and advertises evdev characteristics just like the Wacom
with the exception that PROP_INPUT_POINTER is false.

This patch changes the behaviour for on screen active styluses in Ozone to
behave as per other platforms' on-screen styluses. The main change needed is
that touch events are generated by the event converter when the active stylus is
pressed on onto the screen.

Without this change the on screen stylus works as per the graphics tablet which
whilst functional is inconsistent with the other implementers of this type of
device in Chromium.

I will update the issue description to make this clearer.

[spang, 2015-01-20 18:07:10]

On 2015/01/20 16:30:56, robert.bradford wrote:
> On 2015/01/19 20:53:04, spang wrote:
> > On 2015/01/19 20:44:20, Rick Byers wrote:
> > > I don't have any ozone-specific context, so moving myself from reviewer to
> cc.
> > 
> > > But in general this is consistent with the approach we want to take with
> > stylus
> > > approach in chromium.  We should be consistent across platforms, and we
> > already
> > > have a long history on Android - let's not invent something new for
> ChromeOS.
> > > 
> > > Please reference bug 448467 (I added more context/links there).
> > > 
> > >
> >
>
https://codereview.chromium.org/847663004/diff/1/ui/events/ozone/evdev/tablet...
> > > File ui/events/ozone/evdev/tablet_event_converter_evdev.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/847663004/diff/1/ui/events/ozone/evdev/tablet...
> > > ui/events/ozone/evdev/tablet_event_converter_evdev.cc:212: /* touch_id */
1,
> > > What if there is both real touchscreen and stylus contacts present at
once? 
> A
> > > hard-coded touch_id may be problematic in that case.
> > 
> > I need more context here.
> > 
> > If the SPen is for an Android tablet, what value is there in adding support
to
> > ChromeOS? How are you testing it?
> 
> Apologies for being unclear. The hardware i'm using is a 2 in 1 convertible
> laptop similar to the Lenovo Thinkpad Helix. The pen appears on the I2C bus
> (like the touch screen) and advertises evdev characteristics just like the
Wacom
> with the exception that PROP_INPUT_POINTER is false.

Are you saying the touchscreen shows up in a separate event device than the pen?
If so, is there any way to know that they are the same surface?

Have you thought about the multiple monitors case? Like I mentioned on your
previous patch, I think we always want direct touches (stylus or otherwise) to
interact with the pixel underneath them. This patch allows direct touches with
the stylus to cause events on external monitors, which feels wrong.

The exact case is:
  (1) Move the cursor over external display
  (2) Poke the internal display with the stylus

Does it click on

(1) the internal display (at exactly the poked pixel), or
(2) the external display (at (x,y) proportional to the location within the
internal display)

I think (2) would be confusing.

> 
> This patch changes the behaviour for on screen active styluses in Ozone to
> behave as per other platforms' on-screen styluses. The main change needed is
> that touch events are generated by the event converter when the active stylus
is
> pressed on onto the screen.
> 
> Without this change the on screen stylus works as per the graphics tablet
which
> whilst functional is inconsistent with the other implementers of this type of
> device in Chromium.
> 
> I will update the issue description to make this clearer.

[robert.bradford, 2015-01-22 19:08:16]

On 2015/01/20 18:07:10, spang wrote:
> On 2015/01/20 16:30:56, robert.bradford wrote:
> > On 2015/01/19 20:53:04, spang wrote:
> > > On 2015/01/19 20:44:20, Rick Byers wrote:
> > > > I don't have any ozone-specific context, so moving myself from reviewer
to
> > cc.
> > > 
> > > > But in general this is consistent with the approach we want to take with
> > > stylus
> > > > approach in chromium.  We should be consistent across platforms, and we
> > > already
> > > > have a long history on Android - let's not invent something new for
> > ChromeOS.
> > > > 
> > > > Please reference bug 448467 (I added more context/links there).
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/847663004/diff/1/ui/events/ozone/evdev/tablet...
> > > > File ui/events/ozone/evdev/tablet_event_converter_evdev.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/847663004/diff/1/ui/events/ozone/evdev/tablet...
> > > > ui/events/ozone/evdev/tablet_event_converter_evdev.cc:212: /* touch_id
*/
> 1,
> > > > What if there is both real touchscreen and stylus contacts present at
> once? 
> > A
> > > > hard-coded touch_id may be problematic in that case.
> > > 
> > > I need more context here.
> > > 
> > > If the SPen is for an Android tablet, what value is there in adding
support
> to
> > > ChromeOS? How are you testing it?
> > 
> > Apologies for being unclear. The hardware i'm using is a 2 in 1 convertible
> > laptop similar to the Lenovo Thinkpad Helix. The pen appears on the I2C bus
> > (like the touch screen) and advertises evdev characteristics just like the
> Wacom
> > with the exception that PROP_INPUT_POINTER is false.
> 
> Are you saying the touchscreen shows up in a separate event device than the
pen?
> If so, is there any way to know that they are the same surface?

Yes the touchscreen and pen appear as separate evdev devices (on my hardware
same vendor & product id). I can't see any way to identify that they actually
refer to the same surface. When the pen touches the touch screen no events are
generated on the device for the touchscreen. I guess we could use udev
properties to really associate the two or make the assumption that something
that looks like an active stylus is also for the primary display like the touch
screen.

> Have you thought about the multiple monitors case? Like I mentioned on your
> previous patch, I think we always want direct touches (stylus or otherwise) to
> interact with the pixel underneath them. This patch allows direct touches with
> the stylus to cause events on external monitors, which feels wrong.
> 
> The exact case is:
>   (1) Move the cursor over external display
>   (2) Poke the internal display with the stylus
> 
> Does it click on
> 
> (1) the internal display (at exactly the poked pixel), or
> (2) the external display (at (x,y) proportional to the location within the
> internal display)
> 
> I think (2) would be confusing.
> 

Yes I totally agree and (1) was my intention - I didn't notice this mistake
until you pointed it out as I was only doing my testing on a single (internal)
display. I've spent a few hours looking into this and i'm a bit stumped about
how I can safely get the dimensions of the internal display. So i'd love some
advice about which direction I should take...

The TouchEventConverter uses DataDeviceManager::ApplyTouchTransformer() to
transform from "tuxel space" to the display space. It looks a bit tricky to
populate the DataDeviceManager with the tranformation as there appears to be a
unary mapping from display to touchscreen id. My gut feeling is that this is
probably the right way to go but maybe quite complex as it's not really a
touchscreen per se.

The alternative would be to have some way of getting access to the primary
display bounds like how we can get the current display bounds on the
CursorDelegate and then make the assumption primary == internal.

Any guidance would be much appreciated.

[spang, 2015-01-26 23:28:16]

On 2015/01/22 19:08:16, robert.bradford wrote:
> On 2015/01/20 18:07:10, spang wrote:
> > On 2015/01/20 16:30:56, robert.bradford wrote:
> > > On 2015/01/19 20:53:04, spang wrote:
> > > > On 2015/01/19 20:44:20, Rick Byers wrote:
> > > > > I don't have any ozone-specific context, so moving myself from
reviewer
> to
> > > cc.
> > > > 
> > > > > But in general this is consistent with the approach we want to take
with
> > > > stylus
> > > > > approach in chromium.  We should be consistent across platforms, and
we
> > > > already
> > > > > have a long history on Android - let's not invent something new for
> > > ChromeOS.
> > > > > 
> > > > > Please reference bug 448467 (I added more context/links there).
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/847663004/diff/1/ui/events/ozone/evdev/tablet...
> > > > > File ui/events/ozone/evdev/tablet_event_converter_evdev.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/847663004/diff/1/ui/events/ozone/evdev/tablet...
> > > > > ui/events/ozone/evdev/tablet_event_converter_evdev.cc:212: /* touch_id
> */
> > 1,
> > > > > What if there is both real touchscreen and stylus contacts present at
> > once? 
> > > A
> > > > > hard-coded touch_id may be problematic in that case.
> > > > 
> > > > I need more context here.
> > > > 
> > > > If the SPen is for an Android tablet, what value is there in adding
> support
> > to
> > > > ChromeOS? How are you testing it?
> > > 
> > > Apologies for being unclear. The hardware i'm using is a 2 in 1
convertible
> > > laptop similar to the Lenovo Thinkpad Helix. The pen appears on the I2C
bus
> > > (like the touch screen) and advertises evdev characteristics just like the
> > Wacom
> > > with the exception that PROP_INPUT_POINTER is false.
> > 
> > Are you saying the touchscreen shows up in a separate event device than the
> pen?
> > If so, is there any way to know that they are the same surface?
> 
> Yes the touchscreen and pen appear as separate evdev devices (on my hardware
> same vendor & product id). I can't see any way to identify that they actually
> refer to the same surface.

What is the sysfs path for the stylus device? (output of readlink -f
/sys/class/input/event[0-9])

> When the pen touches the touch screen no events are
> generated on the device for the touchscreen. I guess we could use udev
> properties to really associate the two or make the assumption that something
> that looks like an active stylus is also for the primary display like the
touch
> screen.
> 
> > Have you thought about the multiple monitors case? Like I mentioned on your
> > previous patch, I think we always want direct touches (stylus or otherwise)
to
> > interact with the pixel underneath them. This patch allows direct touches
with
> > the stylus to cause events on external monitors, which feels wrong.
> > 
> > The exact case is:
> >   (1) Move the cursor over external display
> >   (2) Poke the internal display with the stylus
> > 
> > Does it click on
> > 
> > (1) the internal display (at exactly the poked pixel), or
> > (2) the external display (at (x,y) proportional to the location within the
> > internal display)
> > 
> > I think (2) would be confusing.
> > 
> 
> Yes I totally agree and (1) was my intention - I didn't notice this mistake
> until you pointed it out as I was only doing my testing on a single (internal)
> display. I've spent a few hours looking into this and i'm a bit stumped about
> how I can safely get the dimensions of the internal display. So i'd love some
> advice about which direction I should take...
> 
> The TouchEventConverter uses DataDeviceManager::ApplyTouchTransformer() to
> transform from "tuxel space" to the display space. It looks a bit tricky to
> populate the DataDeviceManager with the tranformation as there appears to be a
> unary mapping from display to touchscreen id. My gut feeling is that this is
> probably the right way to go but maybe quite complex as it's not really a
> touchscreen per se.

It is basically a touchscreen because it's attached to the screen. Which means
we have to transform the input coordinates carefully into graphical coordinates
- proportional scaling like the tablet isn't good enough.

I'm sure we could rename bits of code to better capture this, but it's not a
huge stretch to just say it's a 2nd touchscreen.

> 
> The alternative would be to have some way of getting access to the primary
> display bounds like how we can get the current display bounds on the
> CursorDelegate and then make the assumption primary == internal.

You really don't want to replicate the code that builds this transform. So we'll
need to extend it  for this case.

Unfortunately, the code is not currently prepared to have multiple
"touchscreens" per

> 
> Any guidance would be much appreciated.

[spang, 2015-01-26 23:33:39]

Hit send a bit too soon...

On Mon, Jan 26, 2015 at 6:28 PM, <spang@chromium.org> wrote:

> On 2015/01/22 19:08:16, robert.bradford wrote:
>
>> On 2015/01/20 18:07:10, spang wrote:
>> > On 2015/01/20 16:30:56, robert.bradford wrote:
>> > > On 2015/01/19 20:53:04, spang wrote:
>> > > > On 2015/01/19 20:44:20, Rick Byers wrote:
>> > > > > I don't have any ozone-specific context, so moving myself from
>>
> reviewer
>
>> to
>> > > cc.
>> > > >
>> > > > > But in general this is consistent with the approach we want to
>> take
>>
> with
>
>> > > > stylus
>> > > > > approach in chromium.  We should be consistent across platforms,
>> and
>>
> we
>
>> > > > already
>> > > > > have a long history on Android - let's not invent something new
>> for
>> > > ChromeOS.
>> > > > >
>> > > > > Please reference bug 448467 (I added more context/links there).
>> > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/847663004/diff/1/ui/
> events/ozone/evdev/tablet_event_converter_evdev.cc
>
>> > > > > File ui/events/ozone/evdev/tablet_event_converter_evdev.cc
>> (right):
>> > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/847663004/diff/1/ui/
> events/ozone/evdev/tablet_event_converter_evdev.cc#newcode212
>
>> > > > > ui/events/ozone/evdev/tablet_event_converter_evdev.cc:212: /*
>> touch_id
>> */
>> > 1,
>> > > > > What if there is both real touchscreen and stylus contacts
>> present at
>> > once?
>> > > A
>> > > > > hard-coded touch_id may be problematic in that case.
>> > > >
>> > > > I need more context here.
>> > > >
>> > > > If the SPen is for an Android tablet, what value is there in adding
>> support
>> > to
>> > > > ChromeOS? How are you testing it?
>> > >
>> > > Apologies for being unclear. The hardware i'm using is a 2 in 1
>>
> convertible
>
>> > > laptop similar to the Lenovo Thinkpad Helix. The pen appears on the
>> I2C
>>
> bus
>
>> > > (like the touch screen) and advertises evdev characteristics just
>> like the
>> > Wacom
>> > > with the exception that PROP_INPUT_POINTER is false.
>> >
>> > Are you saying the touchscreen shows up in a separate event device than
>> the
>> pen?
>> > If so, is there any way to know that they are the same surface?
>>
>
>  Yes the touchscreen and pen appear as separate evdev devices (on my
>> hardware
>> same vendor & product id). I can't see any way to identify that they
>> actually
>> refer to the same surface.
>>
>
> What is the sysfs path for the stylus device? (output of readlink -f
> /sys/class/input/event[0-9])
>
>
>  When the pen touches the touch screen no events are
>> generated on the device for the touchscreen. I guess we could use udev
>> properties to really associate the two or make the assumption that
>> something
>> that looks like an active stylus is also for the primary display like the
>>
> touch
>
>> screen.
>>
>
>  > Have you thought about the multiple monitors case? Like I mentioned on
>> your
>> > previous patch, I think we always want direct touches (stylus or
>> otherwise)
>>
> to
>
>> > interact with the pixel underneath them. This patch allows direct
>> touches
>>
> with
>
>> > the stylus to cause events on external monitors, which feels wrong.
>> >
>> > The exact case is:
>> >   (1) Move the cursor over external display
>> >   (2) Poke the internal display with the stylus
>> >
>> > Does it click on
>> >
>> > (1) the internal display (at exactly the poked pixel), or
>> > (2) the external display (at (x,y) proportional to the location within
>> the
>> > internal display)
>> >
>> > I think (2) would be confusing.
>> >
>>
>
>  Yes I totally agree and (1) was my intention - I didn't notice this
>> mistake
>> until you pointed it out as I was only doing my testing on a single
>> (internal)
>> display. I've spent a few hours looking into this and i'm a bit stumped
>> about
>> how I can safely get the dimensions of the internal display. So i'd love
>> some
>> advice about which direction I should take...
>>
>
>  The TouchEventConverter uses DataDeviceManager::ApplyTouchTransformer()
>> to
>> transform from "tuxel space" to the display space. It looks a bit tricky
>> to
>> populate the DataDeviceManager with the tranformation as there appears to
>> be a
>> unary mapping from display to touchscreen id. My gut feeling is that this
>> is
>> probably the right way to go but maybe quite complex as it's not really a
>> touchscreen per se.
>>
>
> It is basically a touchscreen because it's attached to the screen. Which
> means
> we have to transform the input coordinates carefully into graphical
> coordinates
> - proportional scaling like the tablet isn't good enough.
>
> I'm sure we could rename bits of code to better capture this, but it's not
> a
> huge stretch to just say it's a 2nd touchscreen.
>
>
>  The alternative would be to have some way of getting access to the primary
>> display bounds like how we can get the current display bounds on the
>> CursorDelegate and then make the assumption primary == internal.
>>
>
> You really don't want to replicate the code that builds this transform. So
> we'll
> need to extend it  for this case.
>
> Unfortunately, the code is not currently prepared to have multiple
> "touchscreens" per


... not currently prepared to have multiple "touchscreens" per screen. So
that code will need adjustment.


>
>
>
>  Any guidance would be much appreciated.
>>
>
>
>
> https://codereview.chromium.org/847663004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.