webdriverio icon indicating copy to clipboard operation
webdriverio copied to clipboard

scroll calculations feature

Open udarrr opened this issue 1 year ago • 4 comments

related to https://github.com/webdriverio/webdriverio/issues/11958, https://github.com/webdriverio/webdriverio/issues/11866

udarrr avatar Jan 20 '24 11:01 udarrr

Can you describe this PR a little bit? What are you trying to resolve? What is the approach?

Yes sure. Generally it's correct calculations for deltaX and deltaY, previously when an error 'Request failed with status 500 due to move target out of bounds: move target out of bounds' has appeared that meant 'action' method moved scrolling to 'end', because it is how the method working even with wrong deltaX, deltaY it's still working just throws an error, then when the error has been thrown, it being moved to fallback with web scrollIntoView. Currently both of them are calling 'actions' and 'web scroll'. From my perspective it doesn't have a big sense to call 'actions' then falling back to web scroll and then execute it again, the issue has been described here https://github.com/webdriverio/webdriverio/issues/11866, you can reproduce it here https://github.com/udarrr/moveToExample.

In my solution as it's knows that 'actions' move it like 'end' for block and 'nearest' for inline, we're calculating deltaX or deltaY regarding 'end' position in case if element wasn't into view before scrolling. If element intoview 'actions' behave as 'nearest' for block and 'nearest' for inline, that's why we're using simple web scroll if it's needed to force it to center, start or end position, if no just skip it

udarrr avatar Jan 20 '24 21:01 udarrr

Possible it makes sense to left it just like

await browser.action('wheel').scroll({ duration: 0, deltaX: 0, deltaY: 0, origin: this }).perform()

without calculation at all, by default it's handling 'end' and 'nearest'

And for 'center' and 'start' we can use 'action' for scrolling to 'end' and 'scrollIntoViewWeb' for shifting

await browser.action('wheel').scroll({ duration: 0, deltaX: 0, deltaY: 0, origin: this}).perform() 
await scrollIntoViewWeb.call(this, options as any)

that's it like it's working now just without annoyed an error.

What's your take on this @christian-bromann ?

udarrr avatar Feb 04 '24 13:02 udarrr

I'm not sure what others think but looking at the code I feel like we are adding a lot of complexity, are we sure this is really needed?

erwinheitzman avatar Feb 06 '24 09:02 erwinheitzman

@udarrr

I am also not 100% convinced about this one. Our goal should it be to provide the best result using WebDriver primitives for scrolling. It is fine to me that we do element calculations through execute but the actual scroll should happen through WebDriver. Happy to document the escape hatch to do this via await browser.execute((elem) => elem.scrollIntoView(), elem) if an error occurs, or have this escape hatch within a catch block.

christian-bromann avatar Feb 06 '24 17:02 christian-bromann

thank you for pointing it out @christian-bromann, as we're going to rely on webdriver action native only, there should be fixed an issue with delta https://github.com/webdriverio/webdriverio/issues/11958. I've raised an issue here https://github.com/w3c/webdriver/issues/1798. I believe to that moment we can postpone the PR, i'll reopen it again and being able to proceed with the PR after clarification or fixes in webdriver

udarrr avatar Feb 24 '24 11:02 udarrr

Sounds good to me. Thanks for clarifying and raising the issue. I will go ahead and close this. Feel free to re-raise any time.

christian-bromann avatar Feb 24 '24 23:02 christian-bromann