changed moveTo to like it's been implemented in click #11529

udarrr · 2023-10-25T07:08:26Z

Proposed changes

change moveTo to click similarity, because of flaky moveTo

Types of changes

I'd like to suggest to change it to a way like it's been implemented in click method with 'origin: this' instead of calculation full offset. Even more i don't understand why we should calculate it instead of using 'origin: this' as click method perform particularly the same, with stable behavior in each cases, just with some additional actions like 'down' and 'up'.

erwinheitzman · 2023-10-25T11:07:12Z

Hi @udarrr

Could you provide examples of actual cases in which the command is not doing what you would expect it to do?
This way we can verify that this indeed fixes the problem(s)

udarrr · 2023-10-25T11:19:30Z

Hi @udarrr

Could you provide examples of actual cases in which the command is not doing what you would expect it to do? This way we can verify that this indeed fixes the problem(s)

Hi @erwinheitzman unfortunately i can't because known reproduceable an example with no public resources, but from my perspective here something wrong. Let me explain

we have implementation of clicking, click https://github.com/webdriverio/webdriverio/blob/main/packages/webdriverio/src/commands/element/click.ts already have an implementation of moveTo but with some extra actions

something like

if (this.isW3C) {
const browser = getBrowserObject(this)
await browser.action('pointer', {
parameters: { pointerType: 'mouse' }
})
.move({
origin: this,
x: xoffset,
y: yoffset
})
.down({ button })
.up({ button })
.perform(skipRelease)
return
}

it's moving pointer to element with scrolling and then down and up pointer, if we'd like to be moved to element without pressing pointer i believe we should just use 'move' without down and up. It's the same without useless calculations. And it works everywhere even with my apps.

erwinheitzman · 2023-10-25T11:30:55Z

Hi @udarrr

Could you provide examples of actual cases in which the command is not doing what you would expect it to do? This way we can verify that this indeed fixes the problem(s)

Hi @erwinheitzman unfortunately i can't because known reproduceable an example with no public resources, but from my perspective here something wrong. Let me explain

we have implementation of clicking, click https://github.com/webdriverio/webdriverio/blob/main/packages/webdriverio/src/commands/element/click.ts already have an implementation of moveTo but with some extra actions

something like

if (this.isW3C) {
    const browser = getBrowserObject(this)

    await browser.action('pointer', {

        parameters: { pointerType: 'mouse' }

    })

        .move({

            origin: this,

            x: xoffset,

            y: yoffset

        })

        **.down({ button })

        .up({ button })**

        .perform(skipRelease)

    return

}
it's moving pointer to element with scrolling and then down and up pointer, if we'd like to be moved to element without pressing pointer i believe we should just use 'move' without down and up. It's the same without useless calculations. And it works everywhere even with my apps.

I think I understand what you mean and I feel like we should create a utility function to retrieve the coordinates of an element with the passed offsets as right now we would have to maintain it in two places. Also the click implementation is still different from the changes you suggest so that will also be resolved by doing this.
Do you have a clear picture of what it is I am suggesting? 🙂

erwinheitzman · 2023-10-25T11:32:23Z

Additionally there might be more commands that could benefit from such a utility function, like the scrollIntoView command might be able to use it (it does scroll of course)

udarrr · 2023-10-25T11:53:51Z

Do you have a clear picture of what it is I am suggesting?

To be honest no :) it's still not clear for me. Could you clarify that ?

this is click implementation

const browser = getBrowserObject(this)
await browser.action('pointer', {
parameters: { pointerType: 'mouse' }
})
.move({
origin: this,
x: xoffset,
y: yoffset
})
.down({ button })
.up({ button })
.perform(skipRelease)

this is my implementation of moveTo

const browser = getBrowserObject(this)
await browser.action('pointer', {
parameters: { pointerType: 'mouse' }
})
.move({
origin: this,
x: xoffset,
y: yoffset
})
.perform()

udarrr · 2023-10-25T20:00:16Z

Could you provide examples of actual cases in which the command is not doing what you would expect it to do? This way we can verify that this indeed fixes the problem(s)

Generally i was able to reproduce it. Here is description #11534 There are two issue.

It can't be performed properly inside of iframe with current version of moveTo but can be resolved as i've done it above for both cases either none iframe element or iframe element
My assumption that 'move' method can scroll it to element was wrong that's why if element not into view 'moveTo' method throw an Error 'move target out of bounds' Maybe it have sense to add scrolling to the method and timeout for scroll animation. Something like that

const browser = getBrowserObject(this);
await this.scrollIntoView(scrollOptions)
await browser.pause(scrollAnimationTimeout)
return browser.action('pointer', { parameters: { pointerType: 'mouse' } })
.move({origin: this, x: 0, y: 0 })
.perform();

or it could be responsibility of developer to add scrollIntoView and timeout to test. What do you think ?

erwinheitzman · 2023-10-25T22:03:28Z

The command adheres to the webdriver specs by throwing the error and has always acted like this and there are other options to do this (action command) so I would say that it is up to the user to scroll for now.

What I meant is that the whole part of the coordinates can be abstracted away into a function that can be shared between moveTo and click

udarrr · 2023-10-26T07:01:48Z

What I meant is that the whole part of the coordinates can be abstracted away into a function that can be shared between moveTo.

I got it, but i'd like to suggest don't use it at all. moveTo can handle it to center by default for origin if i am not mistaken.

The same for clicking

I've updated the pr.

The command adheres to the webdriver specs by throwing the error and has always acted like this and there are other options to do this (action command) so I would say that it is up to the user to scroll for now.

Ok i'll create for that different discussion. Maybe it could be a good feature to add it.

christian-bromann

I think overall this makes sense. I wonder if there is an easy way to create some e2e tests for this similar to what we have /e2e/wdio/headless/test.e2e.ts#L123.

@erwinheitzman wdyt?

packages/webdriverio/src/commands/element/moveTo.ts

erwinheitzman · 2023-10-27T15:12:33Z

I think overall this makes sense. I wonder if there is an easy way to create some e2e tests for this similar to what we have /e2e/wdio/headless/test.e2e.ts#L123.

@erwinheitzman wdyt?

Yeah that's totally possible.

We can moveTo the same element with different offsets and also things like not passing and offsets, something along the lines of:

const inputs = [
  [undefined, { x: 9999, y: 9999 }],
  [{ xOffset: 10 }, { x: 9999, y: 9999 }],
  [{ yOffset: 10 }, { x: 9999, y: 9999 }],
  [{ xOffset: 25, yOffset: 25 }, { x: 9999, y: 9999 }],
  [{ xOffset: Number.MAX_SAFE_INTEGER, yOffset: Number.MAX_SAFE_INTEGER }, { x: 9999, y: 9999 }],
]

inputs.forEach(([input, output]) => {
  it(`moves to position x ${output.x} y ${output.y} when passing the arguments ${JSON.stringify(input)}`, () => {
    await browser.execute(() => {
      let mouse = { x:0, y:0 }
      document.onmousemove = function(e){ mouse.x = e.clientX, mouse.y = e.clientY }
    })
    
    await elem.moveTo(input);
    
    const { x, y } = await browser.execute(() => mouse)

    expect(x).toEqual(output.x)
    expect(y).toEqual(output.y)
  })
})

Note that this code is an example and I can by no means guarantee it works

udarrr · 2023-10-27T19:43:07Z

Yeah that's totally possible.

We can moveTo the same element with different offsets and also things like not passing and offsets, something along the lines of:

