Support Compat mode inside of the desktop tree.

Support Compat mode inside of the desktop tree.

Depends on
https://codereview.chromium.org/1185753008/

Removes native bindings for Search+Shift+Arrows in favor of the extension commands bindings. Enables all ChromeVox Compat/Next commands inside of the desktop tree. Adds a test to exercise next element and next button commands.

Also, whitelists root nodes without urls for ChromeVox compat. This includes the shelf/launcher.

TEST=SpokenFeedbackTest.*

Committed: https://crrev.com/e7b1c72c03dcf636e50917b2c8122e5d0ef884b6
Cr-Commit-Position: refs/heads/master@{#336181}

[David Tseng, 2015-06-16 16:42:43]

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

[David Tseng, 2015-06-16 16:42:44]

Ready for a quick look. Would like any ideas you may have on traversal.
Currently, moving next/previous does not match focus traversal and goes off into
non-focusable nodes. Some ideas were to restrict within containers with some
annotations or a list of roles.

[dmazzoni, 2015-06-17 19:29:58]

https://codereview.chromium.org/1191783002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc
(right):

https://codereview.chromium.org/1191783002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc:54:
iter->second.accelerator().modifiers() == modifiers)
Should you be filtering the second accelerator modifiers the same way?

Should there maybe be a global constant for "all modifier keys" in
event_constants.h?

https://codereview.chromium.org/1191783002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/host/chrome/host.js (right):

https://codereview.chromium.org/1191783002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/host/chrome/host.js:125: if
(!cvox.ChromeVox.documentHasFocus()) {
Are there messages you do want to listen to here, like activate / inactivate?

[David Tseng, 2015-06-17 21:15:13]

https://codereview.chromium.org/1191783002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc
(right):

https://codereview.chromium.org/1191783002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/spoken_feedback_event_rewriter.cc:54:
iter->second.accelerator().modifiers() == modifiers)
On 2015/06/17 19:29:58, dmazzoni wrote:
> Should you be filtering the second accelerator modifiers the same way?
Done. Done for the key event to make the test pass, but doesn't seem to hurt to
do it for the commands accelerator in case a modifier ever gets added.

> 
> Should there maybe be a global constant for "all modifier keys" in
> event_constants.h?
> 

Done.

https://codereview.chromium.org/1191783002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/host/chrome/host.js (right):

