Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Description • 6 years ago

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.

	Alexei Barantsev
	Comment 1 • 6 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 2 • 6 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 3 • 6 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 4 • 6 years ago

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.

	Andreas Tolfsen ❲:ato❳
	Comment 5 • 6 years ago

(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

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 6 • 6 years ago

(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.

	Andreas Tolfsen ❲:ato❳
	Comment 7 • 6 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 8 • 6 years ago

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.

	Pulsebot
	Comment 15 • 5 years ago

Pushed by hskupin@mozilla.com:
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

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 16 • 5 years ago

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

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 17 • 5 years ago

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)