Remove TransparentActivateWindowButton from Overview Mode

Remove TransparentActivateWindowButton from Overview Mode

This change eliminates the need to have a transparent button on top of
overview mode windows by installing a static event targeter on each of
the item windows. The targeter checks for events on the item bounds and
redirects events to a label button located under each item. This way, we
greatly simplify event handling while preventing events to reach the
actual windows.

The reason why we use a label button now is that it already handles clicks
and taps, as well as accessibility stuff (touch exploration, chromevox
feedback) that were being handled in the transparent window before.

The CL only involves refactoring - no features have been added or
removed.

BUG=446548
TEST=WindowSelectorTest.*

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

[Nina, 2015-01-09 15:43:48]

nsatragno@chromium.org changed reviewers:
+ tdanderson@chromium.org

[Nina, 2015-01-09 15:43:48]

nsatragno@chromium.org changed required reviewers:
+ tdanderson@chromium.org

[Nina, 2015-01-09 15:43:49]

Hey Terry,

Can you give this patch a first look? I'll loop in flackr afterwards, and Sky
for the window.(h|cc) code. By the way, do you think I should add @bruthig as a
non-required reviewer? This seems like a new, neat codereview feature to notify
others of work they might be interested in reviewing, even if in principle they
don't have to.

https://codereview.chromium.org/810033010/diff/1/ash/wm/overview/window_selec...
File ash/wm/overview/window_selector_unittest.cc (left):

https://codereview.chromium.org/810033010/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector_unittest.cc:1352: // Tests that the transparent
overlays z-order matches the windows z-order.
This test is no longer needed as we are actually targeting the panels now.

https://codereview.chromium.org/810033010/diff/1/ash/wm/panels/panel_window_e...
File ash/wm/panels/panel_window_event_handler.cc (right):

https://codereview.chromium.org/810033010/diff/1/ash/wm/panels/panel_window_e...
ash/wm/panels/panel_window_event_handler.cc:29: target->delegate() &&
Without this change, this code crashed when handling events in the overview due
to the container windows having no delegate

https://codereview.chromium.org/810033010/diff/1/ash/wm/panels/panel_window_e...
ash/wm/panels/panel_window_event_handler.cc:45: target->delegate() &&
Same as L29

https://codereview.chromium.org/810033010/diff/1/ash/wm/workspace/workspace_e...
File ash/wm/workspace/workspace_event_handler.cc (right):

https://codereview.chromium.org/810033010/diff/1/ash/wm/workspace/workspace_e...
ash/wm/workspace/workspace_event_handler.cc:28: if (!target->delegate())
Without the change, this code would crash if the user clicked, switched to
overview and quickly clicked again.

https://codereview.chromium.org/810033010/diff/1/ui/aura/window.h
File ui/aura/window.h (right):

https://codereview.chromium.org/810033010/diff/1/ui/aura/window.h#newcode321
ui/aura/window.h:321: // Overridden from ui::EventTarget:
Moving this to public is needed for the new code to work. This shouldn't be a
problem as we are already returning the EventTargeter on the setter, which is
public.

[Nina, 2015-01-09 16:08:08]

nsatragno@chromium.org changed reviewers:
+ bruthig@chromium.org

[Nina, 2015-01-09 16:08:08]

Ben,

I'm looping you in so that you can check the proposed changes  as you've been
working a lot with this code recently. FYI the swipe to dismiss code would
probably go in ash/wm/overview/window_grid.cc:436
once we merge that in.

Cheers!

[bruthig, 2015-01-09 17:04:33]

Made a few comments inline as well as these.

1.  Will clicks/taps outside inside of a selector item's bounds, but not on a
window or the label, still activate that window?  If this behaviour is changing
we should double check with stevet@.

2. Can you confirm that ChromeVox won't start reading out window content while
doing touch exploration while in overview mode.

I think it might be easier for me to finish swipe to close if this is landed
first.  Let's discuss offline.

https://codereview.chromium.org/810033010/diff/1/ash/wm/overview/window_grid.cc
File ash/wm/overview/window_grid.cc (right):

https://codereview.chromium.org/810033010/diff/1/ash/wm/overview/window_grid....
ash/wm/overview/window_grid.cc:180:
static_targeter_container_->set_target_handler(this);
So will all events be consumed by this then?  ie ET_DROP_TARGET_EVENT?

