Add key based scrolling on magnified screen for kiosk mode.

Add key based scrolling on magnified screen for kiosk mode.

* Add MagnifierKeyScroller event handler that interprets
  click and hold action.
* kAshEnableMagnifierKeyScroller to test on desktop.
* Modified EventGenerator to generate native event for key press/release.

BUG=340729
TEST=MagnifierKeyScrollerTest

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249996

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250017

[oshima, 2014-02-06 22:57:31]

https://codereview.chromium.org/155493002/diff/320001/ash/magnifier/magnifica...
File ash/magnifier/magnification_controller.h (left):

https://codereview.chromium.org/155493002/diff/320001/ash/magnifier/magnifica...
ash/magnifier/magnification_controller.h:48: virtual void
EnsurePointIsVisible(const gfx::Point& point, bool animate) = 0;
They were dead code.

[xiyuan, 2014-02-06 23:35:58]

https://codereview.chromium.org/155493002/diff/300001/ash/accelerators/accele...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/155493002/diff/300001/ash/accelerators/accele...
ash/accelerators/accelerator_table.cc:446: { false, ui::VKEY_NUMPAD9,
ui::EF_NONE, MAGNIFY_SCREEN_STOP_SCROLL },
nit: Should we use 8 for up, 4 for left, 6 for right and 2 for down? Then the
directions are inline with a real numpad. And if we want to open it for other
keyboard, we don't need to change.

https://codereview.chromium.org/155493002/diff/300001/ash/magnifier/magnifica...
File ash/magnifier/magnification_controller.h (right):

