Skip to content

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

Merged
christian-bromann merged 2 commits intomainfrom
unknown repository
Oct 31, 2023
Merged

(webdriverio): fix - scrollIntoView calls scroll action with a wrong …#11496
christian-bromann merged 2 commits intomainfrom
unknown repository

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 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

christian-bromann
christian-bromann approved these changes Oct 23, 2023
View reviewed changes
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
erwinheitzman
erwinheitzman requested changes Oct 23, 2023
View reviewed changes
@christian-bromann christian-bromann self-requested a review October 25, 2023 01:27
@ghost ghost requested a review from erwinheitzman October 26, 2023 09:19
erwinheitzman
erwinheitzman approved these changes Oct 28, 2023
View reviewed changes
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

christian-bromann commented Oct 28, 2023

@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

christian-bromann commented Oct 31, 2023

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
@christian-bromann
Copy link
Member

christian-bromann commented Oct 31, 2023

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

forPetProject commented Nov 7, 2023

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

christian-bromann commented Nov 7, 2023

@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
Closed
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@christian-bromann christian-bromann Awaiting requested review from christian-bromann

Assignees

No one assigned

Labels

PR: Bug Fix 🐛 PRs that contain bug fixes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

3 participants

@christian-bromann @forPetProject @erwinheitzman