https://codereview.chromium.org/810033010/diff/1/ash/wm/overview/window_grid....
ash/wm/overview/window_grid.cc:456:
wm::GetWindowState(selected_window)->Activate();
How difficult would it be to move the window activation code to a single
function.  ie WindowSelectorItem::Activate() and call that function here as well
as from the handler for the LabelButton clicks?
I realize that right now it is only a single line of code but if it ever changes
or becomes two lines then having it in the same spot will make it much easier
for the next dev to update/maintain the code.

https://codereview.chromium.org/810033010/diff/1/ash/wm/overview/window_selec...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/810033010/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector_item.cc:277:
wm::GetWindowState(SelectionWindow())->Activate();
I'm assuming this is handling the clicks on the LabelButton?  You should scope
it in an "else if(sender == window_label_button_view_)" block to be explicit.

[Nina, 2015-01-21 16:24:16]

Hi Terry,

Can you please give the patch a first review? Thanks!

https://codereview.chromium.org/810033010/diff/1/ash/wm/overview/window_grid.cc
File ash/wm/overview/window_grid.cc (right):

https://codereview.chromium.org/810033010/diff/1/ash/wm/overview/window_grid....
ash/wm/overview/window_grid.cc:180:
static_targeter_container_->set_target_handler(this);
On 2015/01/09 17:04:33, bruthig wrote:
> So will all events be consumed by this then?  ie ET_DROP_TARGET_EVENT?

They'll be intercepted, and only handled if they either activate windows or
close the overview. So if they don't they'll still move through the event chain.
Do you think there could be a problem with this?

https://codereview.chromium.org/810033010/diff/1/ash/wm/overview/window_grid....
ash/wm/overview/window_grid.cc:456:
wm::GetWindowState(selected_window)->Activate();
On 2015/01/09 17:04:33, bruthig wrote:
> How difficult would it be to move the window activation code to a single
> function.  ie WindowSelectorItem::Activate() and call that function here as
well
> as from the handler for the LabelButton clicks?
> I realize that right now it is only a single line of code but if it ever
changes
> or becomes two lines then having it in the same spot will make it much easier
> for the next dev to update/maintain the code.

We would have to search for the WindowSelectorItem that holds the targeted
aura::Window* here and call that method. We discussed it offline and haven't
arrived at a decision on this yet.

https://codereview.chromium.org/810033010/diff/1/ash/wm/overview/window_selec...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/810033010/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector_item.cc:277:
wm::GetWindowState(SelectionWindow())->Activate();
On 2015/01/09 17:04:33, bruthig wrote:
> I'm assuming this is handling the clicks on the LabelButton?  You should scope
> it in an "else if(sender == window_label_button_view_)" block to be explicit.

I don't think we want to add a redundant if clause. Maybe I can write a comment
instead?

[bruthig, 2015-01-21 16:55:42]

https://codereview.chromium.org/810033010/diff/20001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/810033010/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:338:
SetupFadeInAfterLayout(window_label_->GetNativeWindow());
The call to SetupFadeInAfterLayout was intentionally put in the SetBounds method
to try and keep the label fade in animation in sync with the actual window
layout animations.

By calling CreateWindowLabel from the constructor and embedding the
SetupFadeInAfterLayout here, you are susceptible to any delays between the
construction of the WindowSelectorItem and the call to
WindowSelectorItem::SetItemBounds.

[tdanderson, 2015-01-21 17:24:25]

Hey Nico, I left a few comments below, which we can discuss in further detail
later today.

https://codereview.chromium.org/810033010/diff/20001/ash/wm/overview/overview...
File ash/wm/overview/overview_window_targeter.cc (right):

https://codereview.chromium.org/810033010/diff/20001/ash/wm/overview/overview...
ash/wm/overview/overview_window_targeter.cc:22:
event->set_location(gfx::PointF(0, 0));
I'm not sure why this is necessary - let's discuss offline.

https://codereview.chromium.org/810033010/diff/20001/ash/wm/overview/overview...
File ash/wm/overview/overview_window_targeter.h (right):

https://codereview.chromium.org/810033010/diff/20001/ash/wm/overview/overview...
ash/wm/overview/overview_window_targeter.h:14: // window children and uses the
WindowSelectorItem bounds to check for hits.
It looks like this targeter can be completely generic - let's chat offline.