https://codereview.chromium.org/155493002/diff/300001/ash/magnifier/magnifica...
ash/magnifier/magnification_controller.h:49: enum ScrollDirection {
nit: enum goes the before section of methods.

https://codereview.chromium.org/155493002/diff/300001/ash/magnifier/magnifica...
ash/magnifier/magnification_controller.h:78: 
nit: Move the empty line before "protected:"

https://codereview.chromium.org/155493002/diff/300001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/ash_init.cc (right):

https://codereview.chromium.org/155493002/diff/300001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/ash_init.cc:81:
SetMagnifierAcceleratorsEnabled(chrome::IsRunningInForcedAppMode());
We always start from login screen where IsRunningInForcedAppMode() returns
false. This should be moved to UserManagerImpl::KioskAppLoggedIn.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

https://codereview.chromium.org/155493002/diff/300001/chrome/browser/ui/ash/e...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/155493002/diff/300001/chrome/browser/ui/ash/e...
chrome/browser/ui/ash/event_rewriter.cc:1: 
nit: undo this

https://codereview.chromium.org/155493002/diff/300001/chrome/browser/ui/ash/e...
chrome/browser/ui/ash/event_rewriter.cc:195: // Modify RELEASED event to PRESSED
event.
Would we lost a ET_KEY_RELEASED key event? My worry is that we might break the
app/code where it expects a release event, e.g. some code wants to do action on
"keyup" event.

https://codereview.chromium.org/155493002/diff/300001/chrome/browser/ui/ash/e...
File chrome/browser/ui/ash/event_rewriter_unittest.cc (right):

https://codereview.chromium.org/155493002/diff/300001/chrome/browser/ui/ash/e...
chrome/browser/ui/ash/event_rewriter_unittest.cc:2471:
ash::MagnificationController::ScopedMagnifierAcceleratorsEnablerForTest enabler;
nit: wrap on 80 col?

[oshima, 2014-02-07 00:49:24]

https://codereview.chromium.org/155493002/diff/300001/ash/accelerators/accele...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/155493002/diff/300001/ash/accelerators/accele...
ash/accelerators/accelerator_table.cc:446: { false, ui::VKEY_NUMPAD9,
ui::EF_NONE, MAGNIFY_SCREEN_STOP_SCROLL },
On 2014/02/06 23:35:58, xiyuan wrote:
> nit: Should we use 8 for up, 4 for left, 6 for right and 2 for down? Then the
> directions are inline with a real numpad. And if we want to open it for other
> keyboard, we don't need to change.

So this is for remapped keys, not really meant to be used on num pad.

The accelerator keys are combined with SHIFT keys, and if a user already can
use normal arrow + shift key to control on other keyboards.

https://codereview.chromium.org/155493002/diff/300001/ash/magnifier/magnifica...
File ash/magnifier/magnification_controller.h (right):

https://codereview.chromium.org/155493002/diff/300001/ash/magnifier/magnifica...
ash/magnifier/magnification_controller.h:49: enum ScrollDirection {
On 2014/02/06 23:35:58, xiyuan wrote:
> nit: enum goes the before section of methods.

Done.

https://codereview.chromium.org/155493002/diff/300001/ash/magnifier/magnifica...
ash/magnifier/magnification_controller.h:78: 
On 2014/02/06 23:35:58, xiyuan wrote:
> nit: Move the empty line before "protected:"

Done.

https://codereview.chromium.org/155493002/diff/300001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/ash_init.cc (right):

https://codereview.chromium.org/155493002/diff/300001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/ash_init.cc:81:
SetMagnifierAcceleratorsEnabled(chrome::IsRunningInForcedAppMode());
On 2014/02/06 23:35:58, xiyuan wrote:
> We always start from login screen where IsRunningInForcedAppMode() returns
> false. This should be moved to UserManagerImpl::KioskAppLoggedIn.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

Moved to chrome_shell_delegate_chromeos as discussed offline.

https://codereview.chromium.org/155493002/diff/300001/chrome/browser/ui/ash/e...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/155493002/diff/300001/chrome/browser/ui/ash/e...
chrome/browser/ui/ash/event_rewriter.cc:1: 
On 2014/02/06 23:35:58, xiyuan wrote:
> nit: undo this

Done.

https://codereview.chromium.org/155493002/diff/300001/chrome/browser/ui/ash/e...
chrome/browser/ui/ash/event_rewriter.cc:195: // Modify RELEASED event to PRESSED
event.
On 2014/02/06 23:35:58, xiyuan wrote:
> Would we lost a ET_KEY_RELEASED key event? My worry is that we might break the
> app/code where it expects a release event, e.g. some code wants to do action
on
> "keyup" event.

I'm doing this only for the arrow + shift that we have control.
I agree that we don't want to do this in general.

https://codereview.chromium.org/155493002/diff/300001/chrome/browser/ui/ash/e...
File chrome/browser/ui/ash/event_rewriter_unittest.cc (right):

https://codereview.chromium.org/155493002/diff/300001/chrome/browser/ui/ash/e...
chrome/browser/ui/ash/event_rewriter_unittest.cc:2471:
ash::MagnificationController::ScopedMagnifierAcceleratorsEnablerForTest enabler;
On 2014/02/06 23:35:58, xiyuan wrote:
> nit: wrap on 80 col?

Yeah, the next patch had a fix

[xiyuan, 2014-02-07 01:01:03]

lgtm

[oshima, 2014-02-07 01:40:09]

sadrul -> ui/event

[sadrul, 2014-02-07 03:04:47]

https://codereview.chromium.org/155493002/diff/300002/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/155493002/diff/300002/ui/events/event.h#newco...
ui/events/event.h:53: void set_type(EventType type) { type_ = type; }
We deliberately don't allow changing the event-type in the API (see the
protected SetType()).

I haven't looked at how the event-rewriter works, but there should be some way
to do "Add key based scrolling on magnified screen for kiosk mode." that doesn't
require this.

[oshima, 2014-02-07 05:55:30]

On 2014/02/07 03:04:47, sadrul wrote:
> https://codereview.chromium.org/155493002/diff/300002/ui/events/event.h
> File ui/events/event.h (right):
> 
>
https://codereview.chromium.org/155493002/diff/300002/ui/events/event.h#newco...
> ui/events/event.h:53: void set_type(EventType type) { type_ = type; }
> We deliberately don't allow changing the event-type in the API (see the
> protected SetType()).
> 
> I haven't looked at how the event-rewriter works, but there should be some way
> to do "Add key based scrolling on magnified screen for kiosk mode." that
doesn't
> require this.

Hotrod (chromebox for meeting) has a control pad with cursor keys, which is
already programmed to generate shift + arrow keys. This is normally used to move
focus between buttons, textfield, and we
want to use the same key's hold action to scroll the screen when magnifier is
enabled.

In order to avoid the focus move operation in this scenario, I need to rewrite
events so that we first ignores pressed event and then generates a pressed event
for release event.

This is a simplest way to implement the desired behavior (from hotrod point of
view).
What is the issue in using SetType()?  Technically, it's possible to implement
the similar behavior without rewriting the type, but it'll require putting more
hotrod specific code into chrome, which I would like to avoid if possible.

[sadrul, 2014-02-07 06:14:44]

On 2014/02/07 05:55:30, oshima wrote:
> On 2014/02/07 03:04:47, sadrul wrote:
> > https://codereview.chromium.org/155493002/diff/300002/ui/events/event.h
> > File ui/events/event.h (right):
> > 
> >
>
https://codereview.chromium.org/155493002/diff/300002/ui/events/event.h#newco...
> > ui/events/event.h:53: void set_type(EventType type) { type_ = type; }
> > We deliberately don't allow changing the event-type in the API (see the
> > protected SetType()).
> > 
> > I haven't looked at how the event-rewriter works, but there should be some
way
> > to do "Add key based scrolling on magnified screen for kiosk mode." that
> doesn't
> > require this.
> 
> Hotrod (chromebox for meeting) has a control pad with cursor keys, which is
> already programmed to generate shift + arrow keys. This is normally used to
move
> focus between buttons, textfield, and we
> want to use the same key's hold action to scroll the screen when magnifier is
> enabled.
> 
> In order to avoid the focus move operation in this scenario, I need to rewrite
> events so that we first ignores pressed event and then generates a pressed
event
> for release event.

