Fix the ChromeVox "sticky key" (pressing Search twice).

Fix the ChromeVox "sticky key" (pressing Search twice).

This broke due to r285254 (http://crbug.com/391008), the fix is trivial.

BUG=408809
Committed: https://crrev.com/c01d045bd9b259e5746a3d237965178b47feddda
Cr-Commit-Position: refs/heads/master@{#293350}

[dmazzoni, 2014-09-03 23:08:14]

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

[dmazzoni, 2014-09-03 23:08:15]

Test coming in separate change, could you look over the logic for how I fixed
this first?

The only reason for this change was because pressing Search now sets the metaKey
flag, so this fixes it so the generated event matches what's in the key map.

[David Tseng, 2014-09-04 15:33:33]

https://codereview.chromium.org/540583002/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/chromevox/common/key_sequence.js (right):

https://codereview.chromium.org/540583002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/common/key_sequence.js:390: return
(!cvox.ChromeVox.isChromeOS &&
This line of fixes worries me and seems to say that it's a poor choice to
simulate touch exploration as the "meta" key. This will probably have some
negative consequences for the way we want to do commands in ChromeVox next for
example.

As for this temp fix, why not just unify this with the other todo'ed fix you
have and just strip metakey entirely when on Chrome OS (just earlier in the
event processing)?

[dmazzoni, 2014-09-04 15:42:53]

On 2014/09/04 15:33:33, David Tseng wrote:
>
https://codereview.chromium.org/540583002/diff/1/chrome/browser/resources/chr...
> File chrome/browser/resources/chromeos/chromevox/common/key_sequence.js
(right):
> 
>
https://codereview.chromium.org/540583002/diff/1/chrome/browser/resources/chr...
> chrome/browser/resources/chromeos/chromevox/common/key_sequence.js:390: return
> (!cvox.ChromeVox.isChromeOS &&
> This line of fixes worries me and seems to say that it's a poor choice to
> simulate touch exploration as the "meta" key. This will probably have some
> negative consequences for the way we want to do commands in ChromeVox next for
> example.

These fixes aren't really a consequence of touch exploration using the meta key,
they're a consequence of making it so that search key acts as a meta modifier
key.

> As for this temp fix, why not just unify this with the other todo'ed fix you
> have and just strip metakey entirely when on Chrome OS (just earlier in the
> event processing)?

OK, but I think we should do the opposite and get rid of the code that tracks
whether or not the search key is down based on its key code.

I want to fix it the right way, but we also need to fix 38 without breaking
touch exploration and without needing to merge a really large changelist.

How about this idea: since the meta key change triggered a bunch of these bugs,
how about a small changelist that reverts the meta key change, and makes touch
exploration send the alt key instead? That'd be a less complicated fix, we could
merge that to 38, and then go about fixing the meta and search key confusion in
39 the right way.

[David Tseng, 2014-09-04 16:07:57]

On 2014/09/04 15:42:53, dmazzoni wrote:
> On 2014/09/04 15:33:33, David Tseng wrote:
> >
>
https://codereview.chromium.org/540583002/diff/1/chrome/browser/resources/chr...
> > File chrome/browser/resources/chromeos/chromevox/common/key_sequence.js
> (right):
> > 
> >
>
https://codereview.chromium.org/540583002/diff/1/chrome/browser/resources/chr...
> > chrome/browser/resources/chromeos/chromevox/common/key_sequence.js:390:
return
> > (!cvox.ChromeVox.isChromeOS &&
> > This line of fixes worries me and seems to say that it's a poor choice to
> > simulate touch exploration as the "meta" key. This will probably have some
> > negative consequences for the way we want to do commands in ChromeVox next
for
> > example.
> 
> These fixes aren't really a consequence of touch exploration using the meta
key,
> they're a consequence of making it so that search key acts as a meta modifier
> key.
> 
> > As for this temp fix, why not just unify this with the other todo'ed fix you
> > have and just strip metakey entirely when on Chrome OS (just earlier in the
> > event processing)?
> 
> OK, but I think we should do the opposite and get rid of the code that tracks
> whether or not the search key is down based on its key code.
> 
> I want to fix it the right way, but we also need to fix 38 without breaking
> touch exploration and without needing to merge a really large changelist.
> 
> How about this idea: since the meta key change triggered a bunch of these
bugs,
> how about a small changelist that reverts the meta key change, and makes touch
> exploration send the alt key instead? That'd be a less complicated fix, we
could
> merge that to 38, and then go about fixing the meta and search key confusion
in
> 39 the right way.

I'm also concerned with some other event listener getting these "keys"; can that
happen?

[David Tseng, 2014-09-04 16:11:20]

On 2014/09/04 16:07:57, David Tseng wrote:
> On 2014/09/04 15:42:53, dmazzoni wrote:
> > On 2014/09/04 15:33:33, David Tseng wrote:
> > >
> >
>
https://codereview.chromium.org/540583002/diff/1/chrome/browser/resources/chr...
> > > File chrome/browser/resources/chromeos/chromevox/common/key_sequence.js
> > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/540583002/diff/1/chrome/browser/resources/chr...
> > > chrome/browser/resources/chromeos/chromevox/common/key_sequence.js:390:
> return
> > > (!cvox.ChromeVox.isChromeOS &&
> > > This line of fixes worries me and seems to say that it's a poor choice to
> > > simulate touch exploration as the "meta" key. This will probably have some
> > > negative consequences for the way we want to do commands in ChromeVox next
> for
> > > example.
> > 
> > These fixes aren't really a consequence of touch exploration using the meta
> key,
> > they're a consequence of making it so that search key acts as a meta
modifier
> > key.
> > 
> > > As for this temp fix, why not just unify this with the other todo'ed fix
you
> > > have and just strip metakey entirely when on Chrome OS (just earlier in
the
> > > event processing)?
> > 
> > OK, but I think we should do the opposite and get rid of the code that
tracks
> > whether or not the search key is down based on its key code.
> > 
> > I want to fix it the right way, but we also need to fix 38 without breaking
> > touch exploration and without needing to merge a really large changelist.
> > 
> > How about this idea: since the meta key change triggered a bunch of these
> bugs,
> > how about a small changelist that reverts the meta key change, and makes
touch
> > exploration send the alt key instead? That'd be a less complicated fix, we
> could
> > merge that to 38, and then go about fixing the meta and search key confusion
> in
> > 39 the right way.
> 
> I'm also concerned with some other event listener getting these "keys"; can
that
> happen?

On second thought, I think getting this fix as is seems ok. I don't know if alt
or meta is less dangerous. 
The comment regarding stripping meta key earlier was just so it would be a lot
easier to remove once we get rid of the search key detection logic entirely.

[dmazzoni, 2014-09-04 16:13:03]

> > I'm also concerned with some other event listener getting these "keys"; can
> that
> > happen?

Can you clarify what you mean by this? Are you talking about as a result of this
fix, or as a result of the switch to use meta as a modifier?

[dmazzoni, 2014-09-04 17:58:24]

This is a cleaner fix. It just removes Chrome OS as a special case and handles
the meta key the same way on all platforms.

I just tested sticky keys, search+shift keys, and prefix key with arrows,
single-letter, and two-letter combinations and they all work.

[David Tseng, 2014-09-04 18:02:27]

lgtm

[dmazzoni, 2014-09-04 18:11:42]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-09-04 18:13:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/540583002/20001

[commit-bot: I haz the power, 2014-09-04 22:03:38]

Committed patchset #2 (id:20001) as 62d701807014fbf733088d61a9ef781118c752e3

[commit-bot: I haz the power, 2014-09-10 03:33:05]

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