Fire an accessible click event when a web node is clicked on.

Fire an accessible click event when a web node is clicked on.

The Android platform wants an accessible "click" event on any on-screen
view that gets clicked on, whether the source of the click is the touch
screen, mouse, keyboard, or even a direct API call.

Implement it by hooking into EventDispatcher::dispatchEventPostProcess
so that it doesn't matter what the source of the click event was.

BUG=566166
Committed: https://crrev.com/d33b01f23e72c32f330a0295f8a696f7d55c6cef
Cr-Commit-Position: refs/heads/master@{#374404}

[dmazzoni, 2016-02-02 17:28:56]

dmazzoni@chromium.org changed reviewers:
+ dtseng@chromium.org, rbyers@chromium.org

[dmazzoni, 2016-02-02 17:28:57]

rbyers: could you review the change to EventDispatcher, or
suggest a better reviewer?

dtseng: everything else

[Rick Byers, 2016-02-02 18:22:57]

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

https://codereview.chromium.org/1655153003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/EventDispatcher.cpp:204:
cache->handleClicked(m_node);
This is being called for ALL types of events (not just click), I don't think
that's what you want (eg. a scroll shouldn't be considered a click), and it's
also potentially a performance issue (eg. for every touchmove/mousemove).

Do you want to include the case where JavaScript generates an event named
"click"?

What about other sources of internal "clicks", eg. the enter key is pressed on a
button?

What about the common case of a touchscreen tap, where the page is calling
preventDefault on the touch events (eg. using a FastClick library even though
there's no benefit on Chrome)?  Handling that case will be trickier.  In general
"preventDefault" means "don't do any native browser handling" - should that
include disabling this accessibility hook?

Based on the above, I can help you refine / relocate this hook.

[Rick Byers, 2016-02-02 18:26:20]

On 2016/02/02 18:22:57, Rick Byers (slow until Feb 3) wrote:
>
https://codereview.chromium.org/1655153003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/events/EventDispatcher.cpp (right):
> 
>
https://codereview.chromium.org/1655153003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/events/EventDispatcher.cpp:204:
> cache->handleClicked(m_node);
> This is being called for ALL types of events (not just click), I don't think
> that's what you want (eg. a scroll shouldn't be considered a click), and it's
> also potentially a performance issue (eg. for every touchmove/mousemove).
> 
> Do you want to include the case where JavaScript generates an event named
> "click"?
> 
> What about other sources of internal "clicks", eg. the enter key is pressed on
a
> button?
> 
> What about the common case of a touchscreen tap, where the page is calling
> preventDefault on the touch events (eg. using a FastClick library even though
> there's no benefit on Chrome)?  Handling that case will be trickier.  In
general
> "preventDefault" means "don't do any native browser handling" - should that
> include disabling this accessibility hook?
> 
> Based on the above, I can help you refine / relocate this hook.

Also, some tests would probably be useful in capturing exactly which scenarios
you want this API to be called in, and which you don't.

[dmazzoni, 2016-02-02 18:35:48]

Thanks

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

https://codereview.chromium.org/1655153003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/EventDispatcher.cpp:204:
cache->handleClicked(m_node);
On 2016/02/02 18:22:57, Rick Byers (slow until Feb 3) wrote:
> This is being called for ALL types of events (not just click), I don't think
> that's what you want (eg. a scroll shouldn't be considered a click), and it's
> also potentially a performance issue (eg. for every touchmove/mousemove).

Oops, I had a line of code that checked for the event type matching
EventTypeNames::click, I accidentally deleted that while removing logging.

> Do you want to include the case where JavaScript generates an event named
> "click"?
> 
> What about other sources of internal "clicks", eg. the enter key is pressed on
a
> button?

Yes, my intent was to capture that case too. I want real clicks, simulated
clicks.

> What about the common case of a touchscreen tap, where the page is calling
> preventDefault on the touch events (eg. using a FastClick library even though
> there's no benefit on Chrome)?  Handling that case will be trickier.  In
general
> "preventDefault" means "don't do any native browser handling" - should that
> include disabling this accessibility hook?

Great question.

If we could fire the accessible click event when someone's using FastClick
that'd be nice.

Overall it'd be desirable if we didn't fire the event if the user clicks on
something like a static text node or the document body and there are no click
handlers. But if the page has some click handlers for tracking, or if there's no
good way to do this, that's okay.

Most important is that I do not want this to fire if you drag or scroll or
something like that.

I hope that's clear.

Also, do I probably want to fire the event on the
eventTargetRespectingTargetRules or on m_node? What will
eventTargetRespectingTargetRules be if it's not a node, how would I map it to a
node in that case?

[David Tseng, 2016-02-02 18:49:59]

https://codereview.chromium.org/1655153003/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java
(left):

https://codereview.chromium.org/1655153003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:240:
sendAccessibilityEvent(virtualViewId,
Great; looks like we were always sending clicked events regardless if one
actually occurred?

https://codereview.chromium.org/1655153003/diff/1/ui/accessibility/ax_enums.idl
File ui/accessibility/ax_enums.idl (right):

https://codereview.chromium.org/1655153003/diff/1/ui/accessibility/ax_enums.i...
ui/accessibility/ax_enums.idl:33: clicked,
Please add to automation idl as well.

[Rick Byers, 2016-02-02 18:55:37]

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

https://codereview.chromium.org/1655153003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/EventDispatcher.cpp:204:
cache->handleClicked(m_node);
On 2016/02/02 18:35:48, dmazzoni wrote:
> On 2016/02/02 18:22:57, Rick Byers (slow until Feb 3) wrote:
> > This is being called for ALL types of events (not just click), I don't think
> > that's what you want (eg. a scroll shouldn't be considered a click), and
it's
> > also potentially a performance issue (eg. for every touchmove/mousemove).
> 
> Oops, I had a line of code that checked for the event type matching
> EventTypeNames::click, I accidentally deleted that while removing logging.


Ok.  Please share the logic with the isTrustedOrClick check below (so we're
comparing with event names only once).

> 
> > Do you want to include the case where JavaScript generates an event named
> > "click"?
> > 
> > What about other sources of internal "clicks", eg. the enter key is pressed
on
> a
> > button?
> 
> Yes, my intent was to capture that case too. I want real clicks, simulated
> clicks.
> 
> > What about the common case of a touchscreen tap, where the page is calling
> > preventDefault on the touch events (eg. using a FastClick library even
though
> > there's no benefit on Chrome)?  Handling that case will be trickier.  In
> general
> > "preventDefault" means "don't do any native browser handling" - should that
> > include disabling this accessibility hook?
> 
> Great question.
> 
> If we could fire the accessible click event when someone's using FastClick
> that'd be nice.
> 
> Overall it'd be desirable if we didn't fire the event if the user clicks on
> something like a static text node or the document body and there are no click
> handlers. But if the page has some click handlers for tracking, or if there's
no
> good way to do this, that's okay.
> 
> Most important is that I do not want this to fire if you drag or scroll or
> something like that.
> 
> I hope that's clear.

Yep, makes sense.  Handling FastClick scenarios will be a little tricky but
doable.  If it's not critical, I suggest you file a separate bug for that and we
can discuss the details there for a future CL (I may be making some changes
that'll make it relatively easy). 

> Also, do I probably want to fire the event on the
> eventTargetRespectingTargetRules or on m_node? What will
> eventTargetRespectingTargetRules be if it's not a node, how would I map it to
a
> node in that case?

Looks like the only difference is in handling pseudo elements
(https://developer.mozilla.org/en-US/docs/Web/CSS/Pseudo-elements).  I'm
guessing you probably want eventTargetRespectingTargetRules, but it depends on
what the consumer of this is doing with the node.  Also be sure the consumer can
handle ShadowDOM alright.

[dmazzoni, 2016-02-03 19:13:36]

Ready for another look, I added some tests.

On 2016/02/02 18:55:37, Rick Byers wrote:
> Ok.  Please share the logic with the isTrustedOrClick check below (so we're
> comparing with event names only once).

Great, done.

> Yep, makes sense.  Handling FastClick scenarios will be a little tricky but
> doable.  If it's not critical, I suggest you file a separate bug for that and
we
> can discuss the details there for a future CL (I may be making some changes
> that'll make it relatively easy). 

Sounds great. Totally fine to skip FastClick for now.

> Looks like the only difference is in handling pseudo elements
> (https://developer.mozilla.org/en-US/docs/Web/CSS/Pseudo-elements).  I'm
> guessing you probably want eventTargetRespectingTargetRules, but it depends on
> what the consumer of this is doing with the node.

Got it. We don't expose accessibility for some pseudo elements so
eventTargetRespectingTargetRules does make sense.

Also be sure the consumer can
> handle ShadowDOM alright.

Yes, accessibility handles shadow dom already so that should be fine.

[dmazzoni, 2016-02-03 19:13:46]

https://codereview.chromium.org/1655153003/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java
(left):

https://codereview.chromium.org/1655153003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:240:
sendAccessibilityEvent(virtualViewId,
On 2016/02/02 18:49:59, David Tseng wrote:
> Great; looks like we were always sending clicked events regardless if one
> actually occurred?

Yeah, but only in response to an ACTION_CLICK, which didn't handle the most
common case where the user double-taps on the screen. Now we should send a click
event whenever the event is really sent to an element.

https://codereview.chromium.org/1655153003/diff/1/ui/accessibility/ax_enums.idl
File ui/accessibility/ax_enums.idl (right):

https://codereview.chromium.org/1655153003/diff/1/ui/accessibility/ax_enums.i...
ui/accessibility/ax_enums.idl:33: clicked,
On 2016/02/02 18:49:59, David Tseng wrote:
> Please add to automation idl as well.

Done.

[David Tseng, 2016-02-03 19:15:29]

lgtm

[dmazzoni, 2016-02-05 17:25:54]

Friendly ping - Rick, how does it look now?

[Rick Byers, 2016-02-06 02:53:34]

Just one concern about what the test is actually testing.
But if you want to look into that in a follow-up patch that's fine with me (just
add a TODO so people aren't confused).  So LGTM.

I'm out on vacation now, dtapuska@ can give blink input-related advice while I'm
out if you need it (and you can get an OWNERS stamp from anyone else - eg.
jochen, kouhei).

https://codereview.chromium.org/1655153003/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/accessibility/click-event.html (right):

https://codereview.chromium.org/1655153003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/click-event.html:57: }, "clicking a
button via the keyboard sends an accessible click event");
these tests just verify that a DOM click event was sent, right?  I.e. they'd
pass just as well before your CL as afterward (and are probably redundant with
the mouse tests in fast/events).  Is there some way we can validate that the
accessibility event was actually sent up the stack (eg. expose something for
testing off of accessibilityController)?

[dmazzoni, 2016-02-08 17:21:06]

dmazzoni@chromium.org changed reviewers:
+ aboxhall@chromium.org
- dtseng@chromium.org

[dmazzoni, 2016-02-08 17:21:07]

Tests fixed, see below.

It sounds like Rick was happy with the change other than the tests - Alice,
could you perhaps take a quick look at the tests and approve before I submit
this?

https://codereview.chromium.org/1655153003/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/accessibility/click-event.html (right):

https://codereview.chromium.org/1655153003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/click-event.html:57: }, "clicking a
button via the keyboard sends an accessible click event");
On 2016/02/06 02:53:34, Rick Byers (out until Feb 18) wrote:
> these tests just verify that a DOM click event was sent, right?  I.e. they'd
> pass just as well before your CL as afterward (and are probably redundant with
> the mouse tests in fast/events).  Is there some way we can validate that the
> accessibility event was actually sent up the stack (eg. expose something for
> testing off of accessibilityController)?

Oops, I'm embarrassed to say that I intended to test for the accessibility
'Clicked' notification once I had the tests for the click event working, then
forgot! Yes, it's absolutely possible to test for it, and I just made that
change. Thanks for catching this. I think the tests are much more useful now,
and they definitely wouldn't have passed before.

[aboxhall, 2016-02-08 18:07:12]

LGTM with suggestions.

https://codereview.chromium.org/1655153003/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/accessibility/click-event.html (right):

https://codereview.chromium.org/1655153003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/accessibility/click-event.html:100:
eventSender.dragMode = true;
Add another test for dragging into the button instead of out?

https://codereview.chromium.org/1655153003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/accessibility/click-event.html:112: }, 200);
I suspect this timeout could be zero - the accessibility event processing stuff
all happens synchronously with the regular event processing stuff, right?

https://codereview.chromium.org/1655153003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/accessibility/click-event.html:114: </script>
What about another test for touch?
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
looks reasonably cargo-cult-able.

[dmazzoni, 2016-02-08 21:26:31]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-02-08 21:26:31]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org,
rbyers@chromium.org, aboxhall@chromium.org
Link to the patchset: https://codereview.chromium.org/1655153003/#ps120001
(title: "Address feedback on tests from aboxhall")

[dmazzoni, 2016-02-08 21:26:59]

The CQ bit was unchecked by dmazzoni@chromium.org

[dmazzoni, 2016-02-08 21:34:29]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-02-08 21:34:30]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org,
rbyers@chromium.org, aboxhall@chromium.org
Link to the patchset: https://codereview.chromium.org/1655153003/#ps140001
(title: "Added test for tap")

[dmazzoni, 2016-02-08 21:34:46]

https://codereview.chromium.org/1655153003/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/accessibility/click-event.html (right):

https://codereview.chromium.org/1655153003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/accessibility/click-event.html:100:
eventSender.dragMode = true;
On 2016/02/08 18:07:12, aboxhall wrote:
> Add another test for dragging into the button instead of out?

Done.

https://codereview.chromium.org/1655153003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/accessibility/click-event.html:112: }, 200);
On 2016/02/08 18:07:12, aboxhall wrote:
> I suspect this timeout could be zero - the accessibility event processing
stuff
> all happens synchronously with the regular event processing stuff, right? 

Good idea. Seems to work!

https://codereview.chromium.org/1655153003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/accessibility/click-event.html:114: </script>
On 2016/02/08 18:07:12, aboxhall wrote:
> What about another test for touch?
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> looks reasonably cargo-cult-able.

Good idea! Done.

[commit-bot: I haz the power, 2016-02-08 21:42:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1655153003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1655153003/140001

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

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

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

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dmazzoni, 2016-02-09 16:17:58]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2016-02-09 16:18:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1655153003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1655153003/140001

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

Committed patchset #8 (id:140001)

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

Description was changed from

==========
Fire an accessible click event when a web node is clicked on.

The Android platform wants an accessible "click" event on any on-screen
view that gets clicked on, whether the source of the click is the touch
screen, mouse, keyboard, or even a direct API call.

Implement it by hooking into EventDispatcher::dispatchEventPostProcess
so that it doesn't matter what the source of the click event was.

BUG=566166
==========

to

==========
Fire an accessible click event when a web node is clicked on.

The Android platform wants an accessible "click" event on any on-screen
view that gets clicked on, whether the source of the click is the touch
screen, mouse, keyboard, or even a direct API call.

Implement it by hooking into EventDispatcher::dispatchEventPostProcess
so that it doesn't matter what the source of the click event was.

BUG=566166

Committed: https://crrev.com/d33b01f23e72c32f330a0295f8a696f7d55c6cef
Cr-Commit-Position: refs/heads/master@{#374404}
==========

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

Patchset 8 (id:??) landed as
https://crrev.com/d33b01f23e72c32f330a0295f8a696f7d55c6cef
Cr-Commit-Position: refs/heads/master@{#374404}