Why do you need to convert the released event into a pressed event? To ensure
that it triggers the accelerator? Why not consume both the pressed and release
events, and trigger the accelerator-action by name (using
AcceleratorController::PerformAction()) when consuming the release event?

> 
> This is a simplest way to implement the desired behavior (from hotrod point of
> view).
> What is the issue in using SetType()?

Changing the type of an event when it's in the middle of being dispatched is
bad, and will cause hard-to-find/understand bugs.

> Technically, it's possible to implement
> the similar behavior without rewriting the type, but it'll require putting
more
> hotrod specific code into chrome, which I would like to avoid if possible.

[xiyuan, 2014-02-07 06:39:16]

On 2014/02/07 06:14:44, sadrul wrote:
> Why do you need to convert the released event into a pressed event? To ensure
> that it triggers the accelerator? Why not consume both the pressed and release
> events, and trigger the accelerator-action by name (using
> AcceleratorController::PerformAction()) when consuming the release event?
> 

The CL eats the initial pressed event before it can tell whether it is a short
pressed-released case or a press-hold-released case. The later hold case is used
to generate accelerators for moving magnifier viewport. The event type
conversion happens for the former case because we need to put back the pressed
events. It is not ideal but works with the current code that moves focus on the
pressed events.

[oshima, 2014-02-07 07:58:50]

On 2014/02/07 06:14:44, sadrul wrote:
> On 2014/02/07 05:55:30, oshima wrote:
> > On 2014/02/07 03:04:47, sadrul wrote:
> > > https://codereview.chromium.org/155493002/diff/300002/ui/events/event.h
> > > File ui/events/event.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/155493002/diff/300002/ui/events/event.h#newco...
> > > ui/events/event.h:53: void set_type(EventType type) { type_ = type; }
> > > We deliberately don't allow changing the event-type in the API (see the
> > > protected SetType()).
> > > 
> > > I haven't looked at how the event-rewriter works, but there should be some
> way
> > > to do "Add key based scrolling on magnified screen for kiosk mode." that
> > doesn't
> > > require this.
> > 
> > Hotrod (chromebox for meeting) has a control pad with cursor keys, which is
> > already programmed to generate shift + arrow keys. This is normally used to
> move
> > focus between buttons, textfield, and we
> > want to use the same key's hold action to scroll the screen when magnifier
is
> > enabled.
> > 
> > In order to avoid the focus move operation in this scenario, I need to
rewrite
> > events so that we first ignores pressed event and then generates a pressed
> event
> > for release event.
> 
> Why do you need to convert the released event into a pressed event? To ensure
> that it triggers the accelerator? Why not consume both the pressed and release
> events, and trigger the accelerator-action by name (using
> AcceleratorController::PerformAction()) when consuming the release event?

This particular event is not for accelerator. It is to be consumed by an app, so
this does not work.

The other events, however, can be handled by PerformAction, and it's probably
better that way.
I'll change that.

> > This is a simplest way to implement the desired behavior (from hotrod point
of
> > view).
> > What is the issue in using SetType()?
> 
> Changing the type of an event when it's in the middle of being dispatched is
> bad, and will cause hard-to-find/understand bugs.

It'd be bad if we change the key event to, say, motion event because they
belongs to different event class, but this particular rewrite stays in the same
class. Can we allow only a subset (say only press/release)?

If this doesn't work for you, then we need to change the app to respond to
release event.
(which may or may not be fine. I will to ask hotrod team)

> > Technically, it's possible to implement
> > the similar behavior without rewriting the type, but it'll require putting
> more
> > hotrod specific code into chrome, which I would like to avoid if possible.

[oshima, 2014-02-07 17:29:16]

sadrul, I changed so that we only allow ET_KEY_PRESSED to ET_KEY_RELEASED
change. Let me know if this works for you.

[oshima, 2014-02-08 00:48:47]

xiyuan, I'm still working on tests, but the rest is ready for review. PTAL.

[xiyuan, 2014-02-08 01:36:28]

https://codereview.chromium.org/155493002/diff/810001/ash/magnifier/magnifier...
File ash/magnifier/magnifier_key_scroller.cc (right):

