Closed Bug 1477977 Opened 6 years ago Closed 5 years ago

Add support for dynamic handling of modal dialogs and user prompts

Categories

(Remote Protocol :: Marionette, enhancement, P1)

Product:
Remote Protocol
Component:
Marionette
Type:
enhancement

Tracking

(firefox69 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla69
Tracking Flags:
Tracking Status
firefox69 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1477977 - [marionette] Handle "tabmodal-dialog-loaded" to observe new tab modal dialogs. r=#webdriver
(deleted), text/x-phabricator-request
Details
Bug 1477977 - [marionette] Support dynamic handling of modal dialogs and tab modal alerts. r=#webdriver
(deleted), text/x-phabricator-request
Details
Bug 1477977 - [wdspec] Add test to make sure "Switch To Window" keeps tab modal status between tabs. r=#webdriver
(deleted), text/x-phabricator-request
Details
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.
Blocks: 1375451
Blocks: 1241679
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.
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.
Depends on: 1487358
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.
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.
(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
(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.
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.
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.
Blocks: 1545460
Blocks: 1487358
No longer depends on: 1487358

(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: nobody → hskupin
Blocks: webdriver
Status: NEW → ASSIGNED
No longer depends on: marionette-window-tracking
Priority: P3 → P1

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

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

Depends on: 1558763
Blocks: 1552742
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
Blocks: 1560010
No longer blocks: 1560010

(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

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?

Flags: needinfo?(james)

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 ;)

Flags: needinfo?(james)

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

Regressions: 1601160
Regressions: 1619481
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: