First-cut at fixing unhandled Tap event returns in Blink.

First-cut at fixing unhandled Tap event returns in Blink.

We want to be able to recognize tap events that the page has not handled and provide some useful default behavior for those events.  Currently taps are considered handled in many cases when they really have not been handled by the page or default behavior.  This CL is a beginning at correcting this.  We plan to also check if the tap interacted with an element with an ARIA role that indicates it was interactive.

BUG=355154
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175514

[donnd, 2014-05-09 01:57:50]

Guys, PTAL at my hacky "fix" for the unhandled Tap event in Blink.  I have an
end-to-end implementation that uses this, and it looks pretty good as a
first-cut.  Any suggestions as how to make it less hacky?

Thanks!

[jdduke (slow), 2014-05-09 23:33:33]

Adding a few folks that may be more familiar with this code.  zeeshanq@, we're
seeing that GestureTap events are always being reported as "consumed" to the
browser.  Ideally we'd get an ack that was more truthful, as the contextual
search folks would like to do useful things with an unhandled tap event.

[Rick Byers, 2014-05-16 11:10:42]

Sorry for the delay - I missed this due to travel to BlinkOn etc.  I don't think
this is the right way to tackle this problem, but I can work with you to try to
come up with something better.

I think the way to tackle this is probably to treat mouse and touch the same
here.  For 'click' events at least, anything that we want not to set the
'default handled' bit for touch also shouldn't set 'default handled' for mouse
(we want the web platform to be consistent).

So I think we need to tackle these on a case-by-case basis.  You mention click
causing the selection to change as the case you're concerned with.  Can you
provide more details / a test case?  Perhaps we can just make such selection
change not affect the handled state.  Eg. if I select text with the mouse then
click on a link, the selection is cleared AND I navigate to the link.  This says
to me that clearing the selection does not conceptually HANDLE the event (it
just happens as a side effect of the event). 

The mousedown/mouseup events are perhaps a little different in that they're
primarily legacy compatibility events.

https://chromiumcodereview.appspot.com/267313008/diff/1/Source/core/page/Even...
File Source/core/page/EventHandler.cpp (right):

https://chromiumcodereview.appspot.com/267313008/diff/1/Source/core/page/Even...
Source/core/page/EventHandler.cpp:619: swallowEvent = false;
This seems wrong.  A click from touch is as legitimate as a click from mouse
(it's not "fake" - click really means activate and is sent for keyboard as
well).  So, for example, if a webpage calls preventDefault on click then you
should absolutely NOT be doing some extra browser UI in that case ("content not
chrome" - the app owns it's event stream first).

[Donn Denman, 2014-05-27 05:58:14]

Rick, Please take another look at this CL.  I've updated the code along the
lines we discussed, however I had to take a slightly different approach in a
couple of areas.  Since I'm new to Blink, I'd like your opinion on whether these
approaches seem reasonable before investing too much time in a particular
direction:
1) I did a unit test instead of a layout test because I couldn't get layout
tests to work on Clank, and it seems like unit tests are prefered.  It looks
like the unit test is doing the job for us, though we may need more test cases. 
Does this seem reasonable?
2) Our use case, and I think any reasonable use case, wants to ignore events
sent to interactive elements -- they are never "unhandled".  So I added a method
isInteractiveNode to encapsulate this policy.  Right now it only checks if the
node is editable (since that's interactive too), but I'll add the additional
logic assuming you think that's a reasonable direction to go in.  The
alternative would be to put checking for interactive at the client level, but to
me that doesn't make much sense and it's more efficient this way.
3) I'd be inclined to try to land this soon in order to see if there are
problems, and if things look good update it to handle all the details in a
subsequent CL.  Do you think we're getting close enough that I can send it out
to regular reviewers?

Thanks for all your help on this!

https://chromiumcodereview.appspot.com/267313008/diff/1/Source/core/page/Even...
File Source/core/page/EventHandler.cpp (right):

https://chromiumcodereview.appspot.com/267313008/diff/1/Source/core/page/Even...
Source/core/page/EventHandler.cpp:619: swallowEvent = false;
On 2014/05/16 11:10:43, Rick Byers wrote:
> This seems wrong.  A click from touch is as legitimate as a click from mouse
> (it's not "fake" - click really means activate and is sent for keyboard as
> well).  So, for example, if a webpage calls preventDefault on click then you
> should absolutely NOT be doing some extra browser UI in that case ("content
not
> chrome" - the app owns it's event stream first).

Done.

[Rick Byers, 2014-05-28 17:36:57]

On 2014/05/27 05:58:14, Donn Denman wrote:
> Rick, Please take another look at this CL.  I've updated the code along the
> lines we discussed, however I had to take a slightly different approach in a
> couple of areas.  Since I'm new to Blink, I'd like your opinion on whether
these
> approaches seem reasonable before investing too much time in a particular
> direction:
> 1) I did a unit test instead of a layout test because I couldn't get layout
> tests to work on Clank, and it seems like unit tests are prefered.  It looks
> like the unit test is doing the job for us, though we may need more test
cases. 
> Does this seem reasonable?

I don't think we should choose the testing strategy based on (presumably
temporary) infrastructure issues.  peter@ was working on getting LayoutTests
running reliably on Android, but I'm not sure of the current status of that.  In
general the blink team prefers layout tests because they can often be run both
as an automated test with content_shell and loaded interactively into different
browsers to manually compare behavior and so are also more easily inspectable
and often simpler (a few lines of JS instead of a bunch of C++).  So my
preference in this case would be for you to update EventSender::GestureTap to
return a bool with the handled state (unfortunately that's now in the chromium
tree so you'll have to land that first in a separate CL) then write a LayoutTest
in LayoutTests/fast/events/touch/gestures/ similar to your existing .html file
that uses eventSender to send a tap to each element.  Another benefit of this is
that you can be more sure you're getting a valid tap gesture event (right now
you're not initializing all the fields expected for a WebGestureEvent of type
GestureTap, but EventSender will take care of this for you).

If (with peter@'s help) you're not able to easily make your layout test run on
Android, then (as I mentioned earlier) I suggest you do linux builds for your
edit/test/debug cycle - I think most blink engineers find they can be more
productive that way (plus there's a decent chance you're going to eventually
break some layout test you need to debug with a Linux build anyway).

That said, perhaps there's some other reason a unit test is better here that I'm
missing.  Your unit test does look basically OK (other than the fact we do very
similar things in layout tests today)...

> 2) Our use case, and I think any reasonable use case, wants to ignore events
> sent to interactive elements -- they are never "unhandled".  So I added a
method
> isInteractiveNode to encapsulate this policy.  Right now it only checks if the
> node is editable (since that's interactive too), but I'll add the additional
> logic assuming you think that's a reasonable direction to go in.  The
> alternative would be to put checking for interactive at the client level, but
to
> me that doesn't make much sense and it's more efficient this way.

I left some comments inline for this.

> 3) I'd be inclined to try to land this soon in order to see if there are
> problems, and if things look good update it to handle all the details in a
> subsequent CL.  Do you think we're getting close enough that I can send it out
> to regular reviewers?

Absolutely.  I suggest you first land the CLs for just the text selection
clearing issue (which I think we can land ASAP with my review) and work on the
editable nodes issue in a follow-up CL.  This will also make it easier for us if
one of your changes ends up breaking some web site - it can be reverted in
isolation from the others. 
 
> Thanks for all your help on this!
> 
>
https://chromiumcodereview.appspot.com/267313008/diff/1/Source/core/page/Even...
> File Source/core/page/EventHandler.cpp (right):
> 
>
https://chromiumcodereview.appspot.com/267313008/diff/1/Source/core/page/Even...
> Source/core/page/EventHandler.cpp:619: swallowEvent = false;
> On 2014/05/16 11:10:43, Rick Byers wrote:
> > This seems wrong.  A click from touch is as legitimate as a click from mouse
> > (it's not "fake" - click really means activate and is sent for keyboard as
> > well).  So, for example, if a webpage calls preventDefault on click then you
> > should absolutely NOT be doing some extra browser UI in that case ("content
> not
> > chrome" - the app owns it's event stream first).
> 
> Done.

https://codereview.chromium.org/267313008/diff/100001/Source/core/page/EventH...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/267313008/diff/100001/Source/core/page/EventH...
Source/core/page/EventHandler.cpp:552:
updateSelectionForMouseDownDispatchingSelectStart(innerNode, newSelection,
granularity);
I think this is fine for addressing the original problem you were concerned
with.  Please add a comment saying that we consider updating the selection to be
a side-effect of the event and so it doesn't impact the handled state.

https://codereview.chromium.org/267313008/diff/100001/Source/core/page/EventH...
Source/core/page/EventHandler.cpp:554: // If the target node is editable, then
it handles the click, either directly or through a handler.
Hmm, it seems wrong / error-prone to have this logic separately from what is
actually handling the event.  Ideally the code that sets the handled state
should be in the exact same place as the code that is actually doing the
handling.  So, for example, where are mouse events actually handled by
content-editable nodes?

Remember you've got a few events here (mousedown, mouseup, click) - are you sure
it's just the mousedown (that we're dealing with in this function) that editable
nodes handle?  Or maybe it's actually the mouseup or click whose handled state
should be corrected?

[donnd, 2014-05-31 03:22:54]

