1731795 - Error when sending text to an alert with checkbox is not correct. Throw unhandled exception instead of ElementNotInteractableError

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[David Burns :automatedtester]

From Firefox ~92 we get the following error message when sending text to an alert modal

selenium.common.exceptions.WebDriverException: Message: User prompt of type alertCheck is not supported
248
E       Stacktrace:
249
E       WebDriverError@chrome://remote/content/shared/webdriver/Errors.jsm:181:5
250
E       UnsupportedOperationError@chrome://remote/content/shared/webdriver/Errors.jsm:499:5
251
E       GeckoDriver.prototype.sendKeysToDialog@chrome://remote/content/marionette/driver.js:2512:13

This can be seen in https://github.com/SeleniumHQ/selenium/runs/3662546941?check_suite_focus=true

A minimal test case can be found in https://github.com/web-platform-tests/wpt/pull/30897

Comment 1

[Julian Descottes [:jdescottes]]

Thanks for sharing the test case.

I am surprised that you expect a InvalidElementStateException in this test. According to the specs (and marionette's implementation) send alert text should throw a ElementNotInteractableError when used for alert or confirm dialogs (cf https://www.w3.org/TR/webdriver/#send-alert-text )

So regardless of the alert/alertCheck mismatch I don't really understand how this could work before. This code has not changed for quite a long time.

Comment 2

[Julian Descottes [:jdescottes]]

After reproducing the issue with the Selenium test suite, we get this error because the test triggers the dialog abuse behavior from Firefox. Instead of showing a regular alert, we show an alert with a checkbox, which has the promptType==alertCheck. We should probably handle those similarly to promptType==alert, meaning we should throw a ElementNotInteractableError.

According to https://searchfox.org/mozilla-central/source/toolkit/components/prompts/src/CommonDialog.jsm, we can have the following promptType values: alert, alertCheck, confirm, confirmCheck, confirmEx, prompt, promptUserAndPass, promptPassword. At the moment, we only handle alert, confirm and prompt.

Note that fixing this will most likely not fix the root issue with Selenium, since the test expects an InvalidElementStateException here. I have still no idea how earlier FF versions could pass this test.

Slightly repurposing into something that is quickly actionable while we investigate the rest of the problem.

Comment 3

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

Sounds good to me. I would suggest that we only add alertCheck and confirmCheck for now. The others don't seem to be related to the situation when a user prompt gets opened too frequently, and might be internal (not web platform) features.

Comment 4

[Julian Descottes [:jdescottes]]

I suspect promptPassword & promptUserAndPass to be triggered for authentication prompts (eg with a .htaccess).
confirmEx seems to be used in situations such as the "confirm repost" (navigation that will trigger post data to be sent again).

So they are not exactly web platform features, but could still be encountered in legitimate use cases by testers?

In any case I'll start with alertCheck and confirmCheck. I think the preferences at https://searchfox.org/mozilla-central/source/testing/profiles/unittest-required/user.js prevent us from triggering them in our tests at the moment.

Comment 5

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

(In reply to Julian Descottes [:jdescottes] from comment #4)

So they are not exactly web platform features, but could still be encountered in legitimate use cases by testers?

The WebDriver spec doesn't have support for HTTP authentication dialogs. So lets not worry about that.

In any case I'll start with alertCheck and confirmCheck. I think the preferences at https://searchfox.org/mozilla-central/source/testing/profiles/unittest-required/user.js prevent us from triggering them in our tests at the moment.

Oh, that could be the reason. I was worried when we moved wdspec to using this profile setup technique when James implemented it. And as it looks like this is one case of the story where a feature is blocked that we do not detect with wdspec jobs even with tests present.

James, I assume that also the upstream wpt jobs make use of these preferences? I wonder if we should get closer to these prefs that we actually release geckodriver with.

Comment 6

[Julian Descottes [:jdescottes]]

For what it's worth, simply handling "alertCheck" in the same way as "alert" fixes the selenium suite. Will now try to add a test for repeated alerts, although I don't know if it should be a webplatform test since the dialog abuse behavior is probably browser-specific?

Still need to understand why we get a InvalidElementStateException here though.

Comment 7

[Julian Descottes [:jdescottes]]

Also as discussed on Element, the issue only appeared with Firefox 92 because before the dialog abuse detection used to be reset after every reload.

Since the selenium test test_alerts.py reloads the page before triggering any alert, the dialog abuse never kicked in with FF91 or older. Bug 1732053 was filed to check if the behavior change was expected or not.

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: Bug 1731795 - [marionette] Disable dialog abuse time limit with RecommendedPreferences

Comment 9

[James Graham [:jgraham]]

James, I assume that also the upstream wpt jobs make use of these preferences? I wonder if we should get closer to these prefs that we actually release geckodriver with.

As discussed, in this case we actually want to ship geckodriver with the same pref we use for our own CI. In general if there are prefs we're unconditionally setting for testing Gecko, we should probably at least consider setting them for geckodriver. It might not always be appropriate (if it's really just setting something specific to testing gecko itself), but there's a lot of overlap between the prefs we want to set to make our CI reliable and the prefs that developers should be setting to make their CI reliable.

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: Bug 1731795 - [wdspec] Add wdspec test for send alert text with successive alerts

Depends on D126356

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/507b41eddff3
[marionette] Disable dialog abuse time limit with RecommendedPreferences r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/eeb72c58bef2
[wdspec] Add wdspec test for send alert text with successive alerts r=webdriver-reviewers,jgraham,whimboo

Comment 12

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/507b41eddff3
https://hg.mozilla.org/mozilla-central/rev/eeb72c58bef2

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

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

Comment 14

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

(In reply to Web Platform Test Sync Bot (Matrix: #interop:mozilla.org) from comment #13)

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

I've merged upstream given that it was stuck due to some reason.