Close the save card bubble on tab switch.

Close the save card bubble on tab switch.

As of https://codereview.chromium.org/1402363013/ location bar bubbles are initially inactive when shown. This results in the save card bubble failing to close when switching tabs.

When a location bar bubble is *active*, loss of focus will deactivate and thus close the bubble. This CL adds an implementation of TabStripModelObserver::TabDeactivated to close it when it isn't active.

BUG=535784
Committed: https://crrev.com/238b112dd7fda58a408f0c351b8b207aaa33f142
Cr-Commit-Position: refs/heads/master@{#368884}

[Justin Donnelly, 2016-01-07 19:26:28]

Description was changed from

==========
Fix save credit card bubble behavior.

BUG=
==========

to

==========
Close the save card bubble even when it's not active.

As of https://codereview.chromium.org/1402363013/ location bar bubbles are
initially inactive when shown. This results in the save card bubble failing to
close when clicking its location bar icon or switching tabs.

When a location bar bubble is *active* it's the loss of focus that ends up
deactivating and as a result closing the bubble. When not active, the other
location bar bubbles have two other mechanisms to close the bubble which this CL
adds to the save card bubble:
- A mouse pressed handler on the location bar icon. This covers the case where
the icon is clicked.
- A static method to close the bubble which the location bar calls when the icon
is no longer active. This covers the tab switch case.

BUG=535784
==========

[Justin Donnelly, 2016-01-07 19:28:11]

jdonnelly@chromium.org changed reviewers:
+ estade@chromium.org

[Justin Donnelly, 2016-01-07 19:28:12]

https://codereview.chromium.org/1568983002/diff/1/chrome/browser/ui/views/aut...
File chrome/browser/ui/views/autofill/save_card_bubble_views.h (right):

https://codereview.chromium.org/1568983002/diff/1/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autofill/save_card_bubble_views.h:45: static void
CloseBubble();
The static method is unfortunate given the otherwise nice design of the save
card bubble controller and view but it's how the other bubbles handle this and I
can't find another approach that implements all of the desired behaviors.

I'm open to other suggestions if you have any.

[Justin Donnelly, 2016-01-07 21:58:14]

Description was changed from

==========
Close the save card bubble even when it's not active.

As of https://codereview.chromium.org/1402363013/ location bar bubbles are
initially inactive when shown. This results in the save card bubble failing to
close when clicking its location bar icon or switching tabs.

When a location bar bubble is *active* it's the loss of focus that ends up
deactivating and as a result closing the bubble. When not active, the other
location bar bubbles have two other mechanisms to close the bubble which this CL
adds to the save card bubble:
- A mouse pressed handler on the location bar icon. This covers the case where
the icon is clicked.
- A static method to close the bubble which the location bar calls when the icon
is no longer active. This covers the tab switch case.

BUG=535784
==========

to

==========
Make the save card bubble closeable even when it's not active.

As of https://codereview.chromium.org/1402363013/ location bar bubbles are
initially inactive when shown. This results in the save card bubble failing to
close when clicking its location bar icon or switching tabs.

When a location bar bubble is *active* it's the loss of focus that ends up
deactivating and as a result closing the bubble. When not active, the other
location bar bubbles have two other mechanisms to close the bubble which this CL
adds to the save card bubble:
- A mouse pressed handler on the location bar icon. This covers the case where
the icon is clicked.
- A static method to close the bubble which the location bar calls when the icon
is no longer active. This covers the tab switch case.

BUG=535784
==========

[Evan Stade, 2016-01-08 03:19:45]

is there a way we can refactor and share code instead of copying from bubble to
bubble?

https://codereview.chromium.org/1568983002/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1568983002/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:994:
autofill::SaveCardBubbleViews::CloseBubble();
save_credit_card_icon_view_->GetBubble()->Hide()?
save_credit_card_icon_view_->GetController()->HideBubble()?

[Justin Donnelly, 2016-01-08 18:13:53]

On 2016/01/08 03:19:45, Evan Stade wrote:
> is there a way we can refactor and share code instead of copying from bubble
to
> bubble?

I sure hope we can overhaul these so they're all much more similar but in terms
of the the close behavior in these cases but they all work in slightly different
ways. I don't see any simple consolidation possible here.

[Justin Donnelly, 2016-01-08 18:14:05]

https://codereview.chromium.org/1568983002/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1568983002/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:994:
autofill::SaveCardBubbleViews::CloseBubble();
On 2016/01/08 03:19:45, Evan Stade wrote:
> save_credit_card_icon_view_->GetBubble()->Hide()?
> save_credit_card_icon_view_->GetController()->HideBubble()?

Yes, that's very much what I'd prefer to do. The problem is that this would get
the controller and view associated with the current webview. This code works by
running after we've switched tabs and are dealing with a new web view. The
static methods used by each of the location bar bubbles are a means of keeping a
reference to the currently visible bubble independent of which tab is showing.

[Evan Stade, 2016-01-08 22:06:22]

> Yes, that's very much what I'd prefer to do. The problem is that this would
get
> the controller and view associated with the current webview. This code works
by
> running after we've switched tabs and are dealing with a new web view. The
> static methods used by each of the location bar bubbles are a means of keeping
a
> reference to the currently visible bubble independent of which tab is showing.

LocationBarView could be a TabStripModelObserver and implement
TabDeactivated() by closing the relevant save card bubble, if any.

[Evan Stade, 2016-01-08 22:09:11]

On 2016/01/08 22:06:22, Evan Stade wrote:
> > Yes, that's very much what I'd prefer to do. The problem is that this would
> get
> > the controller and view associated with the current webview. This code works
> by
> > running after we've switched tabs and are dealing with a new web view. The
> > static methods used by each of the location bar bubbles are a means of
keeping
> a
> > reference to the currently visible bubble independent of which tab is
showing.
> 
> LocationBarView could be a TabStripModelObserver and implement
> TabDeactivated() by closing the relevant save card bubble, if any.

or s/LocationBarView/SaveCardIconView since that also already has access to
browser. All you need to do is browser->tab_strip_model()->AddObserver(this);

[Justin Donnelly, 2016-01-08 22:23:47]

Awesome, thanks, I'll give that a shot.

On Fri, Jan 8, 2016 at 2:09 PM, <estade@chromium.org> wrote:

> On 2016/01/08 22:06:22, Evan Stade wrote:
>
>> > Yes, that's very much what I'd prefer to do. The problem is that this
>> would
>> get
>> > the controller and view associated with the current webview. This code
>> works
>> by
>> > running after we've switched tabs and are dealing with a new web view.
>> The
>> > static methods used by each of the location bar bubbles are a means of
>>
> keeping
>
>> a
>> > reference to the currently visible bubble independent of which tab is
>>
> showing.
>
> LocationBarView could be a TabStripModelObserver and implement
>> TabDeactivated() by closing the relevant save card bubble, if any.
>>
>
> or s/LocationBarView/SaveCardIconView since that also already has access to
> browser. All you need to do is
> browser->tab_strip_model()->AddObserver(this);
>
> https://codereview.chromium.org/1568983002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Justin Donnelly, 2016-01-11 16:32:10]

Description was changed from

==========
Make the save card bubble closeable even when it's not active.

As of https://codereview.chromium.org/1402363013/ location bar bubbles are
initially inactive when shown. This results in the save card bubble failing to
close when clicking its location bar icon or switching tabs.

When a location bar bubble is *active* it's the loss of focus that ends up
deactivating and as a result closing the bubble. When not active, the other
location bar bubbles have two other mechanisms to close the bubble which this CL
adds to the save card bubble:
- A mouse pressed handler on the location bar icon. This covers the case where
the icon is clicked.
- A static method to close the bubble which the location bar calls when the icon
is no longer active. This covers the tab switch case.

BUG=535784
==========

to

==========
Make the save card bubble closeable even when it's not active.

As of https://codereview.chromium.org/1402363013/ location bar bubbles are
initially inactive when shown. This results in the save card bubble failing to
close when clicking its location bar icon or switching tabs.

When a location bar bubble is *active* it's the loss of focus that ends up
deactivating and as a result closing the bubble. When not active, a couple new
event handlers on the save card location bar icon take care of closing the
bubble:
- A mouse pressed handler for the case where the icon is clicked.
- An implementation of TabStripModelObserver::TabDeactivated to cover the case
where the user switches tabs.

BUG=535784
==========

[Justin Donnelly, 2016-01-11 16:33:33]

Ok, static nonsense gone. The TabStripModelObserver approach works great. A
similar approach may be possible for the other location bar bubbles but they use
the static method in multiple locations so I'm going to have to investigate
further.

[Evan Stade, 2016-01-11 18:05:24]

https://codereview.chromium.org/1568983002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/save_card_icon_view.cc (right):

https://codereview.chromium.org/1568983002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_icon_view.cc:34: bool
SaveCardIconView::OnMousePressed(const ui::MouseEvent& event) {
BubbleIconView handles more than just mouses, it also handles keyboard and
gestures.

I think what you want is to change this[1]

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[Justin Donnelly, 2016-01-11 19:32:37]

https://codereview.chromium.org/1568983002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/save_card_icon_view.cc (right):

https://codereview.chromium.org/1568983002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_icon_view.cc:34: bool
SaveCardIconView::OnMousePressed(const ui::MouseEvent& event) {
On 2016/01/11 18:05:24, Evan Stade wrote:
> BubbleIconView handles more than just mouses, it also handles keyboard and
> gestures.

Understood.

> I think what you want is to change this[1]
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

The BubbleIconView/BrowserCommandController code path handles showing the bubble
when it's not currently shown. It suppresses any action when it's already
showing which prevents side effects (e.g. logging). Closing the bubble only
works because any loss of focus closes the bubble (it doesn't matter whether you
click on the icon or not or whether it's a keyboard event that causes the loss
of focus).

This handler adds a mechanism (matching the one used by some of the other
location bar bubbles) to handle the specific case of closing the bubble when
it's not active. It might make sense to make the browser command path handle
opening *and* closing and being aware of whether the bubble is active or not but
that would requiring overhauling the way each of the bubbles work. If we want to
do that I'd be happy to discuss but I'd like to fix this bug before the broken
behavior lands on the branch.

[Evan Stade, 2016-01-11 22:50:42]

On 2016/01/11 19:32:37, Justin Donnelly wrote:
>
https://codereview.chromium.org/1568983002/diff/20001/chrome/browser/ui/views...
> File chrome/browser/ui/views/autofill/save_card_icon_view.cc (right):
> 
>
https://codereview.chromium.org/1568983002/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/autofill/save_card_icon_view.cc:34: bool
> SaveCardIconView::OnMousePressed(const ui::MouseEvent& event) {
> On 2016/01/11 18:05:24, Evan Stade wrote:
> > BubbleIconView handles more than just mouses, it also handles keyboard and
> > gestures.
> 
> Understood.

Do you agree this means this patch as it stands doesn't actually fully fix the
bug?

> 
> > I think what you want is to change this[1]
> > 
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> 
> The BubbleIconView/BrowserCommandController code path handles showing the
bubble
> when it's not currently shown. It suppresses any action when it's already
> showing which prevents side effects (e.g. logging). Closing the bubble only
> works because any loss of focus closes the bubble (it doesn't matter whether
you
> click on the icon or not or whether it's a keyboard event that causes the loss
> of focus).
> 
> This handler adds a mechanism (matching the one used by some of the other
> location bar bubbles) to handle the specific case of closing the bubble when
> it's not active. It might make sense to make the browser command path handle
> opening *and* closing and being aware of whether the bubble is active or not

yes, that's what I'm suggesting

> but
> that would requiring overhauling the way each of the bubbles work.

can't we fix just this one bubble for now?

> If we want to
> do that I'd be happy to discuss but I'd like to fix this bug before the broken
> behavior lands on the branch.

[Justin Donnelly, 2016-01-11 23:38:59]

I believe it fully fixes the bug insofar as it's not currently possible to get
into a state where keyboard interactions should but don't close the bubble. I
don't think it's possible to fix just this one bubble in the way you're
suggesting because the suppression of mouse clicks is in the parent class
BubbleIconView .

It may be helpful for me to walk you through the behavior I'm seeing. Feel free
to grab me whenever you have a few minutes.

[Justin Donnelly, 2016-01-12 00:22:36]

Description was changed from

==========
Make the save card bubble closeable even when it's not active.

As of https://codereview.chromium.org/1402363013/ location bar bubbles are
initially inactive when shown. This results in the save card bubble failing to
close when clicking its location bar icon or switching tabs.

When a location bar bubble is *active* it's the loss of focus that ends up
deactivating and as a result closing the bubble. When not active, a couple new
event handlers on the save card location bar icon take care of closing the
bubble:
- A mouse pressed handler for the case where the icon is clicked.
- An implementation of TabStripModelObserver::TabDeactivated to cover the case
where the user switches tabs.

BUG=535784
==========

to

==========
Close the save card bubble on tab switch.

As of https://codereview.chromium.org/1402363013/ location bar bubbles are
initially inactive when shown. This results in the save card bubble failing to
close when switching tabs.

When a location bar bubble is *active*, loss of focus will deactivate and thus
close the bubble. This CL adds an implementation of
TabStripModelObserver::TabDeactivated to close it when it isn't active.

BUG=535784
==========

[Justin Donnelly, 2016-01-12 00:23:35]

Per our offline discussion, I removed the mouse handler part of this CL. Now
it's just has the tab switching part.

[Evan Stade, 2016-01-12 00:29:00]

https://codereview.chromium.org/1568983002/diff/40001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller.h (right):

https://codereview.chromium.org/1568983002/diff/40001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller.h:45: virtual void
HideBubble() = 0;
doesn't need to be here, SaveCardBubbleView doesn't use it and the one callsite
is a SaveCardBubbleControllerImpl instance.

[Justin Donnelly, 2016-01-12 00:39:26]

Patchset #4 (id:60001) has been deleted

[Justin Donnelly, 2016-01-12 00:40:39]

https://codereview.chromium.org/1568983002/diff/40001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller.h (right):

https://codereview.chromium.org/1568983002/diff/40001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller.h:45: virtual void
HideBubble() = 0;
On 2016/01/12 00:29:00, Evan Stade wrote:
> doesn't need to be here, SaveCardBubbleView doesn't use it and the one
callsite
> is a SaveCardBubbleControllerImpl instance.

Done.

[Evan Stade, 2016-01-12 01:24:37]

https://codereview.chromium.org/1568983002/diff/80001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/1568983002/diff/80001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:206:
save_card_bubble_view_ = nullptr;
do we not get onbubbleclosed?

[Justin Donnelly, 2016-01-12 04:24:37]

https://codereview.chromium.org/1568983002/diff/80001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right):

https://codereview.chromium.org/1568983002/diff/80001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:206:
save_card_bubble_view_ = nullptr;
On 2016/01/12 01:24:37, Evan Stade wrote:
> do we not get onbubbleclosed?

No. Calling Hide() here nulls out the view's reference to the controller. The
view's WindowClosing method is then called but it exits early since controller_
is nullptr.

[Evan Stade, 2016-01-12 05:05:05]

lgtm

[Justin Donnelly, 2016-01-12 15:23:18]

The CQ bit was checked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2016-01-12 15:23:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1568983002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1568983002/80001

[commit-bot: I haz the power, 2016-01-12 15:55:48]

Description was changed from

==========
Close the save card bubble on tab switch.

As of https://codereview.chromium.org/1402363013/ location bar bubbles are
initially inactive when shown. This results in the save card bubble failing to
close when switching tabs.

When a location bar bubble is *active*, loss of focus will deactivate and thus
close the bubble. This CL adds an implementation of
TabStripModelObserver::TabDeactivated to close it when it isn't active.

BUG=535784
==========

to

==========
Close the save card bubble on tab switch.

As of https://codereview.chromium.org/1402363013/ location bar bubbles are
initially inactive when shown. This results in the save card bubble failing to
close when switching tabs.

When a location bar bubble is *active*, loss of focus will deactivate and thus
close the bubble. This CL adds an implementation of
TabStripModelObserver::TabDeactivated to close it when it isn't active.

BUG=535784
==========

[commit-bot: I haz the power, 2016-01-12 15:55:48]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-01-12 15:57:40]

Description was changed from

==========
Close the save card bubble on tab switch.

As of https://codereview.chromium.org/1402363013/ location bar bubbles are
initially inactive when shown. This results in the save card bubble failing to
close when switching tabs.

When a location bar bubble is *active*, loss of focus will deactivate and thus
close the bubble. This CL adds an implementation of
TabStripModelObserver::TabDeactivated to close it when it isn't active.

BUG=535784
==========

to

==========
Close the save card bubble on tab switch.

As of https://codereview.chromium.org/1402363013/ location bar bubbles are
initially inactive when shown. This results in the save card bubble failing to
close when switching tabs.

When a location bar bubble is *active*, loss of focus will deactivate and thus
close the bubble. This CL adds an implementation of
TabStripModelObserver::TabDeactivated to close it when it isn't active.

BUG=535784

Committed: https://crrev.com/238b112dd7fda58a408f0c351b8b207aaa33f142
Cr-Commit-Position: refs/heads/master@{#368884}
==========

[commit-bot: I haz the power, 2016-01-12 15:57:40]

Patchset 4 (id:??) landed as
https://crrev.com/238b112dd7fda58a408f0c351b8b207aaa33f142
Cr-Commit-Position: refs/heads/master@{#368884}