fix: enable autofill popups on mac (#16308)

* feat: enable autofill popups on mac

* fix: make popup positioning better

* fix: don't try to show popup when widget is closing or not visible

* fix: unify conditions

* refactor: use PopupViewCommon from chrome directly

* lint: mark constructor explicit

* fix: use a patch instead of dummy functions to make things compile on Windows

* chore: address review suggestions

* Update atom/browser/ui/cocoa/views_delegate_mac.mm

Co-Authored-By: brenca <benecene@gmail.com>

BUILD.gn

@@ -397,8 +397,6 @@ static_library("electron_lib") {
       "*_views.cc",
       "*_views.h",
       "*\bviews/*",
-      "*/autofill_popup.cc",
-      "*/autofill_popup.h",
     ]
   }
 
@@ -423,6 +421,10 @@ static_library("electron_lib") {
       "//third_party/crashpad/crashpad/client",
       "//ui/accelerated_widget_mac",
     ]
+    sources += [
+      "atom/browser/ui/views/autofill_popup_view.cc",
+      "atom/browser/ui/views/autofill_popup_view.h",
+    ]
     include_dirs += [
       # NOTE(nornagon): other chromium files use the full path to include
       # crashpad; this is just here for compatibility between GN and GYP, so that

atom/browser/api/atom_api_web_contents.cc

@@ -586,7 +586,7 @@ void WebContents::SetContentsBounds(content::WebContents* source,
 
 void WebContents::CloseContents(content::WebContents* source) {
   Emit("close");
-#if defined(TOOLKIT_VIEWS) && !defined(OS_MACOSX)
+#if defined(TOOLKIT_VIEWS)
   HideAutofillPopup();
 #endif
   if (managed_web_contents())
@@ -1015,7 +1015,7 @@ void WebContents::DevToolsClosed() {
   Emit("devtools-closed");
 }
 
-#if defined(TOOLKIT_VIEWS) && !defined(OS_MACOSX)
+#if defined(TOOLKIT_VIEWS)
 void WebContents::ShowAutofillPopup(content::RenderFrameHost* frame_host,
                                     const gfx::RectF& bounds,
                                     const std::vector<base::string16>& values,
@@ -1063,7 +1063,7 @@ bool WebContents::OnMessageReceived(const IPC::Message& message,
         FrameDispatchHelper::OnSetTemporaryZoomLevel)
     IPC_MESSAGE_FORWARD_DELAY_REPLY(AtomFrameHostMsg_GetZoomLevel, &helper,
                                     FrameDispatchHelper::OnGetZoomLevel)
-#if defined(TOOLKIT_VIEWS) && !defined(OS_MACOSX)
+#if defined(TOOLKIT_VIEWS)
     IPC_MESSAGE_HANDLER(AtomAutofillFrameHostMsg_ShowPopup, ShowAutofillPopup)
     IPC_MESSAGE_HANDLER(AtomAutofillFrameHostMsg_HidePopup, HideAutofillPopup)
 #endif

atom/browser/common_web_contents_delegate.cc

@@ -206,7 +206,7 @@ void CommonWebContentsDelegate::SetOwnerWindow(
     NativeWindow* owner_window) {
   if (owner_window) {
     owner_window_ = owner_window->GetWeakPtr();
-#if defined(TOOLKIT_VIEWS) && !defined(OS_MACOSX)
+#if defined(TOOLKIT_VIEWS)
     autofill_popup_.reset(new AutofillPopup());
 #endif
     NativeWindowRelay::CreateForWebContents(web_contents,
@@ -619,4 +619,24 @@ void CommonWebContentsDelegate::SetHtmlApiFullscreen(bool enter_fullscreen) {
   native_fullscreen_ = false;
 }
 
+void CommonWebContentsDelegate::ShowAutofillPopup(
+    content::RenderFrameHost* frame_host,
+    content::RenderFrameHost* embedder_frame_host,
+    bool offscreen,
+    const gfx::RectF& bounds,
+    const std::vector<base::string16>& values,
+    const std::vector<base::string16>& labels) {
+  if (!owner_window())
+    return;
+
+  autofill_popup_->CreateView(frame_host, embedder_frame_host, offscreen,
+                              owner_window()->content_view(), bounds);
+  autofill_popup_->SetItems(values, labels);
+}
+
+void CommonWebContentsDelegate::HideAutofillPopup() {
+  if (autofill_popup_)
+    autofill_popup_->Hide();
+}
+
 }  // namespace atom

atom/browser/common_web_contents_delegate.h

@@ -18,7 +18,7 @@
 #include "content/public/browser/web_contents_delegate.h"
 #include "electron/buildflags/buildflags.h"
 
-#if defined(TOOLKIT_VIEWS) && !defined(OS_MACOSX)
+#if defined(TOOLKIT_VIEWS)
 #include "atom/browser/ui/autofill_popup.h"
 #endif
 
@@ -105,7 +105,7 @@ class CommonWebContentsDelegate : public content::WebContentsDelegate,
       const content::NativeWebKeyboardEvent& event) override;
 
   // Autofill related events.
-#if defined(TOOLKIT_VIEWS) && !defined(OS_MACOSX)
+#if defined(TOOLKIT_VIEWS)
   void ShowAutofillPopup(content::RenderFrameHost* frame_host,
                          content::RenderFrameHost* embedder_frame_host,
                          bool offscreen,
@@ -175,7 +175,7 @@ class CommonWebContentsDelegate : public content::WebContentsDelegate,
   bool native_fullscreen_ = false;
 
   // UI related helper classes.
-#if defined(TOOLKIT_VIEWS) && !defined(OS_MACOSX)
+#if defined(TOOLKIT_VIEWS)
   std::unique_ptr<AutofillPopup> autofill_popup_;
 #endif
   std::unique_ptr<WebDialogHelper> web_dialog_helper_;

atom/browser/common_web_contents_delegate_views.cc

@@ -41,27 +41,6 @@ bool CommonWebContentsDelegate::HandleKeyboardEvent(
   return false;
 }
 
-void CommonWebContentsDelegate::ShowAutofillPopup(
-    content::RenderFrameHost* frame_host,
-    content::RenderFrameHost* embedder_frame_host,
-    bool offscreen,
-    const gfx::RectF& bounds,
-    const std::vector<base::string16>& values,
-    const std::vector<base::string16>& labels) {
-  if (!owner_window())
-    return;
-
-  auto* window = static_cast<NativeWindowViews*>(owner_window());
-  autofill_popup_->CreateView(frame_host, embedder_frame_host, offscreen,
-                              window->content_view(), bounds);
-  autofill_popup_->SetItems(values, labels);
-}
-
-void CommonWebContentsDelegate::HideAutofillPopup() {
-  if (autofill_popup_)
-    autofill_popup_->Hide();
-}
-
 gfx::ImageSkia CommonWebContentsDelegate::GetDevToolsWindowIcon() {
   if (!owner_window())
     return gfx::ImageSkia();

atom/browser/ui/autofill_popup.cc

@@ -9,6 +9,8 @@
 #include "atom/browser/native_window_views.h"
 #include "atom/browser/ui/autofill_popup.h"
 #include "atom/common/api/api_messages.h"
+#include "base/i18n/rtl.h"
+#include "chrome/browser/ui/autofill/popup_view_common.h"
 #include "electron/buildflags/buildflags.h"
 #include "ui/display/display.h"
 #include "ui/display/screen.h"
@@ -25,85 +27,18 @@
 
 namespace atom {
 
-namespace {
+class PopupViewCommon : public autofill::PopupViewCommon {
+ public:
+  explicit PopupViewCommon(const gfx::Rect& window_bounds)
+      : window_bounds_(window_bounds) {}
 
-std::pair<int, int> CalculatePopupXAndWidth(
-    const display::Display& left_display,
-    const display::Display& right_display,
-    int popup_required_width,
-    const gfx::Rect& element_bounds,
-    bool is_rtl) {
-  int leftmost_display_x = left_display.bounds().x();
-  int rightmost_display_x =
-      right_display.GetSizeInPixel().width() + right_display.bounds().x();
-
-  // Calculate the start coordinates for the popup if it is growing right or
-  // the end position if it is growing to the left, capped to screen space.
-  int right_growth_start = std::max(
-      leftmost_display_x, std::min(rightmost_display_x, element_bounds.x()));
-  int left_growth_end =
-      std::max(leftmost_display_x,
-               std::min(rightmost_display_x, element_bounds.right()));
-
-  int right_available = rightmost_display_x - right_growth_start;
-  int left_available = left_growth_end - leftmost_display_x;
-
-  int popup_width =
-      std::min(popup_required_width, std::max(right_available, left_available));
-
-  std::pair<int, int> grow_right(right_growth_start, popup_width);
-  std::pair<int, int> grow_left(left_growth_end - popup_width, popup_width);
-
-  // Prefer to grow towards the end (right for LTR, left for RTL). But if there
-  // is not enough space available in the desired direction and more space in
-  // the other direction, reverse it.
-  if (is_rtl) {
-    return left_available >= popup_width || left_available >= right_available
-               ? grow_left
-               : grow_right;
+  gfx::Rect GetWindowBounds(gfx::NativeView container_view) override {
+    return window_bounds_;
   }
-  return right_available >= popup_width || right_available >= left_available
-             ? grow_right
-             : grow_left;
-}
 
-std::pair<int, int> CalculatePopupYAndHeight(
-    const display::Display& top_display,
-    const display::Display& bottom_display,
-    int popup_required_height,
-    const gfx::Rect& element_bounds) {
-  int topmost_display_y = top_display.bounds().y();
-  int bottommost_display_y =
-      bottom_display.GetSizeInPixel().height() + bottom_display.bounds().y();
-
-  // Calculate the start coordinates for the popup if it is growing down or
-  // the end position if it is growing up, capped to screen space.
-  int top_growth_end = std::max(
-      topmost_display_y, std::min(bottommost_display_y, element_bounds.y()));
-  int bottom_growth_start =
-      std::max(topmost_display_y,
-               std::min(bottommost_display_y, element_bounds.bottom()));
-
-  int top_available = bottom_growth_start - topmost_display_y;
-  int bottom_available = bottommost_display_y - top_growth_end;
-
-  // TODO(csharp): Restrict the popup height to what is available.
-  if (bottom_available >= popup_required_height ||
-      bottom_available >= top_available) {
-    // The popup can appear below the field.
-    return std::make_pair(bottom_growth_start, popup_required_height);
-  } else {
-    // The popup must appear above the field.
-    return std::make_pair(top_growth_end - popup_required_height,
-                          popup_required_height);
-  }
-}
-
-display::Display GetDisplayNearestPoint(const gfx::Point& point) {
-  return display::Screen::GetScreen()->GetDisplayNearestPoint(point);
-}
-
-}  // namespace
+ private:
+  gfx::Rect window_bounds_;
+};
 
 AutofillPopup::AutofillPopup() {
   bold_font_list_ = gfx::FontList().DeriveWithWeight(gfx::Font::Weight::BOLD);
@@ -181,34 +116,14 @@ void AutofillPopup::UpdatePopupBounds() {
   DCHECK(parent_);
   gfx::Point origin(element_bounds_.origin());
   views::View::ConvertPointToScreen(parent_, &origin);
+
   gfx::Rect bounds(origin, element_bounds_.size());
+  gfx::Rect window_bounds = parent_->GetBoundsInScreen();
 
-  int desired_width = GetDesiredPopupWidth();
-  int desired_height = GetDesiredPopupHeight();
-  bool is_rtl = false;
-
-  gfx::Point top_left_corner_of_popup =
-      origin + gfx::Vector2d(bounds.width() - desired_width, -desired_height);
-
-  // This is the bottom right point of the popup if the popup is below the
-  // element and grows to the right (since the is the lowest and furthest right
-  // the popup could go).
-  gfx::Point bottom_right_corner_of_popup =
-      origin + gfx::Vector2d(desired_width, bounds.height() + desired_height);
-
-  display::Display top_left_display =
-      GetDisplayNearestPoint(top_left_corner_of_popup);
-  display::Display bottom_right_display =
-      GetDisplayNearestPoint(bottom_right_corner_of_popup);
-
-  std::pair<int, int> popup_x_and_width = CalculatePopupXAndWidth(
-      top_left_display, bottom_right_display, desired_width, bounds, is_rtl);
-  std::pair<int, int> popup_y_and_height = CalculatePopupYAndHeight(
-      top_left_display, bottom_right_display, desired_height, bounds);
-
-  popup_bounds_ =
-      gfx::Rect(popup_x_and_width.first, popup_y_and_height.first,
-                popup_x_and_width.second, popup_y_and_height.second);
+  PopupViewCommon popup_view_common(window_bounds);
+  popup_bounds_ = popup_view_common.CalculatePopupBounds(
+      GetDesiredPopupWidth(), GetDesiredPopupHeight(), bounds,
+      gfx::NativeView(), base::i18n::IsRTL());
 }
 
 gfx::Rect AutofillPopup::popup_bounds_in_view() {

atom/browser/ui/cocoa/views_delegate_mac.mm

@@ -16,7 +16,19 @@ ViewsDelegateMac::~ViewsDelegateMac() {}
 void ViewsDelegateMac::OnBeforeWidgetInit(
     views::Widget::InitParams* params,
     views::internal::NativeWidgetDelegate* delegate) {
-  DCHECK(params->native_widget);
+  // If we already have a native_widget, we don't have to try to come
+  // up with one.
+  if (params->native_widget)
+    return;
+
+  if (!native_widget_factory().is_null()) {
+    params->native_widget = native_widget_factory().Run(*params, delegate);
+    if (params->native_widget)
+      return;
+  }
+
+  // Setting null here causes Widget to create the default NativeWidget implementation.
+  params->native_widget = nullptr;
 }
 
 ui::ContextFactory* ViewsDelegateMac::GetContextFactory() {

atom/browser/ui/views/autofill_popup_view.cc

@@ -56,19 +56,12 @@ AutofillPopupView::~AutofillPopupView() {
 }
 
 void AutofillPopupView::Show() {
-  if (!popup_)
+  if (!popup_ || !parent_widget_->IsVisible() || parent_widget_->IsClosed())
     return;
 
   const bool initialize_widget = !GetWidget();
   if (initialize_widget) {
     parent_widget_->AddObserver(this);
-    views::FocusManager* focus_manager = parent_widget_->GetFocusManager();
-    focus_manager->RegisterAccelerator(
-        ui::Accelerator(ui::VKEY_RETURN, ui::EF_NONE),
-        ui::AcceleratorManager::kNormalPriority, this);
-    focus_manager->RegisterAccelerator(
-        ui::Accelerator(ui::VKEY_ESCAPE, ui::EF_NONE),
-        ui::AcceleratorManager::kNormalPriority, this);
 
     // The widget is destroyed by the corresponding NativeWidget, so we use
     // a weak pointer to hold the reference and don't have to worry about
@@ -487,7 +480,6 @@ void AutofillPopupView::ClearSelection() {
 }
 
 void AutofillPopupView::RemoveObserver() {
-  parent_widget_->GetFocusManager()->UnregisterAccelerators(this);
   parent_widget_->RemoveObserver(this);
   views::WidgetFocusManager::GetInstance()->RemoveFocusChangeListener(this);
 }

chromium_src/BUILD.gn

@@ -41,6 +41,8 @@ static_library("chrome") {
     "//chrome/browser/net/proxy_service_factory.h",
     "//chrome/browser/ssl/security_state_tab_helper.cc",
     "//chrome/browser/ssl/security_state_tab_helper.h",
+    "//chrome/browser/ui/autofill/popup_view_common.cc",
+    "//chrome/browser/ui/autofill/popup_view_common.h",
     "//chrome/browser/win/chrome_process_finder.cc",
     "//chrome/browser/win/chrome_process_finder.h",
     "//extensions/browser/app_window/size_constraints.cc",
@@ -51,6 +53,7 @@ static_library("chrome") {
   ]
   deps = [
     "//chrome/common",
+    "//components/feature_engagement:buildflags",
     "//components/keyed_service/content",
     "//components/proxy_config",
     "//components/security_state/content",

patches/common/chromium/.patches

@@ -70,3 +70,4 @@ support_mixed_sandbox_with_zygote.patch
 disable_color_correct_rendering.patch
 disable_time_ticks_dcheck.patch
 fix_test_compilation_error.patch
+autofill_size_calculation.patch

patches/common/chromium/autofill_size_calculation.patch

@@ -0,0 +1,31 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Heilig Benedek <benecene@gmail.com>
+Date: Wed, 30 Jan 2019 17:04:33 +0100
+Subject: don't call into chrome internals for autofill popup size calculations
+
+The default GetWindowBounds calls into chrome internal functions to
+find out the size of the window - this can be overridden but even
+then some methods call into the original. Let's just return an empty
+gfx::Rect and do the actual job in the subclass.
+
+diff --git a/chrome/browser/ui/autofill/popup_view_common.cc b/chrome/browser/ui/autofill/popup_view_common.cc
+index 004e9cb86bee7c10f6a68cdf6ceb60bf39627e1d..c6da9a8f5c14615bf22192f540b6fd95fa1ccb0e 100644
+--- a/chrome/browser/ui/autofill/popup_view_common.cc
++++ b/chrome/browser/ui/autofill/popup_view_common.cc
+@@ -176,14 +176,14 @@ gfx::Rect PopupViewCommon::GetWindowBounds(gfx::NativeView container_view) {
+       views::Widget::GetTopLevelWidgetForNativeView(container_view);
+   if (widget)
+     return widget->GetWindowBoundsInScreen();
+-
++#if 0
+   // If the widget is null, try to get these bounds from a browser window. This
+   // is common on Mac when the window is drawn using Cocoa.
+   gfx::NativeWindow window = platform_util::GetTopLevel(container_view);
+   Browser* browser = chrome::FindBrowserWithWindow(window);
+   if (browser)
+     return browser->window()->GetBounds();
+-
++#endif
+   // If the browser is null, simply return an empty rect. The most common reason
+   // to end up here is that the NativeView has been destroyed externally, which
+   // can happen at any time. This happens fairly commonly on Windows (e.g., at