Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(webdriverio): fix - scrollIntoView calls scroll action with a wrong … #11496

Merged

Conversation

ghost
Copy link

@ghost ghost commented Oct 23, 2023

Proposed changes

From the last improvement in the function, a bad argument is sent to the scroll action. We should not use deltaX value into x argument.

This fixes #11272

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Checklist

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • I have added proper type definitions for new commands (if appropriate)

Further comments

I tested in a local project but I wasn't sure how to add or update existing tests inside webdriverio project. I was not able to reproduce the issue with existing tests but with my own.
Here is the test I used to see the log disappeared.

The fix should be also testable using example provided in the initial ticket description by Lara.

Reviewers: @webdriverio/project-committers

@erwinheitzman since you worked in it recently

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 23, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@erwinheitzman what do you think?

@christian-bromann christian-bromann added the PR: Bug Fix 🐛 PRs that contain bug fixes label Oct 23, 2023
@ghost ghost force-pushed the fix-scrollIntoView-actions-params branch from 870d6dc to 650b81a Compare October 26, 2023 09:16
@ghost ghost force-pushed the fix-scrollIntoView-actions-params branch from 650b81a to a103bc1 Compare October 26, 2023 09:16
@ghost ghost requested a review from erwinheitzman October 26, 2023 09:19
Copy link
Member

@erwinheitzman erwinheitzman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution, LGTM 👍

@christian-bromann
Copy link
Member

@qaflorent mind updating the snapshots and make the tests pass?

@ghost
Copy link
Author

ghost commented Oct 30, 2023

@qaflorent mind updating the snapshots and make the tests pass?

Of course, sorry I missed that. Should be good now.

@christian-bromann
Copy link
Member

Tests are failing for different reasons. I will go ahead and merge this one and fix the tests in main.

@christian-bromann christian-bromann merged commit bcdf1b4 into webdriverio:main Oct 31, 2023
3 of 8 checks passed
@christian-bromann
Copy link
Member

Congratulations @qaflorent on your remarkable first contribution to WebdriverIO! This project thrives on the invaluable involvement of our community, and we are truly grateful for your contribution. We eagerly anticipate witnessing more of your exceptional work, so please don't hesitate to inform us if we can assist you in identifying intriguing areas where you can make further contributions. Join our lively Discord channel and reach out to us; we would be delighted to connect with you. Your efforts are deeply appreciated, and we extend our heartfelt gratitude to you. 🙏 ❤️

@forPetProject
Copy link

Congratulations @qaflorent on your remarkable first contribution to WebdriverIO! This project thrives on the invaluable involvement of our community, and we are truly grateful for your contribution. We eagerly anticipate witnessing more of your exceptional work, so please don't hesitate to inform us if we can assist you in identifying intriguing areas where you can make further contributions. Join our lively Discord channel and reach out to us; we would be delighted to connect with you. Your efforts are deeply appreciated, and we extend our heartfelt gratitude to you. 🙏 ❤️

Could you please help understand in which version this will be delivered?

@ghost
Copy link
Author

ghost commented Nov 7, 2023

Could you please help understand in which version this will be delivered?

It should be delivered in 8.21.0, but it seems the fix is not sufficient/good. I still have big troubles to test it (trying and trying again locally), but @erwinheitzman , I would like to try replacing .scroll({ duration: 0, x: deltaX, y: deltaY, origin: this }) with .scroll({ duration: 0, deltaX, deltaY, origin: this }) as I originally thought.

@christian-bromann
Copy link
Member

@qaflorent you can make the change directly in your node_modules to test it in your project.

@ghost ghost deleted the fix-scrollIntoView-actions-params branch November 8, 2023 08:54
@ghost ghost mentioned this pull request Nov 8, 2023
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Bug Fix 🐛 PRs that contain bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: Request failed with status 500 if using scrollIntoView()
3 participants