Changed logic so that _scrollToTopIfVisible is called only if element is in viewport. Previously it was called only when the element was outside it.

[oskarolaussen]

Description (*)

Noticed that _scrollToTopIfVisible was called only if changing element was outside of viewport and thus wasn't working as intended.

Fixed Issues (if relevant)

Manual testing scenarios (*)

On product page scroll so that .product.data.items is partially visible.
Change tab. The whole .product.data.items should scroll into view.

Questions or comments

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds are green)

[m2-assistant]

Hi @oskarolaussen. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

[magento-cicd2]

All committers have signed the CLA.

[sidolov]

Hi @oskarolaussen , looks like you made some commits with email different than in your GitHub profile, please, add email from commits to your profile!
Thank you!

[oskarolaussen]

Hi @sidolov! Commits should now be correctly attributed to me.

[oskarolaussen]

I reverted _isElementOutOfViewport to how it was originally as it worked correctly. I had my coordinate system mixed up. Sorry! Edited the title and description accordingly.

But the fix for _scrollToTopIfVisible should still be valid.

[magento-engcom-team]

Hi @sidolov, thank you for the review.
ENGCOM-5362 has been created to process this Pull Request

[magento-engcom-team]

@oskarolaussen thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

[engcom-Delta]

✔️ QA passed

[m2-assistant]

Hi @oskarolaussen, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

[Karlasa]

@magento-engcom-team I think this PR should be reverted.

1. It was working as intended before. No need to position screen with each element change. (maybe just function name is odd)
2. fix for this PR introduces more issues on page load with collapsible element that has default state open.

[vovayatsyuk]

I'm not happy with this update too.

Now the whole page is jumping each time I click the tab 😕
Layered navigation is affected too (As well as all other collapsible elements).

Tabs	Filters

I think that the _scrollToTopIfVisible was written as _scrollToTopIfNotVisible intentionally (It was just an invalidly named function). I can't imagine why you would need to scroll the page if the content is already visible.

[vovayatsyuk]

p.s. Do you realize that now scroll will not work when the element is outside of the viewport?

[oskarolaussen]

I encountered a situation where the previous function caused undesired behaviour and since the name didn't match the logic I decided to "fix" it. As it seems to cause more problems than it fixes I'm fully onboard with reverting this PR. Sorry about the inconvenience.