For proton, to achieve consistent styling, we're considering moving window-modal dialogs into the main browser window, and either preventing tabswitches (probably hard, because it's not just user interaction that can cause tab switches) or just continuing to show the same dialog irrespective of which tab is selected.

I'm looking into this.

From discussing with Paul, we would probably want to add another tabdialogbox manager, specific to this usecase. We'd presumably want another modal type. And then we'd want to either opt consumers in one by one, or default all the ones that currently use WINDOW type modals, and pass a parent window that is a browser window, into the new style modal.

The main issue is that it's tricky to both allow user input into the modal dialog, but "stop" the rest of the window doing anything (switching tabs, responding to mouse/keyboard input, updating the selected tab due to focus() calls from the content process, etc.). We normally use enterModalState on the (content) window where we want this to happen, but unfortunately in this case the dialog is a frame inside the parent process browser window, so if we called it on the parent window the frame with the dialog would also enter modal state...

Olli, :mconley suggested that we have some machinery for this type of thing relating to tabswitching, where we are able to paint tabs when switching to them even if the main thread is busy, cf. https://searchfox.org/mozilla-central/rev/c03e8de87cdb0ce0378c0886d3c0ce8bbf9dc44e/dom/ipc/ProcessHangMonitor.cpp#352-461 . Would a similar kind of thing be possible here, ie could we somehow block the main thread for the rest of the window but keep updating the framed modal? Or is there another kind of helper that could be used to deal with this? (Looks like we haven't shipped <dialog> element support to release yet - would that help here?)

I don't quite understand how that tabswitching code could be related to this. It is very specific to child processes, and this bug is about parent process, right?

<dialog> element might work here (using showModal()) quite well, and it is enabled for chrome code.
At least showModal seem to stop basic input events going through the normal UI.
(but didn't try how the layout side of dialog works in browser chrome.)

(In reply to Olli Pettay [:smaug] from comment #2)

I don't quite understand how that tabswitching code could be related to this. It is very specific to child processes, and this bug is about parent process, right?

Correct, this is parent process. Sounds like we won't be able to use the same mechanism; I wasn't sure how specific to child processes it was (and/or if it was easy to extend it to support parent processes).

<dialog> element might work here (using showModal()) quite well, and it is enabled for chrome code.
At least showModal seem to stop basic input events going through the normal UI.
(but didn't try how the layout side of dialog works in browser chrome.)

OK, I will play around with this, then. Thanks!

OK, I'm having some issues with getting HTML dialogs to reliably show up. To see what I mean, if I run:

let x = document.createElement("dialog");
x.append("Hello this is a test");
document.body.insertBefore(x, document.getElementById("mainPopupSet"));
x.showModal();

in the browser console, this doesn't visually change the window, but if you try and click anywhere, it won't do anything. So the dialog is preventing input to the window (good!), but it's not being painted (not so good). Looking at the browser toolbox, the dialog has layout and width and height and visibility is visible, opacity is 1, and I can't for the life of me work out why nothing is showing up. I tried messing with z-index, or using visibility: hidden on the other items in the window, but nothing appears to help.

Kicker: if you right click the dialog node in the browser toolbox inspector and click "duplicate node", the duplicate shows up in the browser window (but not in the toolbox, confusingly - at least until you restart the toolbox).

I think this might be to do with display: fixed and XUL layout or something? But it's kind of hard to tell from frontend code.

Olli/Emilio, can you help me get unstuck?

(because of the toolbox messing showing a dialog, I actually worked out a reasonable amount of code to get this approach to work with SubDialog.jsm and other code we already have, so I'm hoping I'm actually close to a solution here - but obviously I have no idea how difficult it is to resolve this issue...)

Sean might know this better.

The issue is that we only build the display list for the top layer from the root scroll frame in ScrollFrameHelper::MaybeCreateTopLayerItems. But in the browser UI there's no root scrollframe.

So we're literally just not painting them, and it needs to be fixed. "duplicate node" seems to work but that just duplicates the dom so the resulting dialog is not modal.

Filed bug 1688004 to track this, will fix.

OK, with the fixes from bug 1688004 and having had some help on "interesting" behaviour of promises if you call alert() directly on browser windows (promises stop resolving!), I now have something workable. It wasn't significantly more difficult to just make something generic, so I'm morphing the summary a little. I'm hoping to put up the patch tonight or tomorrow.

This removes observation of the 'disabled' attribute from the macOS
full screen menu items, because removing the attribute doesn't work
correctly. This is a scenario that, as far as I can tell, didn't
happen elsewhere until now. On other OSes we use a single item which
gets disabled state directly from the command attribute (ie without
an 'observes' child) which appears to work fine.

It also exempts the editing commands from being disabled, but it
appears that at least on macOS, undo history is lost anyway. It's
not clear to me why this is the case, but I don't think it needs
to block an initial landing of this work.

Depends on D103388

Depends on D103389

Backed out for failing test_prompt.html

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=fkf7bpz3Tb2x9nSnim-8cQ.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&searchStr=3%2Cwindows&revision=7bdcbdc2d57fedec002319c8df42502684d2395f

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

Backout: https://hg.mozilla.org/integration/autoland/rev/5d5ff4c32b04ec8f24ba9854b9ce3babb7c521b0

Seems fixed by dispatching the DOMModalDialogClosed event correctly:

https://treeherder.mozilla.org/jobs?repo=try&revision=0ffa06802599ccfe61b672c8b0dff0686ac5ceff

So I re-triggered landing.

https://hg.mozilla.org/mozilla-central/rev/01606a3ed359
https://hg.mozilla.org/mozilla-central/rev/6ff29d6f9b49
https://hg.mozilla.org/mozilla-central/rev/1129347e2aad

Hi Gijs, are there any steps I could use to Verify the fix on this issue? Following this thread I'm not exactly sure what the issue is.

(In reply to Rares Doghi from comment #17)

Hi Gijs, are there any steps I could use to Verify the fix on this issue? Following this thread I'm not exactly sure what the issue is.

When this patch landed, the quit / close multiple tabs warning moved from being a separate window (or, on macOS, a native macOS sheet) to being the white-background dialog inside the window. Does that help?

Yup, Thank you.
But this is only displayed with the white-background in 89 and 90 right ? in non proton 88 its still the same grey background.

88 build.

Not sure how to mark the 87 tracking flag but this issue is verified as fixed in 89.0b10 and our latest Nightly builds 90.0a1 (2021-05-11).

(In reply to Rares Doghi from comment #19)

Yup, Thank you.
But this is only displayed with the white-background in 89 and 90 right ? in non proton 88 its still the same grey background.

It's behind the prompts.windowPromptSubDialog pref which was flipped to true for 89 - you can probably verify in 88/87 by flipping that pref and restarting, if you wanted - though tbh I'm fine with just leaving this as verified for 89 as well, depends on how busy you already are. ;-)

Yep this issue is verified as fixed in 87 as well on Mac, Windows and Ubuntu. Thanks for the help Gijs.