From ccc60a1f337ca033c80f7352d1dc8e36102833ed Mon Sep 17 00:00:00 2001 From: Heilig Benedek Date: Mon, 11 Feb 2019 20:38:58 +0100 Subject: [PATCH] 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 --- BUILD.gn | 6 +- atom/browser/api/atom_api_web_contents.cc | 6 +- atom/browser/common_web_contents_delegate.cc | 22 +++- atom/browser/common_web_contents_delegate.h | 6 +- .../common_web_contents_delegate_views.cc | 21 ---- atom/browser/ui/autofill_popup.cc | 119 +++--------------- atom/browser/ui/cocoa/views_delegate_mac.mm | 14 ++- atom/browser/ui/views/autofill_popup_view.cc | 10 +- chromium_src/BUILD.gn | 3 + patches/common/chromium/.patches | 1 + .../chromium/autofill_size_calculation.patch | 31 +++++ 11 files changed, 97 insertions(+), 142 deletions(-) create mode 100644 patches/common/chromium/autofill_size_calculation.patch diff --git a/BUILD.gn b/BUILD.gn index ad0e8e7302e7..c906344d0961 100644 --- a/BUILD.gn +++ b/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 diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index fb4e4cfe3efe..a18161a5584f 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/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& 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 diff --git a/atom/browser/common_web_contents_delegate.cc b/atom/browser/common_web_contents_delegate.cc index 0a95d50210c1..49430bae6b12 100644 --- a/atom/browser/common_web_contents_delegate.cc +++ b/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& values, + const std::vector& 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 diff --git a/atom/browser/common_web_contents_delegate.h b/atom/browser/common_web_contents_delegate.h index d38aa16082e3..0cad606f542f 100644 --- a/atom/browser/common_web_contents_delegate.h +++ b/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 autofill_popup_; #endif std::unique_ptr web_dialog_helper_; diff --git a/atom/browser/common_web_contents_delegate_views.cc b/atom/browser/common_web_contents_delegate_views.cc index 5170bd1cbfe3..f416e3ee428b 100644 --- a/atom/browser/common_web_contents_delegate_views.cc +++ b/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& values, - const std::vector& labels) { - if (!owner_window()) - return; - - auto* window = static_cast(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(); diff --git a/atom/browser/ui/autofill_popup.cc b/atom/browser/ui/autofill_popup.cc index a24319a8a44c..ddae9b121950 100644 --- a/atom/browser/ui/autofill_popup.cc +++ b/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 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 grow_right(right_growth_start, popup_width); - std::pair 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 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 popup_x_and_width = CalculatePopupXAndWidth( - top_left_display, bottom_right_display, desired_width, bounds, is_rtl); - std::pair 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() { diff --git a/atom/browser/ui/cocoa/views_delegate_mac.mm b/atom/browser/ui/cocoa/views_delegate_mac.mm index 25e72980367b..4831f8828f4d 100644 --- a/atom/browser/ui/cocoa/views_delegate_mac.mm +++ b/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() { diff --git a/atom/browser/ui/views/autofill_popup_view.cc b/atom/browser/ui/views/autofill_popup_view.cc index 8a726916f452..7558611c0eac 100644 --- a/atom/browser/ui/views/autofill_popup_view.cc +++ b/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); } diff --git a/chromium_src/BUILD.gn b/chromium_src/BUILD.gn index 05e5255812ef..208fea0420f4 100644 --- a/chromium_src/BUILD.gn +++ b/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", diff --git a/patches/common/chromium/.patches b/patches/common/chromium/.patches index 53e803ba1c1c..2b1db2270ddf 100644 --- a/patches/common/chromium/.patches +++ b/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 diff --git a/patches/common/chromium/autofill_size_calculation.patch b/patches/common/chromium/autofill_size_calculation.patch new file mode 100644 index 000000000000..b66dfcffc7cd --- /dev/null +++ b/patches/common/chromium/autofill_size_calculation.patch @@ -0,0 +1,31 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Heilig Benedek +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