From cc058e14a5b1a67b2ee769decc3fef43ffeb2058 Mon Sep 17 00:00:00 2001 From: David Parks Date: Fri, 18 Oct 2024 21:58:28 +0000 Subject: [PATCH] Bug 1917201: Remove cancel button from modal while waiting for user to respond to Windows geolocation dialog r=Gijs a=RyanVM The cancel button is not really useful here and is probably confusing some users, since they are pressing it anyway, and it is causing this crash. The ideal fix would be for the Windows dialog to be modal instead of just immovable, but it isn't. Differential Revision: https://phabricator.services.mozilla.com/D225154 --- dom/geolocation/Geolocation.cpp | 35 ++++++++---- .../prompts/src/CommonDialog.sys.mjs | 7 ++- .../components/prompts/test/chromeScript.js | 14 ++++- .../prompts/test/test_modal_prompts.html | 54 +++++++++++++++++++ 4 files changed, 95 insertions(+), 15 deletions(-) diff --git a/dom/geolocation/Geolocation.cpp b/dom/geolocation/Geolocation.cpp index 9499ee0dc234..05e31b752bbf 100644 --- a/dom/geolocation/Geolocation.cpp +++ b/dom/geolocation/Geolocation.cpp @@ -360,20 +360,37 @@ void Geolocation::ReallowWithSystemPermissionOrCancel( do_GetService("@mozilla.org/prompter;1", &rv); NS_ENSURE_SUCCESS_VOID(rv); - RefPtr cancelDialogPromise; + // The dialog should include a cancel button if Gecko is prompting the user + // for system permission. It should have no buttons if the OS will be + // doing the prompting. + bool geckoWillPrompt = + GetLocationOSPermission() == + geolocation::SystemGeolocationPermissionBehavior::GeckoWillPromptUser; + // This combination of flags removes the default yes and no buttons and adds a + // spinner to the title. + const auto kSpinnerNoButtonFlags = nsIPromptService::BUTTON_TITLE_IS_STRING * + nsIPromptService::BUTTON_POS_0 + + nsIPromptService::BUTTON_TITLE_IS_STRING * + nsIPromptService::BUTTON_POS_1 + + nsIPromptService::SHOW_SPINNER; + // This combination of flags indicates there is only one button labeled + // "Cancel". + const auto kCancelButtonFlags = + nsIPromptService::BUTTON_TITLE_CANCEL * nsIPromptService::BUTTON_POS_0; + RefPtr tabBlockingDialogPromise; rv = promptSvc->AsyncConfirmEx( aBrowsingContext, nsIPromptService::MODAL_TYPE_TAB, title.get(), message.get(), - nsIPromptService::BUTTON_TITLE_CANCEL * nsIPromptService::BUTTON_POS_0, - nullptr, nullptr, nullptr, nullptr, false, JS::UndefinedHandleValue, - getter_AddRefs(cancelDialogPromise)); + geckoWillPrompt ? kCancelButtonFlags : kSpinnerNoButtonFlags, nullptr, + nullptr, nullptr, nullptr, false, JS::UndefinedHandleValue, + getter_AddRefs(tabBlockingDialogPromise)); NS_ENSURE_SUCCESS_VOID(rv); - MOZ_ASSERT(cancelDialogPromise); + MOZ_ASSERT(tabBlockingDialogPromise); - // If the cancel dialog promise is resolved or rejected then the dialog is no - // longer visible so we should stop waiting for permission, whether it was - // granted or not. - cancelDialogPromise->AppendNativeHandler( + // If the tab blocking dialog promise is resolved or rejected then the dialog + // is no longer visible so we should stop waiting for permission, whether it + // was granted or not. + tabBlockingDialogPromise->AppendNativeHandler( new CancelSystemGeolocationPermissionRequest(permissionRequest)); cancelRequestOnError.release(); diff --git a/toolkit/components/prompts/src/CommonDialog.sys.mjs b/toolkit/components/prompts/src/CommonDialog.sys.mjs index c788248442a6..bbfe76da8f90 100644 --- a/toolkit/components/prompts/src/CommonDialog.sys.mjs +++ b/toolkit/components/prompts/src/CommonDialog.sys.mjs @@ -64,9 +64,6 @@ CommonDialog.prototype = { if (this.args.button3Label) { numButtons++; } - if (numButtons == 0) { - throw new Error("A dialog with no buttons? Can not haz."); - } this.numButtons = numButtons; this.hasInputField = false; this.iconClass = ["question-icon"]; @@ -139,7 +136,9 @@ CommonDialog.prototype = { this.setLabelForNode(this.ui.button1, this.args.button1Label); } break; - + case 0: + this.ui.button0.hidden = true; + // fall through case 1: this.ui.button1.hidden = true; break; diff --git a/toolkit/components/prompts/test/chromeScript.js b/toolkit/components/prompts/test/chromeScript.js index cfcc2591c874..114d1003071a 100644 --- a/toolkit/components/prompts/test/chromeScript.js +++ b/toolkit/components/prompts/test/chromeScript.js @@ -204,12 +204,13 @@ function dismissPrompt(ui, action) { case 2: ui.button2.click(); break; - case "ESC": + case "ESC": { // XXX This is assuming tab-modal. let browserWin = Services.wm.getMostRecentWindow("navigator:browser"); EventUtils.synthesizeKey("KEY_Escape", {}, browserWin); break; - case "pollOK": + } + case "pollOK": { // Buttons are disabled at the moment, poll until they're reenabled. // Can't use setInterval here, because the window's in a modal state // and thus DOM events are suppressed. @@ -221,6 +222,15 @@ function dismissPrompt(ui, action) { clearInterval(interval); }, 100); break; + } + case "abort_dialogs": { + let abortDialogEvent = new ui.prompt.CustomEvent("dialogclosing", { + bubbles: true, + detail: { abort: true }, + }); + ui.prompt.close(abortDialogEvent); + break; + } case "none": break; diff --git a/toolkit/components/prompts/test/test_modal_prompts.html b/toolkit/components/prompts/test/test_modal_prompts.html index 7e1aafc8c9b1..ac6b7f182296 100644 --- a/toolkit/components/prompts/test/test_modal_prompts.html +++ b/toolkit/components/prompts/test/test_modal_prompts.html @@ -749,6 +749,60 @@ async function runTests(util) { await promptDone; + // ===== + info("Starting test: ConfirmEx (no buttons)"); + state = { + msg: "This is the confirmEx text.", + title: "TestTitle", + iconClass: "question-icon", + titleHidden: true, + textHidden: true, + passHidden: true, + checkHidden: true, + textValue: "", + passValue: "", + checkMsg: "", + checked: false, + butt0Label: "", + butt1Label: "", + focused: null, + defButton: "button0", + }; + + if (isOSX) { + // OS X doesn't initially focus the button, but rather the infoBody. + state.focused = "infoBody"; + } + + action = { + buttonClick: "abort_dialogs", + }; + + promptDone = handlePrompt(state, action); + + // BUTTON_TITLE_IS_STRING with no title removes the button. + flags = + Ci.nsIPromptService.BUTTON_TITLE_IS_STRING * Ci.nsIPromptService.BUTTON_POS_1 + + Ci.nsIPromptService.BUTTON_TITLE_IS_STRING * Ci.nsIPromptService.BUTTON_POS_0; + promptArgs = [ + "TestTitle", + "This is the confirmEx text.", + flags, + null, + null, + null, + null, + util.useAsync ? false : {}, + ]; + result = await util.prompt("confirmEx", promptArgs); + is( + util.useAsync ? result.buttonNumClicked : result, + 1, + "checked button click num unchanged from default" + ); + + await promptDone; + // ===== info("Starting test: ConfirmEx (ok/cancel, ok)"); state = {