Codecov Report

Patch coverage: 18.75% and project coverage change: -0.09 ⚠️

Comparison is base (f7d3df2) 56.01% compared to head (eae4fa9) 55.93%.

❗ Current head eae4fa9 differs from pull request most recent head 1dadde5. Consider uploading reports for the commit 1dadde5 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##            trunk   #11521      +/-   ##
==========================================
- Coverage   56.01%   55.93%   -0.09%     
==========================================
  Files          86       86              
  Lines        5732     5746      +14     
  Branches      233      233              
==========================================
+ Hits         3211     3214       +3     
- Misses       2288     2299      +11     
  Partials      233      233              

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

First, thanks for taking a look at this, I've been dreading digging back into this code because I'm not sure I did the right thing in java last year... :)

The primary reason for this functionality is that the original implementation of Actions was one mouse one keyboard. In the move to w3c actions, you can have more devices, but the code we have doesn't support that concept. You can't easily use a pen or a finger type.

I think we may just want to keep it simple for now and not think about creating multiple devices and switching among existing devices, etc.

I'm looking at what I originally wrote in Java a year ago, and what I'm seeing doesn't make sense. We're looking for an existing name. But I don't think an existing name can exist because there is no place that we can add a device name. @pujagani am I missing something? (besides writing a test last year that would demonstrate it worked as I expected).

Like in the Python code here, we never add the new active pointer we are setting to the list of devices...

So I think we might be best off with the user doing:

ActionChains(driver).set_active_pointer(interaction.POINTER_PEN, "pen").move_by_offset(1, 1)

and the implementation:

    def set_active_pointer(self, kind, name):
            pointer_input = self.w3c_actions.add_pointer_input(kind, name)
            self.w3c_actions.pointer_action = PointerActions(pointer_input)

        return self

Do you think the additional functionality is important? Do we need to discuss more?

Oh and just because lambdas are cool. The way to avoid the loop you have in Python is:

pointer_input = next(filter(lambda x: x.name == name, self.w3c_actions.devices), None)

@titusfortner I understand what you are saying. The addition functionality will be important if the user needs to switch between multiple devices. For example,

action_provider = ActionChains(driver).set_active_pointer(PointerInput(interaction.POINTER_MOUSE, "test mouse"))
{ do some action }
action_provider = ActionChains(driver).set_active_pointer(PointerInput(interaction. POINTER_PEN, "pen"))
{ do some action }
action_provider = ActionChains(driver).set_active_pointer(PointerInput(interaction.POINTER_MOUSE, "test mouse"))
{ do some action }

If we don't use the addition functionality, the above code will create POINTER_MOUSE objects twice. That's how I understand it.

@titusfortner You are right. I think we need additional functionality. @TamsilAmani 's point sounds fair. And for python, it looks like it is possible to implement it, so we can add it. What do you think?

So, all the languages (except Ruby, for *reasons) have a simple API and an advanced API for working with Actions. It is possible to do all the things with the advanced API, it's just not intuitive and often a huge pain.

The discussion is about what features we want to make possible for users to do with the simple API:

1. Using a pen/finger instead of a mouse
2. Switching between different pointer/key/wheel devices
3. Using multiple fingers at the same time

The first I think is important, and a simple version of this PR can accomplish it.

The second I actually don't think is important (I can't think of a valid use case for people using it in normal testing), and I don't think the existing code in Java is actually doing this (we can test to make sure, but I'm pretty sure it is creating new devices rather than using existing ones, and I'm pretty sure advanced usage of it won't work as expected).

More importantly, the current implementation will not help with multiple simultaneous actions. I think the correct solution for number 2 might be a trivial use case of number 3.

So, my current thought is we should remove the functionality for searching for existing devices for now, and figure out the advanced features in the future. What do you think?

Thank you for the detailed context. I now have a better understanding of the concern here. I agree with you, this PR can achieve "Using a pen/finger instead of a mouse" mouse bit for now.
For advanced feature, let's look at it in the future and also check how it is possible in different languages.

