Refactor BubbleDelegateView::use_focuseless()

chrome/browser/ui/views/location_bar/zoom_bubble_view.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/location_bar/zoom_bubble_view.h"
 
 #include "base/i18n/rtl.h"
 #include "base/strings/stringprintf.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/chrome_page_zoom.h"
@@ skipping 57 matching lines @@
   // extension, then the bubble can be reused and only the label text needs to
   // be updated.
   if (zoom_bubble_ &&
       zoom_bubble_->GetAnchorView() == anchor_view &&
       zoom_bubble_->auto_close_ == auto_close &&
       !extension) {
     zoom_bubble_->Refresh();
     return;
   }
 
   // If the bubble is already showing but its |auto_close_| value is not equal

[msw, 2014/07/23 18:57:00]

I wonder if this is still true/necessary?

[Dan Beam, 2014/07/23 23:03:34]

this only happens on ctrl+ and a click on the zoom icon in 2s (while the
auto-close button is still open).  i think it's possible you could just change
|zoom_bubble_->auto_close_| to false and cancel the close timer but it's a
fairly rare case and the user would have no indication that the click or tap
worked.  also, focus might differ.

[vasilii, 2014/07/24 16:32:13]

I reproduced this case.

[msw, 2014/07/24 16:52:27]

Why do we need to recreate the bubble on click after [Ctrl]+[+]? Now that the
original bubble can activate, can we just call zoom_bubble_->StopTimer()?

   // to |auto_close|, the bubble's focus properties must change, so the
   // current bubble must be closed and a new one created.
   CloseBubble();
 
   zoom_bubble_ = new ZoomBubbleView(anchor_view,
                                     web_contents,
                                     auto_close,
                                     browser_view->immersive_mode_controller(),
                                     browser->fullscreen_controller());
 
   // If the zoom change was initiated by an extension, capture the relevent
   // information from it.
   if (extension)
     zoom_bubble_->SetExtensionInfo(extension);
 
   // If we do not have an anchor view, parent the bubble to the content area.
   if (!anchor_to_view)
     zoom_bubble_->set_parent_window(web_contents->GetTopLevelNativeWindow());
 
   views::BubbleDelegateView::CreateBubble(zoom_bubble_);
 
   // Adjust for fullscreen after creation as it relies on the content size.
   if (is_fullscreen)
     zoom_bubble_->AdjustForFullscreen(browser_view->GetBoundsInScreen());
 
-  if (zoom_bubble_->use_focusless())
+  if (zoom_bubble_->can_activate())

[msw, 2014/07/23 18:57:00]

nit: I think this can just call Show and it'll respect can_activate.
(that's if can_activate is set properly, see related comments)

[Dan Beam, 2014/07/23 23:03:34]

the last time i checked, can_active() { return false; } + Show() still hijacked
focus on some platforms (or maybe it was some cases?).  i can't remember exactly
why.

[vasilii, 2014/07/24 16:32:14]

This line is my typo for sure.
Dan, I touched this problem in the description.

+    zoom_bubble_->GetWidget()->Show();
+  else
     zoom_bubble_->GetWidget()->ShowInactive();
-  else
-    zoom_bubble_->GetWidget()->Show();
 }
 
 // static
 void ZoomBubbleView::CloseBubble() {
   if (zoom_bubble_)
     zoom_bubble_->Close();
 }
 
 // static
 bool ZoomBubbleView::IsShowing() {
@@ skipping 14 matching lines @@
     FullscreenController* fullscreen_controller)
     : BubbleDelegateView(anchor_view, anchor_view ?
           views::BubbleBorder::TOP_RIGHT : views::BubbleBorder::NONE),
       image_button_(NULL),
       label_(NULL),
       web_contents_(web_contents),
       auto_close_(auto_close),
       immersive_mode_controller_(immersive_mode_controller) {
   // Compensate for built-in vertical padding in the anchor view's image.
   set_anchor_view_insets(gfx::Insets(5, 0, 5, 0));
-  set_use_focusless(auto_close);

[msw, 2014/07/23 18:57:00]

You've removed all the calls to set_use_focusless / set_can_activate for this
bubble, which is likely problematic. I think the zoom bubble needs to
ShowInactive on the [Ctrl]-[+] accelerator (to avoid altering focus), but it
needs to take activation when the user clicks that timed bubble, and when the
user invokes the magnifying glass page action icon by click or keyboard.

[Dan Beam, 2014/07/23 23:03:34]

correct, ctrl+/- should not move focus away from the web contents

[vasilii, 2014/07/24 16:32:13]

ctrl+ doesn't remove focus, I checked manually.

[Dan Beam, 2014/07/24 23:24:51]

did you check on linux chromeos, linux aura, and windows?

[vasilii, 2014/07/25 15:27:43]

Yeah.

   set_notify_enter_exit_on_child(true);
 
   // Add observers to close the bubble if the fullscreen state or immersive
   // fullscreen revealed state changes.
   registrar_.Add(this,
                  chrome::NOTIFICATION_FULLSCREEN_CHANGED,
                  content::Source<FullscreenController>(fullscreen_controller));
   immersive_mode_controller_->AddObserver(this);
 }
 
@@ skipping 90 matching lines @@
 }
 
 void ZoomBubbleView::OnExtensionIconImageChanged(
     extensions::IconImage* /* image */) {
   image_button_->SetImage(views::Button::STATE_NORMAL,
                           &extension_info_.icon_image->image_skia());
   image_button_->SchedulePaint();
 }
 
 void ZoomBubbleView::OnMouseEntered(const ui::MouseEvent& event) {
-  set_use_focusless(false);
   StopTimer();
 }
 
 void ZoomBubbleView::OnMouseExited(const ui::MouseEvent& event) {
-  set_use_focusless(auto_close_);
   StartTimerIfNecessary();
 }
 
 void ZoomBubbleView::OnGestureEvent(ui::GestureEvent* event) {
   if (!zoom_bubble_ || !zoom_bubble_->auto_close_ ||
       event->type() != ui::ET_GESTURE_TAP) {
     return;
   }
 
   // If an auto-closing bubble was tapped, show a non-auto-closing bubble in
@@ skipping 90 matching lines @@
 void ZoomBubbleView::WindowClosing() {
   // |zoom_bubble_| can be a new bubble by this point (as Close(); doesn't
   // call this right away). Only set to NULL when it's this bubble.
   if (zoom_bubble_ == this)
     zoom_bubble_ = NULL;
 }
 
 ZoomBubbleView::ZoomBubbleExtensionInfo::ZoomBubbleExtensionInfo() {}
 
 ZoomBubbleView::ZoomBubbleExtensionInfo::~ZoomBubbleExtensionInfo() {}