1477977 - Add support for dynamic handling of modal dialogs and user prompts

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Currently we suffer from the following:

* We only handle user prompts from a single tab. If multiple tabs have user prompts open, those others are ignored.

* If an opened user prompt by a command is manually dismissed the internal state is not getting updated, and Marionette still thinks that the user prompt is present.

Well, there might also be more...

Before we can get started with that work we have to wait for bug 1311041 being fixed.

Comment 1

[Alexei Barantsev]

One of Selenium tests started to fail in Nightly "63.0a1 (2018-08-29) (64-bit)" (but it used to pass in "63.0a1 (2018-08-27) (64-bit)".
Test code: https://github.com/SeleniumHQ/selenium/blob/308609fa32844e406cf52e41e609e59bf522ba07/py/test/selenium/webdriver/common/alerts_tests.py#L209-L223
Sample page: https://github.com/SeleniumHQ/selenium/blob/308609fa32844e406cf52e41e609e59bf522ba07/common/src/web/alerts.html

:whimboo supposed it is related to a recently fixed issue 1479368 and now to get this working again we need to fix the current issue.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The problem here is that Marionette isn't able to handle multiple user prompts. It can only keep track of a single one as of now. The reason why the test was working before simply is because of the race condition. A second call to `accept()` or `dismiss()` was fired while the user prompt hasn't been closed yet. As such the command succeeded.

Now that we wait for the prompt to be closed, this will fail.

Maybe we can have an interim solution for Marionette and store user prompts in an array, as they appear. Once a prompt gets closed, we would still have the remaining ones present.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This regression as mentioned is actually easier to solve, which would also give us the opportunity to handle multiple user prompts in the same tab. I filed bug 1487358 to get this landed until we have a sane implementation here.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I'm not sure if dynamic handling would be doable. With my new unit test for bug 1487358 I see the following in the log:

[task 2018-08-31T10:41:21.761Z] 10:41:21     INFO -  1535712081755	Marionette	TRACE	21 -> [0,15,"WebDriver:ElementClick",{"id":"46b805da-5da2-4439-9b5d-8d2e12f198d1"}]
[task 2018-08-31T10:41:21.801Z] 10:41:21     INFO -  1535712081786	Marionette	DEBUG	[2147483649] Canceled page load listener because no navigation has been detected
[task 2018-08-31T10:41:21.801Z] 10:41:21     INFO -  1535712081791	Marionette	DEBUG	Received observer notification tabmodal-dialog-loaded

Clicking the link which opens two user prompts, results in only a single `tabmodal-dialog-loaded` notification. This would need further investigation.

Comment 5

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #2)

> The problem here is that Marionette isn't able to handle multiple
> user prompts. It can only keep track of a single one as of now.

This is specifically depending on the ability to keep
track of windows and content browsers in a reliable way:
https://bugzilla.mozilla.org/show_bug.cgi?id=1311041

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Andreas Tolfsen ﹝:ato﹞ from comment #5)
> > The problem here is that Marionette isn't able to handle multiple
> > user prompts. It can only keep track of a single one as of now.
> 
> This is specifically depending on the ability to keep
> track of windows and content browsers in a reliable way:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1311041

No, it's actually not as my patch on bug 1487358 shows. Whenever we switch a window, or close an alert we can check for existent tab modal prompts or modal dialogs. That way it will just work fine.

Comment 7

[Andreas Tolfsen ❲:ato❳]

So we are _able_ to make it work _without_
https://bugzilla.mozilla.org/show_bug.cgi?id=1311041, but in a very
tedious way.  When I say it’s “dependent upon”, it’s because it
“would be most conveniently solved” that way.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Well, I understand your point but mine here simply is what can we get now in case it is really required, and what is ok to be waiting for bug 1311041. And to be clear I don't see a reason to invest more time right now, beside bug 1487358 which revealed a web-compat issue with Firefox.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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

Currently we suffer from the following:

• We only handle user prompts from a single tab. If multiple tabs have user
  prompts open, those others are ignored.

Basically we interact with a single tab, and switching between all of them via WebDriver:SwitchToWindow. So once the switch has been done, the driver should check if there are open modal dialogs, and populate the internal variable. Otherwise it stays null.

• If an opened user prompt by a command is manually dismissed the internal
  state is not getting updated, and Marionette still thinks that the user
  prompt is present.

This is the reason because we don't have the right observer notifications registered. When manually dismissing an alert (or by the website itself) Marionette will not be informed about it, and we also do not update the internal variable at any point. Reason here is that we do not register for the DOMModalDialogClosed observer notification.

Well, there might also be more...

Right, for example when the website opens an alert with a delay. So the next command as executed by Marionette will fail. Reason here is that we do not register for the tabmodal-dialog-loaded observer notification.

Before we can get started with that work we have to wait for bug 1311041
being fixed.

Actually this isn't true. And given that this bug is wontfix now, lets get started...

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b0edf1cbfbd1cbe7baa59c5378b9a710abf4cb8

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1477977 - [marionette] Handle "tabmodal-dialog-loaded" to observe new tab modal dialogs. r=#webdriver

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1477977 - [marionette] Support dynamic handling of modal dialogs and tab modal alerts. r=#webdriver

With this patch Marionette registers globally for the dialog notifications
and events while a session is active. Also it provides an interface for
custom dialog handlers to hook in.

Instead of the callbacks custom events could have been fired, but that would
be some more work, and should preferable be done in a follow-up bug.

Depends on D34138

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1477977 - [wdspec] Add test to make sure "Switch To Window" keeps tab modal status between tabs. r=#webdriver

Depends on D34139

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I will have a fix for bug 1558763 landed soon, so I should remove the workaround in the patch.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e9e20d087e33
[marionette] Handle "tabmodal-dialog-loaded" to observe new tab modal dialogs. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/5d6397e990cb
[marionette] Support dynamic handling of modal dialogs and tab modal alerts. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/4651b31ffe5c
[wdspec] Add test to make sure "Switch To Window" keeps tab modal status between tabs. r=webdriver-reviewers,automatedtester

Comment 16

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

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

Comment 17

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/17319
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/eAueGSBNRKyFg8Q03R_Lkw)

Comment 18

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/e9e20d087e33
https://hg.mozilla.org/mozilla-central/rev/5d6397e990cb
https://hg.mozilla.org/mozilla-central/rev/4651b31ffe5c

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Web Platform Test Sync Bot from comment #17)

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/17319

• Taskcluster (pull_request)
  (https://tools.taskcluster.net/task-group-inspector/#/eAueGSBNRKyFg8Q03R_Lkw)

It got manually merged by James yesterday. As it looks like no comment is getting added to Bugzilla in such a case. James, is that a missing feature or just a bug?

Comment 20

[James Graham [:jgraham]]

Missing feature I think. Feel free to file issues at https://github.com/mozilla/wpt-sync/issues dheiberg is going to work on this stuff, so it might even get fixed ;)

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

For reference this has been filed as https://github.com/mozilla/wpt-sync/issues/371.