Closed Bug 1365886 Opened 4 years ago Closed 6 months ago

Add support for Actions in chrome scope

Categories

(Testing :: Marionette, enhancement, P5)

Version 3
enhancement

Tracking

(Fission Milestone:M7, firefox84 fixed)

RESOLVED FIXED
84 Branch
Fission Milestone M7
Tracking Status
firefox84 --- fixed

People

(Reporter: automatedtester, Assigned: impossibus)

References

(Blocks 4 open bugs)

Details

(Whiteboard: [marionette-fission-mvp])

Attachments

(2 files)

It would be good to add the actions support to Chrome eventually.

Original request in https://github.com/mozilla/geckodriver/issues/742
Summary: Add Actions support to Chrome → Add support for Actions in chrome scope
Hello,
I raised the request on GitHub.

I'd like to take a stab at implementing, and have setup the development environment, following the instructions (more or less) at https://wiki.mozilla.org/User:Mjzffr/New_Contributors.

Could I get some mentorship in terms of which  files (or zipcode there of) that I should start looking at to implement?

Thanks,
Chase

chase  at   cwpdev.com
Maja, can you mentor this bug?
Flags: needinfo?(mjzffr)
You're welcome to take a look. I think the main reason we haven't implemented this yet is that we haven't yet thought about how to maintain and possibly share state (seen elements, input state, inputs to cancel) between driver.js and listener.js.

The main entry-point for actions is here: http://searchfox.org/mozilla-central/source/testing/marionette/driver.js#1774-1813 and this is where you should handle the case of chrome scope. The other actions-related functions below are the legacy actions implementation -- ignore them. 

The corresponding content-scope actions code is in listener.js: http://searchfox.org/mozilla-central/source/testing/marionette/listener.js#886-908 and http://searchfox.org/mozilla-central/source/testing/marionette/listener.js#627-631

I think ato might have some opinions regarding how to extend actions to work in chrome scope (it should be an "extension command" I guess) and how it should work.
Flags: needinfo?(mjzffr) → needinfo?(ato)
First of all, I don’t know if there is a pressing need for actions in chrome context: we mostly get by with the high-level Element Click and Element Send Keys commands to interact with XUL elements at the moment.  I can see how having actions available might be useful to write more sophisticated Firefox UI interaction tests, but it’s not clear to me if we have anyone lined up to write such tests?

For keyboard input, the current implementation in testing/marionette/action.js should work fine also for chrome space.  It ends up calling down to testing/marionette/event.js, our synthetic interaction module, which Element Send Keys also uses.  This might be the easiest thing to do first.

For mouse interaction I’m unsure if the same assumptions about the location of the cursor that we make for content context will be valid for chrome.  Specifically we need to handle the case where a chrome context mouse click ends up interacting with web content/the viewport.  Not sure how we should handle this, but maybe an error could be returned.

When the mouse is moved over the browser, we also potentially could end up causing DOM events to be triggered in content context, which isn’t desired but may be a natural consequence.

When it comes to shared state, I don’t think we would have to share the known web element cache between chrome and content, but it’s a bit of an open question whether input state, inputs to cancel, and generally any state associated with the individual pointer sources.

If we do want to share this state, I would suggest standing up a service in chrome context (testing/marionette/driver.js), much like our cookie service (testing/marionette/cookie.js), to keep all this information.  This means our current content implementation of actions needs to change to use this service.
Flags: needinfo?(ato)
OS: Unspecified → All
Hardware: Unspecified → All
Priority: P4 → P5

To get actions to work in chrome scope we would have to fix bug 1669176 first.

Further there seems to be a mix-up of WebElement types:

1601969089139	Marionette	DEBUG	2 -> [0,27,"WebDriver:FindElement",{"using":"id","value":"link"}]
1601969089139	Marionette	TRACE	[36] Parent actor created
1601969089139	Marionette	TRACE	[36] Child actor created for window id 25
1601969089141	Marionette	DEBUG	2 <- [1,27,null,{"value":{"chromeelement-9fc5-4b51-a3c8-01716eedeb04":"0d036a9d-9bee-c04d-a106-06d676b78220"}}]
1601969089142	Marionette	DEBUG	2 -> [0,28,"WebDriver:GetElementProperty",{"id":"0d036a9d-9bee-c04d-a106-06d676b78220","name":"localName"}]
1601969089142	Marionette	DEBUG	2 <- [1,28,null,{"value":"a"}]
1601969089143	Marionette	DEBUG	2 -> [0,29,"WebDriver:PerformActions",{"actions":[{"parameters":{"pointerType":"mouse"},"type":"pointer","id":"pointer_id","actions":[{"y":0,"x":0,"type":"pointerMove","origin":{"element-6066-11e4-a52e-4f735466cecf":"0d036a9d-9bee-c04d-a106-06d676b78220"}},{"button":0,"type":"pointerDown"},{"button":0,"type":"pointerUp"}]}]}]
1601969089144	Marionette	DEBUG	2 <- [1,29,{"error":"invalid argument","message":"Expected 'origin' to be undefined, \"viewport\", \"pointer\", or an element, got: <a id=\"link\" href=\"#\">","stacktrace":"WebDriverError@chrome://marionette/content/error.js:181:5\nInvalidArgumentError@chrome://marionette/content/error.js:310:5\naction.PointerOrigin.get@chrome://marionette/content/action.js:371:11\nfromJSON@chrome://marionette/content/action.js:744:44\nfromJSON@chrome://marionette/content/action.js:868:34\nfromJSON@chrome://marionette/content/action.js:809:48\nperformActions@chrome://marionette/content/actors/MarionetteFrameChild.jsm:392:20\nreceiveMessage@chrome://marionette/content/actors/MarionetteFrameChild.jsm:142:31\n"},null]

