1354578 - [meta] Replace legacy actions with WebDriver action commands in Marionette client

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement, P3)

Description

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

The new spec-compliant actions endpoints, performActions and releaseActions (Bug 1292178), currently support action chains with key, mouse and pause. It's best for clients to be using these new endpoints. Moreover, we believe only that only internal Marionette tests are using the old actions endpoints (actionChain, multiAction, singleTap), so let's remove them.

Comment 1

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

This changeset shows what else needs to be removed to get a green try push with no references to legacyaction.js: 
https://hg.mozilla.org/try/rev/798a8bcd042097dec9c059f6155c10e5163f1cf6

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2e3330225eeefffb7339d3e4aabce5a280516bf

Conclusion: a few tests under dom/layout rely on actions, as well as a few Marionette unit tests for accessibility and switching window content. We should be able to write modify these tests to use the new actions implementation.

Comment 2

[u602518]

I will start working on this bug. Right now I'm reading the Marionette documentation, since I'm fairly new around here and not entirely familiar with it. If anyone has tips on how to get started, I will surely appreciate it!

Comment 3

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

Hi Vinicius. As discussed via email it's great to see that you have interest to start on this bug. To get familiar with Marionette make sure to read everything on the following page:

https://firefox-source-docs.mozilla.org/testing/marionette/marionette/NewContributors.html

Then as first step you will have to identify which of the existent Marionette tests in tree make use of the legacy actions commands. That way we will see how much work it will be, and if we better have to split-up this bug into smaller portions of work.

Comment 4

[Andreas Tolfsen ❲:ato❳]

You might want to also see
https://firefox-source-docs.mozilla.org/testing/marionette/marionette/Contributing.html
which is more thorough.

Comment 5

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

(In reply to Andreas Tolfsen 「:ato」 from comment #4)
> You might want to also see
> https://firefox-source-docs.mozilla.org/testing/marionette/marionette/
> Contributing.html
> which is more thorough.

Which is part of the new contributor docs. :)

Comment 6

[u602518]

Thank you both for the tips. Looks like these files are still using the legacy commands: https://searchfox.org/mozilla-central/search?q=import+Actions&path=

Comment 7

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

As discussed with Vnicius on IRC he will get started to implement the new action commands in action.py so that we can move out all Actions code into a separate module. By doing that he will start with the Null action which is easiest to implement, and while doing that also creating new testcases in test_actions.py. Once done he will follow-up with key and mouse actions.

Vinicius, please let me know if you have further questions or if you are stuck with something.

Comment 8

[u602518]

Henrik, I'm reading the template class you've mentioned here: 

https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/webdriver/webdriver/client.py#208) 

I've noticed this class receives a 'Session' object as parameter, and its methods use 'send_session_command', which is defined in:

https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/webdriver/webdriver/client.py#492

Ok, so when the perform() method is called in there, it receives an actions list as a parameter, which are added as values to the "actions" keys in the body dictionary. This is used to call the 'send_session_command' method, which in turn forms an url with the parameters received.

Where is this url being used? I imagine it's a way of communicating with the mentioned actions endpoints, or am I mislead?

Comment 9

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

So you don't have to go that in-depth with the wdclient given that it makes use of an extra layer, what Marionette client doesn't have to. When you look at everything above `send_session_command()` that will be fine. Note that when you check the Marionette unit tests, and Marionette specifically there is a `_send_message()` command, which will also receive the action details via its body argument.

Maybe lets do the following:

1) Create a new legacy.py module for Marionette client, and move all the existent action code in there. Make sure to get all the above mentioned tests working, and if rename test_key_actions.py to test_legacy_key_action.py. 

This will make sure that consumers of Marionette client won't have to immediately change their code but only have to update the class names, and we can keep backward-compatibility for a couple of Firefox releases.

2) Create a new actions.py module for Marionette client for the new WebDriver conforming implementation. While doing so create new tests in test_mouse_action.py.

I think step 1 should be kinda straight forward, and you should be able to get this done yourself. Once done we can get started with step 2, and I will definitely help you with that.

Comment 10

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

Hi Vinicius, it's been a while since I heard from you. I would like to know if you are still interested to work on this bug, and if you need help to continue. Please let me know. Thanks.

Comment 11

[u602518]

Hi Henrik. Sorry for being away for so much time. I've had some exams during the last two weeks and I've been unable to work on the bug during this period. But I've started looking into it again yesterday. I'm still interested in working on the bug.

