1879324 - Support "beforeunload" prompts and related open / close events on Android

Status: NEW

(GeckoView :: General, enhancement, P2)

Description

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

Over on bug 1693857 we are improving our support for the beforeunload prompt. While it works fine for desktop we currently are not able to run our webdriver tests with the testrunner.

Here the reply from Olivia as copied from that other bug:

(In reply to Olivia Hall [:olivia] from comment #26)

Hi Henrik ,

Thanks for looking into GeckoView too!

Maybe that's indeed an issue with the testrunner?

Just taking a quick look, it does look like onBeforeUnloadPrompt is missing in the TestRunnerActivity. Probably just needs to be similar to the other test prompt implementations to make sure it is intercepted as well.

Depending on the needs, we might also need to add more in GeckoViewPrompterParent to add any info or add the prompt to the window. I'm guessing it conforms to one of these types and needs intercepted over here too possibly to make everything aware of it.

• onBeforeUnloadPrompt Stub
• onBeforeUnloadPrompt missing in TestRunnerActivity
• GeckoView example implementation for onBeforeUnloadPrompt
• Android Components implementation for onBeforeUnloadPrompt - Fenix would use this one

Comment 1

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

Actually this doesn't block landing the Marionette changes given that for now we automatically close this dialog and that's the only behavior the tests are checking. But our implementation for WebDriver BiDi would be blocked because when using this protocol we explicitly want to handle this type of prompt.

Comment 2

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

Sorry I was wrong. Something went wrong with a merge conflict and the removal of dom.disable_beforeunload=true was not done in the wpt's user.js file.

Actually removing this line will indeed cause issues with wpt tests that trigger the "beforeunload" prompt. I may temporarily keep the preference set for Android only until this bug is fixed.

Comment 3

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

Support for this prompt now landed for Marionette on desktop via bug 1693857. To not break wdspec tests on Android and to keep passing the tests in the upstream wpt repository and wpt.fyi I've disabled the prompt for the TestRunner only in wptrunner:

https://hg.mozilla.org/integration/autoland/diff/d01d1d933b227b26fd4911912e57a1e069034386/testing/web-platform/tests/tools/wptrunner/wptrunner/browsers/firefox_android.py

By removing these lines you can verify that the implementation as wanted on this bug does actually work with wpt tests. Some example tests to run can be found in https://hg.mozilla.org/mozilla-central/rev/56277fbbcdf8.

Comment 4

[Mathew Hodson]

Bug 1693857 landed so this doesn't block it anymore.

Comment 5

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

While the prompt support is there for other packages than TestRunner, we still miss the related events, because the beforeUnload prompt type is not registered. Because of .isDialog() filters out this type of prompt no geckoview-prompt-show notification is send out.

Some comments / questions:

1. Can we use beforeunload as prompt type as everywhere else in JS, or why do we have camel-case here?
2. Would it be possible to send the prompt instance directly via the geckoview-prompt-show notification so that we wouldn't have to first search for the prompt?
3. It looks like the closed prompts never get unregistered and pile-up in the list of available prompts.
4. Somehow we have to use .wrappedJSObject to access the prompt id as submitted via subject for the geckoview-prompt-show notification. We don't have that for other notifications so it's not clear to me why it is needed.

Comment 6

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

I've filed also bug 1902264 for our implementation in the Remote Protocol, which is partially broken at the moment.