[ash] Wrong LauncherItem state when overflow bubble is shown

[ash] Wrong LauncherItem state when overflow bubble is shown

When overflow bubble is shown, active state item in overflow bubble is changed
to running state item because overflow bubble get focus.

R=jamescook@chromium.org
BUG=283206
TEST=Compiles, Manual tests

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

[simonhong_, 2013-09-01 08:09:58]

Dear jamescook,
Please take a look!

[James Cook, 2013-09-03 17:31:26]

https://codereview.chromium.org/23540013/diff/2001/ash/launcher/launcher_view.cc
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/23540013/diff/2001/ash/launcher/launcher_view...
ash/launcher/launcher_view.cc:1606: ShowOverflowBubble();
Comment here on how the bubble is closed (especially how it works when you click
on the overflow button a second time).

https://codereview.chromium.org/23540013/diff/2001/ash/shelf/overflow_bubble.cc
File ash/shelf/overflow_bubble.cc (right):

https://codereview.chromium.org/23540013/diff/2001/ash/shelf/overflow_bubble....
ash/shelf/overflow_bubble.cc:287: event->StopPropagation();
A few questions:
1. Why do you need to change the logic for when the overflow bubble opens and
closes?
2. Do you need to do anything with
BubbleDelegateView::set_close_on_deactivate()?
3. Right now we don't stop the propagation of events when you click outside the
bubble.  Do you need to stop it?

[simonhong_, 2013-09-03 20:16:03]

Dear jamescook,
Please check again.

https://codereview.chromium.org/23540013/diff/2001/ash/launcher/launcher_view.cc
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/23540013/diff/2001/ash/launcher/launcher_view...
ash/launcher/launcher_view.cc:1606: ShowOverflowBubble();
On 2013/09/03 17:31:26, James Cook wrote:
> Comment here on how the bubble is closed (especially how it works when you
click
> on the overflow button a second time).

Done.

https://codereview.chromium.org/23540013/diff/2001/ash/shelf/overflow_bubble.cc
File ash/shelf/overflow_bubble.cc (right):

https://codereview.chromium.org/23540013/diff/2001/ash/shelf/overflow_bubble....
ash/shelf/overflow_bubble.cc:287: event->StopPropagation();
On 2013/09/03 17:31:26, James Cook wrote:
> A few questions:
> 1. Why do you need to change the logic for when the overflow bubble opens and
> closes?
When Overflow bubble makes focusless by set_use_focusless(true),this bubble
cannot get widget activation change notification. So I need to add hiding logic
like this.
> 2. Do you need to do anything with
> BubbleDelegateView::set_close_on_deactivate()?
This can't affect because this bubble is focusless.
> 3. Right now we don't stop the propagation of events when you click outside
the
> bubble.  Do you need to stop it?
The reason of stop propagating is to prevent opening bubble again by clicking
overflow button when overflow bubble is shown. If not, overflow bubble shows up
again by clicking overflow button when user just want to hide bubble.

[James Cook, 2013-09-03 20:56:33]

> > 3. Right now we don't stop the propagation of events when you click outside
> the
> > bubble.  Do you need to stop it?
> The reason of stop propagating is to prevent opening bubble again by clicking
> overflow button when overflow bubble is shown. If not, overflow bubble shows
up
> again by clicking overflow button when user just want to hide bubble.

I talked to our UI designer. We want all the various menus in Chrome and the
launcher to behave the same way when you click off of them.  The overflow button
should behave like the browser Tools menu, the app list and the status tray:

* Click outside the menu propagates the click
* Clicking on the button closes the menu

This might require a little more work in this CL but I think all the
menus/bubbles behaving consistently is important.

[simonhong_, 2013-09-03 21:01:28]

On 2013/09/03 20:56:33, James Cook wrote:
> > > 3. Right now we don't stop the propagation of events when you click
outside
> > the
> > > bubble.  Do you need to stop it?
> > The reason of stop propagating is to prevent opening bubble again by
clicking
> > overflow button when overflow bubble is shown. If not, overflow bubble shows
> up
> > again by clicking overflow button when user just want to hide bubble.
> 
> I talked to our UI designer. We want all the various menus in Chrome and the
> launcher to behave the same way when you click off of them.  The overflow
button
> should behave like the browser Tools menu, the app list and the status tray:
> 
> * Click outside the menu propagates the click
> * Clicking on the button closes the menu
> 
> This might require a little more work in this CL but I think all the
> menus/bubbles behaving consistently is important.

Ok, I think I need to find a better approach for this. :)

[James Cook, 2013-09-03 21:02:28]

On 2013/09/03 21:01:28, Simon YoungKi Hong wrote:
> On 2013/09/03 20:56:33, James Cook wrote:
> > > > 3. Right now we don't stop the propagation of events when you click
> outside
> > > the
> > > > bubble.  Do you need to stop it?
> > > The reason of stop propagating is to prevent opening bubble again by
> clicking
> > > overflow button when overflow bubble is shown. If not, overflow bubble
shows
> > up
> > > again by clicking overflow button when user just want to hide bubble.
> > 
> > I talked to our UI designer. We want all the various menus in Chrome and the
> > launcher to behave the same way when you click off of them.  The overflow
> button
> > should behave like the browser Tools menu, the app list and the status tray:
> > 
> > * Click outside the menu propagates the click
> > * Clicking on the button closes the menu
> > 
> > This might require a little more work in this CL but I think all the
> > menus/bubbles behaving consistently is important.
> 
> Ok, I think I need to find a better approach for this. :)

I wonder if it might work to change LauncherView back to toggling on click, then
allowing propagation in OverflowBubble.

[simonhong_, 2013-09-03 22:00:39]

On 2013/09/03 21:02:28, James Cook wrote:
> On 2013/09/03 21:01:28, Simon YoungKi Hong wrote:
> > On 2013/09/03 20:56:33, James Cook wrote:
> > > > > 3. Right now we don't stop the propagation of events when you click
> > outside
> > > > the
> > > > > bubble.  Do you need to stop it?
> > > > The reason of stop propagating is to prevent opening bubble again by
> > clicking
> > > > overflow button when overflow bubble is shown. If not, overflow bubble
> shows
> > > up
> > > > again by clicking overflow button when user just want to hide bubble.
> > > 
> > > I talked to our UI designer. We want all the various menus in Chrome and
the
> > > launcher to behave the same way when you click off of them.  The overflow
> > button
> > > should behave like the browser Tools menu, the app list and the status
tray:
> > > 
> > > * Click outside the menu propagates the click
> > > * Clicking on the button closes the menu
> > > 
> > > This might require a little more work in this CL but I think all the
> > > menus/bubbles behaving consistently is important.
> > 
> > Ok, I think I need to find a better approach for this. :)
> 
> I wonder if it might work to change LauncherView back to toggling on click,
then
> allowing propagation in OverflowBubble.

I enabled propagation again by checking overflow_button's bound in
OnMouseEvent().
Please check again.

[James Cook, 2013-09-03 22:54:03]

https://codereview.chromium.org/23540013/diff/16001/ash/shelf/overflow_bubble.cc
File ash/shelf/overflow_bubble.cc (right):

https://codereview.chromium.org/23540013/diff/16001/ash/shelf/overflow_bubble...
ash/shelf/overflow_bubble.cc:249: launcher_view_(NULL) {
init anchor_ here

https://codereview.chromium.org/23540013/diff/16001/ash/shelf/overflow_bubble...
ash/shelf/overflow_bubble.cc:295:
!bubble_->GetBoundsInScreen().Contains(event->root_location())) {
Should you check anchor_ here also?

https://codereview.chromium.org/23540013/diff/16001/ash/shelf/overflow_bubble...
ash/shelf/overflow_bubble.cc:297: event->StopPropagation();
Did you mean to leave this?

https://codereview.chromium.org/23540013/diff/16001/ash/shelf/overflow_bubble...
ash/shelf/overflow_bubble.cc:304: launcher_view_ = NULL;
should anchor_ be set null here?

[simonhong_, 2013-09-04 00:00:22]

Dear jamescook,
I forgot many things.
I addressed your comments.
Please check again!

https://codereview.chromium.org/23540013/diff/16001/ash/shelf/overflow_bubble.cc
File ash/shelf/overflow_bubble.cc (right):

https://codereview.chromium.org/23540013/diff/16001/ash/shelf/overflow_bubble...
ash/shelf/overflow_bubble.cc:249: launcher_view_(NULL) {
On 2013/09/03 22:54:03, James Cook wrote:
> init anchor_ here

Done.

https://codereview.chromium.org/23540013/diff/16001/ash/shelf/overflow_bubble...
ash/shelf/overflow_bubble.cc:295:
!bubble_->GetBoundsInScreen().Contains(event->root_location())) {
On 2013/09/03 22:54:03, James Cook wrote:
> Should you check anchor_ here also?

Done.

https://codereview.chromium.org/23540013/diff/16001/ash/shelf/overflow_bubble...
ash/shelf/overflow_bubble.cc:297: event->StopPropagation();
On 2013/09/03 22:54:03, James Cook wrote:
> Did you mean to leave this?

Done.

https://codereview.chromium.org/23540013/diff/16001/ash/shelf/overflow_bubble...
ash/shelf/overflow_bubble.cc:304: launcher_view_ = NULL;
On 2013/09/03 22:54:03, James Cook wrote:
> should anchor_ be set null here?

Done.

[James Cook, 2013-09-04 15:14:07]

lgtm

[commit-bot: I haz the power, 2013-09-04 16:42:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simon.hong81@gmail.com/23540013/21001

[commit-bot: I haz the power, 2013-09-04 19:04:55]

Change committed as 221235