Just one quick question: where should be legacy.py located? I will try to have that working by tomorrow, so I can contact you via IRC to work on step 2.

Comment 12

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

Hey, that is great to hear! I'm looking forward to continue mentoring you on that bug.

Just place it right next to the marionette.py module in the driver directory. Even I mentioned above that we want to keep that file for allowing consumers to change we might want to remove it before landing the patch. We will have to see how much code is changing and if it is worth keeping this ballast, especially if there no big use anymore.

Comment 13

[u602518]

I've had some problems with my local build of Firefox. Hopefully it will be fixed by tomorrow, and I will be able to test my local changes.

Comment 14

[u602518]

I've fixed my build and moved the Actions class (and the necessary MouseButton class) to a separate legacy.py file. I've just tested it with test_key_actions.py and it's working fine. Henrik, should I change all files currently using this class to the new legacy.py file?

Comment 15

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

Yes, that's what you should do. It will make it clear that this is for the legacy actions.

Comment 16

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

Hey Vinícius. I wanted to check back how you are, and if it is something you still want to work on. Meanwhile the removal of the old API blocks us in some areas, and it would be nice to see it removed in the near future. 

Please let me know what you think, and if this bug is too hard, we can certainly find an easier one for you. That's totally fine.

Comment 17

[u602518]

Hi Henrik. At the moment I'm trying to update the most simple action commands (click, key_down) in actions.py to use the WebDriver conforming ones (since those have equivalents in client.py).

Like, for example, client.py has the following code for click:


>    def click(self, element=None, button=0):
>        """Queue a click with the specified button.
>       If an element is given, move the pointer to that element first,
>        otherwise click current pointer coordinates.
>        :param element: Optional element to click.
>        :param button: Integer representing pointer button to perform action
>                       with. Default: 0, which represents main device button.
>        """
>        if element:
>            self.pointer_move(0, 0, origin=element)
>        return self.pointer_down(button).pointer_up(button)

While the legacy code uses

>    def click(self, element, button=MouseButton.LEFT, count=1):
>        '''
>        Performs a click with additional parameters to allow for double clicking,
>        right click, middle click, etc.
>        :param element: The element to click.
>        :param button: The mouse button to click (indexed from 0, left to right).
>        :param count: Optional, the count of clicks to synthesize (for double
>                      click events).
>        '''
>        el = element.id
>        self.action_chain.append(['click', el, button, count])
>        return self

I'm trying to learn how WebDriver does the same action differently. I've noticed it uses key and pointer actions, for instance. This is taking some time to learn (since I'm only working during weekends, I end up forgetting some parts of the code and have to read it again).

Although it's a little bit hard, I'm learning a lot with it. But yes, I'm making a slow progress. If it's urgent for the team, that's no problem if you find it better for me to work on something else.

Meanwhile, I will try running some tests later today to see if I can make the click action run correctly. I think that writing the other actions will be easier after that.

Comment 18

[u602518]

I have a question regarding the 'perform' action here https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/webdriver/webdriver/client.py#213-221. In the legacy code, the perform action is executed by sending a message to the Marionette server with the Action Chain. We will continue using this? (the WebDriver code uses a session. Not entirely sure how that works yet, so I will look into it).

Comment 19

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

(In reply to Vinícius Costa from comment #17)
> Although it's a little bit hard, I'm learning a lot with it. But yes, I'm
> making a slow progress. If it's urgent for the team, that's no problem if
> you find it better for me to work on something else.

That's good to hear that such a challenging task isn't holding you off, and you enjoy learning from it! 

Regarding the timing, it would be indeed good to have this done better sooner than later. And I can understand how hard it is when you have to pick up the work from 6 or more days ago. So my proposal would be that we find something else for you, which is also challenging but doesn't hold of other projects. And sorry that this wasn't that clear earlier.
 
(In reply to Vinícius Costa from comment #18)
> tools/webdriver/webdriver/client.py#213-221. In the legacy code, the perform
> action is executed by sending a message to the Marionette server with the
> Action Chain. We will continue using this? (the WebDriver code uses a
> session. Not entirely sure how that works yet, so I will look into it).

To also reply to this question... Yes, that is what we want to continue to do. The `perform` command in Marionette client would also call `send_message()` similar to the one from the legacy commands. That method is actually handling the session transparently for you, and as such we don't have to pass it into each and every method.

Comment 20

[u602518]

That's fine Henrik. It was a nice learning experience! So, if there's some other challenging and less urgent task for me to work on, I would gladly look into it. Like I said to you in e-mail, I'm only able to work during weekends until late November, then I will have more spare time. But I can start working on something until then.

Comment 21

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

Maybe lets chat first on IRC so you can show me the current status as a patch, and we can discuss what would be the best option? If you already made big progress, I actually don't want to pull it off from you.

Comment 22

[u602518]

All right. Maybe IRC will be difficult for me this week, but I will try to wrap up what I've been working on and send a patch this weekend (after I run some tests and make sure I'm not breaking anything). Either way I will keep in touch with you via e-mail, so there won't be long periods without you hearing from me.

Comment 23

[u602518]

Ok, I think I hit a wall with the perform action.

Based on webdriver, I'm trying to implement `key_up` the following way in Marionette client:

>    def _key_action(self, subtype, value):
>        self.action_chain.append({"type": subtype, "value": value})


>    def key_up(self, key_code):
>        self._key_action("keyUp", key_code)
>        return self

That's of course different from how these objects were stored in action_chain in legacy code. The `_send_message()` method in `perform()` expects a body like this to be sent to the marionette server:

> body = {"chain": self.action_chain, "nextId": self.current_id}

So, I'm stuck at how should I update this `perform()` function so it works with the new webdriver conforming code. I think the problem is that I don't understand why are actions stored like that, with "type" and "value" fields (what goes in these?). And what does the marionette driver server expects to receive in the body?

Comment 24

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

You will have to also change `perform()`. The command's name and body to be sent are different now. So the new endpoint is `WebDriver:PerformActions`. For how the body looks like you can as best check one of the Wd jobs on Treeherder (https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=wd&bugfiler=&selectedJob=209090840). The action tests are most likely part of Wd2. Click on one of those and then on `live_backing.log` in the panel below. Search for `WebDriver:PerformActions`. Then you will see how it looks like. Here an example:

> Marionette	TRACE	0 -> [0,3,"WebDriver:PerformActions",{"actions":[{"actions":[{"type":"keyUp","value":"a"}],"id":"keyboard_id","type":"key"}]}]

Let me know if there are still open questions. Thanks!

Comment 25

[u602518]

Attachment: Bug 1354578 - Adding Webdriver endpoint to .perform action.r=whimboo

Comment 26

[u602518]

Hey Henrik. I've submitted a partial patch just so you can have a look if I'm in the right path. At the moment, I'm trying to udpate the .perform action so it uses the new actions endpoint that you said. I'm doing things similar to this other file :https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/webdriver/webdriver/client.py#208, but the main problem now is that I've updated the __init__ method in the Actions class so it receives input_id and action_type as arguments, and now the tests in test_key_actions.py are failing because they should only send only one argument.

Since the new endpoint expects to receive these new arguments (as I've seen in the `live_backing.log` you've mentioned), how should I instantiate the class in the tests so it receives these new parameters?

This is one example (in test_key_actions.py) :
>    def setUp(self):
>        self.key_action = Actions(self.marionette)

>    def test_key_action_basic_input(self):
>        self.key_action.key_down("a").key_down("b").key_down("c").perform()
>        self.assertEqual(self.key_reporter_value, "abc")

This test obviously gives an error because I'm instantiating the Actions class with just one argument, when it should also receive input_id and action_type.

Comment 27

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

We will continue the conversation in phabricator to not have two independent discussion threads.

Comment 28

[u602518]

Attachment: Bug 1354578 - Updated ActionSequence class to WebDriver.r=whimboo

Comment 29

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

Hi Vinícius. I haven't heard from you for a while, and wonder how things are. Will you be able to continue on this bug, or should I take it over? Thanks.

Comment 30

[u602518]

Hi Henrik. I think you should take over the bug. I haven't worked actively on it for quite some time now, and I don't want this feature to be delayed even further because of that. Sorry for taking this long.

Comment 31

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

Ok, thanks a lot. I will take it over to clean-up our code base before making element send keys using the action primitives.

Comment 32

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

We will first land the new Action API support via bug 1533786.

Comment 33

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

The following patch is waiting for review from an inactive reviewer:

ID	Title	Author	Reviewer Status
D15931	Bug 1354578 - Updated ActionSequence class to WebDriver.r=whimboo	whimboo	viniciuscosta: Disabled

:whimboo, could you please find another reviewer or abandon the patch if it is no longer relevant?

For more information, please visit auto_nag documentation.

Comment 34

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

With bug 1683755 the legacy actions support got actually removed in Firefox 86. So I don't see that anything else still needs to be done here.