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

Add missing mouseover before click for select and radio elements. #14

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

vever001
Copy link

Following #10 and #13 logic, I noticed radios and select elements also require a scrollIntoView which are currently missing. Without this, we get MoveTargetOutOfBounds exception when attempting to click on select/radios which are out of the viewport.

@alexpott
Copy link
Collaborator

@vever001 thanks for the MR. Any chance you could add test coverage for this? Have a look in \Behat\Mink\Tests\Driver\Custom\LargePageClickTest and the mink driver-test-suite dependency.

@vever001
Copy link
Author

vever001 commented Aug 20, 2024

@alexpott

I'm a bit confused... I tried to reproduce with tests initially but couldn't.
I then tried with the form from my project and found out that size attributes on form elements might be the cause.

The test passes if we comment out the JS that adds the attribute.
But when we uncomment that line (adding size="200" on the first_name text element) then we get
WebDriver\Exception\ElementClickIntercepted: Element <input name="sex" type="radio"> is not clickable at point (516,811) because another element <html> obscures it

It looks like scrollIntoView block: 'center' fixes it but I'm not sure what is going on exactly...

@@ -953,7 +953,7 @@ public function mouseOver(string $xpath)
private function scrollElementIntoView(Element $element): void {
$script = <<<JS
var e = arguments[0];
e.scrollIntoView({ behavior: 'instant', block: 'end', inline: 'nearest' });
e.scrollIntoView({ behavior: 'instant', block: 'center', inline: 'nearest' });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking about this change. I'm wondering why it is necessary?

Copy link
Author

@vever001 vever001 Aug 26, 2024

Choose a reason for hiding this comment

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

I tried the fix from #15 but it didn't solve the issue.
I'll spend some more time shortly to try to understand what's going on but I'm not sure the issue is specific to my project, I think it's an edge case (with size attributes?).
If we revert this line, the test fails.

Choose a reason for hiding this comment

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

@vever001
#15 does not scroll for you. it only prevents the re-scrolling. If you apply #15, you would need to scroll to the element before making an assertion.

Choose a reason for hiding this comment

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

We faced similar issue after upgrading Drupal 10.2 to 10.3, where behat/mink-selenium2-driver got replaced with this library. The tests started to fail and patching the e.scrollIntoView() block argument to center fixed the issues of layouts with floating toolbar receiving the press/click events.

@alexpott
Copy link
Collaborator

I think maybe in this instance this might be very related to #15 - and given the way your project works maybe it is your projects job to do the necessary scrolling. It's really tricky when people overlap elements that are suppose to receive clicks.

@vever001
Copy link
Author

I just hit this on another project. This happens somewhat intermittently for elements where the browser has to scroll down. I think it's either:

  • a bug or edge case from chrome/firefox with scrollIntoView({ block: 'end' }} as using block: center fixes it (but maybe it's unlikely that both chrome and firefox have the same bug)
  • or it's not a browser bug and there is the tiniest layout shift happening after the scroll, causing the element to go just what it needs to be off-page for selenium not to be able to click it (we get: "move target out of bounds"). Meaning block: 'end' is brittle on this (layout shifts) and using block: 'center' might be safer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants