Closed Bug 1680637 Opened 4 years ago Closed 4 years ago

Implement tabmodal alert/confirm/prompt dialogs to use SubDialog instead of TabModalPrompt

Categories

(Firefox :: General, enhancement)

Product:
Firefox
Component:
General
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
86 Branch
Tracking Flags:
Tracking Status
firefox86 --- fixed

People

(Reporter: mtigley, Assigned: mtigley)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [proton-modals])

Attachments

(3 files)

Bug 1680637 - Add a dialog manager for content prompts in TabDialogBox
48 bytes, text/x-phabricator-request
Details | Review
Bug 1680637 - Rename TabDialogBox's manager to tabDialogManager
48 bytes, text/x-phabricator-request
Details | Review
Bug 1680637 - Update tests using the old dialog UI to only use the old UI. r=jaws,Gijs
48 bytes, text/x-phabricator-request
Details | Review

We should update the styling for tab modal prompts to be consistent with other "dialog" implementations as part of the visual refresh. This can be done by moving the tab modal prompts to use the same SubDialog implementation. The dialog should be centered vertically and its contents should display the origin at the top.

Showing the new tab modal prompt should be based on a pref. In this case we can instantiate the new one if the pref is enabled or instantiate the old one when it's disabled.

Whiteboard: [proton-modals]

(The pref it should be based on is described in bug 1680069.)

Summary: Update styling for TabModalPrompt → Implement tabmodal alert/confirm/prompt dialogs to use SubDialog instead of TabModalPrompt
Assignee: nobody → mtigley
Status: NEW → ASSIGNED

Micah and I met and discussed how to move forward here. Right now we have 3 implementations of prompting:

  1. window-modal prompts
  2. tab-modal prompts (the "pretty" blue buttons / white background dialogs that we use for e.g. the http auth dialog)
  3. content-modal prompts (the "ugly" ones that we wanna fix)

You can see all 3 in action using:

Services.prompt.alertBC(gBrowser.selectedBrowser.browsingContext, Ci.nsIPromptService.Ci.nsIPromptService.MODAL_TYPE_WINDOW, "Title", "message")

and for the other two, substitute Ci.nsIPromptService.MODAL_TYPE_TAB and Ci.nsiPromptService.MODAL_TYPE_CONTENT, respectively.

At one point, both (2) and (3) were implemented on TabModalPromptBox, and we made some display differentiation but used the same tabprompts.jsm code to display both. This is documented.

Unfortunately, the documentation is now slightly outdated - we switched to the new-ish ("pretty") dialogs for MODAL_TYPE_TAB in Firefox 82. The pref for this still exists, and so does a bunch of the old code, but because the pref is now always true, the check in PromptParent will now always use the new dialog for TAB type prompts.

In this bug, we basically want to (behind the proton pref, which is still off by default everywhere) switch to using the new/"pretty" implementation for the TYPE_CONTENT dialogs, too. Some other changes (like vertical centering, and displaying the origin in the dialog) are in other deps of the
proton-modals bug.

In theory, this could maybe use openChromePrompt in PromptParent, but in practice that runs the risk of showing a window-modal dialog. I think we'll want to make sure that we never show dialogs from web content in window-modal dialogs, so we should modify openContentPrompt and/or add a separate method to deal with the TYPE_CONTENT prompts when the proton pref is true.

Paul, can you double-check my representation above is correct? :-)

Flags: needinfo?(pbz)

Great summary, thanks Gijs!

Since we want to enable web content to open the SubDialog prompts directly, we should make sure that they can't be abused for DoS. The SubDialog based prompts are not as lightweight as the tabprompts.jsm so they will use more resources per dialog. Also, as you said, the content SubDialog prompts should be distinguishable from the tab (chrome) prompts.

Currently the tab dialogs are managed via the TabDialogBox: https://searchfox.org/mozilla-central/rev/23c25cd32a1e87095301273937b4ee162f41e860/browser/base/content/browser.js#8903
Perhaps this class can be reused. However, it's important that we have two different prompt queues for tab and content prompts. Tab prompts should always have priority and be on top.

Flags: needinfo?(pbz)

Thanks for the background everyone!