https://codereview.chromium.org/810033010/diff/20001/ash/wm/overview/overview...
ash/wm/overview/overview_window_targeter.h:21: // Sets the targetable bounds in
the root window coordinates.
You should instead mention the coordinate space of |bounds_| where you declare
it (i.e., line 33).

https://codereview.chromium.org/810033010/diff/20001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/810033010/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:88: // the mouse up to target this view.
Does this mean that the window will be selected and OM will be disengaged on a
mousedown only? If so, that's a change in behaviour you should clear with
stevet@ first.

https://codereview.chromium.org/810033010/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:248: // sender ==
window_label_button_view_
Possibly worthwhile to turn this comment into a DCHECK instead?

https://codereview.chromium.org/810033010/diff/20001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.h (right):

https://codereview.chromium.org/810033010/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.h:133: // View for the label button under
the window.
Mention in your CL description why you're now using a label button instead of
just a label.

[Nina, 2015-01-22 19:44:32]

Terry, Ben, please review the changes.

Thanks!

https://codereview.chromium.org/810033010/diff/20001/ash/wm/overview/overview...
File ash/wm/overview/overview_window_targeter.cc (right):

https://codereview.chromium.org/810033010/diff/20001/ash/wm/overview/overview...
ash/wm/overview/overview_window_targeter.cc:22:
event->set_location(gfx::PointF(0, 0));
On 2015/01/21 17:24:24, tdanderson wrote:
> I'm not sure why this is necessary - let's discuss offline.

Discussed online, will change if we find a better way to redirect events to
views.

https://codereview.chromium.org/810033010/diff/20001/ash/wm/overview/overview...
File ash/wm/overview/overview_window_targeter.h (right):

https://codereview.chromium.org/810033010/diff/20001/ash/wm/overview/overview...
ash/wm/overview/overview_window_targeter.h:14: // window children and uses the
WindowSelectorItem bounds to check for hits.
On 2015/01/21 17:24:24, tdanderson wrote:
> It looks like this targeter can be completely generic - let's chat offline.

Acknowledged.

https://codereview.chromium.org/810033010/diff/20001/ash/wm/overview/overview...
ash/wm/overview/overview_window_targeter.h:21: // Sets the targetable bounds in
the root window coordinates.
On 2015/01/21 17:24:24, tdanderson wrote:
> You should instead mention the coordinate space of |bounds_| where you declare
> it (i.e., line 33).

Done.

https://codereview.chromium.org/810033010/diff/20001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/810033010/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:88: // the mouse up to target this view.
On 2015/01/21 17:24:24, tdanderson wrote:
> Does this mean that the window will be selected and OM will be disengaged on a
> mousedown only? If so, that's a change in behaviour you should clear with
> stevet@ first.

I added mouse release tracking to the OverviewLabelButton to bring the old
behaviour back, so we should be fine now.

https://codereview.chromium.org/810033010/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:248: // sender ==
window_label_button_view_
On 2015/01/21 17:24:24, tdanderson wrote:
> Possibly worthwhile to turn this comment into a DCHECK instead?

Done.

https://codereview.chromium.org/810033010/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:338:
SetupFadeInAfterLayout(window_label_->GetNativeWindow());
On 2015/01/21 16:55:42, bruthig - OOO Jan 22-26 wrote:
> The call to SetupFadeInAfterLayout was intentionally put in the SetBounds
method
> to try and keep the label fade in animation in sync with the actual window
> layout animations.
> 
> By calling CreateWindowLabel from the constructor and embedding the
> SetupFadeInAfterLayout here, you are susceptible to any delays between the
> construction of the WindowSelectorItem and the call to
> WindowSelectorItem::SetItemBounds.  

Thanks for the heads up. Because we need the window label to exist in the
constructor, I refactored the call to SetupFadeInAfterLayout in SetBounds to
depend on whether the label is visible or not. What do you think?

https://codereview.chromium.org/810033010/diff/20001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.h (right):

https://codereview.chromium.org/810033010/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.h:133: // View for the label button under
the window.
On 2015/01/21 17:24:24, tdanderson wrote:
> Mention in your CL description why you're now using a label button instead of
> just a label.

Done.

[tdanderson, 2015-01-22 20:43:58]

