Select-to-speak event handler

Select-to-speak event handler

This change implements the key C++ portion of the Select-to-speak feature
by adding an event handler that takes Search plus the mouse button, and
fires accessibility events instead.

In the future we could modify or extend this event handler for use in
ChromeVox too, but for now I think building something specifically for
Select-to-speak makes sense.

Adds new accessibility events for mouse down, mouse up, and so on - allowing
the select-to-speak extension to get not only the mouse events and global
coordinates, but most importantly the proper accessibility object that's hit
for each event.

This change implements support for hit testing in Views. For web,
the extension will get the event fired on the views::WebView and it can do
its own hit testing within the accessibility tree from there. A follow-up
will improve on this by doing native hit testing in the web before firing
the accessibility event.

BUG=593887
Committed: https://crrev.com/735c79df7859362bd059b5c233286910eff25ef9
Cr-Commit-Position: refs/heads/master@{#434744}

[dmazzoni, 2016-11-10 22:05:23]

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

[commit-bot: I haz the power, 2016-11-10 22:05:57]

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

[dmazzoni, 2016-11-10 22:06:04]

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

[commit-bot: I haz the power, 2016-11-10 22:56:33]

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

[commit-bot: I haz the power, 2016-11-10 22:56:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[David Tseng, 2016-11-11 19:18:30]

https://codereview.chromium.org/2493923002/diff/1/chrome/browser/chromeos/acc...
File chrome/browser/chromeos/accessibility/select_to_speak_event_handler.cc
(right):

https://codereview.chromium.org/2493923002/diff/1/chrome/browser/chromeos/acc...
chrome/browser/chromeos/accessibility/select_to_speak_event_handler.cc:39:
state_ = INACTIVE;
Should you set state to inactive for all other key release events (or at least
search released)? For example, if I pressed search and an arrow key. We'd remain
in the search down state, right?

https://codereview.chromium.org/2493923002/diff/1/chrome/browser/chromeos/acc...
chrome/browser/chromeos/accessibility/select_to_speak_event_handler.cc:49: if
((state_ == SEARCH_DOWN || state_ == MOUSE_RELEASED) &&
How do we go from mouse released -> mouse pressed = capturing?

https://codereview.chromium.org/2493923002/diff/1/chrome/browser/chromeos/acc...
chrome/browser/chromeos/accessibility/select_to_speak_event_handler.cc:61: if
(state_ != CAPTURING)
Rather than one state, I think it would be a lot clearer to have:
- a boolean search_down that tracks whether tha search key is down or not
- mouse down

capturing = serach_down && mouse_down

https://codereview.chromium.org/2493923002/diff/1/chrome/browser/chromeos/acc...
File chrome/browser/chromeos/accessibility/select_to_speak_event_handler.h
(right):

https://codereview.chromium.org/2493923002/diff/1/chrome/browser/chromeos/acc...
chrome/browser/chromeos/accessibility/select_to_speak_event_handler.h:8:
#include <vector>
Unused?

[dmazzoni, 2016-11-14 22:11:42]

https://codereview.chromium.org/2493923002/diff/1/chrome/browser/chromeos/acc...
File chrome/browser/chromeos/accessibility/select_to_speak_event_handler.cc
(right):

https://codereview.chromium.org/2493923002/diff/1/chrome/browser/chromeos/acc...
chrome/browser/chromeos/accessibility/select_to_speak_event_handler.cc:39:
state_ = INACTIVE;
On 2016/11/11 19:18:30, David Tseng wrote:
> Should you set state to inactive for all other key release events (or at least
> search released)? For example, if I pressed search and an arrow key. We'd
remain
> in the search down state, right?

Great idea. Done, and added a new test for this case.

https://codereview.chromium.org/2493923002/diff/1/chrome/browser/chromeos/acc...
chrome/browser/chromeos/accessibility/select_to_speak_event_handler.cc:49: if
((state_ == SEARCH_DOWN || state_ == MOUSE_RELEASED) &&
On 2016/11/11 19:18:30, David Tseng wrote:
> How do we go from mouse released -> mouse pressed = capturing?

This is to support the user holding down search and
clicking more than once, does that make sense?
The mouse released state means that the mouse was released
but Search was still down. I tried to explain this in the
header file.

https://codereview.chromium.org/2493923002/diff/1/chrome/browser/chromeos/acc...
chrome/browser/chromeos/accessibility/select_to_speak_event_handler.cc:61: if
(state_ != CAPTURING)
On 2016/11/11 19:18:30, David Tseng wrote:
> Rather than one state, I think it would be a lot clearer to have:
> - a boolean search_down that tracks whether tha search key is down or not
> - mouse down
> 
> capturing = serach_down && mouse_down

That doesn't handle all of the current states. For example it doesn't handle the
case where Search is still down, mouse released vs search held down but no click
yet. The difference is that if the user has pressed the mouse, we want to handle
the release of the Search key.

The problem with bools is that if we had enough bools to express all of
the different states we need, there would be some permutations that make
no sense. Having a discrete number of states makes it easier to reason
about every possible transition.

https://codereview.chromium.org/2493923002/diff/1/chrome/browser/chromeos/acc...
File chrome/browser/chromeos/accessibility/select_to_speak_event_handler.h
(right):

https://codereview.chromium.org/2493923002/diff/1/chrome/browser/chromeos/acc...
chrome/browser/chromeos/accessibility/select_to_speak_event_handler.h:8:
#include <vector>
On 2016/11/11 19:18:30, David Tseng wrote:
> Unused?

Done.

[David Tseng, 2016-11-22 00:33:58]

lgtm

https://codereview.chromium.org/2493923002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/select_to_speak_event_handler.h
(right):

https://codereview.chromium.org/2493923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/select_to_speak_event_handler.h:29: // The
Search key is not held down and the mouse button is not down.
nit: Neither the search key or mouse key is down

https://codereview.chromium.org/2493923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/select_to_speak_event_handler.h:38:
CAPTURING,
I was just thinking you could reprsent this call using a command stack where the
down's can be cancelled by the up's e.g.
[sd] = search down
[sd, md] = mouse down
[sd, md, mr] = mouse released
[sd, md] = capturing
[sd, md, sr] = waiting for mouse released.
[sd, sr] inactive

So, I think you could maybe get away with just looking at the top of your
command stack.

[dmazzoni, 2016-11-22 17:49:49]

https://codereview.chromium.org/2493923002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/select_to_speak_event_handler.h
(right):

https://codereview.chromium.org/2493923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/select_to_speak_event_handler.h:29: // The
Search key is not held down and the mouse button is not down.
On 2016/11/22 00:33:58, David Tseng wrote:
> nit: Neither the search key or mouse key is down

Done.

https://codereview.chromium.org/2493923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/select_to_speak_event_handler.h:38:
CAPTURING,
On 2016/11/22 00:33:58, David Tseng wrote:
> I was just thinking you could reprsent this call using a command stack where
the
> down's can be cancelled by the up's e.g.
> [sd] = search down
> [sd, md] = mouse down
> [sd, md, mr] = mouse released
> [sd, md] = capturing
> [sd, md, sr] = waiting for mouse released.
> [sd, sr] inactive
> 
> So, I think you could maybe get away with just looking at the top of your
> command stack.

I think it would still require looking at more than the top of the stack.

Also the touch exploration controller and touch accessibility enabler are also
both based on similar state machine code, so I think this model has been
found to work well if the complexity increases.

[dmazzoni, 2016-11-22 17:50:16]

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

[commit-bot: I haz the power, 2016-11-22 17:50:39]

Dry run: 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-11-22 19:50:40]

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

[commit-bot: I haz the power, 2016-11-22 19:50:41]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dmazzoni, 2016-11-22 21:38:39]

dmazzoni@chromium.org changed reviewers:
+ sky@chromium.org

[dmazzoni, 2016-11-22 21:38:39]

@sky: could you take a look at the unit test?
I'd like to create a subclass of ViewsDelegate
for testing, but I'm hitting the assert enforcing
that there wasn't already a ViewsDelegate.

Any suggestions? Would it be reasonable to add
an API to AshTestBase or AshTestEnvironment to
make it possible to override creating the
ViewsDelegate?

[sky, 2016-11-22 22:52:21]

On 2016/11/22 21:38:39, dmazzoni wrote:
> @sky: could you take a look at the unit test?
> I'd like to create a subclass of ViewsDelegate
> for testing, but I'm hitting the assert enforcing
> that there wasn't already a ViewsDelegate.
> 
> Any suggestions? Would it be reasonable to add
> an API to AshTestBase or AshTestEnvironment to
> make it possible to override creating the
> ViewsDelegate?

That, or create an ash specific delegate (test only) that the ViewsDelegate
implementation in ash calls.

[dmazzoni, 2016-11-23 23:03:46]

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

[dmazzoni, 2016-11-23 23:03:47]

On 2016/11/22 22:52:21, sky wrote:
> That, or create an ash specific delegate (test only) that the ViewsDelegate
> implementation in ash calls.

OK, how about this?

[commit-bot: I haz the power, 2016-11-23 23:04:52]

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

[sky, 2016-11-23 23:58:18]

LGTM

[commit-bot: I haz the power, 2016-11-24 01:06:57]

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

[commit-bot: I haz the power, 2016-11-24 01:06:58]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[dmazzoni, 2016-11-28 07:50:27]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-11-28 07:50:28]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org
Link to the patchset: https://codereview.chromium.org/2493923002/#ps60001
(title: "Add TestAccessibilityEventDelegate")

[commit-bot: I haz the power, 2016-11-28 07:50:42]

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-11-28 07:56:36]

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

[commit-bot: I haz the power, 2016-11-28 07:56:37]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dmazzoni, 2016-11-28 17:44:04]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-11-28 17:44:04]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2493923002/#ps80001
(title: "Fix presubmit warning")

[commit-bot: I haz the power, 2016-11-28 17:44:33]

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-11-28 22:39:45]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480355044052720,
"parent_rev": "4354d02787184c30c68bb597f0c8544936d3f9c3", "commit_rev":
"d92d59c7b298cd0739573142be3d667f5f069103"}

[commit-bot: I haz the power, 2016-11-28 22:40:11]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-28 22:44:20]

Description was changed from

==========
Select-to-speak event handler

This change implements the key C++ portion of the Select-to-speak feature
by adding an event handler that takes Search plus the mouse button, and
fires accessibility events instead.

In the future we could modify or extend this event handler for use in
ChromeVox too, but for now I think building something specifically for
Select-to-speak makes sense.

Adds new accessibility events for mouse down, mouse up, and so on - allowing
the select-to-speak extension to get not only the mouse events and global
coordinates, but most importantly the proper accessibility object that's hit
for each event.

This change implements support for hit testing in Views. For web,
the extension will get the event fired on the views::WebView and it can do
its own hit testing within the accessibility tree from there. A follow-up
will improve on this by doing native hit testing in the web before firing
the accessibility event.

BUG=593887
==========

to

==========
Select-to-speak event handler

This change implements the key C++ portion of the Select-to-speak feature
by adding an event handler that takes Search plus the mouse button, and
fires accessibility events instead.

In the future we could modify or extend this event handler for use in
ChromeVox too, but for now I think building something specifically for
Select-to-speak makes sense.

Adds new accessibility events for mouse down, mouse up, and so on - allowing
the select-to-speak extension to get not only the mouse events and global
coordinates, but most importantly the proper accessibility object that's hit
for each event.

This change implements support for hit testing in Views. For web,
the extension will get the event fired on the views::WebView and it can do
its own hit testing within the accessibility tree from there. A follow-up
will improve on this by doing native hit testing in the web before firing
the accessibility event.

BUG=593887

Committed: https://crrev.com/735c79df7859362bd059b5c233286910eff25ef9
Cr-Commit-Position: refs/heads/master@{#434744}
==========

[commit-bot: I haz the power, 2016-11-28 22:44:22]

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