I also agree with @titusfortner. So, wrt to point 1 ... Shall I change the implementation code to.... ?

    def set_active_pointer(self, kind, name):
            pointer_input = self.w3c_actions.add_pointer_input(kind, name)
            self.w3c_actions.pointer_action = PointerActions(pointer_input)

        return self

to make it even simpler.

I think for the scope of this PR, @TamsilAmani yes!

So actually, I realized the only reason I used "setActive" in Java is because I didn't want to create a bunch of new constructors. Can we do what Ruby does and add optional parameters to each of the mouse methods?:

    pen = PointerInput(interaction.POINTER_PEN, "pen")
    hoverable = driver.find_element(By.ID, "hover")
    ActionChains(driver)\
        .move_to_element(hoverable, device=pen)\
        .perform()

@pujagani I think we can technically do this in .NET as well? Though maybe better to match Java as much as possible.

@titusfortner I will have to check if we can do this in C#. I have tried to match it to Java actually #11513. Let me know if that looks fine. Probably, will need to remove the logic of finding a device exists or not based on the discussion done here.

There are still few methods which need to implement these device=None parameter. I will do it later as a TODO or someone else can pick up.

@TamsilAmani if you can't do it now, please create an issue so we can track it, thanks!

I have created a separate test file "interactions_with_device_tests.py" which is a replica of the already present "interactions_tests.py" with the modification that the new test file uses device parameters in the functions to test for the feature being implemented in this PR.

If there's a better place/approach to place the tests elsewhere, let me know :)

@TamsilAmani showed me code last week that made me realize there is another complication in Python.

When there's only ever one pointer, you can create as many instances of the class as you want. If you need a non-default pointer, now things get weird.

We need to take a look at the spec and see what the driver is supposed to remember of pointers and their state. The driver does maintain state between calls to the endpoint (perform() method calls), so we need to make sure that Python implementation maintains state the way the driver does.

@AutomatedTester I have added the latest changes according to what we finalised. Now regarding the question that "Different instances of ActionChains pointing to the same device", which you said is not possible, here according to @titusfortner (my discussion with him),
The question is what does the driver "remember" between calls
I'm pretty sure the pointer doesn't reset after each call.
but does it share the info between the different types? or just within a type?

Can you throw some light on this?

The discussion I had with @TamsilAmani was about the follow code block.The following is psuedo-ish code so don't worry about the syntax too much.

hold = ActionChains(driver).click_and_hold(element)
move = ActionChains(driver).move_to(element)
release = ActionChains(driver).release()

This would create 3 different action chains in python all with their own default. If you by chance got things to work in the way you intended with a mouse there is no guarantee that it would work if you only put a new device on any of the commands.

variable = SomeObject() in python is equivalent to variable = new SomeObject() in Java.

So if we did the same example but with defaults annotated for ease and had 1 set as pen we would get

hold = ActionChains(driver).click_and_hold(element, device=Pen) #  device == Pen
move = ActionChains(driver).move_to(element) # device == Mouse
release = ActionChains(driver).release() # device == Mouse

Again, this is 3 separate chains. We can form one by doing the follow, and it will then keep the history in the client which is sent to the browser.

hold = ActionChains(driver).click_and_hold(element, device=Pen) #  device == Pen
move = hold.move_to(element) # device == Pen
release = move.release() # device == Pen

Since the first code block was sending 3 different chains the browser it doesn't know that they are related. It makes the assumption that you know what you are sending over the wire is correct. It will just process them as 3 items. If things work out... awesome but that's not guaranteed as it's 3 different chains.

The Perform Actions command does not make any allowances for storing the action chain in the driver after executing them. It may store some global key state between actions chains but again, not guaranteed.

If you were to call Release Actions then it would process the 3 chains separately, again causing undefined behaviour as there is no guarantee that when the event loop fired it would give the desired affect.

Having had a cursory glance over the current code I think it moved to having a way of storing the device to be persistent if people want without having to remember to use it on all interaction

I squashed things locally and rebased and am trying to fix the linting issues. The code itself is good and can be merged as soon as the CI is happy with it.