Don't handle the tab press event in the window cycle event filter.

Don't handle the tab press event in the window cycle event filter.

Because fullscreen windows are allowed to handle alt+tab, we have to be careful
not to handle the tab press event in the window cycle event filter as it will
be treated as a fullscreen window preventing alt-tabbing.

BUG=622396
TEST=WindowCycleControllerTest.TabPastFullscreenWindow,WindowCycleControllerTest.TabKeyNotLeaked

Committed: https://crrev.com/66b4142cfaf0acc038f220b6288089f8a60b923d
Cr-Commit-Position: refs/heads/master@{#412616}

[flackr, 2016-08-16 14:35:57]

flackr@chromium.org changed reviewers:
+ varkha@chromium.org

[flackr, 2016-08-16 14:35:58]

PTAL, this fixes alt+tab stopping at fullscreen windows. We need to let that
keypress go through because we allow fullscreen windows to handle it. We should
perhaps consider in the future that once alt-tabbing has started letting you tab
past a fullscreen window which handles it but that is out of scope for this fix.

[Evan Stade, 2016-08-16 16:37:48]

On 2016/08/16 14:35:58, flackr wrote:
> PTAL, this fixes alt+tab stopping at fullscreen windows. We need to let that
> keypress go through because we allow fullscreen windows to handle it. We
should
> perhaps consider in the future that once alt-tabbing has started letting you
tab
> past a fullscreen window which handles it but that is out of scope for this
fix.

This will break a patch I have in flight (or may have already landed) which adds
alt-tab auto-repeat. Do we really need to address this problem? I thought that
with the new alt tab ui, the bug no longer occurred.

[flackr, 2016-08-16 18:13:08]

On 2016/08/16 at 16:37:48, estade wrote:
> On 2016/08/16 14:35:58, flackr wrote:
> > PTAL, this fixes alt+tab stopping at fullscreen windows. We need to let that
> > keypress go through because we allow fullscreen windows to handle it. We
should
> > perhaps consider in the future that once alt-tabbing has started letting you
tab
> > past a fullscreen window which handles it but that is out of scope for this
fix.
> 
> This will break a patch I have in flight (or may have already landed) which
adds alt-tab auto-repeat. Do we really need to address this problem? I thought
that with the new alt tab ui, the bug no longer occurred.

Your patch has landed - I've tested with it and it doesn't break the auto-repeat
behavior but it no longer seems to be necessary. That being said, unless your
new UI is going to be merged back into M53 we need to merge this back to fix the
stable blocker.

[flackr, 2016-08-17 17:47:14]

On 2016/08/16 at 18:13:08, flackr wrote:
> On 2016/08/16 at 16:37:48, estade wrote:
> > On 2016/08/16 14:35:58, flackr wrote:
> > > PTAL, this fixes alt+tab stopping at fullscreen windows. We need to let
that
> > > keypress go through because we allow fullscreen windows to handle it. We
should
> > > perhaps consider in the future that once alt-tabbing has started letting
you tab
> > > past a fullscreen window which handles it but that is out of scope for
this fix.
> > 
> > This will break a patch I have in flight (or may have already landed) which
adds alt-tab auto-repeat. Do we really need to address this problem? I thought
that with the new alt tab ui, the bug no longer occurred.
> 
> Your patch has landed - I've tested with it and it doesn't break the
auto-repeat behavior but it no longer seems to be necessary. That being said,
unless your new UI is going to be merged back into M53 we need to merge this
back to fix the stable blocker.

Ping, if this is needed to fix M53 we should land it ASAP.

[varkha, 2016-08-17 18:06:57]

Sorry, didn't realize I am the sole reviewer on this and I am travelling. Have a
nit and a question about some scenario. If you think is already handled, you can
land it.
lgtm in case Alt+Shift+Tab is already handled.

https://codereview.chromium.org/2252673002/diff/1/ash/wm/window_cycle_control...
File ash/wm/window_cycle_controller_unittest.cc (right):

https://codereview.chromium.org/2252673002/diff/1/ash/wm/window_cycle_control...
ash/wm/window_cycle_controller_unittest.cc:632: ui::test::EventGenerator&
generator = GetEventGenerator();
nit: Could swap the line before and above this comment.

https://codereview.chromium.org/2252673002/diff/1/ash/wm/window_cycle_event_f...
File ash/wm/window_cycle_event_filter_aura.cc (right):

https://codereview.chromium.org/2252673002/diff/1/ash/wm/window_cycle_event_f...
ash/wm/window_cycle_event_filter_aura.cc:27: event->type() !=
ui::ET_KEY_PRESSED) {
Does this properly deal with Alt+Shift+Tab?

[Evan Stade, 2016-08-17 18:18:07]

On 2016/08/16 18:13:08, flackr wrote:
> On 2016/08/16 at 16:37:48, estade wrote:
> > On 2016/08/16 14:35:58, flackr wrote:
> > > PTAL, this fixes alt+tab stopping at fullscreen windows. We need to let
that
> > > keypress go through because we allow fullscreen windows to handle it. We
> should
> > > perhaps consider in the future that once alt-tabbing has started letting
you
> tab
> > > past a fullscreen window which handles it but that is out of scope for
this
> fix.
> > 
> > This will break a patch I have in flight (or may have already landed) which
> adds alt-tab auto-repeat. Do we really need to address this problem? I thought
> that with the new alt tab ui, the bug no longer occurred.
> 
> Your patch has landed - I've tested with it and it doesn't break the
auto-repeat
> behavior but it no longer seems to be necessary. That being said, unless your
> new UI is going to be merged back into M53 we need to merge this back to fix
the
> stable blocker.

ok, interesting, I'm ok with landing and merging this then. After merging we
should revert.

[flackr, 2016-08-17 19:07:19]

Had to adjust the test since with the new UI we don't change focus during the
alt+tab (so w0 gets the event if it is also fullscreen) but it should now work
with and without the new UI.

https://codereview.chromium.org/2252673002/diff/1/ash/wm/window_cycle_control...
File ash/wm/window_cycle_controller_unittest.cc (right):

https://codereview.chromium.org/2252673002/diff/1/ash/wm/window_cycle_control...
ash/wm/window_cycle_controller_unittest.cc:632: ui::test::EventGenerator&
generator = GetEventGenerator();
On 2016/08/17 at 18:06:57, varkha wrote:
> nit: Could swap the line before and above this comment.

Done.

https://codereview.chromium.org/2252673002/diff/1/ash/wm/window_cycle_event_f...
File ash/wm/window_cycle_event_filter_aura.cc (right):

https://codereview.chromium.org/2252673002/diff/1/ash/wm/window_cycle_event_f...
ash/wm/window_cycle_event_filter_aura.cc:27: event->type() !=
ui::ET_KEY_PRESSED) {
On 2016/08/17 at 18:06:57, varkha wrote:
> Does this properly deal with Alt+Shift+Tab?

Yes.

[flackr, 2016-08-17 19:12:36]

The CQ bit was checked by flackr@chromium.org

[flackr, 2016-08-17 19:12:37]

The patchset sent to the CQ was uploaded after l-g-t-m from varkha@chromium.org
Link to the patchset: https://codereview.chromium.org/2252673002/#ps20001
(title: "Merge and fix test for new alt+tab UI.")

[commit-bot: I haz the power, 2016-08-17 19:12:56]

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-08-17 19:55:09]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-17 19:58:44]

Description was changed from

==========
Don't handle the tab press event in the window cycle event filter.

Because fullscreen windows are allowed to handle alt+tab, we have to be careful
not to handle the tab press event in the window cycle event filter as it will
be treated as a fullscreen window preventing alt-tabbing.

BUG=622396
TEST=WindowCycleControllerTest.TabPastFullscreenWindow,WindowCycleControllerTest.TabKeyNotLeaked
==========

to

==========
Don't handle the tab press event in the window cycle event filter.

Because fullscreen windows are allowed to handle alt+tab, we have to be careful
not to handle the tab press event in the window cycle event filter as it will
be treated as a fullscreen window preventing alt-tabbing.

BUG=622396
TEST=WindowCycleControllerTest.TabPastFullscreenWindow,WindowCycleControllerTest.TabKeyNotLeaked

Committed: https://crrev.com/66b4142cfaf0acc038f220b6288089f8a60b923d
Cr-Commit-Position: refs/heads/master@{#412616}
==========

[commit-bot: I haz the power, 2016-08-17 19:58:45]

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