Rick, this code should now be up-to-date with your original guidelines:
* New Layout test for Tap which checks if the selection is cleared or not due to
preventDefault().
* Returning false instead of whether the selection was cleared for
handleMousePressEventSingleClick when it doesn't do an early return.

I've tested on Linux, but still can't run the Layout tests on Android.  I am
able to bring up the page on android and test it interactively.

https://codereview.chromium.org/267313008/diff/100001/Source/core/page/EventH...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/267313008/diff/100001/Source/core/page/EventH...
Source/core/page/EventHandler.cpp:552:
updateSelectionForMouseDownDispatchingSelectStart(innerNode, newSelection,
granularity);
On 2014/05/28 17:36:57, Rick Byers wrote:
> I think this is fine for addressing the original problem you were concerned
> with.  Please add a comment saying that we consider updating the selection to
be
> a side-effect of the event and so it doesn't impact the handled state.

Done.

https://codereview.chromium.org/267313008/diff/100001/Source/core/page/EventH...
Source/core/page/EventHandler.cpp:554: // If the target node is editable, then
it handles the click, either directly or through a handler.
On 2014/05/28 17:36:57, Rick Byers wrote:
> Hmm, it seems wrong / error-prone to have this logic separately from what is
> actually handling the event.  Ideally the code that sets the handled state
> should be in the exact same place as the code that is actually doing the
> handling.  So, for example, where are mouse events actually handled by
> content-editable nodes?
> 
> Remember you've got a few events here (mousedown, mouseup, click) - are you
sure
> it's just the mousedown (that we're dealing with in this function) that
editable
> nodes handle?  Or maybe it's actually the mouseup or click whose handled state
> should be corrected?
For now, we just always return false.  That should help us figure out if this
change will break anything.

[donnd, 2014-05-31 03:28:08]

On 2014/05/28 17:36:57, Rick Byers wrote:
> I don't think we should choose the testing strategy based on (presumably
> temporary) infrastructure issues.  peter@ was working on getting LayoutTests
> running reliably on Android, but I'm not sure of the current status of that. 
In
> general the blink team prefers layout tests because they can often be run both
> as an automated test with content_shell and loaded interactively into
different
> browsers to manually compare behavior and so are also more easily inspectable
> and often simpler (a few lines of JS instead of a bunch of C++).  So my
> preference in this case would be for you to update EventSender::GestureTap to
> return a bool with the handled state (unfortunately that's now in the chromium
> tree so you'll have to land that first in a separate CL) then write a
LayoutTest
> in LayoutTests/fast/events/touch/gestures/ similar to your existing .html file
> that uses eventSender to send a tap to each element.  Another benefit of this
is
> that you can be more sure you're getting a valid tap gesture event (right now
> you're not initializing all the fields expected for a WebGestureEvent of type
> GestureTap, but EventSender will take care of this for you).
Unit tests seemed better because they are fast and easy to run.  Layout tests
seem to be problematic on Android.  However, having said that, You have
convinced me to do a Layout test, and I'm happy testing on Linux and will follow
up with peter@ for Android help if needed.

[donnd, 2014-06-03 04:41:12]

