1686741 - Add complete support for the new tab modal dialog

Status: ASSIGNED

(Testing :: Marionette, enhancement, P3)

Description

[Micah Tigley [:mtigley]]

Marionette tests currently have the new modal UI disabled. This issue is for updating the existing tests.

Comment 1

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

It's not only around updating the tests in here:
https://searchfox.org/mozilla-central/source/testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py

Most of the tests actually work, but the ones that create a modal dialog before the Marionette session gets actually started cannot be found.

When starting to work on this bug the preference as set in recommended preferences has to be removed:
https://searchfox.org/mozilla-central/source/testing/marionette/components/marionette.js

Comment 2

[Release mgmt bot [:sylvestre / :calixte / :marco for bugbug]]

Set release status flags based on info from the regressing bug 1680637

Comment 3

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

Micah, would you have the time to get the support added to Marionette? I would kinda appreciate. If not mind giving some advice in how to use the new modal dialog type?

Comment 4

[Micah Tigley [:mtigley]]

Hey Henrik, sorry I dropped the ball on this. I've been swept up with other work.

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

Micah, would you have the time to get the support added to Marionette? I would kinda appreciate. If not mind giving some advice in how to use the new modal dialog type?

Of course! Here are some things we've been using in mochitests to test the new modal dialog type:

• We can get a modal container for a tab using gBrowser.getTabDialogBox.

• Modals opened by JS are stored in a separate manager on TabDialogBox. To get access to this manager we need to call TabDialogBox.getContentManager.

• We get access to its opened modal dialogs by accessing its ._dialogs member.

• To examine the contents of a single dialog. We have to access its dialog._frame.contentWindow object.

It might be easier to show an example of all these pieces being used in an existing test. I recently wrote a test for checking the title text of a dialog at: https://searchfox.org/mozilla-central/rev/fd853f4aea89186efdb368e759a71b7a90c2b89c/browser/base/content/test/tabdialogs/browser_tabdialogbox_content_prompts.js#50-53.

Comment 5

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

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

It might be easier to show an example of all these pieces being used in an existing test. I recently wrote a test for checking the title text of a dialog at: https://searchfox.org/mozilla-central/rev/fd853f4aea89186efdb368e759a71b7a90c2b89c/browser/base/content/test/tabdialogs/browser_tabdialogbox_content_prompts.js#50-53.

Thanks! I seem to have gotten it working for tab modals like alert and such, but I still wonder about those like for HTTP authentication. Those are still using getTabDialogManager(), right? So it's only alert, confirm, and prompt that implement the SubDialog path? It means we have to keep those existing checks?

Comment 6

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

Attachment: Bug 1686741 - [marionette] Add support for the new tab modal dialog.

Comment 7

[Micah Tigley [:mtigley]]

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

Thanks! I seem to have gotten it working for tab modals like alert and such, but I still wonder about those like for HTTP authentication. Those are still using getTabDialogManager(), right? So it's only alert, confirm, and prompt that implement the SubDialog path? It means we have to keep those existing checks?

Yes the HTTP auth modal is still using getTabDialogManager. Both the HTTP auth and alert/confirm/prompt modals used SubDialog, which begins around here: https://searchfox.org/mozilla-central/source/browser/actors/PromptParent.jsm#293-305.

The only difference between them is that prompts like HTTP auth are caused by webcontent but need to look like it's part of secure browser UI whereas alert/confirm/prompt don't need to be. It's mostly a styling difference. And to manage the two prompt types, we store them in separate managers, which are retrievable via getTabDialogManager() (for "tab" prompt types) or getContentDialogManager() (for "content" prompt types).

Comment 8

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

Making this change actually triggers a known intermittent test failure for TestTabModalAlerts.test_handle_two_dialogs (bug 1559992) pretty permanently. And as it looks like the first "WebDriver:AcceptAlert command returns to early and as such fires off immediately the next command, whereby the common-dialog-loaded observer notification is arrived afterward:

Here how it looks like when it works:

1613406564562   Marionette  DEBUG   10 -> [0,16,"WebDriver:ElementClick",{"id":"7991fec3-763f-cd44-8dba-302a6b1cc3a5"}]
1613406564605   Marionette  TRACE   Received observer notification common-dialog-loaded
1613406564605   Marionette  TRACE   Received observer notification common-dialog-loaded
1613406564605   Marionette  TRACE   Canceled page load listener because a dialog opened
1613406564605   Marionette  DEBUG   10 <- [1,16,null,{"value":null}]
1613406564643   Marionette  DEBUG   10 -> [0,17,"WebDriver:SendAlertText",{"text":"foo"}]
1613406564643   Marionette  DEBUG   10 <- [1,17,null,{"value":null}]
1613406564646   Marionette  DEBUG   10 -> [0,18,"WebDriver:AcceptAlert",{}]
1613406564650   Marionette  TRACE   Received event DOMModalDialogClosed
1613406564650   Marionette  TRACE   Received event DOMModalDialogClosed
1613406564650   Marionette  TRACE   Received DOM event DOMModalDialogClosed for [object XULFrameElement]
1613406564655   Marionette  DEBUG   10 <- [1,18,null,{"value":null}]
1613406564712   Marionette  TRACE   Received observer notification common-dialog-loaded
1613406564712   Marionette  TRACE   Received observer notification common-dialog-loaded
1613406564712   Marionette  DEBUG   10 -> [0,19,"WebDriver:SendAlertText",{"text":"bar"}]
1613406564712   Marionette  DEBUG   10 <- [1,19,null,{"value":null}]

And here when it fails:

1613406565071   Marionette  DEBUG   11 -> [0,16,"WebDriver:ElementClick",{"id":"65fb781e-b867-f94b-ae23-2f5ccd4c6516"}]
1613406565142   Marionette  TRACE   Received observer notification common-dialog-loaded
1613406565142   Marionette  TRACE   Received observer notification common-dialog-loaded
1613406565142   Marionette  TRACE   Canceled page load listener because a dialog opened
1613406565142   Marionette  DEBUG   11 <- [1,16,null,{"value":null}]
1613406565182   Marionette  DEBUG   11 -> [0,17,"WebDriver:SendAlertText",{"text":"foo"}]
1613406565182   Marionette  DEBUG   11 <- [1,17,null,{"value":null}]
1613406565184   Marionette  DEBUG   11 -> [0,18,"WebDriver:AcceptAlert",{}]
1613406565187   Marionette  TRACE   Received event DOMModalDialogClosed
1613406565187   Marionette  TRACE   Received event DOMModalDialogClosed
1613406565187   Marionette  TRACE   Received DOM event DOMModalDialogClosed for [object XULFrameElement]
1613406565188   Marionette  DEBUG   11 <- [1,18,null,{"value":null}]
1613406565216   Marionette  DEBUG   11 -> [0,19,"WebDriver:SendAlertText",{"text":"bar"}]
1613406565216   Marionette  DEBUG   11 <- [1,19,{"error":"no such alert","message":"","stacktrace":"WebDriverError@chrome://marionette/content/error.js:181:5\nNoSuchAl ... t@chrome://marionette/content/server.js:239:9\n_onJSONObjectReady/<@chrome://marionette/content/transport.js:504:20\n"},null]
1613406565222   Marionette  TRACE   Received observer notification common-dialog-loaded
1613406565222   Marionette  TRACE   Received observer notification common-dialog-loaded

Comment 9

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

Micah, could it be that the creation of the new tab modals is a bit slower than before? In the above case it's actually for the 2nd prompt as opened from here:

https://searchfox.org/mozilla-central/rev/b32d4ca055ca9cf717be480df640f8970724a0ce/testing/marionette/harness/marionette_harness/www/test_tab_modal_dialogs.html#27-30,37

Is there any kind of additional logging available for prompts? Not sure how to best investigate that race condition.

Comment 10

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

We clarified the problem, and actually have to wait for the second user prompt to appear.

Comment 11

[Henrik Skupin (:whimboo) [⌚️UTC+1]]

Attachment: Bug 1686741 - [marionette] Use dialogObserver when awaiting dialog to be closed.

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