https://codereview.chromium.org/155493002/diff/810001/ash/magnifier/magnifier...
ash/magnifier/magnifier_key_scroller.cc:47: aura::Window* target =
*(tracker->windows().begin());
Check tracker->windows().empty()  before doing this, just in case the window is
gone.

https://codereview.chromium.org/155493002/diff/810001/ash/magnifier/magnifier...
ash/magnifier/magnifier_key_scroller.cc:83: bool enabled) {
nit: this could fit on previous line.

[oshima, 2014-02-08 02:14:19]

added test.

https://codereview.chromium.org/155493002/diff/810001/ash/magnifier/magnifier...
File ash/magnifier/magnifier_key_scroller.cc (right):

https://codereview.chromium.org/155493002/diff/810001/ash/magnifier/magnifier...
ash/magnifier/magnifier_key_scroller.cc:47: aura::Window* target =
*(tracker->windows().begin());
On 2014/02/08 01:36:28, xiyuan wrote:
> Check tracker->windows().empty()  before doing this, just in case the window
is
> gone.

Thank you for the catch. I used Tracker to check it but forgot to really check
it ;)

https://codereview.chromium.org/155493002/diff/810001/ash/magnifier/magnifier...
ash/magnifier/magnifier_key_scroller.cc:83: bool enabled) {
On 2014/02/08 01:36:28, xiyuan wrote:
> nit: this could fit on previous line.

Done.

[oshima, 2014-02-08 02:15:48]

sadrul, can you review ui/aura/test change?

https://codereview.chromium.org/155493002/diff/890001/ash/magnifier/magnifier...
File ash/magnifier/magnifier_key_scroller.cc (right):

https://codereview.chromium.org/155493002/diff/890001/ash/magnifier/magnifier...
ash/magnifier/magnifier_key_scroller.cc:56: dispatcher.set_target(target);
sadrul, I'm not sure if this is right. let me know.

[xiyuan, 2014-02-08 04:09:34]

LGTM!

[sadrul, 2014-02-08 16:34:02]

https://codereview.chromium.org/155493002/diff/790013/ash/magnifier/magnifier...
File ash/magnifier/magnifier_key_scroller.cc (right):

https://codereview.chromium.org/155493002/diff/790013/ash/magnifier/magnifier...
ash/magnifier/magnifier_key_scroller.cc:49: return;
|tracker| leaks here, right?

Why not use a scoped_ptr<WindowTracker> in PostPressedEvent, and bind it with
base::Passed? That way, you don't have to explicitly delete it here, and avoid
this leak.

https://codereview.chromium.org/155493002/diff/790013/ash/magnifier/magnifier...
ash/magnifier/magnifier_key_scroller.cc:56: dispatcher.set_target(target);
You shouldn't need to use DispatcherApi here (and using it here should not have
any effect, since OnEventFromSource() will find the target and dispatch to
that).

[sadrul, 2014-02-08 16:41:58]

ui/aura/ change lgtm

(Note: there are a couple of comments in ash land in the previous review)

[oshima, 2014-02-09 02:06:17]

https://codereview.chromium.org/155493002/diff/790013/ash/magnifier/magnifier...
File ash/magnifier/magnifier_key_scroller.cc (right):

https://codereview.chromium.org/155493002/diff/790013/ash/magnifier/magnifier...
ash/magnifier/magnifier_key_scroller.cc:49: return;
On 2014/02/08 16:34:03, sadrul wrote:
> |tracker| leaks here, right?
> 
> Why not use a scoped_ptr<WindowTracker> in PostPressedEvent, and bind it with
> base::Passed? That way, you don't have to explicitly delete it here, and avoid
> this leak.

good idea. done.

https://codereview.chromium.org/155493002/diff/790013/ash/magnifier/magnifier...
ash/magnifier/magnifier_key_scroller.cc:56: dispatcher.set_target(target);
On 2014/02/08 16:34:03, sadrul wrote:
> You shouldn't need to use DispatcherApi here (and using it here should not
have
> any effect, since OnEventFromSource() will find the target and dispatch to
> that).

Thanks, done.

[oshima, 2014-02-09 02:07:03]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-02-09 02:07:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/155493002/1030001

[commit-bot: I haz the power, 2014-02-09 10:53:29]

Change committed as 249996

[Vitaly Buka (NO REVIEWS), 2014-02-09 22:48:06]

On 2014/02/09 10:53:29, I haz the power (commit-bot) wrote:
> Change committed as 249996

It was blink roll that breaks Fullscreen tests. Putting CL to CQ.

[Vitaly Buka (NO REVIEWS), 2014-02-09 22:48:12]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-02-09 22:48:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/155493002/1030001

[commit-bot: I haz the power, 2014-02-09 22:50:45]

Change committed as 250017