Rick, PTAL.  The change to the test infrastructure to return whether the event
was handled or not has landed (https://codereview.chromium.org/301103002).

[Rick Byers, 2014-06-03 14:41:53]

Thanks Donn, sorry for the delay.
Just a couple little questions on the test then I think this is good to land.

Rick

https://codereview.chromium.org/267313008/diff/120001/LayoutTests/fast/events...
File LayoutTests/fast/events/touch/gesture/gesture-tap-result.html (right):

https://codereview.chromium.org/267313008/diff/120001/LayoutTests/fast/events...
LayoutTests/fast/events/touch/gesture/gesture-tap-result.html:30: // event to
end the test without temporal flakiness.
Chrome now always has a "gesture recognizer" (the same code used by Android is
available, eg. on Mac, for devtools touch emulation to use).  But regardless
you're not invoking the gesture recognizer since you're really testing blink is
isolation here.

I'm not sure what the 'temporal flakiness' issue you're referring to is, but I
think you can simplify all of this by making it synchronous (i.e. don't call
waitUntilDone or notifyDone at all).  eventSender runs synchronously so you
shouldn't have to wait for anything.

https://codereview.chromium.org/267313008/diff/120001/LayoutTests/fast/events...
LayoutTests/fast/events/touch/gesture/gesture-tap-result.html:36: if
(mouseEventsReceived == 0) {
No reason to make this it's own function - just keep it simple (making it clear
it's not used in the normal automated test path) and stick it into the above.

https://codereview.chromium.org/267313008/diff/120001/LayoutTests/fast/events...
LayoutTests/fast/events/touch/gesture/gesture-tap-result.html:81:
eventSender.leapForward(10);
Is this necessary?  I thought the saved events / leapForward mechanism in
eventSender was for drag and drop testing only, which I wouldn't expect you to
be hitting here.

[donnd, 2014-06-03 20:24:58]

Rick, PTAL.

It seemed that there were a few things that were not really needed in the test,
so I removed them.  Does this look good now?

https://codereview.chromium.org/267313008/diff/120001/LayoutTests/fast/events...
File LayoutTests/fast/events/touch/gesture/gesture-tap-result.html (right):

https://codereview.chromium.org/267313008/diff/120001/LayoutTests/fast/events...
LayoutTests/fast/events/touch/gesture/gesture-tap-result.html:30: // event to
end the test without temporal flakiness.
On 2014/06/03 14:41:54, Rick Byers wrote:
> Chrome now always has a "gesture recognizer" (the same code used by Android is
> available, eg. on Mac, for devtools touch emulation to use).  But regardless
> you're not invoking the gesture recognizer since you're really testing blink
is
> isolation here.
> 
> I'm not sure what the 'temporal flakiness' issue you're referring to is, but I
> think you can simplify all of this by making it synchronous (i.e. don't call
> waitUntilDone or notifyDone at all).  eventSender runs synchronously so you
> shouldn't have to wait for anything.
Sorry about not removing this earlier.  I copied some code that looked like test
boilerplate, but didn't check if it was really needed.

It looks like none of the key-to-quit logic is needed, either for manual or
automated testing, so I removed it.

https://codereview.chromium.org/267313008/diff/120001/LayoutTests/fast/events...
LayoutTests/fast/events/touch/gesture/gesture-tap-result.html:36: if
(mouseEventsReceived == 0) {
On 2014/06/03 14:41:54, Rick Byers wrote:
> No reason to make this it's own function - just keep it simple (making it
clear
> it's not used in the normal automated test path) and stick it into the above.

It looks like this isn't needed either.  Also, that means we no long need the
mouseEventsReceived variable.

https://codereview.chromium.org/267313008/diff/120001/LayoutTests/fast/events...
LayoutTests/fast/events/touch/gesture/gesture-tap-result.html:81:
eventSender.leapForward(10);
On 2014/06/03 14:41:54, Rick Byers wrote:
> Is this necessary?  I thought the saved events / leapForward mechanism in
> eventSender was for drag and drop testing only, which I wouldn't expect you to
> be hitting here.

Done.

[Rick Byers, 2014-06-03 21:27:25]

Nice simplifications, thanks!  One more suggestion to simplify it further, then
it LGTM (feel free to put it in the CQ if you make no other changes).

https://codereview.chromium.org/267313008/diff/140001/LayoutTests/fast/events...
File LayoutTests/fast/events/touch/gesture/gesture-tap-result.html (right):

https://codereview.chromium.org/267313008/diff/140001/LayoutTests/fast/events...
LayoutTests/fast/events/touch/gesture/gesture-tap-result.html:31:
consumes.addEventListener("mousedown", consumeCallback, false);
Now that you're not logging even info, I suggest you remove all these other
handlers too.  It's only the mousedown handler that matters.  In fact you don't
need any handlers at all on 'plain' (although having an empty mousedown handler
doesn't hurt and perhaps improves the test coverage slightly).

[donnd, 2014-06-03 21:40:24]

Thanks Rick!

https://codereview.chromium.org/267313008/diff/140001/LayoutTests/fast/events...
File LayoutTests/fast/events/touch/gesture/gesture-tap-result.html (right):

https://codereview.chromium.org/267313008/diff/140001/LayoutTests/fast/events...
LayoutTests/fast/events/touch/gesture/gesture-tap-result.html:31:
consumes.addEventListener("mousedown", consumeCallback, false);
On 2014/06/03 21:27:25, Rick Byers wrote:
> Now that you're not logging even info, I suggest you remove all these other
> handlers too.  It's only the mousedown handler that matters.  In fact you
don't
> need any handlers at all on 'plain' (although having an empty mousedown
handler
> doesn't hurt and perhaps improves the test coverage slightly).

Done.

[donnd, 2014-06-03 21:40:58]

The CQ bit was checked by donnd@google.com

[commit-bot: I haz the power, 2014-06-03 21:41:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/donnd@chromium.org/267313008/160001

[commit-bot: I haz the power, 2014-06-04 01:18:51]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[donnd, 2014-06-04 20:38:52]

The CQ bit was checked by donnd@google.com

[commit-bot: I haz the power, 2014-06-04 20:39:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/donnd@chromium.org/267313008/160001

[commit-bot: I haz the power, 2014-06-04 20:40:40]

Change committed as 175514