Permalink
Browse files

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>
  • Loading branch information...
brenca authored and jkleinsc committed Feb 11, 2019
1 parent 36ce3e9 commit ccc60a1f337ca033c80f7352d1dc8e36102833ed
@@ -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
@@ -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
@@ -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
@@ -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_;
@@ -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();
@@ -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 {

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;
}
return right_available >= popup_width || right_available >= left_available
? grow_right
: grow_left;
}
class PopupViewCommon : public autofill::PopupViewCommon {
public:
explicit PopupViewCommon(const gfx::Rect& window_bounds)
: window_bounds_(window_bounds) {}

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);
gfx::Rect GetWindowBounds(gfx::NativeView container_view) override {
return window_bounds_;
}
}

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());

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);
gfx::Rect bounds(origin, element_bounds_.size());
gfx::Rect window_bounds = parent_->GetBoundsInScreen();

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() {
@@ -16,7 +16,19 @@
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() {
@@ -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);
}
@@ -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",
@@ -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
@@ -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

0 comments on commit ccc60a1

Please sign in to comment.