Somehow the ChromeWebElement reference is turned into a WebElement reference for WebDriver:PerformActions. Note that other commands don't explicitly use the ChromeWebElement reference but just its value. As it looks like this should be a problem with the Marionette Python client.

Maja, as I can remember right you wanted to check the actions vs legacy actions bit. Maybe it's something you are interested in to investigate? We should figure out if it makes sense for us to add this support now, or simply convert legacy actions to JSWindowActors for now. From my point of view the latter might make more sense even that we know we actually don't want to continue support for these.

Depends on: 1669176
Flags: needinfo?(mjzffr)

Here the simple testcase that I was using for testing actions in chrome scope. It's based on test_mouse_action.py.

I've started looking at this and I've figured out with the Python client. Not done yet, but I think it might not be too hard to get actor-based actions working in chrome scope.

That being said, the tests that use legacy actions use tap/touch, and we don't have that implemented in the spec-compliant actions yet, so it might make sense to port legacy actions over to JSWindowActors regardless if it's not much work.

More on this soon.

Yay, I have spec-compliant actions working in chrome scope with JSWindowActors (in a basic test). So next, I'll check if I can get AWSY tests working -- that is the only thing that depends on chrome-scope actions.

While I'm here, here's a breakdown of legacy actions usage:

  • actionChain, used in:
    • testing/awsy/awsy/awsy_test_case.py depends on this in chrome scope
    • layout/base/tests/marionette/test_accessiblecaret_cursor_mode.py (content only)
    • layout/base/tests/marionette/test_accessiblecaret_selection_mode.py (content only)
    • Marionette unit tests like testing/marionette/harness/marionette_harness/tests/unit/test_key_actions.py
  • multiAction
    • Nothing is using this except Marionette Python client (gestures)
  • singleTap - this is content scope only, so not relevant to this bug. Tests that depend on tap are:
    • layout/base/tests/marionette/test_accessiblecaret_cursor_mode.py
    • layout/base/tests/marionette/test_accessiblecaret_selection_mode.py
    • testing/marionette/harness/marionette_harness/tests/unit/test_accessibility.py

singleTap is pretty entwined with the rest of legacy actions, so I don't think we can drop legacy actions until we add touch support to spec-compliant actions.

Thanks for this analysis Maja! And it's great to hear that you got actions working in chrome scope with the actors!

Something which would be good to know is what should happen with actionChain and multiAction? If we can convert those usages to the spec compliant actions we would only have to keep the singleTap from legacy actions. That would be at least a good step forward.

So as it sounds lets convert singleTap to use the actor implementation, and get the others removed / replaced with webdriver actions - in the hope this all will work smoothly.

I commented on Bug 1534582 regarding AWSY.

My conclusion here is that we'll get chrome support for actions "for free" once Bug 1669169 is resolved.

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

Thanks for this analysis Maja! And it's great to hear that you got actions working in chrome scope with the actors!

Something which would be good to know is what should happen with actionChain and multiAction? If we can convert those usages to the spec compliant actions we would only have to keep the singleTap from legacy actions. That would be at least a good step forward.

Yeah, we should be able to convert actionChain usages to performActions. multiAction doesn't really matter because nothing is using it as far as I know, but performActions should be able to cover that, too.

So as it sounds lets convert singleTap to use the actor implementation, and get the others removed / replaced with webdriver actions - in the hope this all will work smoothly.

That's worth a shot. Filed Bug 1670295

Depends on: 1669169
Flags: needinfo?(mjzffr)

(In reply to Maja Frydrychowicz :maja_zf (UTC-4) (maja@mozilla.com) from comment #10)

My conclusion here is that we'll get chrome support for actions "for free" once Bug 1669169 is resolved.

That and we'll need some minor modifications to actions.js and the Marionette Python client.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e13224bc905ec5d6640ba039246bc978c313c7f4

Assignee: nobody → mjzffr
Status: NEW → ASSIGNED

Tracking this as part of Marionette Fission work since it blocks Bug 1534582

Whiteboard: [marionette-fission-mvp]

Tracking marionette-fission-mvp bugs for Fission Beta milestone (M7).

Fission Milestone: --- → M7
Pushed by mjzffr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3a4b62af9597
[marionette] Allow performActions to operate on chrome elements r=marionette-reviewers,jdescottes,whimboo
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → 84 Branch

This change 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();

Emond, can you please file this as a new bug? And please provide a trace log. Thanks.

Flags: needinfo?(emond.papegaaij)

I've filed bug 1685291. Can you explain what kind of trace log you are looking for and how to get that? The logging of our Firefox docker container selenium/standalone-firefox-debug gives no information on this. The stacktrace of our test is also not very interesting as it's simply a call to WebElement.sendKeys going into the command execution of the RemoteWebDriver and returning with an error.

Flags: needinfo?(emond.papegaaij)

I would suggest that we follow-up on the other bug.

You need to log in before you can comment on or make changes to this bug.