From 05ba6359d016a6c80f24d4e1573eafcaf796cd42 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 15 Jul 2021 07:59:27 +0900 Subject: [PATCH] feat: add signal option to dialog.showMessageBox (#26102) * mac: add dialog.closeMessageBox API * win: Implement dialog.closeMessageBox * mac: Return cancelId with closeMessageBox * gtk: Implement dialog.closeMessageBox * win: Fix 32bit build * win: Reduce the scope of lock * fix: Build error after rebase * feat: Use AbortSignal to close message box * chore: silently handle duplicate ID * win: Add more notes about the threads * chore: apply reviews * fix: base::NoDestructor should be warpped in function * chore: fix style on windows --- docs/api/dialog.md | 7 + lib/browser/api/dialog.ts | 17 +++ shell/browser/api/electron_api_dialog.cc | 1 + shell/browser/ui/message_box.h | 7 +- shell/browser/ui/message_box_gtk.cc | 33 ++++- shell/browser/ui/message_box_mac.mm | 34 +++++ shell/browser/ui/message_box_win.cc | 138 ++++++++++++++++-- .../gin_converters/message_box_converter.cc | 5 +- spec-main/api-dialog-spec.ts | 58 +++++++- 9 files changed, 276 insertions(+), 24 deletions(-) diff --git a/docs/api/dialog.md b/docs/api/dialog.md index c1fc0a2b2947c..ab2833d14251a 100644 --- a/docs/api/dialog.md +++ b/docs/api/dialog.md @@ -273,6 +273,11 @@ If `browserWindow` is not shown dialog will not be attached to it. In such case will result in one button labeled "OK". * `defaultId` Integer (optional) - Index of the button in the buttons array which will be selected by default when the message box opens. + * `signal` AbortSignal (optional) - Pass an instance of [AbortSignal][] to + optionally close the message box, the message box will behave as if it was + cancelled by the user. On macOS, `signal` does not work with message boxes + that do not have a parent window, since those message boxes run + synchronously due to platform limitations. * `title` String (optional) - Title of the message box, some platforms will not show it. * `detail` String (optional) - Extra information of the message. * `checkboxLabel` String (optional) - If provided, the message box will @@ -360,3 +365,5 @@ window is provided. You can call `BrowserWindow.getCurrentWindow().setSheetOffset(offset)` to change the offset from the window frame where sheets are attached. + +[AbortSignal]: https://nodejs.org/api/globals.html#globals_class_abortsignal diff --git a/lib/browser/api/dialog.ts b/lib/browser/api/dialog.ts index 12c3152aab166..a46bf01a1d066 100644 --- a/lib/browser/api/dialog.ts +++ b/lib/browser/api/dialog.ts @@ -22,6 +22,11 @@ enum OpenFileDialogProperties { dontAddToRecent = 1 << 8 // Windows } +let nextId = 0; +const getNextId = function () { + return ++nextId; +}; + const normalizeAccessKey = (text: string) => { if (typeof text !== 'string') return text; @@ -157,6 +162,7 @@ const messageBox = (sync: boolean, window: BrowserWindow | null, options?: Messa let { buttons = [], cancelId, + signal, checkboxLabel = '', checkboxChecked, defaultId = -1, @@ -196,10 +202,21 @@ const messageBox = (sync: boolean, window: BrowserWindow | null, options?: Messa } } + // AbortSignal processing. + let id: number | undefined; + if (signal) { + // Generate an ID used for closing the message box. + id = getNextId(); + // Close the message box when signal is aborted. + if (signal.aborted) { return Promise.resolve({ cancelId, checkboxChecked }); } + signal.addEventListener('abort', () => dialogBinding._closeMessageBox(id)); + } + const settings = { window, messageBoxType, buttons, + id, defaultId, cancelId, noLink, diff --git a/shell/browser/api/electron_api_dialog.cc b/shell/browser/api/electron_api_dialog.cc index e6c5bc3803e07..f5e47ad4c6b15 100644 --- a/shell/browser/api/electron_api_dialog.cc +++ b/shell/browser/api/electron_api_dialog.cc @@ -91,6 +91,7 @@ void Initialize(v8::Local exports, gin_helper::Dictionary dict(isolate, exports); dict.SetMethod("showMessageBoxSync", &ShowMessageBoxSync); dict.SetMethod("showMessageBox", &ShowMessageBox); + dict.SetMethod("_closeMessageBox", &electron::CloseMessageBox); dict.SetMethod("showErrorBox", &electron::ShowErrorBox); dict.SetMethod("showOpenDialogSync", &ShowOpenDialogSync); dict.SetMethod("showOpenDialog", &ShowOpenDialog); diff --git a/shell/browser/ui/message_box.h b/shell/browser/ui/message_box.h index 9cc2d716f5466..da29f96708fa1 100644 --- a/shell/browser/ui/message_box.h +++ b/shell/browser/ui/message_box.h @@ -6,10 +6,10 @@ #define SHELL_BROWSER_UI_MESSAGE_BOX_H_ #include -#include #include #include "base/callback_forward.h" +#include "third_party/abseil-cpp/absl/types/optional.h" #include "ui/gfx/image/image_skia.h" namespace electron { @@ -24,12 +24,11 @@ enum class MessageBoxType { kQuestion, }; -using DialogResult = std::pair; - struct MessageBoxSettings { electron::NativeWindow* parent_window = nullptr; MessageBoxType type = electron::MessageBoxType::kNone; std::vector buttons; + absl::optional id; int default_id; int cancel_id; bool no_link = false; @@ -53,6 +52,8 @@ typedef base::OnceCallback void ShowMessageBox(const MessageBoxSettings& settings, MessageBoxCallback callback); +void CloseMessageBox(int id); + // Like ShowMessageBox with simplest settings, but safe to call at very early // stage of application. void ShowErrorBox(const std::u16string& title, const std::u16string& content); diff --git a/shell/browser/ui/message_box_gtk.cc b/shell/browser/ui/message_box_gtk.cc index b1290a6a5783f..253cd210117a1 100644 --- a/shell/browser/ui/message_box_gtk.cc +++ b/shell/browser/ui/message_box_gtk.cc @@ -2,15 +2,19 @@ // Use of this source code is governed by the MIT license that can be // found in the LICENSE file. -#include "shell/browser/ui/gtk_util.h" #include "shell/browser/ui/message_box.h" +#include + #include "base/callback.h" +#include "base/containers/contains.h" +#include "base/no_destructor.h" #include "base/strings/string_util.h" #include "base/strings/utf_string_conversions.h" #include "shell/browser/browser.h" #include "shell/browser/native_window_observer.h" #include "shell/browser/native_window_views.h" +#include "shell/browser/ui/gtk_util.h" #include "shell/browser/unresponsive_suppressor.h" #include "ui/base/glib/glib_signal.h" #include "ui/gfx/image/image_skia.h" @@ -38,10 +42,17 @@ MessageBoxSettings::~MessageBoxSettings() = default; namespace { +// map +std::map& GetDialogsMap() { + static base::NoDestructor> dialogs; + return *dialogs; +} + class GtkMessageBox : public NativeWindowObserver { public: explicit GtkMessageBox(const MessageBoxSettings& settings) - : cancel_id_(settings.cancel_id), + : id_(settings.id), + cancel_id_(settings.cancel_id), parent_(static_cast(settings.parent_window)) { // Create dialog. dialog_ = @@ -50,6 +61,8 @@ class GtkMessageBox : public NativeWindowObserver { GetMessageType(settings.type), // type GTK_BUTTONS_NONE, // no buttons "%s", settings.message.c_str()); + if (id_) + GetDialogsMap()[*id_] = dialog_; if (!settings.detail.empty()) gtk_message_dialog_format_secondary_text(GTK_MESSAGE_DIALOG(dialog_), "%s", settings.detail.c_str()); @@ -183,6 +196,9 @@ class GtkMessageBox : public NativeWindowObserver { private: electron::UnresponsiveSuppressor unresponsive_suppressor_; + // The id of the dialog. + absl::optional id_; + // The id to return when the dialog is closed without pressing buttons. int cancel_id_ = 0; @@ -196,6 +212,8 @@ class GtkMessageBox : public NativeWindowObserver { }; void GtkMessageBox::OnResponseDialog(GtkWidget* widget, int response) { + if (id_) + GetDialogsMap().erase(*id_); gtk_widget_hide(dialog_); if (response < 0) @@ -217,9 +235,20 @@ int ShowMessageBoxSync(const MessageBoxSettings& settings) { void ShowMessageBox(const MessageBoxSettings& settings, MessageBoxCallback callback) { + if (settings.id && base::Contains(GetDialogsMap(), *settings.id)) + CloseMessageBox(*settings.id); (new GtkMessageBox(settings))->RunAsynchronous(std::move(callback)); } +void CloseMessageBox(int id) { + auto it = GetDialogsMap().find(id); + if (it == GetDialogsMap().end()) { + LOG(ERROR) << "CloseMessageBox called with nonexistent ID"; + return; + } + gtk_window_close(GTK_WINDOW(it->second)); +} + void ShowErrorBox(const std::u16string& title, const std::u16string& content) { if (Browser::Get()->is_ready()) { electron::MessageBoxSettings settings; diff --git a/shell/browser/ui/message_box_mac.mm b/shell/browser/ui/message_box_mac.mm index 55fbf746e538d..b034aa33cc8aa 100644 --- a/shell/browser/ui/message_box_mac.mm +++ b/shell/browser/ui/message_box_mac.mm @@ -4,6 +4,7 @@ #include "shell/browser/ui/message_box.h" +#include #include #include #include @@ -11,8 +12,10 @@ #import #include "base/callback.h" +#include "base/containers/contains.h" #include "base/mac/mac_util.h" #include "base/mac/scoped_nsobject.h" +#include "base/no_destructor.h" #include "base/strings/sys_string_conversions.h" #include "shell/browser/native_window.h" #include "skia/ext/skia_utils_mac.h" @@ -26,6 +29,12 @@ namespace { +// map +std::map& GetDialogsMap() { + static base::NoDestructor> dialogs; + return *dialogs; +} + NSAlert* CreateNSAlert(const MessageBoxSettings& settings) { // Ignore the title; it's the window title on other platforms and ignorable. NSAlert* alert = [[NSAlert alloc] init]; @@ -128,6 +137,12 @@ void ShowMessageBox(const MessageBoxSettings& settings, int ret = [[alert autorelease] runModal]; std::move(callback).Run(ret, alert.suppressionButton.state == NSOnState); } else { + if (settings.id) { + if (base::Contains(GetDialogsMap(), *settings.id)) + CloseMessageBox(*settings.id); + GetDialogsMap()[*settings.id] = alert; + } + NSWindow* window = settings.parent_window ? settings.parent_window->GetNativeWindow().GetNativeNSWindow() @@ -136,9 +151,19 @@ void ShowMessageBox(const MessageBoxSettings& settings, // Duplicate the callback object here since c is a reference and gcd would // only store the pointer, by duplication we can force gcd to store a copy. __block MessageBoxCallback callback_ = std::move(callback); + __block absl::optional id = std::move(settings.id); + __block int cancel_id = settings.cancel_id; [alert beginSheetModalForWindow:window completionHandler:^(NSModalResponse response) { + if (id) + GetDialogsMap().erase(*id); + // When the alert is cancelled programmatically, the + // response would be something like -1000. This currently + // only happens when users call CloseMessageBox API, and we + // should return cancelId as result. + if (response < 0) + response = cancel_id; std::move(callback_).Run( response, alert.suppressionButton.state == NSOnState); [alert release]; @@ -146,6 +171,15 @@ void ShowMessageBox(const MessageBoxSettings& settings, } } +void CloseMessageBox(int id) { + auto it = GetDialogsMap().find(id); + if (it == GetDialogsMap().end()) { + LOG(ERROR) << "CloseMessageBox called with nonexistent ID"; + return; + } + [NSApp endSheet:it->second.window]; +} + void ShowErrorBox(const std::u16string& title, const std::u16string& content) { NSAlert* alert = [[NSAlert alloc] init]; [alert setMessageText:base::SysUTF16ToNSString(title)]; diff --git a/shell/browser/ui/message_box_win.cc b/shell/browser/ui/message_box_win.cc index 2cf56e61d2496..31484724d013d 100644 --- a/shell/browser/ui/message_box_win.cc +++ b/shell/browser/ui/message_box_win.cc @@ -11,8 +11,11 @@ #include #include +#include "base/containers/contains.h" +#include "base/no_destructor.h" #include "base/strings/string_util.h" #include "base/strings/utf_string_conversions.h" +#include "base/synchronization/lock.h" #include "base/task/post_task.h" #include "base/win/scoped_gdi_object.h" #include "shell/browser/browser.h" @@ -30,6 +33,41 @@ MessageBoxSettings::~MessageBoxSettings() = default; namespace { +struct DialogResult { + int button_id; + bool verification_flag_checked; +}; + +// map. +// +// Note that the HWND is stored in a unique_ptr, because the pointer of HWND +// will be passed between threads and we need to ensure the memory of HWND is +// not changed while dialogs map is modified. +std::map>& GetDialogsMap() { + static base::NoDestructor>> dialogs; + return *dialogs; +} + +// Speical HWND used by the dialogs map. +// +// - ID is used but window has not been created yet. +const HWND kHwndReserve = reinterpret_cast(-1); +// - Notification to cancel message box. +const HWND kHwndCancel = reinterpret_cast(-2); + +// Lock used for modifying HWND between threads. +// +// Note that there might be multiple dialogs being opened at the same time, but +// we only use one lock for them all, because each dialog is independent from +// each other and there is no need to use different lock for each one. +// Also note that the |GetDialogsMap| is only used in the main thread, what is +// shared between threads is the memory of HWND, so there is no need to use lock +// when accessing dialogs map. +base::Lock& GetHWNDLock() { + static base::NoDestructor lock; + return *lock; +} + // Small command ID values are already taken by Windows, we have to start from // a large number to avoid conflicts with Windows. const int kIDStart = 100; @@ -76,6 +114,31 @@ void MapToCommonID(const std::vector& buttons, } } +// Callback of the task dialog. The TaskDialogIndirect API does not provide the +// HWND of the dialog, and we have to listen to the TDN_CREATED message to get +// it. +// Note that this callback runs in dialog thread instead of main thread, so it +// is possible for CloseMessageBox to be called before or all after the dialog +// window is created. +HRESULT CALLBACK +TaskDialogCallback(HWND hwnd, UINT msg, WPARAM, LPARAM, LONG_PTR data) { + if (msg == TDN_CREATED) { + HWND* target = reinterpret_cast(data); + // Lock since CloseMessageBox might be called. + base::AutoLock lock(GetHWNDLock()); + if (*target == kHwndCancel) { + // The dialog is cancelled before it is created, close it directly. + ::PostMessage(hwnd, WM_CLOSE, 0, 0); + } else if (*target == kHwndReserve) { + // Otherwise save the hwnd. + *target = hwnd; + } else { + NOTREACHED(); + } + } + return S_OK; +} + DialogResult ShowTaskDialogWstr(NativeWindow* parent, MessageBoxType type, const std::vector& buttons, @@ -87,7 +150,8 @@ DialogResult ShowTaskDialogWstr(NativeWindow* parent, const std::u16string& detail, const std::u16string& checkbox_label, bool checkbox_checked, - const gfx::ImageSkia& icon) { + const gfx::ImageSkia& icon, + HWND* hwnd) { TASKDIALOG_FLAGS flags = TDF_SIZE_TO_CONTENT | // Show all content. TDF_ALLOW_DIALOG_CANCELLATION; // Allow canceling the dialog. @@ -169,12 +233,17 @@ DialogResult ShowTaskDialogWstr(NativeWindow* parent, config.dwFlags |= TDF_USE_COMMAND_LINKS; // custom buttons as links. } - int button_id; + // Pass a callback to receive the HWND of the message box. + if (hwnd) { + config.pfCallback = &TaskDialogCallback; + config.lpCallbackData = reinterpret_cast(hwnd); + } int id = 0; - BOOL verificationFlagChecked = FALSE; - TaskDialogIndirect(&config, &id, nullptr, &verificationFlagChecked); + BOOL verification_flag_checked = FALSE; + TaskDialogIndirect(&config, &id, nullptr, &verification_flag_checked); + int button_id; if (id_map.find(id) != id_map.end()) // common button. button_id = id_map[id]; else if (id >= kIDStart) // custom button. @@ -182,10 +251,11 @@ DialogResult ShowTaskDialogWstr(NativeWindow* parent, else button_id = cancel_id; - return std::make_pair(button_id, verificationFlagChecked); + return {button_id, static_cast(verification_flag_checked)}; } -DialogResult ShowTaskDialogUTF8(const MessageBoxSettings& settings) { +DialogResult ShowTaskDialogUTF8(const MessageBoxSettings& settings, + HWND* hwnd) { std::vector buttons; for (const auto& button : settings.buttons) buttons.push_back(base::UTF8ToWide(button)); @@ -199,31 +269,67 @@ DialogResult ShowTaskDialogUTF8(const MessageBoxSettings& settings) { return ShowTaskDialogWstr( settings.parent_window, settings.type, buttons, settings.default_id, settings.cancel_id, settings.no_link, title, message, detail, - checkbox_label, settings.checkbox_checked, settings.icon); + checkbox_label, settings.checkbox_checked, settings.icon, hwnd); } } // namespace int ShowMessageBoxSync(const MessageBoxSettings& settings) { electron::UnresponsiveSuppressor suppressor; - DialogResult result = ShowTaskDialogUTF8(settings); - return result.first; + DialogResult result = ShowTaskDialogUTF8(settings, nullptr); + return result.button_id; } void ShowMessageBox(const MessageBoxSettings& settings, MessageBoxCallback callback) { - dialog_thread::Run(base::BindOnce(&ShowTaskDialogUTF8, settings), - base::BindOnce( - [](MessageBoxCallback callback, DialogResult result) { - std::move(callback).Run(result.first, result.second); - }, - std::move(callback))); + // The dialog is created in a new thread so we don't know its HWND yet, put + // kHwndReserve in the dialogs map for now. + HWND* hwnd = nullptr; + if (settings.id) { + if (base::Contains(GetDialogsMap(), *settings.id)) + CloseMessageBox(*settings.id); + auto it = GetDialogsMap().emplace(*settings.id, + std::make_unique(kHwndReserve)); + hwnd = it.first->second.get(); + } + + dialog_thread::Run( + base::BindOnce(&ShowTaskDialogUTF8, settings, base::Unretained(hwnd)), + base::BindOnce( + [](MessageBoxCallback callback, absl::optional id, + DialogResult result) { + if (id) + GetDialogsMap().erase(*id); + std::move(callback).Run(result.button_id, + result.verification_flag_checked); + }, + std::move(callback), settings.id)); +} + +void CloseMessageBox(int id) { + auto it = GetDialogsMap().find(id); + if (it == GetDialogsMap().end()) { + LOG(ERROR) << "CloseMessageBox called with nonexistent ID"; + return; + } + HWND* hwnd = it->second.get(); + // Lock since the TaskDialogCallback might be saving the dialog's HWND. + base::AutoLock lock(GetHWNDLock()); + DCHECK(*hwnd != kHwndCancel); + if (*hwnd == kHwndReserve) { + // If the dialog window has not been created yet, tell it to cancel. + *hwnd = kHwndCancel; + } else { + // Otherwise send a message to close it. + ::PostMessage(*hwnd, WM_CLOSE, 0, 0); + } } void ShowErrorBox(const std::u16string& title, const std::u16string& content) { electron::UnresponsiveSuppressor suppressor; ShowTaskDialogWstr(nullptr, MessageBoxType::kError, {}, -1, 0, false, - u"Error", title, content, u"", false, gfx::ImageSkia()); + u"Error", title, content, u"", false, gfx::ImageSkia(), + nullptr); } } // namespace electron diff --git a/shell/common/gin_converters/message_box_converter.cc b/shell/common/gin_converters/message_box_converter.cc index 5403827ed5a32..acf22289ca094 100644 --- a/shell/common/gin_converters/message_box_converter.cc +++ b/shell/common/gin_converters/message_box_converter.cc @@ -4,9 +4,9 @@ #include "shell/common/gin_converters/message_box_converter.h" -#include "gin/dictionary.h" #include "shell/common/gin_converters/image_converter.h" #include "shell/common/gin_converters/native_window_converter.h" +#include "shell/common/gin_helper/dictionary.h" namespace gin { @@ -14,7 +14,7 @@ bool Converter::FromV8( v8::Isolate* isolate, v8::Local val, electron::MessageBoxSettings* out) { - gin::Dictionary dict(nullptr); + gin_helper::Dictionary dict; int type = 0; if (!ConvertFromV8(isolate, val, &dict)) return false; @@ -22,6 +22,7 @@ bool Converter::FromV8( dict.Get("messageBoxType", &type); out->type = static_cast(type); dict.Get("buttons", &out->buttons); + dict.GetOptional("id", &out->id); dict.Get("defaultId", &out->default_id); dict.Get("cancelId", &out->cancel_id); dict.Get("title", &out->title); diff --git a/spec-main/api-dialog-spec.ts b/spec-main/api-dialog-spec.ts index 96fd2779030fa..2b520075ae85a 100644 --- a/spec-main/api-dialog-spec.ts +++ b/spec-main/api-dialog-spec.ts @@ -1,7 +1,7 @@ import { expect } from 'chai'; import { dialog, BrowserWindow } from 'electron/main'; import { closeAllWindows } from './window-helpers'; -import { ifit } from './spec-helpers'; +import { ifit, delay } from './spec-helpers'; describe('dialog module', () => { describe('showOpenDialog', () => { @@ -121,6 +121,62 @@ describe('dialog module', () => { }); }); + describe('showMessageBox with signal', () => { + afterEach(closeAllWindows); + + it('closes message box immediately', async () => { + const controller = new AbortController(); + const signal = controller.signal; + const w = new BrowserWindow(); + const p = dialog.showMessageBox(w, { signal, message: 'i am message' }); + controller.abort(); + const result = await p; + expect(result.response).to.equal(0); + }); + + it('closes message box after a while', async () => { + const controller = new AbortController(); + const signal = controller.signal; + const w = new BrowserWindow(); + const p = dialog.showMessageBox(w, { signal, message: 'i am message' }); + await delay(500); + controller.abort(); + const result = await p; + expect(result.response).to.equal(0); + }); + + it('cancels message box', async () => { + const controller = new AbortController(); + const signal = controller.signal; + const w = new BrowserWindow(); + const p = dialog.showMessageBox(w, { + signal, + message: 'i am message', + buttons: ['OK', 'Cancel'], + cancelId: 1 + }); + controller.abort(); + const result = await p; + expect(result.response).to.equal(1); + }); + + it('cancels message box after a while', async () => { + const controller = new AbortController(); + const signal = controller.signal; + const w = new BrowserWindow(); + const p = dialog.showMessageBox(w, { + signal, + message: 'i am message', + buttons: ['OK', 'Cancel'], + cancelId: 1 + }); + await delay(500); + controller.abort(); + const result = await p; + expect(result.response).to.equal(1); + }); + }); + describe('showErrorBox', () => { it('throws errors when the options are invalid', () => { expect(() => {