1712414 - Provide event to marionette whenever a new prompt is opened

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P1)

Description

[[ex-Mozilla] Agi Sferro | :agi]

Marionette needs to know whenever a new prompt is opened, we should fire an observer event every time a prompt is opened with the prompt object so that marionette/Web Driver can integrate with it. See also blocking bug.

Comment 1

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

Now that bug 1710668 is fixed, could this feature be implemented?

Comment 2

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

Hi, do we have an update for this bug? It would be great to see such support. Thanks.

Comment 3

[Olivia Hall [:olivia]]

Attachment: Bug 1712414 - GeckoView Prompts Adjustment for Marionette

Comment 4

[Olivia Hall [:olivia]]

Attachment: WIP: Bug 1712414 Added topic geckoview-show-prompt for notifying observers that GeckoView is opening a prompt.

Comment 5

[Olivia Hall [:olivia]]

Hi Henrik, I added a topic “geckoview-prompt-show” right after we dispatch the event that will open the prompt and it'll have the prompt object. Do you think that'll work for your needs? I’m also starting adding the additional support for 1708105

Comment 6

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

Thanks Olivia. I actually already replied in the Phabricator review. Basically it looks fine but I would like to see if Marionette can correctly handle that. I'm happy to mentor you over on bug 1708105. Feel free to join us on Element in the #webdriver channel.

Comment 7

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

Hi Olivia, with your work on prompt support for Marionette (bug 1708105), do you think that the patch on this bug is complete or does it require further changes? If it's all what we need maybe we can indeed get it landed and do all the rest over on the other bug?

Comment 8

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

Actually as it looks like most of the additions in D134448 are actually about GeckoView and probably have to be moved to the revision on this bug?

Comment 9

[Olivia Hall [:olivia]]

Sure, I can definitely move the more Marionette parts over here! For example, the changes to modal.js.

I think driver.js is also going to need a few updates too. What do you think?

For example, at GeckoDriver.prototype.dismissDialog dismisses the dialogue by clicking the UI button programmatically. I think for GeckoView it will need to call this dismiss() on GeckoViewPrompter. I'm working on trying to get the messaging setup so that Marionette can get the GeckoViewPrompter prompts from the window and then call those methods. Does that sound like the right thing to do?

Comment 10

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

I've applied your patch to my local tree and tested a bit the two different modes (handling prompts that open during a command is processed, and finding open prompts). Here is what I can see:

1. The modal dialog observer as registered for geckoview-prompt-show doesn't actually receive the notification when a prompt gets opened. There should be a Received observer notification geckoview-prompt-show in the adb log. As such the just opened prompt is not recognized and a follow up command to accept/dismiss the prompt will fail because there is basically no prompt set.

2. If GeckoView cannot provide a UI path to close the dialog we might have to fallback to directly call the appropriate methods. That wouldn't be ideal given that WebDriver is build to actually simulate user interaction but if the platform cannot offer it for us, we should work with the features that we get.

I hope that we can sort out 1) fast enough. 2) doesn't sound to be complex. Let me know if you have further questions.

Comment 11

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

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

1. The modal dialog observer as registered for geckoview-prompt-show doesn't actually receive the notification when a prompt gets opened. There should be a Received observer notification geckoview-prompt-show in the adb log. As such the just opened prompt is not recognized and a follow up command to accept/dismiss the prompt will fail because there is basically no prompt set.

Could this maybe happen because the observer is fired on the child process and not from the parent process?

Comment 12

[Olivia Hall [:olivia]]

Thanks for the help! The issue with the observer was 100% that it was firing off of the child process, I moved the observer notification to the parent and it seems to be notifying as expected now.

I'll get back to you on the second issue soon after making some changes, now that the first is working.

Comment 13

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

That's great to hear. I'm looking forward in how it will look like when having all the bits together.

Please adjust the two commits so they are attached to the correct bugs. Right now the patch on this bug contains Marionette changes while it's for GeckoView. Thanks!

Comment 14

[Olivia Hall [:olivia]]

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

That's great to hear. I'm looking forward in how it will look like when having all the bits together.

Please adjust the two commits so they are attached to the correct bugs. Right now the patch on this bug contains Marionette changes while it's for GeckoView. Thanks!

Thanks, patches should be switched now!

Comment 15

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

It's great to see that we now can run the user prompt tests and don't end-up in timeouts anymore because the events for opening and closing user prompts exist. As Olivia stated several tests are still failing. So I did a quick check and can see that dialogs aren't always closed. As such here some steps that might help to identify the underlying issue:

• Concentrate on the test testing/web-platform/tests/webdriver/tests/get_alert_text/get.py for now and only run the last two tests in that file (rename the others from i.e. test_ to tst_) because here we do not have unsupported features for GeckoView.

• Here the first test opens a prompt with the text Enter Your Name. That text is correctly detected but then in test teardown the following error is visible when trying to close the dialog:

03-10 09:46:36.563  8072  8098 I Gecko   : 1646901996563	Marionette	DEBUG	0 -> [0,11,"WebDriver:DismissAlert",{}]
03-10 09:46:36.564  8072  8098 I Gecko   : 1646901996564	Marionette	DEBUG	0 <- [1,11,{"error":"no such alert","message":"","stacktrace":"WebDriverError@chrome://remote/content/shared/webdriver/Errors.jsm:183:5\nNoSuchAlertError@chrome://remote/content/shared/webdriver/Errors.jsm:384:5\nGeckoDriver.prototype._checkIfAlertIsPresent@chrome://remote/content/marionette/driver.js:2530:11\nGeckoDriver.prototype.dismissDialog@chrome://remote/content/marionette/driver.js:2419:8\ndespatch@chrome://remote/content/marionette/server.js:306:40\nexecute@chrome://remote/content/marionette/server.js:279:16\nonPacket/<@chrome://remote/content/marionette/server.js:252:20\nonPacket@chrome://remote/content/marionette/server.js:253:9\n_onJSONObjectReady/<@chrome://remote/content/marionette/transport.js:500:20\n"},null]

• This seems to happen because the window is switched before and this command triggers searching for open dialogs in the current window. And as it looks like the new code in Marionette is not able to find the open prompt which results in the prompt being left open. To reproduce that just run test_get_prompt_text and add the following lines to the end of the test:

    session.window_handle = session.window_handle
    session.alert.dismiss()
    with pytest.raises(NoSuchAlertException):
        session.alert.text

With that fixed a couple of other tests will then pass as well.

Comment 16

[Olivia Hall [:olivia]]

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

It's great to see that we now can run the user prompt tests and don't end-up in timeouts anymore because the events for opening and closing user prompts exist. As Olivia stated several tests are still failing. So I did a quick check and can see that dialogs aren't always closed. As such here some steps that might help to identify the underlying issue:

• Concentrate on the test testing/web-platform/tests/webdriver/tests/get_alert_text/get.py for now and only run the last two tests in that file (rename the others from i.e. test_ to tst_) because here we do not have unsupported features for GeckoView.

• Here the first test opens a prompt with the text Enter Your Name. That text is correctly detected but then in test teardown the following error is visible when trying to close the dialog:

03-10 09:46:36.563  8072  8098 I Gecko   : 1646901996563	Marionette	DEBUG	0 -> [0,11,"WebDriver:DismissAlert",{}]
03-10 09:46:36.564  8072  8098 I Gecko   : 1646901996564	Marionette	DEBUG	0 <- [1,11,{"error":"no such alert","message":"","stacktrace":"WebDriverError@chrome://remote/content/shared/webdriver/Errors.jsm:183:5\nNoSuchAlertError@chrome://remote/content/shared/webdriver/Errors.jsm:384:5\nGeckoDriver.prototype._checkIfAlertIsPresent@chrome://remote/content/marionette/driver.js:2530:11\nGeckoDriver.prototype.dismissDialog@chrome://remote/content/marionette/driver.js:2419:8\ndespatch@chrome://remote/content/marionette/server.js:306:40\nexecute@chrome://remote/content/marionette/server.js:279:16\nonPacket/<@chrome://remote/content/marionette/server.js:252:20\nonPacket@chrome://remote/content/marionette/server.js:253:9\n_onJSONObjectReady/<@chrome://remote/content/marionette/transport.js:500:20\n"},null]

• This seems to happen because the window is switched before and this command triggers searching for open dialogs in the current window. And as it looks like the new code in Marionette is not able to find the open prompt which results in the prompt being left open. To reproduce that just run test_get_prompt_text and add the following lines to the end of the test:

    session.window_handle = session.window_handle
    session.alert.dismiss()
    with pytest.raises(NoSuchAlertException):
        session.alert.text

With that fixed a couple of other tests will then pass as well.

Thanks for the tips and help! The user_prompts.py tests are passing locally for me now.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/460b9abdbec3
GeckoView Prompts Adjustment for Marionette r=geckoview-reviewers,agi

Comment 18

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eee8f4778c79
Backed out 2 changesets (bug 1712414, bug 1708105) for causing wpt failures CLOSED TREE

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/75e28a1b288a
GeckoView Prompts Adjustment for Marionette r=agi

Comment 20

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/75e28a1b288a