Hey Nico, I took a look at your latest patch set and it lgtm, feel free to send
to Rob. A few nits below.

https://codereview.chromium.org/810033010/diff/40001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/810033010/diff/40001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:113: // the mouse up to target this
view.
Since you're activating on mouse release (the current behavior) then this
comment is out of date.

https://codereview.chromium.org/810033010/diff/40001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:122: // Updates the |event_bounds|,
converting them to our coordinates.
|event_bounds_|.

https://codereview.chromium.org/810033010/diff/40001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:142: gfx::Rect event_bounds_;
It's not immediately clear what "event bounds" means. I would rename this (and
SetEventBounds()) to something such as "selector item bounds" to make it
clearer.

[Nina, 2015-01-22 22:25:38]

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

[Nina, 2015-01-22 22:25:38]

nsatragno@chromium.org changed required reviewers:
+ flackr@chromium.org

[Nina, 2015-01-22 22:25:39]

Flackr, can you give this patch a review? Thanks

https://codereview.chromium.org/810033010/diff/40001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/810033010/diff/40001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:113: // the mouse up to target this
view.
On 2015/01/22 20:43:58, tdanderson wrote:
> Since you're activating on mouse release (the current behavior) then this
> comment is out of date.

Done.

https://codereview.chromium.org/810033010/diff/40001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:122: // Updates the |event_bounds|,
converting them to our coordinates.
On 2015/01/22 20:43:58, tdanderson wrote:
> |event_bounds_|. 

Done.

https://codereview.chromium.org/810033010/diff/40001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:142: gfx::Rect event_bounds_;
On 2015/01/22 20:43:58, tdanderson wrote:
> It's not immediately clear what "event bounds" means. I would rename this (and
> SetEventBounds()) to something such as "selector item bounds" to make it
> clearer.

Done.

[flackr, 2015-01-22 23:21:17]

https://codereview.chromium.org/810033010/diff/60001/ash/wm/overview/overview...
File ash/wm/overview/overview_window_targeter.h (right):

https://codereview.chromium.org/810033010/diff/60001/ash/wm/overview/overview...
ash/wm/overview/overview_window_targeter.h:29: bool
EventLocationInsideBounds(ui::EventTarget* target,
can you re-state the parent class which this overrides? i.e. //
ui::EventTargeter:

https://codereview.chromium.org/810033010/diff/60001/ash/wm/overview/overview...
ash/wm/overview/overview_window_targeter.h:38: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/810033010/diff/60001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/810033010/diff/60001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:285:
ScreenUtil::ConvertRectFromScreen(root_window_, target_bounds));
Does this need to be updated here or can it be done when UpdateWindowLables is
called? It seems odd that we wouldn't also need to update
window_label_button_view_->SetSelectorItemBounds every time the bounds change.

https://codereview.chromium.org/810033010/diff/60001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.h (right):

https://codereview.chromium.org/810033010/diff/60001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.h:151:
scoped_ptr<aura::ScopedWindowTargeter> scoped_window_targeter_;
Just a thought, if you make the OverviewLabelButton class manage the window
(window_label_), and the view (window_label_button_view_), it can then
encapsulate all of the code (and members like overview_window_targeter_,
scoped_window_targeter_) related to targeting, the actual View, and the window.

[Nina, 2015-01-23 18:38:58]

Hi flackr, can you give the code another look? 

I refactored the code as you suggested in the last comment.

Thanks.

https://codereview.chromium.org/810033010/diff/60001/ash/wm/overview/overview...
File ash/wm/overview/overview_window_targeter.h (right):

https://codereview.chromium.org/810033010/diff/60001/ash/wm/overview/overview...
ash/wm/overview/overview_window_targeter.h:29: bool
EventLocationInsideBounds(ui::EventTarget* target,
On 2015/01/22 23:21:17, flackr wrote:
> can you re-state the parent class which this overrides? i.e. //
> ui::EventTargeter:

Done.

https://codereview.chromium.org/810033010/diff/60001/ash/wm/overview/overview...
ash/wm/overview/overview_window_targeter.h:38: };
On 2015/01/22 23:21:17, flackr wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/810033010/diff/60001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/810033010/diff/60001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:285:
ScreenUtil::ConvertRectFromScreen(root_window_, target_bounds));
On 2015/01/22 23:21:17, flackr wrote:
> Does this need to be updated here or can it be done when UpdateWindowLables is
> called? It seems odd that we wouldn't also need to update
> window_label_button_view_->SetSelectorItemBounds every time the bounds change.

Moved to the equivalent to UpdateWindowLabels on the new code. As you say, it
did not make sense to have those two method calls in different sections of the
code.

https://codereview.chromium.org/810033010/diff/60001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.h (right):

https://codereview.chromium.org/810033010/diff/60001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.h:151:
scoped_ptr<aura::ScopedWindowTargeter> scoped_window_targeter_;
On 2015/01/22 23:21:17, flackr wrote:
> Just a thought, if you make the OverviewLabelButton class manage the window
> (window_label_), and the view (window_label_button_view_), it can then
> encapsulate all of the code (and members like overview_window_targeter_,
> scoped_window_targeter_) related to targeting, the actual View, and the
window.

I went forward and refactored the code this way, as I think it'll make adding
gesture support easier. WDTY of the implementation?

[flackr, 2015-01-23 19:31:55]

LGTM, thanks for splitting out the label button behavior.

https://codereview.chromium.org/810033010/diff/80001/ash/wm/overview/overview...
File ash/wm/overview/overview_window_button.cc (right):

https://codereview.chromium.org/810033010/diff/80001/ash/wm/overview/overview...
ash/wm/overview/overview_window_button.cc:34: static const int kShadowBlur = 10;
nit: newline before "}  // namespace"

https://codereview.chromium.org/810033010/diff/80001/ash/wm/overview/overview...
File ash/wm/overview/overview_window_button.h (right):

https://codereview.chromium.org/810033010/diff/80001/ash/wm/overview/overview...
ash/wm/overview/overview_window_button.h:23: explicit
OverviewWindowButton(aura::Window* target);
nit: brief description. I.e. what is |target|.

https://codereview.chromium.org/810033010/diff/80001/ash/wm/overview/overview...
File ash/wm/overview/overview_window_targeter.h (right):

https://codereview.chromium.org/810033010/diff/80001/ash/wm/overview/overview...
ash/wm/overview/overview_window_targeter.h:13: // A targeter for the windows in
the overview. Avoids propagating events to
This is actually for the window buttons right?
nit: This functionality / class could probably be merged into the
OverviewWindowButton class since it's not much extra code.

[Nina, 2015-01-23 20:35:54]

Moving forward with the commit.

https://codereview.chromium.org/810033010/diff/80001/ash/wm/overview/overview...
File ash/wm/overview/overview_window_button.cc (right):

https://codereview.chromium.org/810033010/diff/80001/ash/wm/overview/overview...
ash/wm/overview/overview_window_button.cc:34: static const int kShadowBlur = 10;
On 2015/01/23 19:31:55, flackr wrote:
> nit: newline before "}  // namespace"

Done.

https://codereview.chromium.org/810033010/diff/80001/ash/wm/overview/overview...
File ash/wm/overview/overview_window_button.h (right):

https://codereview.chromium.org/810033010/diff/80001/ash/wm/overview/overview...
ash/wm/overview/overview_window_button.h:23: explicit
OverviewWindowButton(aura::Window* target);
On 2015/01/23 19:31:55, flackr wrote:
> nit: brief description. I.e. what is |target|.

Done.

https://codereview.chromium.org/810033010/diff/80001/ash/wm/overview/overview...
File ash/wm/overview/overview_window_targeter.h (right):

https://codereview.chromium.org/810033010/diff/80001/ash/wm/overview/overview...
ash/wm/overview/overview_window_targeter.h:13: // A targeter for the windows in
the overview. Avoids propagating events to
On 2015/01/23 19:31:55, flackr wrote:
> This is actually for the window buttons right?
Yes.
> nit: This functionality / class could probably be merged into the
> OverviewWindowButton class since it's not much extra code.
The reason while it's on its own class is that tdanderson@ commented it has a
high chance of being useful for other parts of the code.

[Nina, 2015-01-23 20:36:02]

The CQ bit was checked by nsatragno@chromium.org

[commit-bot: I haz the power, 2015-01-23 20:36:49]

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

[commit-bot: I haz the power, 2015-01-23 21:53:18]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-01-23 21:54:19]

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