1776190 - Modifier keys are no longer reset when getting sent multiple times for "WebDriver:ElementSendKeys"

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

[michael.ketting]

Steps to reproduce:

Use OpenQA.Selenium.IWebElement.GetAttribute("value") to retrieve the value of an input element.

Used gecko driver version: 0.30.
Please note, that this is NOT a regression in gecko but in Firefox. This call worked in Firefox v97 and works in Chrome and Edge. To me, this implies a regrssion in Firefox v98.

The issue can be reproduced in Firefox v99, v100, and the current v101 as well.

This defect blocks automated web testing with Firefox.

Actual results:

The web driver call returns an empty string.

Expected results:

The current value of the input element is returned.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Toolkit::Startup and Profile System' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[michael.ketting]

Correction: the first affected version is Firefox v99. The issue can be reproduced both with the older geckodriver v0.30 (used with Firefox v98) and geckodriver v0.31 (normally used with Firefox v99). This still implies that geckodriver is not at fault.

Comment 3

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

Would you mind attaching a trace log of your problem? Also a small complete test case would be great. We have automated tests which didn't show any problem yet so it would be good to know what exactly is causing it for you. Thanks.

Comment 4

[michael.ketting]

Hi Henrik,
will do. Unfortunately, it turns out that it's caused by something in the Selenium/Coypu/project-specific-workarounds stack. Doesn't change the fact that the only changed variable is the Firefox version, but getting to the most minimal repro sample just become more...involved.

Comment 5

[Robert Longson [:longsonr]]

Perhaps https://mozilla.github.io/mozregression/ can help then? It should allow us to see exactly what firefox change it really is. You can just add the regression range to the bug.

Comment 6

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

No reply from the reporter for more than 2 weeks. I'm going to close this bug as incomplete but I'm happy to reopen in case a trace log can be provided which shows this issue.

Comment 7

[michael.ketting]

Henrik,

we've now been able to reproduce this in an isolated scenario (and also found a workaround):
https://github.com/Frazerus/FirefoxSendkeysProblem

Tracelog:
https://github.com/Frazerus/FirefoxSendkeysProblem/blob/main/TraceLog.txt

In short, the problem appears when you use SendKeys to clear the input field and in the same roundtrip, send the new data, too. Once you split this into separate operations, the bug disappears. This happens first with Firefox v99.

Comment 8

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

Thank you for the update! So basically I can see bug 1757636 and bug 1686666 landed for this Firefox release, which might have triggered that. Could I ask you to run the following Firefox Nightly builds to check when the regression actually happened?

Bug 1757636

Without: https://archive.mozilla.org/pub/firefox/nightly/2022/03/2022-03-01-21-52-24-mozilla-central/
With: https://archive.mozilla.org/pub/firefox/nightly/2022/03/2022-03-02-21-25-01-mozilla-central/

Bug 1686666

Without: https://archive.mozilla.org/pub/firefox/nightly/2022/02/2022-02-25-10-47-05-mozilla-central/
With: https://archive.mozilla.org/pub/firefox/nightly/2022/02/2022-02-26-09-46-10-mozilla-central/

Does it regress between those two possible landings?

Comment 9

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

I'm actually able to reproduce it now with the following Marionette test:

        self.marionette.navigate(inline("""<input type="text" value="foo">"""))
        elem = self.marionette.find_element(By.TAG_NAME, "input")
        elem.send_keys(Keys.META + 'a' + Keys.META + Keys.DELETE + "cheese")
        self.assertEqual(elem.get_property("value"), "cheese")

I'll run mozregression to find the exact range.

Comment 10

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

The regression range is: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c875dbd49223&tochange=f123578cbe2e

Bug 1686666 is clearly in range here so I would suspect that it triggered this regression. James can you please have a look?

Comment 11

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

I had a quick look and the problem seems to be actually in this line:
https://searchfox.org/mozilla-central/rev/2ce39261ea6a69e49d87f76a119494b2a7a7e42a/remote/marionette/event.sys.mjs#265

As it can be seen the modifier key is always set to true even if it is send multiple times. Its state should actually get negated.

Comment 12

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

As Julian pointed out on Matrix we actually should only reset the modifier keys after the full string has been dispatched. As such the current behavior actually makes sense and would be compliant with the WebDriver classic specification.

We do not have specific WebDriver classic tests for this scenario yet, which we should get added.

I'm leaving the regression and needinfo for now for a final feedback from James.

Comment 13

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

Actually there is Keys.NULL which would have to be used to reset the modifier keys. But sadly we do not support this as well yet (see bug 1494661). Per se this is not a regression and as such I'm removing the flags now.

We will discuss in next week's triage meeting.

Comment 14

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

I run the following test as Selenium test with Chrome and it passes while Firefox fails:

    driver.get(inline('<input id="input" type="text" value="foo">'))
    elem = driver.find_element(By.ID, "input")
    elem.send_keys(Keys.META + 'a' + Keys.META + Keys.DELETE + "cheese")
    assert elem.get_property("value") == 'cheese'

Comment 15

[michael.ketting]

Thanks, Henrik, for finding the root cause! Am I correct in my interpretation that you have all the infos you need right now?

Comment 16

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

As discussed on Matrix today we will actually handle that bug as regression and revert the behavior even it will be non Webdriver spec compliant. Reason is because Chrome shows the same behavior.

I filed https://github.com/w3c/webdriver/issues/1734 for a discussion in the W3C group what we actually should do.

Comment 17

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

Attachment: Bug 1776190 - [marionette] Allow a modifier key to be reset when sending it again within the same command.

Comment 18

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

Attachment: Bug 1776190 - [wdspec] Move keys.py module and modifier_key fixture to global supports folder.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf20a16f172f
[wdspec] Move keys.py module and modifier_key fixture to global supports folder. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/baccc7b9491f
[marionette] Allow a modifier key to be reset when sending it again within the same command. r=webdriver-reviewers,jdescottes

Comment 20

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

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

Comment 21

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/cf20a16f172f
https://hg.mozilla.org/mozilla-central/rev/baccc7b9491f

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:whimboo, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox113 to wontfix.

For more information, please visit auto_nag documentation.

Comment 23

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

Just a single report since the release of Firefox 99. We do not see a reason for an uplift. As such lets get this fix ride the trains.

Comment 24

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

Upstream PR merged by jgraham