https://codereview.chromium.org/1191783002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/host/chrome/host.js:125: if
(!cvox.ChromeVox.documentHasFocus()) {
On 2015/06/17 19:29:58, dmazzoni wrote:
> Are there messages you do want to listen to here, like activate / inactivate?

toggling ChromeVox is now special cased; it turns out that on Chrome OS that
commnad is disallowed. Any others come to mind?

[dmazzoni, 2015-06-18 16:45:50]

https://codereview.chromium.org/1191783002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/host/chrome/host.js (right):

https://codereview.chromium.org/1191783002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/host/chrome/host.js:125: if
(!cvox.ChromeVox.documentHasFocus()) {
On 2015/06/17 21:15:13, David Tseng wrote:
> On 2015/06/17 19:29:58, dmazzoni wrote:
> > Are there messages you do want to listen to here, like activate /
inactivate?
> 
> toggling ChromeVox is now special cased; it turns out that on Chrome OS that
> commnad is disallowed. Any others come to mind?

This source file runs on other platforms, though. You don't want to break
ChromeVox Classic on Mac with this change.

[David Tseng, 2015-06-18 17:32:13]

PTAL.

I went with a slightly different approach. There were too many subtle issues
with sending commands from the background page like sequencing not working in
exactly the same way, classic's sticky key breaking, etc; there seemed to also
be some performance issues and a few crashes due to us calling focus() on
automation nodes that no longer exist. We still need to fix some of those
issues, but not in this cl.

The changes.
It comes down to the various modes we're supporting.

In classic mode, we do not add a onCommands listener. In the native event
rewriter, whenever there's no onCommands listener, we let the event continue and
the content script eventually receives and does what it needs to with the keys.

We ensure we don't disable the content script whenever transitioning to compat
mode with focus outside of web content. I tried enabling / disabling the content
script whenever transitions from classic to compat and vice versa, and it caused
lots of user perceivable lag.

This whole code path is still being exercised by the addition to
SpokenFeedbackTest

https://codereview.chromium.org/1191783002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/host/chrome/host.js (right):

https://codereview.chromium.org/1191783002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/host/chrome/host.js:125: if
(!cvox.ChromeVox.documentHasFocus()) {
On 2015/06/18 16:45:50, dmazzoni wrote:
> On 2015/06/17 21:15:13, David Tseng wrote:
> > On 2015/06/17 19:29:58, dmazzoni wrote:
> > > Are there messages you do want to listen to here, like activate /
> inactivate?
> > 
> > toggling ChromeVox is now special cased; it turns out that on Chrome OS that
> > commnad is disallowed. Any others come to mind?
> 
> This source file runs on other platforms, though. You don't want to break
> ChromeVox Classic on Mac with this change.

If there's another sender of this message, then yes, this would break. I think
braille commands originating from the background page uses a different message
('brailleEvt').

With the latest patchset, I'm no longer sending ChromeVox commands from the
background page.

[dmazzoni, 2015-06-18 22:23:41]

This approach lgm

https://codereview.chromium.org/1191783002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
(right):

https://codereview.chromium.org/1191783002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:326:
var actionNode = current.getStart().getNode();
Is this change necessary for this to succeed?

https://codereview.chromium.org/1191783002/diff/120001/ui/events/event_consta...
File ui/events/event_constants.h (right):

https://codereview.chromium.org/1191783002/diff/120001/ui/events/event_consta...
ui/events/event_constants.h:98: EF_SHIFT_DOWN | EF_CONTROL_DOWN | EF_ALT_DOWN |
EF_COMMAND_DOWN,
I wonder if ALTGR and MOD3 should be included?

If you're not sure, you can just define this constant in the file where you use
it, for now.

[dmazzoni, 2015-06-18 22:23:48]

lgtm

[David Tseng, 2015-06-22 19:06:45]

Please take a look at the parent change.

I separated out the cl into pieces so that I could continue working without
removing the native ChromeVox accelerators. This change actually does the
removal whenever we're ready for that and adds a SpokenFeedbackTest change to
exercise the event re-writing.



On 2015/06/18 22:23:48, dmazzoni wrote:
> lgtm

[David Tseng, 2015-06-24 20:40:47]

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

[David Tseng, 2015-06-24 20:40:48]

+ oshima for ash/accelerators/*

[oshima, 2015-06-24 22:14:33]

https://codereview.chromium.org/1191783002/diff/200001/ash/accelerators/accel...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/1191783002/diff/200001/ash/accelerators/accel...
ash/accelerators/accelerator_controller.cc:82: inline ui::Accelerator
CreateAccelerator(ui::KeyboardCode keycode,
remove "inline"

https://codereview.chromium.org/1191783002/diff/200001/ash/accelerators/accel...
ash/accelerators/accelerator_controller.cc:103: }
These changes were reverted yesterday
(https://codereview.chromium.org/1202923002).

Can you sync again?

[oshima, 2015-06-24 22:14:50]

On 2015/06/24 22:14:33, oshima wrote:
>
https://codereview.chromium.org/1191783002/diff/200001/ash/accelerators/accel...
> File ash/accelerators/accelerator_controller.cc (right):
> 
>
https://codereview.chromium.org/1191783002/diff/200001/ash/accelerators/accel...
> ash/accelerators/accelerator_controller.cc:82: inline ui::Accelerator
> CreateAccelerator(ui::KeyboardCode keycode,
> remove "inline"
sorry ignore this part.

> 
>
https://codereview.chromium.org/1191783002/diff/200001/ash/accelerators/accel...
> ash/accelerators/accelerator_controller.cc:103: }
> These changes were reverted yesterday
> (https://codereview.chromium.org/1202923002).
> 
> Can you sync again?

[David Tseng, 2015-06-24 22:24:27]

https://codereview.chromium.org/1191783002/diff/200001/ash/accelerators/accel...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/1191783002/diff/200001/ash/accelerators/accel...
ash/accelerators/accelerator_controller.cc:82: inline ui::Accelerator
CreateAccelerator(ui::KeyboardCode keycode,
On 2015/06/24 22:14:33, oshima wrote:
> remove "inline"

Acknowledged.

https://codereview.chromium.org/1191783002/diff/200001/ash/accelerators/accel...
ash/accelerators/accelerator_controller.cc:103: }
On 2015/06/24 22:14:33, oshima wrote:
> These changes were reverted yesterday
> (https://codereview.chromium.org/1202923002).
> 
> Can you sync again?

Done.

[oshima, 2015-06-24 22:35:22]

ash lgtm

[David Tseng, 2015-06-24 22:49:01]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2015-06-24 22:49:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1191783002/#ps220001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-06-24 22:49:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1191783002/220001

[commit-bot: I haz the power, 2015-06-24 23:44:12]

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

[commit-bot: I haz the power, 2015-06-24 23:44:13]

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

[David Tseng, 2015-06-25 16:07:41]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2015-06-25 16:09:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1191783002/220001

[commit-bot: I haz the power, 2015-06-25 17:10:18]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-06-25 17:11:10]

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

[David Tseng, 2015-06-25 17:49:46]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/1213513003/ by dtseng@chromium.org.

The reason for reverting is: Parent change reverted.
https://codereview.chromium.org/1185753008/.

[Mike Wittman, 2015-06-25 18:06:05]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/1211023002/ by wittman@chromium.org.

The reason for reverting is: Crashes tests on Linux ChromiumOS Tests (1):
TestAsNormalAndGuestUser_SpokenFeedbackTest.NavigateSystemTray_0
TestAsNormalAndGuestUser_SpokenFeedbackTest.NavigateSystemTray_1

https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%....

[David Tseng, 2015-06-25 18:12:23]

Already reverted.

On Thu, Jun 25, 2015 at 11:06 AM, <wittman@chromium.org> wrote:

> A revert of this CL (patchset #12 id:220001) has been created in
> https://codereview.chromium.org/1211023002/ by wittman@chromium.org.
>
> The reason for reverting is: Crashes tests on Linux ChromiumOS Tests (1):
> TestAsNormalAndGuestUser_SpokenFeedbackTest.NavigateSystemTray_0
> TestAsNormalAndGuestUser_SpokenFeedbackTest.NavigateSystemTray_1
>
>
>
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...
> .
>
>
> https://codereview.chromium.org/1191783002/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

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

[Mike Wittman, 2015-06-25 18:30:30]

On 2015/06/25 18:12:23, David Tseng wrote:
> Already reverted.
> 
> On Thu, Jun 25, 2015 at 11:06 AM, <mailto:wittman@chromium.org> wrote:
> 
> > A revert of this CL (patchset #12 id:220001) has been created in
> > https://codereview.chromium.org/1211023002/ by mailto:wittman@chromium.org.
> >
> > The reason for reverting is: Crashes tests on Linux ChromiumOS Tests (1):
> > TestAsNormalAndGuestUser_SpokenFeedbackTest.NavigateSystemTray_0
> > TestAsNormalAndGuestUser_SpokenFeedbackTest.NavigateSystemTray_1
> >
> >
> >
>
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...
> > .
> >
> >
> > https://codereview.chromium.org/1191783002/
> >
> > To unsubscribe from this group and stop receiving emails from it, send an
> > email to mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

FYI, Win7 Tests had a failure on
DumpAccessibilityEventsTest.AccessibilityEventsListboxNext that may also be
related to this CL:
https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%281%29/bui...