(In reply to Paul Zühlcke [:pbz] from comment #3)

Since we want to enable web content to open the SubDialog prompts directly, we should make sure that they can't be abused for DoS.

Gijs, do we do something similar for the current content prompts?

Based on the rest of the information given, we can potentially start with these changes:

We'll be addressing the content prompts being distinguishable in Bug 1682395 by making them vertically centered.

Flags: needinfo?(gijskruitbosch+bugs)

(In reply to Micah Tigley [:mtigley] from comment #4)

Thanks for the background everyone!

(In reply to Paul Zühlcke [:pbz] from comment #3)

Since we want to enable web content to open the SubDialog prompts directly, we should make sure that they can't be abused for DoS.

Gijs, do we do something similar for the current content prompts?

I elaborated more on slack, but the TL;DR: is that the DoS protection for alert/prompt/confirm generally lives in DOM global window code

Based on the rest of the information given, we can potentially start with these changes:

This sounds good!

Flags: needinfo?(gijskruitbosch+bugs)
Blocks: 1685317
Attachment #9193833 - Attachment description: Bug 1680637 - explore adding a dialog manager for content prompts in TabDialogBox → Bug 1680637 - Add a dialog manager for content prompts in TabDialogBox
Blocks: 1686082
Pushed by mtigley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/66563136c84e
Add a dialog manager for content prompts in TabDialogBox r=jaws,Gijs
https://hg.mozilla.org/integration/autoland/rev/e3566f596187
Rename TabDialogBox's manager to tabDialogManager r=marionette-reviewers,Gijs

Backed out 3 changesets (bug 1680637, bug 1682395) for browser/base/content/* failures.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&fromchange=7509aacc91b609dd6ccbc1bdffc5594cd4f878c4&searchStr=browser%2Cchrome&selectedTaskRun=YRJ8gy8jS7y0sLIKWci4XA.0&tochange=f493f8c4736ffa35a02dd0ff7ae96da61b15cc8b

Backout link: https://hg.mozilla.org/integration/autoland/rev/f493f8c4736ffa35a02dd0ff7ae96da61b15cc8b

Failure log: https://treeherder.mozilla.org/logviewer?job_id=326382944&repo=autoland&lineNumber=1786

[task 2021-01-11T19:02:40.887Z] 19:02:40     INFO - TEST-START | browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js
[task 2021-01-11T19:03:25.911Z] 19:03:25     INFO - TEST-INFO | started process screenshot
[task 2021-01-11T19:03:25.982Z] 19:03:25     INFO - TEST-INFO | screenshot: exit 0
[task 2021-01-11T19:03:25.982Z] 19:03:25     INFO - Buffered messages logged at 19:02:40
[task 2021-01-11T19:03:25.982Z] 19:03:25     INFO - Entering test bound closeLastTabInWindow
[task 2021-01-11T19:03:25.982Z] 19:03:25     INFO - Buffered messages logged at 19:02:41
[task 2021-01-11T19:03:25.983Z] 19:03:25     INFO - Wait tab event: load
[task 2021-01-11T19:03:25.983Z] 19:03:25     INFO - Tab event received: load
[task 2021-01-11T19:03:25.983Z] 19:03:25     INFO - Buffered messages finished
[task 2021-01-11T19:03:25.983Z] 19:03:25     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js | Test timed out - 
[task 2021-01-11T19:03:25.983Z] 19:03:25     INFO - GECKO(1388) | MEMORY STAT | vsize 2104228MB | vsizeMaxContiguous 69398365MB | residentFast 271MB | heapAllocated 87MB
[task 2021-01-11T19:03:25.983Z] 19:03:25     INFO - TEST-OK | browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js | took 45052ms
[task 2021-01-11T19:03:25.983Z] 19:03:25     INFO - checking window state
[task 2021-01-11T19:03:25.983Z] 19:03:25     INFO - Not taking screenshot here: see the one that was previously logged
[task 2021-01-11T19:03:25.984Z] 19:03:25     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js | Found a browser window after previous test timed out - 
[task 2021-01-11T19:03:25.984Z] 19:03:25     INFO - GECKO(1388) | JavaScript error: chrome://global/content/elements/browser-custom-element.js, line 1642: Error: permitUnload is already running for this tab.
[task 2021-01-11T19:03:25.984Z] 19:03:25     INFO - GECKO(1388) | must wait for focus
[task 2021-01-11T19:03:25.984Z] 19:03:25     INFO - GECKO(1388) | JavaScript error: chrome://global/content/elements/browser-custom-element.js, line 1696: uncaught exception: undefined
[task 2021-01-11T19:03:26.009Z] 19:03:26     INFO - Console message: [JavaScript Error: "Error: permitUnload is already running for this tab." {file: "chrome://global/content/elements/browser-custom-element.js" line: 1642}]
[task 2021-01-11T19:03:26.009Z] 19:03:26     INFO - asyncPermitUnload@chrome://global/content/elements/browser-custom-element.js:1642:15
[task 2021-01-11T19:03:26.009Z] 19:03:26     INFO - permitUnload@chrome://global/content/elements/browser-custom-element.js:1676:14
[task 2021-01-11T19:03:26.009Z] 19:03:26     INFO - CanCloseWindow@chrome://browser/content/browser.js:7869:36
[task 2021-01-11T19:03:26.009Z] 19:03:26     INFO - canClose@chrome://browser/content/browser.js:6252:12
[task 2021-01-11T19:03:26.009Z] 19:03:26     INFO - Tester_waitForWindowsState@chrome://mochikit/content/browser-test.js:460:13
[task 2021-01-11T19:03:26.009Z] 19:03:26     INFO - nextTest@chrome://mochikit/content/browser-test.js:819:10
[task 2021-01-11T19:03:26.009Z] 19:03:26     INFO - permitUnload@chrome://global/content/elements/browser-custom-element.js:1690:21
[task 2021-01-11T19:03:26.009Z] 19:03:26     INFO - _beginRemoveTab@chrome://browser/content/tabbrowser.js:3446:40
[task 2021-01-11T19:03:26.009Z] 19:03:26     INFO - removeTab@chrome://browser/content/tabbrowser.js:3340:15
[task 2021-01-11T19:03:26.009Z] 19:03:26     INFO - on_click@chrome://browser/content/tabbrowser-tab.js:431:20
[task 2021-01-11T19:03:26.009Z] 19:03:26     INFO - handleEvent@chrome://global/content/customElements.js:466:27
[task 2021-01-11T19:03:26.009Z] 19:03:26     INFO - closeLastTabInWindow@chrome://mochitests/content/browser/browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js:40:24
[task 2021-01-11T19:03:26.009Z] 19:03:26     INFO - 
[task 2021-01-11T19:03:26.019Z] 19:03:26     INFO - Not taking screenshot here: see the one that was previously logged
[task 2021-01-11T19:03:26.019Z] 19:03:26     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js | Uncaught exception received from previously timed out test - at chrome://mochitests/content/browser/browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js:42 - ReferenceError: ok is not defined
[task 2021-01-11T19:03:26.019Z] 19:03:26     INFO - Stack trace:
[task 2021-01-11T19:03:26.019Z] 19:03:26     INFO - closeLastTabInWindow@chrome://mochitests/content/browser/browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js:42:3
[task 2021-01-11T19:03:26.044Z] 19:03:26     INFO - TEST-START | browser/base/content/test/general/browser_bug1299667.js
[task 2021-01-11T19:03:28.053Z] 19:03:28     INFO - GECKO(1388) | MEMORY STAT | vsize 2104219MB | vsizeMaxContiguous 69398365MB | residentFast 269MB | heapAllocated 88MB
[task 2021-01-11T19:03:28.053Z] 19:03:28     INFO - TEST-OK | browser/base/content/test/general/browser_bug1299667.js | took 2010ms
[task 2021-01-11T19:03:28.072Z] 19:03:28     INFO - checking window state
[task 2021-01-11T19:03:28.082Z] 19:03:28     INFO - TEST-START | browser/base/content/test/general/browser_bug356571.js
[task 2021-01-11T19:03:28.132Z] 19:03:28     INFO - GECKO(1388) | MEMORY STAT | vsize 2104219MB | vsizeMaxContiguous 69398365MB | residentFast 270MB | heapAllocated 88MB
[task 2021-01-11T19:03:28.132Z] 19:03:28     INFO - TEST-OK | browser/base/content/test/general/browser_bug356571.js | took 49ms
[task 2021-01-11T19:03:28.151Z] 19:03:28     INFO - checking window state
[task 2021-01-11T19:03:28.161Z] 19:03:28     INFO - TEST-START | browser/base/content/test/general/browser_bug380960.js
[task 2021-01-11T19:03:28.241Z] 19:03:28     INFO - GECKO(1388) | MEMORY STAT | vsize 2104219MB | vsizeMaxContiguous 69398365MB | residentFast 270MB | heapAllocated 89MB
[task 2021-01-11T19:03:28.241Z] 19:03:28     INFO - TEST-OK | browser/base/content/test/general/browser_bug380960.js | took 80ms
...

Also seeing the following starting to perma fail with the backed out changes:
https://treeherder.mozilla.org/logviewer?job_id=326382947&repo=autoland&lineNumber=2330
https://treeherder.mozilla.org/logviewer?job_id=326382922&repo=autoland&lineNumber=12781

Flags: needinfo?(mtigley)

These tests assume we're using the old content prompt UI (which come with built-in handlers for various content prompt cases (i.e: https://searchfox.org/mozilla-central/source/browser/base/content/test/tabPrompts/browser_openPromptInBackgroundTab.js).

But since D100066 introduces a new pref (and is enabled by default in Nightly) for showing a content prompt using the TabDialogBox UI, any subsequent tests using the old UI will fail.

Flags: needinfo?(mtigley)

There's a couple of approaches we can take for this:

a) Go through all the tests expecting the old UI and introduce new tests that will handle the new one.

b) Disable the new UI in Nightly and land it. Then we fix all the tests (in a separate bug). This separate bug will need to be fixed before deciding to flip the pref.

c) Keep the new UI enabled by default in Nightly and set the "prompts.contentPromptSubDialog" pref to false in tests expecting the old UI.

Ideally, we do option "a" here. But this suggests we will be adding more code to TabDialogBox to handle content prompt specific actions. Option "b" lets us get the work in Nightly right away and give us room to fix the tests. Option "c" also lets us get the work into Nightly, but this leaves us open to new bugs (that are covered by the old UI) with the new UI as a result.

Gijs and Jared, do you have any opinions on what we should here?

Flags: needinfo?(jaws)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(bwinton)
Flags: needinfo?(bwinton)

I would recommend option C, and then filing a new bug to update the old tests so they test both dialog implementations.

Flags: needinfo?(jaws)

Either B or C is fine by me, though if we pick C it'd be useful to do at least some cursory manual smoketesting to check that we're not regressing anything major (like beforeunload dialogs, the "attention" state on tabs (and the checkbox + tabswitching if the site has permission).

Flags: needinfo?(gijskruitbosch+bugs)

(In reply to :Gijs (he/him) from comment #13)

if we pick C it'd be useful to do at least some cursory manual smoketesting to check that we're not regressing anything major (like beforeunload dialogs, the "attention" state on tabs (and the checkbox + tabswitching if the site has permission).

There is an issue where a background alert() will steal focus with the new modal UI. A good example of this is from the existing background prompt test: https://searchfox.org/mozilla-central/source/browser/base/content/test/tabPrompts/openPromptOffTimeout.html

As we discussed, we're not bailing out inside the handler for DOMWillOpenModalDialog, so we end up setting the selected tab.

I think the easiest fix for this is to copy what we do when we fire DOMWillOpenModalDialog for the old UI in openContentPrompt: https://searchfox.org/mozilla-central/rev/31ddf859c57e812878a5f817e4097efb06de4d97/browser/actors/PromptParent.jsm#195-207. I'm not sure if this should specifically be for content level prompts. If that's the case, we can add a check for the modal type:

// in openPromptWithTabDialogBox
let eventDetail =
  args.modalType === Services.prompt.MODAL_TYPE_CONTENT
    ? {
        wasPermitUnload: args.inPermitUnload,
        tabPrompt: true,
      }
    : null;
browser.enterModalState();
PromptUtils.fireDialogEvent(
  win,
  "DOMWillOpenModalDialog",
  browser,
  eventDetail
);
Blocks: 1686315
Blocks: 1686316
Pushed by mtigley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d513e228b0ad
Add a dialog manager for content prompts in TabDialogBox r=jaws,Gijs
https://hg.mozilla.org/integration/autoland/rev/c80de6c72779
Rename TabDialogBox's manager to tabDialogManager r=marionette-reviewers,Gijs
https://hg.mozilla.org/integration/autoland/rev/073e0be7db95
Update tests using the old dialog UI to only use the old UI. r=jaws,Gijs
Blocks: 1682393
Pushed by mtigley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c35335c49551
Add a dialog manager for content prompts in TabDialogBox r=jaws,Gijs
https://hg.mozilla.org/integration/autoland/rev/2b7e1df00a1c
Rename TabDialogBox's manager to tabDialogManager r=marionette-reviewers,Gijs
https://hg.mozilla.org/integration/autoland/rev/f5c4603fc02d
Update tests using the old dialog UI to only use the old UI. r=jaws,Gijs,marionette-reviewers,whimboo
Regressions: 1686604

Updated the tests in https://phabricator.services.mozilla.com/D101388 to only use the old modal UI for the browser_javascriptDialog*.js tests.

Flags: needinfo?(mtigley)

Micah, I assumed that we will only enable the new code in our own CI for now, but seeing bugs like bug 1680637 now, I feel it's enabled by default for Nightly users? If that is true it will be a bit more complicated for Marionette, geckodriver, and Puppeteer to get it all updated. It's not enough to just disable the pref for the failing tests, instead we would have to bake it into the test drivers directly. All of them are not only used in tree, their outside usage is much larger.

I assume that we can set this preference at any time and that it doesn't require a restart of Firefox? That would be the best approach here.

Flags: needinfo?(mtigley)

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #21)

Micah, I assumed that we will only enable the new code in our own CI for now, but seeing bugs like bug 1680637 now, I feel it's enabled by default for Nightly users? If that is true it will be a bit more complicated for Marionette, geckodriver, and Puppeteer to get it all updated. It's not enough to just disable the pref for the failing tests, instead we would have to bake it into the test drivers directly. All of them are not only used in tree, their outside usage is much larger.

I assume that we can set this preference at any time and that it doesn't require a restart of Firefox? That would be the best approach here.

Is it really worth doing this immediately, rather than getting a separate patch for marionette/geckodriver/puppeteer to actually work with the new dialogs? Because of update delays, I assume the outside use against nightly or beta would break either way (as they would have neither the code to deal with the new dialogs nor the code to set the pref to get old dialogs).

I would like to set the preference to force the old dialogs for now. I assume that this is possible? For beta users that don't have the pref it would just be a no-op. But therefore doing that I have to know if a restart is required or not.

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #23)

I would like to set the preference to force the old dialogs for now. I assume that this is possible? For beta users that don't have the pref it would just be a no-op. But therefore doing that I have to know if a restart is required or not.

No restart required, the pref is only used from JS.

Ok, so changes for Marionette and Remote Agent have to be made. Instead of just disabling the prompts for the in-tree tests, the feature needs to be completely disabled until it can be supported. As such please disable the feature via the preference for both Marionette and the Remote Agent in the following files:

https://searchfox.org/mozilla-central/source/remote/RecommendedPreferences.jsm
https://searchfox.org/mozilla-central/source/testing/marionette/components/marionette.js#71

Also please file separate bugs for both issues in Testing : Marionette and Remote Protocol : Page, and reference the bug # in a comment when adding the preference at the above locations. Thanks.

Regressions: 1686741
Regressions: 1686743

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #25)

Ok, so changes for Marionette and Remote Agent have to be made. Instead of just disabling the prompts for the in-tree tests, the feature needs to be completely disabled until it can be supported. As such please disable the feature via the preference for both Marionette and the Remote Agent in the following files:

https://searchfox.org/mozilla-central/source/remote/RecommendedPreferences.jsm
https://searchfox.org/mozilla-central/source/testing/marionette/components/marionette.js#71

Also please file separate bugs for both issues in Testing : Marionette and Remote Protocol : Page, and reference the bug # in a comment when adding the preference at the above locations. Thanks.

Thanks, Henrik. I'll get the patch updated!

Flags: needinfo?(mtigley)
No longer regressions: 1686741, 1686743
Regressions: 1686743
Regressions: 1686741
Pushed by mtigley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4788e6b14ff4
Add a dialog manager for content prompts in TabDialogBox r=jaws,Gijs
https://hg.mozilla.org/integration/autoland/rev/996fd993849c
Rename TabDialogBox's manager to tabDialogManager r=marionette-reviewers,Gijs
https://hg.mozilla.org/integration/autoland/rev/5f472f876f79
Update tests using the old dialog UI to only use the old UI. r=jaws,Gijs,marionette-reviewers,whimboo,remote-protocol-reviewers
Regressions: 1686316
Regressions: 1687893
Regressions: 1754759
Regressions: 1857432
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: