Add support for dynamic handling of modal dialogs and user prompts
Categories
(Remote Protocol :: Marionette, enhancement, P1)
Tracking
(firefox69 fixed)
Tracking | Status | |
---|---|---|
firefox69 | --- | fixed |
People
(Reporter: whimboo, Assigned: whimboo)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
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•5 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.
Assignee | ||
Comment 2•5 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.
Assignee | ||
Comment 3•5 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.
Assignee | ||
Comment 4•5 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.
Comment 5•5 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
Assignee | ||
Comment 6•5 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.
Comment 7•5 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.
Assignee | ||
Comment 8•5 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.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 9•4 years ago
|
||
(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...
Assignee | ||
Comment 10•4 years ago
|
||
Assignee | ||
Comment 11•4 years ago
|
||
Assignee | ||
Comment 12•4 years ago
|
||
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
Assignee | ||
Comment 13•4 years ago
|
||
Depends on D34139
Assignee | ||
Comment 14•4 years ago
|
||
I will have a fix for bug 1558763 landed soon, so I should remove the workaround in the patch.
Comment 15•4 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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/17319 for changes under testing/web-platform/tests
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•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e9e20d087e33
https://hg.mozilla.org/mozilla-central/rev/5d6397e990cb
https://hg.mozilla.org/mozilla-central/rev/4651b31ffe5c
Assignee | ||
Comment 19•4 years ago
|
||
(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•4 years ago
|
||
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 ;)
Assignee | ||
Comment 21•4 years ago
|
||
For reference this has been filed as https://github.com/mozilla/wpt-sync/issues/371.
Updated•8 months ago
|
Description
•