const inputs = [
  [undefined, { x: 9999, y: 9999 }],
  [{ xOffset: 10 }, { x: 9999, y: 9999 }],
  [{ yOffset: 10 }, { x: 9999, y: 9999 }],
  [{ xOffset: 25, yOffset: 25 }, { x: 9999, y: 9999 }],
  [{ xOffset: Number.MAX_SAFE_INTEGER, yOffset: Number.MAX_SAFE_INTEGER }, { x: 9999, y: 9999 }],
]

inputs.forEach(([input, output]) => {
  it(`moves to position x ${output.x} y ${output.y} when passing the arguments ${JSON.stringify(input)}`, () => {
    await browser.execute(() => {
      let mouse = { x:0, y:0 }
      document.onmousemove = function(e){ mouse.x = e.clientX, mouse.y = e.clientY } 
    })
    
    await elem.moveTo(input);
    
    const { x, y } = await browser.execute(() => mouse)

    expect(x).toEqual(output.x)
    expect(y).toEqual(output.y)
  })
})

Note that this code is an example and I can by no means guarantee it works

That's great an example @erwinheitzman, It worth to add it. I've added some test cases related to your an example regarding position moving

udarrr · 2023-10-28T08:36:55Z

Tests are failed don't related to last changes, because of

[headless chrome 120.0.6092.0 linux #1-0] Expected substring: "headless chrome"
[headless chrome 120.0.6092.0 linux #1-0] Received string: "mozilla/5.0 (x11; linux x86_64) applewebkit/537.36 (khtml, like gecko) headlesschrome/120.0.6092.0 safari/537.36"

there are 2 tests in different suites

udarrr · 2023-10-28T10:16:15Z

I've fixed 2 test cases regarding message above and finally it can be merged)

e2e/wdio/headless/test.e2e.ts

christian-bromann

LGTM 👍

Thanks a lot!

erwinheitzman

This is looking really good, thank you very much for your hard work 👏

vitaly-kiselev-qa · 2024-02-03T06:54:43Z

@udarrr
Hello.

The comment states that xOffset and yOffset are relative to the top-left corner of the element:
https://github.com/webdriverio/webdriverio/blob/main/packages/webdriverio/src/commands/element/moveTo.ts#L16

Looks like it is no longer true since they are passed unchanged to moveTo as x and y:
https://github.com/webdriverio/webdriverio/blob/main/packages/webdriverio/src/commands/element/moveTo.ts#L44

And move (params: PointerActionMoveParams) expects x and y relative to the center of a specific element:
https://webdriver.io/docs/api/browser/action/#pointer-input-source

Is it possible to update the documentation?

christian-bromann · 2024-02-04T18:11:14Z

@vitaly-kiselev-qa feel free to raise a PR if you have a concrete suggestions in mind.

vitaly-kiselev-qa · 2024-02-05T04:52:02Z

@christian-bromann thank you for your answer.

My comment was more a question not a suggestion.

It looks like, the behaviour has been changed and the documention has not.
The question is — was the change intentional or not?

For my code this was a breaking change and I had to rewrite it (calculate coordinates in a new way).

christian-bromann · 2024-02-06T06:23:19Z

@vitaly-kiselev-qa mind raising an issue with a description of this, I think this should be fixed.

changed moveTo to like in has been implemented in click

664c610

udarrr requested a review from christian-bromann October 25, 2023 07:10

udarrr changed the title ~~changed moveTo to like in has been implemented in click~~ changed moveTo to like it's been implemented in click Oct 25, 2023

udarrr added 3 commits October 25, 2023 09:11

linter fixes

e6b9c9c

test fixes

b2852b9

changed snaps

b0052dd

deleted any calculation from moveTo

755c53f

christian-bromann reviewed Oct 26, 2023

View reviewed changes

packages/webdriverio/src/commands/element/moveTo.ts Show resolved Hide resolved

added e2e tests for moveTo

91f94b7

udarrr added 4 commits October 27, 2023 21:45

deleted getScrollPosition

6d503d5

tests fixes

04ad349

test fixes

e331019

added position moveTo tests

a908348

udarrr requested review from erwinheitzman and christian-bromann October 28, 2023 07:48

changed headless chrome to headlesschrome foobar tests in headless

46480d8

christian-bromann reviewed Oct 28, 2023

View reviewed changes

e2e/wdio/headless/test.e2e.ts Outdated Show resolved Hide resolved

e2e/wdio/headless/test.e2e.ts Outdated Show resolved Hide resolved

e2e/wdio/headless/test.e2e.ts Outdated Show resolved Hide resolved

clean code

9d8173c

christian-bromann approved these changes Oct 28, 2023

View reviewed changes

erwinheitzman approved these changes Oct 28, 2023

View reviewed changes

erwinheitzman merged commit 233fbcd into webdriverio:main Oct 28, 2023
7 of 8 checks passed

erwinheitzman added the PR: Polish 💅 PRs that contain improvements on existing features label Oct 28, 2023

This was referenced Feb 6, 2024

[🐛 Bug]: breaking change in webdriverio@8.21.0 (logic of moveTo is changed) #12185

Closed

Option resetCursor moves the cursor to the center of the screen gemini-testing/testplane#841

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

changed moveTo to like it's been implemented in click #11529

changed moveTo to like it's been implemented in click #11529

udarrr commented Oct 25, 2023

erwinheitzman commented Oct 25, 2023

udarrr commented Oct 25, 2023 •

edited

erwinheitzman commented Oct 25, 2023

erwinheitzman commented Oct 25, 2023

udarrr commented Oct 25, 2023 •

edited

udarrr commented Oct 25, 2023 •

edited

erwinheitzman commented Oct 25, 2023 •

edited

udarrr commented Oct 26, 2023 •

edited

christian-bromann left a comment

erwinheitzman commented Oct 27, 2023 •

edited

udarrr commented Oct 27, 2023 •

edited

udarrr commented Oct 28, 2023 •

edited

udarrr commented Oct 28, 2023

christian-bromann left a comment

erwinheitzman left a comment

vitaly-kiselev-qa commented Feb 3, 2024 •

edited

christian-bromann commented Feb 4, 2024

vitaly-kiselev-qa commented Feb 5, 2024

christian-bromann commented Feb 6, 2024

changed moveTo to like it's been implemented in click #11529

changed moveTo to like it's been implemented in click #11529

Conversation

udarrr commented Oct 25, 2023

Proposed changes

Types of changes

erwinheitzman commented Oct 25, 2023

udarrr commented Oct 25, 2023 • edited

erwinheitzman commented Oct 25, 2023

erwinheitzman commented Oct 25, 2023

udarrr commented Oct 25, 2023 • edited

udarrr commented Oct 25, 2023 • edited

erwinheitzman commented Oct 25, 2023 • edited

udarrr commented Oct 26, 2023 • edited

christian-bromann left a comment

Choose a reason for hiding this comment

erwinheitzman commented Oct 27, 2023 • edited

udarrr commented Oct 27, 2023 • edited

udarrr commented Oct 28, 2023 • edited

udarrr commented Oct 28, 2023

christian-bromann left a comment

Choose a reason for hiding this comment

erwinheitzman left a comment

Choose a reason for hiding this comment

vitaly-kiselev-qa commented Feb 3, 2024 • edited

christian-bromann commented Feb 4, 2024

vitaly-kiselev-qa commented Feb 5, 2024

christian-bromann commented Feb 6, 2024

udarrr commented Oct 25, 2023 •

edited

udarrr commented Oct 25, 2023 •

edited

udarrr commented Oct 25, 2023 •

edited

erwinheitzman commented Oct 25, 2023 •

edited

udarrr commented Oct 26, 2023 •

edited

erwinheitzman commented Oct 27, 2023 •

edited

udarrr commented Oct 27, 2023 •

edited

udarrr commented Oct 28, 2023 •

edited

vitaly-kiselev-qa commented Feb 3, 2024 •

edited