1685291 - sendKeys to context menu in chrome context gives: Element is not reachable by keyboard

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P3)

Description

[Emond Papegaaij]

FF 84, very likely bug 1365886, broke our selenium tests. We used this code (Java) to open the context menu, click on an item and close the menu again:

new Actions(driver).contextClick(element).build().perform();
driver.setContext("chrome");
driver.findElement(By.id("keyhub_topicus_nl-menuitem-_context-fill")).click();
driver.findElement(By.id("keyhub_topicus_nl-menuitem-_context-fill")).sendKeys(Keys.ESCAPE);
driver.setContext("content");

This worked on FF 83, but on 84 it gives:

Element <menuitem id="keyhub_topicus_nl-menuitem-_context-fill" class="menuitem-iconic"> is not reachable by keyboard

We had to change the sendKeys to the following to fix this:

new Actions(driver).sendKeys(Keys.ESCAPE).build().perform();

Comment 1

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

Thanks for filing this bug, and letting us know about the problem in chrome scope.

It would be good to get some trace logs for those steps. Here in this document you can find how to create these:
https://firefox-source-docs.mozilla.org/testing/geckodriver/TraceLogs.html

In general I don't think it's bug 1365886 that broke it given that the code above doesn't use actions at all on chrome scope. Both click() and sendKeys() aren't using action primitives yet. So if this is a regression some other landing has introduced it. Maybe the trace log helps to me figure that out. Otherwise it would be good to run a regression test. I can give the necessary details once it's clear that it needs to be done.

Comment 2

[Emond Papegaaij]

Attachment: sendKeys-error.log

Comment 3

[Emond Papegaaij]

Attachment: sendKeys-ok.log

Comment 4

[Emond Papegaaij]

Attachment: Test006UIBrowserExtension-2021-01-06T15:29:06.554Z-keyhub-firefox_ec8cb9f1-92a0-4b74-93d2-c18d4487e15a.log

Comment 5

[Emond Papegaaij]

I've attached trace output of the webdriver. sendKeys-error.log is the call that is failing, which uses driver.findElement(...).sendKeys(Keys.ESCAPE). The sendKeys-ok.log is the output after my change. I've also attached the entire log for the error case. This file is rather large, but it may contain something you need.

Comment 6

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

Thanks for extracting the necessary lines. Sadly nothing stands out, and I cannot see from the logs why it's not working. So here some things I would like to ask you to do:

1. Please attach an excerpt of the log as above when running the test with Firefox 83.0 without the modification of the test file. I would like to compare.

2. Please run again with the Firefox preference marionette.actors.enabled set to false. Does it still fail with Firefox 84?

Comment 7

[Emond Papegaaij]

Attachment: actor-disabled.log

With marionette.actors.enabled, the test runs fine using the original code. I've attached the relevant part of the log for that run. I can send the whole log if you need it. With Firefox 83.0 the log is identical to the one with that option disabled, so I omitted the log. Do you still need it?

Comment 8

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

Ok, so it means that some actor specific code is wrongly handling it. Can you please test again with Firefox 83 and explicitly setting marionette.actors.enabled to true? Most likely the problem was introduced already in an earlier Firefox release, but manifests now because in Firefox 84 we flipped this pref to true by default. If it also fails please check with 81 too.

Comment 9

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

Oh, and what kind of element is it the code is operating on in this specific case? A checkbox menu item? Do you experience issues with sendKeys also for other elements in chrome scope?

Comment 10

[Emond Papegaaij]

I can confirm that the test also fails with Firefox 83 with the setting enabled. Unfortunately, I cannot test with Firefox 81, because I don't have the docker image for that version anymore. It will require a significant amount of work to rebuild it.

The menu item I'm operating on is this, a simple context menu item from our browser extension:

  <menuitem accesskey="" label="Fill with Topicus KeyHub" id="keyhub_topicus_nl-menuitem-_context-fill" class="menuitem-iconic" image="moz-extension://ca734622-0e6c-43bf-8791-ee9698992a9a/icon/keyhub-icon-32.png">
    <hbox class="menu-iconic-left" align="center" pack="center" aria-hidden="true">
      <image class="menu-iconic-icon" src="moz-extension://ca734622-0e6c-43bf-8791-ee9698992a9a/icon/keyhub-icon-32.png"/>
    </hbox>
    <label class="menu-iconic-text" flex="1" crop="right" aria-hidden="true" value="Fill with Topicus KeyHub" accesskey=""/>
    <label class="menu-iconic-highlightable-text" crop="right" aria-hidden="true" accesskey="">Fill with Topicus KeyHub</label>
    <hbox class="menu-accel-container" aria-hidden="true">
      <label class="menu-iconic-accel"/>
    </hbox>
  </menuitem>

