Fixed scrolling an element into view only if it is not within a view.

[AlexSkrypnyk]

Fixes an issue introduced in #10: the scrolling an element into the view should only take place if the element is not already within the view.

[alexpott]

Thanks @AlexSkrypnyk for filing the PR. Can you tell me the situation where this is necessary. I borrowed the JS and approach from https://github.com/minkphp/webdriver-classic-driver/blob/9567c6e77c2b89dcdc5a5f567ce826c1a831de1f/src/WebdriverClassicDriver.php#L981 and https://github.com/php-webdriver/php-webdriver/blob/11923b4ba312fc9d137266ae67438be65d4d500f/lib/Remote/RemoteWebElement.php#L250

[AlexSkrypnyk]

When there is a bottom fixed bar and the test already has scrolling to the element to offset that (via a custom code), calling a click will reset this adjusted position instead of respecting it.

[alexpott]

@AlexSkrypnyk thanks for the reply. I have further questions; and then what happens? Does the test fail for some reason? Is there a reason the test is so dependent on the position of the page?

[AlexSkrypnyk]

@alexpott

1. The page has a bottom bar of fixed 100 pixels in height. The bar is absolutely positioned to the bottom of viewport with position: fixed.
2. The code in this package without this PR scrolls the page to the position where the element top is just above viewport bottom side.
3. The element is now behind the bottom bar.
4. Clicking the element will result an exception that "another element will receive a click". The bottom bar will receive a click instead of the element that we are trying to click, because it is "behind" the bar on z-axis.

Is there a reason the test is so dependent on the position of the page?
Handling of the bottom fixed bar means that an offset has to be considered. The test must be scrolling the viewport to the amount of the offset of the fixed bar as this is what a user would do.

A manual step is added to scroll the viewport to the amount of the height of the bottom bar so that the element that should receive a click would be vertically above the bottom bar and would now receive a click.

With a change introduced in #10, the offset that a manual step sets, being reset because the code just performs a scroll to the element without assessing if such a scroll is needed at all (it is not needed if the element is already within a viewport).

This PR corrects the code in #10 to consider if the current element is currently within a viewport and only scrolls to it if it is not.

[alexpott]

@AlexSkrypnyk thanks for all the info. For me this means that we should change the scrollIntoView to make sure it scrolls till the element is visible. Then you wouldn't need your workaround and the system would be more robust. This is what Cypress does for example - see https://docs.cypress.io/guides/core-concepts/interacting-with-elements#Scrolling

[AlexSkrypnyk]

Determining if the element is visible is not the same as determining if the element will receive a click. Also, the page may have more than one fixed bar and it can be vertical as well. So allowing a test to control the scroll outside of the click step gives more control to the developer, easier to implement and goes inline with the paradigm than a test should replicate what a user would do (like scrolling to the element instead of auto-scrolling).

I think my solution to only scroll to an element if it is not within a viewport is simple enough to handle majority of the cases without the need to update existing tests.

Alternatively, you could move the auto-scroll added in #10 under a feature flag and let a developer decide if they want to use it.

[AlexSkrypnyk]

Creating heuristics to determine if an element is clickable will be extremely hard because you would need to recursively consider every element on the page that may or may not be in the way of a click, which means that you would need to calculate a visibility of every element, which in own turn is not that easy once you start considering what “visible” means.

Relying solely on this algorithm without allowing to override it will still not allow to let a developer control the scroll when they need to.

[alexpott]

@AlexSkrypnyk okay I'm convinced. This is very similar to https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoViewIfNeeded

I would be great to add some text coverage of this behaviour if you can. I think all we need is a test confirming that clicking on an element does not scroll the page if it is already in window.

[vever001]

FWIW I noticed that this can have side effects when there are overlays (like the drupal admin toolbar).
The changes here means that a button under the toolbar is in view, and when selenium attempts to click on it we get "another element would receive the click" error.
At the moment it calls scrollIntoView unconditionally, and the button goes to the bottom of the window. But I understand it can be an issue when there are bottom overlays.

What about using scrollIntoView with block: center, this would also prevent small layout shifts to cause issues (which is what I think I'm encountering as explained in #14 (comment)).