Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:88.0) Gecko/20100101 Firefox/88.0 ID:20210322093509

Marionettes modal dialog handler uses callbacks to inform about opened and closed modal dialogs:

https://searchfox.org/mozilla-central/rev/d58860eb739af613774c942c3bb61754123e449b/testing/marionette/modal.js#138,156

While this works fine for modal dialogs of the attached window, which block every tab, it's inappropriate for tab modals. Consumers currently only check for the ChromeWindow and as such would handle a tab modal dialog that gets opened in a not currently selected tab (window handle).

https://searchfox.org/mozilla-central/rev/d58860eb739af613774c942c3bb61754123e449b/testing/marionette/actors/MarionetteCommandsParent.jsm#79
https://searchfox.org/mozilla-central/rev/d58860eb739af613774c942c3bb61754123e449b/testing/marionette/driver.js#232-234

Before we fix that we need a good set of modal dialog tests (global and tab only) that open in a background tab.

This bug blocks my work on bug 1686741.

The patch on https://phabricator.services.mozilla.com/D110270 for Remote Agent will help us to only care about dialogs of the currently selected tab.

It actually needs a change in Toolkit, which is part of the patch on bug 1686743.

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

It actually needs a change in Toolkit, which is part of the patch on bug 1686743.

This landed and is marked fixed. Is this bug now fixed too?

No, we have to make use of the new toolkit code to filter out modals from other than the currently selected tab.

So over on bug 1686743 the filtering of newly opened tab modals is done via:

const dialogs = browser.tabDialogBox.getContentDialogManager().dialogs;
dialog = dialogs.find(dialog => dialog.frameContentWindow === dialogWindow);

This wont work for the old tab modals. So given that we would like to keep backward compatibility for Marionette there are two questions:

1. Will we clearly ship with Firefox 89 with the new modal dialogs enabled by default?
2. Will the pref prompts.contentPromptSubDialog continue to exist so that users could switch back to the old modals?

If 2) is true how could we best check if the dialog is part of the current browser? Would dialogWindow.previousSibling === browser be a good check?

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

1. Will we clearly ship with Firefox 89 with the new modal dialogs enabled by default?

Yes, that's currently the plan.

2. Will the pref prompts.contentPromptSubDialog continue to exist so that users could switch back to the old modals?

I'm going to guess "no", but redirecting to Gijs who probably knows better.

(In reply to Mike Conley (:mconley) (:⚙️) (Catching up on needinfos) from comment #6)

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

1. Will we clearly ship with Firefox 89 with the new modal dialogs enabled by default?

Yes, that's currently the plan.

2. Will the pref prompts.contentPromptSubDialog continue to exist so that users could switch back to the old modals?

I'm going to guess "no", but redirecting to Gijs who probably knows better.

Not long-term, though we probably don't have time to remove it for 89, I don't think that means we need to support it though.

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

Not long-term, though we probably don't have time to remove it for 89, I don't think that means we need to support it though.

Ok, but do we see any risks so far in a possible disabling of the feature in a late beta? I know that this is impossible to say but if there are risks I would feel better to keep the support for the old modals in Marionette until all the old code gets removed. If we turn it off late in beta or even in a release this might have an affect for thousands of external CI systems. I would kinda like to avoid this situation.

It's fairly easy to keep support for it in Marionette right now, and file a follow-up to remove the old code once it's gone in Firefox. As such (just to ask again) would dialogWindow.previousSibling === browser be a good idea?

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

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

Not long-term, though we probably don't have time to remove it for 89, I don't think that means we need to support it though.

Ok, but do we see any risks so far in a possible disabling of the feature in a late beta? I know that this is impossible to say

"never say never", but the number of things that depend on this shipping is pretty large at this point.

but if there are risks I would feel better to keep the support for the old modals in Marionette until all the old code gets removed. If we turn it off late in beta or even in a release this might have an affect for thousands of external CI systems. I would kinda like to avoid this situation.

Sure, up to you.

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

So over on bug 1686743 the filtering of newly opened tab modals is done via:

const dialogs = browser.tabDialogBox.getContentDialogManager().dialogs;
dialog = dialogs.find(dialog => dialog.frameContentWindow === dialogWindow);

If 2) is true how could we best check if the dialog is part of the current browser? Would dialogWindow.previousSibling === browser be a good check?

I think for both cases it'd be better to find the container of the dialog in the parent document, and ensure it is a descendant of the same container as the current browser. This avoids depending a lot on the current DOM hierarchy and having to change the code every time specifics in nesting levels or whatever vary. In particular, given a browser, you can do:

let container = browser.closest(".browserSidebarContainer");

and given a dialog window with the new dialogs, you can get its framing element using let frame = win.docShell.chromeEventHandler. Then container.contains(frame) will tell you if it's for the current tab. For the old dialogs, you get some prompt element as the subject of the observer notification, and you can use that directly with container.contains(subject), I think.

Does that help?

Most of the existing tests in that file are meanwhile covered
by Wdspec tests, and as such can be removed. Only the
HTTP authentication tests have to remain for now given that
those aren't part of the WebDriver specification yet.

To allow the handling of all different modal dialog types,
which are displayed as content, tab, or window alert appropriate
tests have been added.

Depends on D112364

Using the "waitForEvent" promise will cause an extra
"DOMModalDialogClosed" event to be logged, which is
confusing.

Depends on D107460

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

Does that help?

Not completely but lets move the discussion over to the following Phabricator revision given that it is better to see real code:
https://phabricator.services.mozilla.com/D112366

Backed out 5 changesets (Bug 1701686) for causing xpcshell failures in test_modal.js
Backout link: https://hg.mozilla.org/integration/autoland/rev/775a656b6ded712269f8b87db0a0ed4185b5e8b8
Push with failures, failure log.

(In reply to Alexandru Michis [:malexandru] from comment #18)

Backed out 5 changesets (Bug 1701686) for causing xpcshell failures in test_modal.js

With reverting an optional change the xpcshell test has been fixed. We will re-check via bug 1707497 how to remove the ownerGlobal checks and improve the test_modal.js test.

https://hg.mozilla.org/mozilla-central/rev/9ffdbe61355a
https://hg.mozilla.org/mozilla-central/rev/6d2c8ff216fc
https://hg.mozilla.org/mozilla-central/rev/e352f905ecf8
https://hg.mozilla.org/mozilla-central/rev/1cddea72d353
https://hg.mozilla.org/mozilla-central/rev/15253a3168d2

(In reply to Web Platform Test Sync Bot (Matrix: #interop:mozilla.org) from comment #17)

Created web-platform-tests PR
https://github.com/web-platform-tests/wpt/pull/28683 for changes under
testing/web-platform/tests

James, can you please poke merging this PR?