Make a pen in eraser mode visible thru PointerEvent.buttons

Make a pen in eraser mode visible thru PointerEvent.buttons

The PointerEvent spec can't gracefully support a hovering pen in eraser
mode because a non-zero |buttons| field indicate an active buttons
state. The issue is currently being discussed in github:
  https://github.com/w3c/pointerevents/issues/134

Until the spec gets fixed, this CL exposes the eraser mode in a
spec-compliant way by hiding the eraser mode until the pen touches the
digitizer. Edge shows the same behavior.

This CL also adds X1, X2 & Eraser button/buttons values to
WebPointerProperties.

BUG=642455
Committed: https://crrev.com/758b11555d5e65c63258731d4fc72b7d7edea0d7
Cr-Commit-Position: refs/heads/master@{#416360}

[mustaq, 2016-08-31 19:15:19]

mustaq@chromium.org changed reviewers:
+ dtapuska@chromium.org, nzolghadr@chromium.org

[mustaq, 2016-08-31 19:15:19]

ptal, this will follow crrev.com/2289273002.

[Navid Zolghadr, 2016-08-31 20:01:16]

https://codereview.chromium.org/2296303002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/2296303002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:99: if
(pointerType == WebPointerProperties::PointerType::Eraser && buttons != 0)
I wonder if we can just do this when pointerType is Eraser regardless of the
fact that any other button is pressed or not. That would make buttons=32 as soon
as an eraser comes into the range of digitizer as oppose to the current model
which hides the fact that the eraser is near the screen and makes it similar to
pen near the screen case but as soon as the eraser touches the screen it shows
button=33.

[mustaq, 2016-08-31 20:29:34]

mustaq@chromium.org changed reviewers:
+ rbyers@chromium.org

[mustaq, 2016-08-31 20:29:37]

I think supporting hovering stylus like Edge should be our immediate goal since
this unblocks Dennis's work while at the same time follows the spec. We can
change the spec and/or the implementation later on.

Rick, wdyt?

https://codereview.chromium.org/2296303002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/2296303002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:99: if
(pointerType == WebPointerProperties::PointerType::Eraser && buttons != 0)
On 2016/08/31 20:01:16, Navid Zolghadr wrote:
> I wonder if we can just do this when pointerType is Eraser regardless of the
> fact that any other button is pressed or not. That would make buttons=32 as
soon
> as an eraser comes into the range of digitizer as oppose to the current model
> which hides the fact that the eraser is near the screen and makes it similar
to
> pen near the screen case but as soon as the eraser touches the screen it shows
> button=33.

I don't like this for two reasons:

[A] That would mean that a flipped pen should either:
- fire a "pointerdown" at the moment if comes within the digitizer range while
hovering, or
- never fire a "pointerdown",
because it will never switch between active/inactive buttons states /after/
being in active state
https://w3c.github.io/pointerevents/#the-pointerdown-event.
Same applies for "pointerup".

[B] From another perspective of the spec, the eraser mode can only switch
between two states (inactive, and active+activeButton). Why should it be so
different from the pen mode that can switch between:
  inactive <-> active <-> activeButton
states?

I think conceptually "pointerdown" and "pointerup" marks the
active<->activeButton state transitions in the spec, and this is very intuitive.

[Rick Byers, 2016-09-01 17:32:18]

On 2016/08/31 20:29:37, mustaq wrote:
> I think supporting hovering stylus like Edge should be our immediate goal
since
> this unblocks Dennis's work while at the same time follows the spec. We can
> change the spec and/or the implementation later on.
> 
> Rick, wdyt?
> 
>
https://codereview.chromium.org/2296303002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):
> 
>
https://codereview.chromium.org/2296303002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/events/PointerEventFactory.cpp:99: if
> (pointerType == WebPointerProperties::PointerType::Eraser && buttons != 0)
> On 2016/08/31 20:01:16, Navid Zolghadr wrote:
> > I wonder if we can just do this when pointerType is Eraser regardless of the
> > fact that any other button is pressed or not. That would make buttons=32 as
> soon
> > as an eraser comes into the range of digitizer as oppose to the current
model
> > which hides the fact that the eraser is near the screen and makes it similar
> to
> > pen near the screen case but as soon as the eraser touches the screen it
shows
> > button=33.
> 
> I don't like this for two reasons:
> 
> [A] That would mean that a flipped pen should either:
> - fire a "pointerdown" at the moment if comes within the digitizer range while
> hovering, or
> - never fire a "pointerdown",
> because it will never switch between active/inactive buttons states /after/
> being in active state
> https://w3c.github.io/pointerevents/#the-pointerdown-event.
> Same applies for "pointerup".
> 
> [B] From another perspective of the spec, the eraser mode can only switch
> between two states (inactive, and active+activeButton). Why should it be so
> different from the pen mode that can switch between:
>   inactive <-> active <-> activeButton
> states?
> 
> I think conceptually "pointerdown" and "pointerup" marks the
> active<->activeButton state transitions in the spec, and this is very
intuitive.

I agree (I think).  The spec as written today (and more importantly - possibly
websites written to expect Edge behavior) does not allow having non-zero buttons
without a corresponding pointerdown.  I don't think we should break that without
doing some spec updates/debate and compat analysis.

For now having no way to differentiate between hovering stylus and hovering
eraser does not seem like much of a downside to me.

One question: so we use buttons=33 on eraser contact to match Edge?  We could
consider using buttons=32 for the case of eraser contact while buttons=33 for
the case of pen contact while an eraser button is being held.  I _think_ that's
consistent with that the spec is saying, even though Edge (or at least the
Surface pen) has no way to trigger buttons=32.

[Navid Zolghadr, 2016-09-01 18:04:09]

On 2016/09/01 17:32:18, Rick Byers wrote:
> On 2016/08/31 20:29:37, mustaq wrote:
> > I think supporting hovering stylus like Edge should be our immediate goal
> since
> > this unblocks Dennis's work while at the same time follows the spec. We can
> > change the spec and/or the implementation later on.
> > 
> > Rick, wdyt?
> > 
> >
>
https://codereview.chromium.org/2296303002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2296303002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/events/PointerEventFactory.cpp:99: if
> > (pointerType == WebPointerProperties::PointerType::Eraser && buttons != 0)
> > On 2016/08/31 20:01:16, Navid Zolghadr wrote:
> > > I wonder if we can just do this when pointerType is Eraser regardless of
the
> > > fact that any other button is pressed or not. That would make buttons=32
as
> > soon
> > > as an eraser comes into the range of digitizer as oppose to the current
> model
> > > which hides the fact that the eraser is near the screen and makes it
similar
> > to
> > > pen near the screen case but as soon as the eraser touches the screen it
> shows
> > > button=33.
> > 
> > I don't like this for two reasons:
> > 
> > [A] That would mean that a flipped pen should either:
> > - fire a "pointerdown" at the moment if comes within the digitizer range
while
> > hovering, or
> > - never fire a "pointerdown",
> > because it will never switch between active/inactive buttons states /after/
> > being in active state
> > https://w3c.github.io/pointerevents/#the-pointerdown-event.
> > Same applies for "pointerup".
> > 
> > [B] From another perspective of the spec, the eraser mode can only switch
> > between two states (inactive, and active+activeButton). Why should it be so
> > different from the pen mode that can switch between:
> >   inactive <-> active <-> activeButton
> > states?
> > 
> > I think conceptually "pointerdown" and "pointerup" marks the
> > active<->activeButton state transitions in the spec, and this is very
> intuitive.
> 
> I agree (I think).  The spec as written today (and more importantly - possibly
> websites written to expect Edge behavior) does not allow having non-zero
buttons
> without a corresponding pointerdown.  I don't think we should break that
without
> doing some spec updates/debate and compat analysis.
> 
> For now having no way to differentiate between hovering stylus and hovering
> eraser does not seem like much of a downside to me.
> 
> One question: so we use buttons=33 on eraser contact to match Edge?  We could
> consider using buttons=32 for the case of eraser contact while buttons=33 for
> the case of pen contact while an eraser button is being held.  I _think_
that's
> consistent with that the spec is saying, even though Edge (or at least the
> Surface pen) has no way to trigger buttons=32.

Just want to point out about not expecting non-zero buttons without
corresponding pointerdown. I believe they would as users can press the button
outside and then bring the mouse in.

[mustaq, 2016-09-01 18:59:12]

On 2016/09/01 17:32:18, Rick Byers wrote:
> On 2016/08/31 20:29:37, mustaq wrote:
> > I think supporting hovering stylus like Edge should be our immediate goal
> since
> > this unblocks Dennis's work while at the same time follows the spec. We can
> > change the spec and/or the implementation later on.
> > 
> > Rick, wdyt?
> > 
> >
>
https://codereview.chromium.org/2296303002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2296303002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/events/PointerEventFactory.cpp:99: if
> > (pointerType == WebPointerProperties::PointerType::Eraser && buttons != 0)
> > On 2016/08/31 20:01:16, Navid Zolghadr wrote:
> > > I wonder if we can just do this when pointerType is Eraser regardless of
the
> > > fact that any other button is pressed or not. That would make buttons=32
as
> > soon
> > > as an eraser comes into the range of digitizer as oppose to the current
> model
> > > which hides the fact that the eraser is near the screen and makes it
similar
> > to
> > > pen near the screen case but as soon as the eraser touches the screen it
> shows
> > > button=33.
> > 
> > I don't like this for two reasons:
> > 
> > [A] That would mean that a flipped pen should either:
> > - fire a "pointerdown" at the moment if comes within the digitizer range
while
> > hovering, or
> > - never fire a "pointerdown",
> > because it will never switch between active/inactive buttons states /after/
> > being in active state
> > https://w3c.github.io/pointerevents/#the-pointerdown-event.
> > Same applies for "pointerup".
> > 
> > [B] From another perspective of the spec, the eraser mode can only switch
> > between two states (inactive, and active+activeButton). Why should it be so
> > different from the pen mode that can switch between:
> >   inactive <-> active <-> activeButton
> > states?
> > 
> > I think conceptually "pointerdown" and "pointerup" marks the
> > active<->activeButton state transitions in the spec, and this is very
> intuitive.
> 
> I agree (I think).  The spec as written today (and more importantly - possibly
> websites written to expect Edge behavior) does not allow having non-zero
buttons
> without a corresponding pointerdown.  I don't think we should break that
without
> doing some spec updates/debate and compat analysis.
> 
> For now having no way to differentiate between hovering stylus and hovering
> eraser does not seem like much of a downside to me.
> 
> One question: so we use buttons=33 on eraser contact to match Edge?  We could
> consider using buttons=32 for the case of eraser contact while buttons=33 for
> the case of pen contact while an eraser button is being held.  I _think_
that's
> consistent with that the spec is saying, even though Edge (or at least the
> Surface pen) has no way to trigger buttons=32.

Surface pen shows buttons=32 when eraser button is pressed. Upon a closer look,
it turns out that its |buttons| never combines multi-button presses which is
clearly a bug.

I switched my code to |buttons|=32 when pen comes in contact in flipped
orientation. The code should work for barrel buttons, couldn't verify since our
plumbing for barrel buttons seems incomplete. Moreover, Chrome vs Edge on
Surface
shows different |buttons| even for non-chorded barrel buttons! crbug.com/526153.

[mustaq, 2016-09-01 19:12:57]

On 2016/09/01 18:59:12, mustaq wrote:
> On 2016/09/01 17:32:18, Rick Byers wrote:
> > On 2016/08/31 20:29:37, mustaq wrote:
> > > I think supporting hovering stylus like Edge should be our immediate goal
> > since
> > > this unblocks Dennis's work while at the same time follows the spec. We
can
> > > change the spec and/or the implementation later on.
> > > 
> > > Rick, wdyt?
> > > 
> > >
> >
>
https://codereview.chromium.org/2296303002/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/events/PointerEventFactory.cpp
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2296303002/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/events/PointerEventFactory.cpp:99: if
> > > (pointerType == WebPointerProperties::PointerType::Eraser && buttons != 0)
> > > On 2016/08/31 20:01:16, Navid Zolghadr wrote:
> > > > I wonder if we can just do this when pointerType is Eraser regardless of
> the
> > > > fact that any other button is pressed or not. That would make buttons=32
> as
> > > soon
> > > > as an eraser comes into the range of digitizer as oppose to the current
> > model
> > > > which hides the fact that the eraser is near the screen and makes it
> similar
> > > to
> > > > pen near the screen case but as soon as the eraser touches the screen it
> > shows
> > > > button=33.
> > > 
> > > I don't like this for two reasons:
> > > 
> > > [A] That would mean that a flipped pen should either:
> > > - fire a "pointerdown" at the moment if comes within the digitizer range
> while
> > > hovering, or
> > > - never fire a "pointerdown",
> > > because it will never switch between active/inactive buttons states
/after/
> > > being in active state
> > > https://w3c.github.io/pointerevents/#the-pointerdown-event.
> > > Same applies for "pointerup".
> > > 
> > > [B] From another perspective of the spec, the eraser mode can only switch
> > > between two states (inactive, and active+activeButton). Why should it be
so
> > > different from the pen mode that can switch between:
> > >   inactive <-> active <-> activeButton
> > > states?
> > > 
> > > I think conceptually "pointerdown" and "pointerup" marks the
> > > active<->activeButton state transitions in the spec, and this is very
> > intuitive.
> > 
> > I agree (I think).  The spec as written today (and more importantly -
possibly
> > websites written to expect Edge behavior) does not allow having non-zero
> buttons
> > without a corresponding pointerdown.  I don't think we should break that
> without
> > doing some spec updates/debate and compat analysis.
> > 
> > For now having no way to differentiate between hovering stylus and hovering
> > eraser does not seem like much of a downside to me.
> > 
> > One question: so we use buttons=33 on eraser contact to match Edge?  We
could
> > consider using buttons=32 for the case of eraser contact while buttons=33
for
> > the case of pen contact while an eraser button is being held.  I _think_
> that's
> > consistent with that the spec is saying, even though Edge (or at least the
> > Surface pen) has no way to trigger buttons=32.
> 
> Surface pen shows buttons=32 when eraser button is pressed. Upon a closer
look,
> it turns out that its |buttons| never combines multi-button presses which is
> clearly a bug.
> 
> I switched my code to |buttons|=32 when pen comes in contact in flipped
> orientation. The code should work for barrel buttons, couldn't verify since
our
> plumbing for barrel buttons seems incomplete. Moreover, Chrome vs Edge on
> Surface
> shows different |buttons| even for non-chorded barrel buttons!
crbug.com/526153.

Note that Dennis's use case doesn't include barrel buttons, so this won't be a
blocker. Still filed a new bug to isolate the button case: crbug.com/643329.

[mustaq, 2016-09-02 15:44:50]

Description was changed from

==========
Make a pen in eraser mode visible thru PointerEvent.buttons

The PointerEvent spec can't gracefully support a hovering pen in eraser
mode because a non-zero |buttons| field indicate an active buttons
state. The issue is currently being discussed in github:
  https://github.com/w3c/pointerevents/issues/134

Until the spec gets fixed, this CL exposes the eraser mode in a
spec-compliant way by hiding the eraser mode until the pen touches the
digitizer. Edge shows the same behavior.

BUG=642455
==========

to

==========
Make a pen in eraser mode visible thru PointerEvent.buttons

The PointerEvent spec can't gracefully support a hovering pen in eraser
mode because a non-zero |buttons| field indicate an active buttons
state. The issue is currently being discussed in github:
  https://github.com/w3c/pointerevents/issues/134

Until the spec gets fixed, this CL exposes the eraser mode in a
spec-compliant way by hiding the eraser mode until the pen touches the
digitizer. Edge shows the same behavior.

This CL also adds X1, X2 & Eraser button/buttons values to
WebPointerProperties.

BUG=642455
==========

[mustaq, 2016-09-02 15:46:25]

ptal:
- Fixed a crack with |button|.
- Cleaned up the code a bit.
- Rebased after Dennis's patch landed.

[mustaq, 2016-09-02 15:47:01]

The CQ bit was checked by mustaq@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 15:47:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Navid Zolghadr, 2016-09-02 16:04:31]

lgtm

https://codereview.chromium.org/2296303002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/2296303002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:90: }
nit: This bracket is not needed here.

https://codereview.chromium.org/2296303002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:106: buttons &=
~static_cast<unsigned>(WebPointerProperties::Buttons::Left);
If I understood from your previous comment, you said we only get eraser type for
reverse pens. So right now we don't have plumbing for pens with eraser button.
Right?

[mustaq, 2016-09-02 16:12:27]

https://codereview.chromium.org/2296303002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/2296303002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:90: }
On 2016/09/02 16:04:31, Navid Zolghadr wrote:
> nit: This bracket is not needed here.

Done.

https://codereview.chromium.org/2296303002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:106: buttons &=
~static_cast<unsigned>(WebPointerProperties::Buttons::Left);
On 2016/09/02 16:04:31, Navid Zolghadr wrote:
> If I understood from your previous comment, you said we only get eraser type
for
> reverse pens. So right now we don't have plumbing for pens with eraser button.
> Right?

Yes, well, mostly:
- Windows: definitely.
- Linux: Mostly likely, Intuos Pro barrel button sometimes appears as
LeftButton, could be for lack of driver.
- Android: No way to check.

[Rick Byers, 2016-09-02 17:59:45]

LGTM

[mustaq, 2016-09-02 18:54:52]

