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

Unable to switch to frame on Selenium 3.x #866

Closed
aik099 opened this issue Feb 13, 2024 · 8 comments
Closed

Unable to switch to frame on Selenium 3.x #866

aik099 opened this issue Feb 13, 2024 · 8 comments

Comments

@aik099
Copy link
Member

aik099 commented Feb 13, 2024

Short story:

After a #856 change (included in Mink 1.11.0 release) the switchToIFrame method signature was massively (DriverInterface, CoreDriver, Selenium2Driver, etc.) changed:

  • from public function switchToIFrame($name = null);
  • to public function switchToIFrame(?string $name = null).

This has broken any code like $session->switchToIFrame(2);, because PHP internally casts frame index to string and Selenium gets {"id":"2"} instead of {"id":2} in it's /session/:sessionId/frame command and frame switching fails.

P.S.
I'm posting an issue here because it affects both Mink's own, the test suite and driver files.

Possible solution:

  • (dirty) Either make the \Behat\Mink\Driver\Selenium2Driver::switchToIFrame method a bit clever and if a given $name is actually a number, then cast it to an integer (if we still have other frame-aware drivers, then this applies to them as well)
  • (clean) Or revert relevant switchToIFrame method signature changes, because $name can't be declared as both integer and string (in supported PHP versions).

In either case, the \Behat\Mink\Tests\Driver\Basic\IFrameTest::testIFrame test needs to be improved with index-based frame-switching support.

Long story

According to JSON Wire Protocol you can switch frame in these ways (Mink code shown):

  • $session->switchToIFrame('frame_name'); - by frame name
  • $session->switchToIFrame(2); - by frame index

In real-life tests, however, the results are the following:

  • the Selenium 2.x + Chrome can switch frames by both name & index (used in driver test suite)
  • the Selenium 2.x + Firefox - was unable even to open a Firefox window locally
  • the Selenium 3.x + Chrome/Firefox can switch frames only by index
  • the Selenium 3.x + Chrome/Firefox returns an error when attempting to switch a frame by name (not sure why)
Firefox:
data did not match any variant of untagged enum FrameId at line 1 column 13
Chrome:
invalid argument: 'id' can not be string

// cc: @stof

Updates:

@aik099
Copy link
Member Author

aik099 commented Feb 18, 2024

(clean) Or revert relevant switchToIFrame method signature changes, because $name can't be declared as both integer and string (in supported PHP versions).

Implemented in these PRs:

  1. Allow specify frame index in the "switchToIFrame" driver/session method #867 (merge this first)
  2. Added tests to verify, that iframe switching by an id works driver-testsuite#87 (rerun tests to see they now pass and then merge)
  3. Allow specify frame index in the "switchToIFrame" driver method MinkSelenium2Driver#378
  4. Allow specify frame index in the "switchToIFrame" driver method webdriver-classic-driver#11

I've also updated relevant DocBlocks to indicate, that frame index can also be specified.

Probably the $name parameter of the switchToIFrame methods should be renamed into the $nameOrIndex to make IDE code assist a bit more helpful.

@uuf6429
Copy link
Member

uuf6429 commented Feb 22, 2024

Before calling this a bug, let's be clear - it was never defined that we can switch an iframe by index, as can be seen by your update of that comment:
https://github.com/minkphp/Mink/pull/867/files#diff-d76d9e3222ed852fe6951aa849d5e7c111042b6a5209a9fe6b53c0c7c7530735R187

I'm fine with supporting this though, just saying that any old usages with integers were misuses.

@aik099
Copy link
Member Author

aik099 commented Feb 22, 2024

@uuf6429 , I'm not saying it's a bug. I'm saying that method signature change has prevented certain (might not be documented) Mink usage scenarios from working.

Since method DocBlock always was string|null $name, then nobody likely thought that specifying a frame index was actually an option. However, due to WebDriver ->frame(...) being used internally, it worked perfectly.

P.S.

  1. Frame switching by name works with Selenium 3 + https://github.com/minkphp/webdriver-classic-driver .
  2. I really want to know why it doesn't with https://github.com/minkphp/MinkSelenium2Driver (the bug in the https://github.com/instaclick/php-webdriver maybe).

@aik099
Copy link
Member Author

aik099 commented Feb 22, 2024

Update: with the help of the @uuf6429 , I was able to get frame switching by name working on Selenium 3 (see minkphp/MinkSelenium2Driver#382).

@aik099
Copy link
Member Author

aik099 commented Feb 22, 2024

@stof , I only need a review from you. The merging and build rerunning I can do myself to save your time.

Also please look at the minkphp/MinkSelenium2Driver#382.

@stof
Copy link
Member

stof commented Feb 22, 2024

I don't think using an integer index was ever meant to be supported in the Mink API. It worked in Selenium2Driver in the past only due to the lack of native parameter types that allowed to ignore the intended method signature.

If we were adding support for integer indexes in the Mink API, we would have to specify the meaning of those indexes. And if we say "this is the index in WebDriver Classic", this would not impossible to implement in any driver not based on the WebDriver protocol.

As you managed to fix the issue in the Selenium2Driver to actually support names, I suggest closing this issue.

@uuf6429
Copy link
Member

uuf6429 commented Nov 10, 2024

@aik099 if all points have been resolved, please consider closing this issue. Thanks!

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

No branches or pull requests

3 participants