Skip to content

Make switch to frame possible #9

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
May 31, 2020
Merged

Make switch to frame possible #9

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7e87dbb
Make switch to frame possible
Sieg Mar 19, 2020
443881e
Fix tag name in debug information
Sieg Mar 19, 2020
0c40752
Merge branch 'master' into master
Naktibalda May 31, 2020
9da9996
Correct description of switchToIframe
Naktibalda May 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 45 additions & 6 deletions src/Codeception/Module/WebDriver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2515,7 +2515,7 @@ public function switchToWindow($name = null)
}

/**
* Switch to another frame on the page.
* Switch to another iframe on the page.
*
* Example:
* ``` html
Expand All @@ -2538,23 +2538,62 @@ public function switchToWindow($name = null)
*/
public function switchToIFrame($locator = null)
{
if (is_null($locator)) {
$this->findAndSwitchToFrame($locator, 'iframe');
}

/**
* Switch to another frame on the page.
*
* Example:
* ``` html
* <frame name="another_frame" id="fr1" src="http://example.com">
*
* ```
*
* ``` php
* <?php
* # switch to frame by name
* $I->switchToFrame("another_frame");
* # switch to frame by CSS or XPath
* $I->switchToFrame("#fr1");
* # switch to parent page
* $I->switchToFrame();
*
* ```
*
* @param string|null $locator (name, CSS or XPath)
*/
public function switchToFrame($locator = null)
{
$this->findAndSwitchToFrame($locator, 'frame');
}

/**
* @param string|null $locator
* @param string $tag
*/
private function findAndSwitchToFrame($locator = null, $tag = 'frame')
{
if ($locator === null) {
$this->webDriver->switchTo()->defaultContent();
return;
}
$els = null;
try {
$els = $this->_findElements("iframe[name='$locator']");
Copy link
Member

Choose a reason for hiding this comment

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

I think that it would be better to make switchToIFrame behave as before by changing locator to match frames and iframes:

$els = $this->_findElements("iframe[name='$locator']  | frame[name='$locator']");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would not say so, as the name of method is missleading, it have IFrame in it, so it should work with iframes, even if it was possible to be used with frames before. Or, the name should be changed to switchToFrame then, and used for iframes as well. But thats just my opinion, would be better to vote for it probably :)

Copy link
Member

Choose a reason for hiding this comment

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

Changing the name is backwards compatibility break,
making switchToIframe work with frames as before is backwards compatibility fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but:

  • Making it working as before will make the method misused again. The fix should be introduced, but with the deprecation of this functionality maybe?
  • Renaming could be done with the deprecation of the old method as well, it's not critical to just rename it.

Sure, it is your decision how to handle this question, but me personally would prefer the chaos to be more organized with the time, but not just fixed to be working somehow :)

Copy link
Member

Choose a reason for hiding this comment

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

@DavertMik thinks that switchToIframe working with frames is a feature, not a bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Naktibalda But it leads to misunderstandings while it does more than its described, and then people can change the code without even thinking about those "features" and one day those are stopping to work. So either you need two methods or better documentation/implementation on the old one. Feel free to do as you think is better.

$els = $this->_findElements("{$tag}[name='$locator']");
} catch (\Exception $e) {
$this->debug('Failed to find locator by name: ' . $e->getMessage());
}
if (!is_array($els) || !count($els)) {
$this->debug('Iframe was not found by name, locating iframe by CSS or XPath');

if (!isset($els) || !is_array($els) || !count($els)) {
$this->debug(ucfirst($tag) . ' was not found by name, locating ' . $tag . ' by CSS or XPath');
$els = $this->_findElements($locator);
}

if (!count($els)) {
throw new ElementNotFound($locator, 'Iframe');
throw new ElementNotFound($locator, ucfirst($tag));
}

$this->webDriver->switchTo()->frame($els[0]);
}

Expand Down