The CQ bit was checked by mustaq@chromium.org

[mustaq, 2016-09-02 18:54:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
nzolghadr@chromium.org
Link to the patchset: https://codereview.chromium.org/2296303002/#ps80001
(title: "nzolghadr's comments.")

[commit-bot: I haz the power, 2016-09-02 18:55:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 20:36:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-02 20:36:12]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[mustaq, 2016-09-02 20:49:26]

The CQ bit was checked by mustaq@chromium.org

[commit-bot: I haz the power, 2016-09-02 20:50:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 22:06:59]

Description was changed from

==========
Make a pen in eraser mode visible thru PointerEvent.buttons

The PointerEvent spec can't gracefully support a hovering pen in eraser
mode because a non-zero |buttons| field indicate an active buttons
state. The issue is currently being discussed in github:
  https://github.com/w3c/pointerevents/issues/134

Until the spec gets fixed, this CL exposes the eraser mode in a
spec-compliant way by hiding the eraser mode until the pen touches the
digitizer. Edge shows the same behavior.

This CL also adds X1, X2 & Eraser button/buttons values to
WebPointerProperties.

BUG=642455
==========

to

==========
Make a pen in eraser mode visible thru PointerEvent.buttons

The PointerEvent spec can't gracefully support a hovering pen in eraser
mode because a non-zero |buttons| field indicate an active buttons
state. The issue is currently being discussed in github:
  https://github.com/w3c/pointerevents/issues/134

Until the spec gets fixed, this CL exposes the eraser mode in a
spec-compliant way by hiding the eraser mode until the pen touches the
digitizer. Edge shows the same behavior.

This CL also adds X1, X2 & Eraser button/buttons values to
WebPointerProperties.

BUG=642455
==========

[commit-bot: I haz the power, 2016-09-02 22:07:00]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-02 22:09:15]

Description was changed from

==========
Make a pen in eraser mode visible thru PointerEvent.buttons

The PointerEvent spec can't gracefully support a hovering pen in eraser
mode because a non-zero |buttons| field indicate an active buttons
state. The issue is currently being discussed in github:
  https://github.com/w3c/pointerevents/issues/134

Until the spec gets fixed, this CL exposes the eraser mode in a
spec-compliant way by hiding the eraser mode until the pen touches the
digitizer. Edge shows the same behavior.

This CL also adds X1, X2 & Eraser button/buttons values to
WebPointerProperties.

BUG=642455
==========

to

==========
Make a pen in eraser mode visible thru PointerEvent.buttons

The PointerEvent spec can't gracefully support a hovering pen in eraser
mode because a non-zero |buttons| field indicate an active buttons
state. The issue is currently being discussed in github:
  https://github.com/w3c/pointerevents/issues/134

Until the spec gets fixed, this CL exposes the eraser mode in a
spec-compliant way by hiding the eraser mode until the pen touches the
digitizer. Edge shows the same behavior.

This CL also adds X1, X2 & Eraser button/buttons values to
WebPointerProperties.

BUG=642455

Committed: https://crrev.com/758b11555d5e65c63258731d4fc72b7d7edea0d7
Cr-Commit-Position: refs/heads/master@{#416360}
==========

[commit-bot: I haz the power, 2016-09-02 22:09:17]

Patchset 5 (id:??) landed as
https://crrev.com/758b11555d5e65c63258731d4fc72b7d7edea0d7
Cr-Commit-Position: refs/heads/master@{#416360}