I've also tested with sending ESC to the context menu itself (By.id("contentAreaContextMenu")). This shows the same regression. It works in when the setting is disabled.

The purpose of the ESC is to close the context menu. Somehow, the menu stays open after clicking on the item when this is done via the chrome context. Normally, when a user clicks the item, the menu closes automatically. So in a sense this sendKeys is a workaround for a mismatch in behavior between the chrome context and normal user interaction. However, sending ARROW_DOWN to the context menu before clicking the item also fails.

Comment 11

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

(In reply to Emond Papegaaij from comment #10)

I can confirm that the test also fails with Firefox 83 with the setting enabled. Unfortunately, I cannot test with Firefox 81, because I don't have the docker image for that version anymore. It will require a significant amount of work to rebuild it.

Did you maybe mean Firefox 82 here? If not and the docker image is still available it would be great to also test this release.

The menu item I'm operating on is this, a simple context menu item from our browser extension:

  <menuitem accesskey="" label="Fill with Topicus KeyHub" id="keyhub_topicus_nl-menuitem-_context-fill" class="menuitem-iconic" image="moz-extension://ca734622-0e6c-43bf-8791-ee9698992a9a/icon/keyhub-icon-32.png">
    <hbox class="menu-iconic-left" align="center" pack="center" aria-hidden="true">
      <image class="menu-iconic-icon" src="moz-extension://ca734622-0e6c-43bf-8791-ee9698992a9a/icon/keyhub-icon-32.png"/>
    </hbox>
    <label class="menu-iconic-text" flex="1" crop="right" aria-hidden="true" value="Fill with Topicus KeyHub" accesskey=""/>
    <label class="menu-iconic-highlightable-text" crop="right" aria-hidden="true" accesskey="">Fill with Topicus KeyHub</label>
    <hbox class="menu-accel-container" aria-hidden="true">
      <label class="menu-iconic-accel"/>
    </hbox>
  </menuitem>

Is that extension publicly available? Could I somehow run the tests on my local machine?

I've also tested with sending ESC to the context menu itself (By.id("contentAreaContextMenu")). This shows the same regression. It works in when the setting is disabled.

The purpose of the ESC is to close the context menu. Somehow, the menu stays open after clicking on the item when this is done via the chrome context. Normally, when a user clicks the item, the menu closes automatically. So in a sense this sendKeys is a workaround for a mismatch in behavior between the chrome context and normal user interaction. However, sending ARROW_DOWN to the context menu before clicking the item also fails.

Ok, we could create a XUL testcase for this purpose.

Comment 12

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

I could imagine that bug 1662473 might have regressed it for the actor (site isolation) implementation.

Comment 13

[Emond Papegaaij]

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

(In reply to Emond Papegaaij from comment #10)

I can confirm that the test also fails with Firefox 83 with the setting enabled. Unfortunately, I cannot test with Firefox 81, because I don't have the docker image for that version anymore. It will require a significant amount of work to rebuild it.

Did you maybe mean Firefox 82 here? If not and the docker image is still available it would be great to also test this release.

You said 81, but I don't have the image for 82 either. We clean up our images on a daily basis. I was lucky I could even find a machine which still had the Firefox 83 image. To test with other versions, I will have to build these images from scratch, because the upstream Dockerfiles simply install the latest version.

Is that extension publicly available? Could I somehow run the tests on my local machine?
The extension is available on our website https://topicus-keyhub.com/browser-extensions/ . However, you will need our application as well. You can download it as well, but that would require quite some effort to get up and running. Unfortunately, I cannot publish the tests, as they are tightly integrated into our build and our source code.

The purpose of the ESC is to close the context menu. Somehow, the menu stays open after clicking on the item when this is done via the chrome context. Normally, when a user clicks the item, the menu closes automatically. So in a sense this sendKeys is a workaround for a mismatch in behavior between the chrome context and normal user interaction. However, sending ARROW_DOWN to the context menu before clicking the item also fails.

Ok, we could create a XUL testcase for this purpose.

Given the fact that the interaction via the chrome context is very minimal. I expect it should be fairly easy to reproduce in a separate testcase, and you probably also don't need our browser extension. Simply opening the context menu and sending ARROW_DOWN should be enough.

Comment 14

[Julian Descottes [:jdescottes]]

Still investigating, but we can reproduce the regression locally by modifying the test_mouse_action.py test.

The subtest test_context_click_action is very similar to the STRs. It opens the context menu and then closes it using send_keys(Keys.ESCAPE). However the difference is that it sends the key to the "main-window" element, and not to the context menu.

If we update the test with the patch below, it will fail with --setpref marionette.actors.enabled=true and pass with --setpref marionette.actors.enabled=false.

diff --git a/testing/marionette/harness/marionette_harness/tests/unit/test_mouse_action.py b/testing/marionette/harness/marionette_harness/tests/unit/test_mouse_action.py
--- a/testing/marionette/harness/marionette_harness/tests/unit/test_mouse_action.py
+++ b/testing/marionette/harness/marionette_harness/tests/unit/test_mouse_action.py
@@ -107,17 +107,17 @@ class TestPointerActions(BaseMouseAction
 
         self.assertEqual("closed", context_menu_state())
         self.mouse_chain.click(element=click_el, button=2).perform()
         Wait(self.marionette).until(
             lambda _: context_menu_state() == "open",
             message="Context menu did not open",
         )
         with self.marionette.using_context("chrome"):
-            self.marionette.find_element(By.ID, "main-window").send_keys(Keys.ESCAPE)
+            self.marionette.find_element(By.ID, "contentAreaContextMenu").send_keys(Keys.ESCAPE)
         Wait(self.marionette).until(
             lambda _: context_menu_state() == "closed",
             message="Context menu did not close",
         )
 
     def test_middle_click_action(self):
         test_html = self.marionette.absolute_url("clicks.html")
         self.marionette.navigate(test_html)

Comment 15

[Julian Descottes [:jdescottes]]

interaction::sendKeysToElement relies on two different implementations:

• webdriverSendKeysToElement
• legacySendKeysToElement

webdriverSendKeysToElement will check for "keyboard interactability". legacySendKeysToElement doesn't.
When using actors, webdriverSendKeysToElement is used for both chrome and content contexts.
When NOT using actors, webdriverSendKeysToElement is only used for content context.

That's why we see a regression here. Looking more closely at sendKeysToElement, it uses webdriverSendKeysToElement if the "moz:webdriverClick" capability was passed. We could avoid passing the whole capabilities when we are in chrome context? I will propose a patch to get a discussion started and we can move from there.

Comment 16

[Julian Descottes [:jdescottes]]

Attachment: Bug 1685291 - [marionette] Special case menupopup elements when checking keyboard interactability

Comment 17

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

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

That's why we see a regression here. Looking more closely at sendKeysToElement, it uses webdriverSendKeysToElement if the "moz:webdriverClick" capability was passed. We could avoid passing the whole capabilities when we are in chrome context? I will propose a patch to get a discussion started and we can move from there.

Interesting that we didn't pass the flag before to the command implementation when chrome scope was active. Note that the webdriver click is the way to go here. The old methods are only around for fallback cases especially in content scope. So we certainly should focus on webdriver click in the future. I'm ok, with having parity with the old framescript code for now, but at the end the interactability checks would also have to be done for chrome scope.

That means we should check why it's detected as not interactable. Reason could be that the context menu is not part of the windows's pointer-interactable paint tree. AFAIR it's attached directly on the ChromeWindow. If that's the case we won't be to get this working at all for context menus, unless we add an exception for this menu in interaction.isKeyboardInteractable(), and always return true.

Comment 18

[Julian Descottes [:jdescottes]]

Attachment: Bug 1685291 - [marionette] Add test for sendKeys with context menus

Depends on D101046

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b1a60f3191ed
[marionette] Special case menupopup elements when checking keyboard interactability r=marionette-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/b5be3173d861
[marionette] Add test for sendKeys with context menus r=marionette-reviewers,whimboo

Comment 20

[Narcis Beleuzu [:NarcisB]]

Backed out for bc failures on browser_all_files_referenced.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/0331466328fe4fc71b437fbb3553c0aa6bf21369
Log link: https://treeherder.mozilla.org/logviewer?job_id=326752451&repo=autoland&lineNumber=3041

Comment 21

[Julian Descottes [:jdescottes]]

Oh looks like we need to add our XUL test files to the exclusion list for browser_all_files_referenced.js

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b65e84add19
[marionette] Special case menupopup elements when checking keyboard interactability r=marionette-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/0027c6bd1e8c
[marionette] Add test for sendKeys with context menus r=marionette-reviewers,whimboo

Comment 23

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/4b65e84add19
https://hg.mozilla.org/mozilla-central/rev/0027c6bd1e8c

Comment 24

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

The patch landed in nightly and beta is affected.
:jdescottes, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.