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.

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

Use interfaces as much as possible; switch to actions convenience class #59

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

Use interfaces as much as possible; switch to actions convenience class #59

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
391db2d
Use interfaces as much as possible; switch to actions convenience class
uuf6429 Jan 4, 2025
eed87e4
Use element click instead of action
uuf6429 Jan 4, 2025
75eac8b
CR fixes
uuf6429 Jan 7, 2025
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
8 changes: 7 additions & 1 deletion phpstan.dist.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,13 @@ parameters:
count: 1
path: src/WebdriverClassicDriver.php
-
message: '#^Parameter \#1 \$handle of method Facebook\\WebDriver\\Remote\\RemoteTargetLocator\:\:window\(\) expects string, mixed given\.$#'
message: '#^Parameter \#1 \$handle of method Facebook\\WebDriver\\WebDriverTargetLocator\:\:window\(\) expects string, mixed given\.$#'
identifier: argument.type
count: 3
path: src/WebdriverClassicDriver.php
-
# See https://github.com/php-webdriver/php-webdriver/blob/998e499b786805568deaf8cbf06f4044f05d91bf/lib/WebDriverElement.php#L43
message: '#^Call to an undefined method Facebook\\WebDriver\\Internal\\WebDriverLocatable\&Facebook\\WebDriver\\WebDriverElement\:\:getDomProperty\(\)\.$#'
identifier: method.notFound
count: 1
path: src/WebdriverClassicDriver.php
121 changes: 71 additions & 50 deletions src/WebdriverClassicDriver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,18 @@
use Facebook\WebDriver\Exception\TimeoutException;
use Facebook\WebDriver\Exception\UnsupportedOperationException;
use Facebook\WebDriver\Exception\WebDriverException;
use Facebook\WebDriver\Interactions\WebDriverActions;
use Facebook\WebDriver\Internal\WebDriverLocatable;
Copy link
Member

Choose a reason for hiding this comment

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

The Internal namespace suggests, that this class/interface shouldn't be used outside of the library and therefore not included in BC promise. But maybe I'm mistaken.

use Facebook\WebDriver\JavaScriptExecutor;
use Facebook\WebDriver\Remote\DesiredCapabilities;
use Facebook\WebDriver\Remote\RemoteWebDriver;
use Facebook\WebDriver\Remote\RemoteWebElement;
use Facebook\WebDriver\Remote\WebDriverBrowserType;
use Facebook\WebDriver\Remote\WebDriverCapabilityType;
use Facebook\WebDriver\WebDriver;
use Facebook\WebDriver\WebDriverBy;
use Facebook\WebDriver\WebDriverDimension;
use Facebook\WebDriver\WebDriverElement;
use Facebook\WebDriver\WebDriverHasInputDevices;
use Facebook\WebDriver\WebDriverPlatform;
use Facebook\WebDriver\WebDriverRadios;
use Facebook\WebDriver\WebDriverSelect;
Expand All @@ -35,7 +39,9 @@
* @phpstan-type TTimeouts array{script?: null|numeric, implicit?: null|numeric, page?: null|numeric, "page load"?: null|numeric, pageLoad?: null|numeric}
* @phpstan-type TCapabilities array<string, mixed>
* @phpstan-type TElementValue array<array-key, mixed>|bool|mixed|string|null
* @phpstan-type TWebDriverInstantiator callable(string $driverHost, DesiredCapabilities $capabilities): RemoteWebDriver
* @phpstan-type TWebDriver WebDriver&JavaScriptExecutor&WebDriverHasInputDevices
* @phpstan-type TWebDriverElement WebDriverElement&WebDriverLocatable
* @phpstan-type TWebDriverInstantiator callable(string $driverHost, DesiredCapabilities $capabilities): TWebDriver
*/
class WebdriverClassicDriver extends CoreDriver
{
Expand Down Expand Up @@ -77,7 +83,10 @@

private const W3C_WINDOW_HANDLE_PREFIX = 'w3cwh:';

private ?RemoteWebDriver $webDriver = null;
/**
* @phpstan-var TWebDriver|null
*/
private ?WebDriver $webDriver = null;

private string $browserName;

Expand Down Expand Up @@ -210,7 +219,7 @@
public function switchToIFrame(?string $name = null): void
{
$frameQuery = $name;
if ($name && $this->getWebDriver()->isW3cCompliant()) {
if ($name && $this->isW3cCompliant()) {
try {
$frameQuery = $this->getWebDriver()->findElement(WebDriverBy::id($name));
} catch (NoSuchElementException $e) {
Expand Down Expand Up @@ -569,14 +578,18 @@
#[Language('XPath')]
string $xpath
): void {
$this->doubleClickOnElement($this->findElement($xpath));
$element = $this->findElement($xpath);
$element->getLocationOnScreenOnceScrolledIntoView();
$this->actions()->doubleClick($element)->perform();
}

public function rightClick(
#[Language('XPath')]
string $xpath
): void {
$this->rightClickOnElement($this->findElement($xpath));
$element = $this->findElement($xpath);
$element->getLocationOnScreenOnceScrolledIntoView();
$this->actions()->contextClick($element)->perform();
}

public function attachFile(
Expand All @@ -601,7 +614,9 @@
#[Language('XPath')]
string $xpath
): void {
$this->mouseOverElement($this->findElement($xpath));
$element = $this->findElement($xpath);
$element->getLocationOnScreenOnceScrolledIntoView();
$this->actions()->moveToElement($element)->perform();
}

public function focus(
Expand Down Expand Up @@ -656,7 +671,7 @@
): void {
$source = $this->findElement($sourceXpath);
$destination = $this->findElement($destinationXpath);
$this->getWebDriver()->action()->dragAndDrop($source, $destination)->perform();
$this->actions()->dragAndDrop($source, $destination)->perform();
}

public function executeScript(
Expand Down Expand Up @@ -751,8 +766,8 @@
*/
public function getWebDriverSessionId(): ?string
{
return $this->isStarted()
? $this->getWebDriver()->getSessionID()
return $this->isStarted() && method_exists($this->getWebDriver(), 'getSessionID')
? $this->getAsString($this->getWebDriver()->getSessionID(), 'Session ID')
: null;
}

Expand Down Expand Up @@ -789,15 +804,16 @@
}

/**
* @phpstan-return TWebDriver
* @throws DriverException
*/
protected function getWebDriver(): RemoteWebDriver
protected function getWebDriver(): WebDriver
{
if ($this->webDriver) {
return $this->webDriver;
if (!$this->webDriver) {
throw new DriverException('Base driver has not been created');
}

throw new DriverException('Base driver has not been created');
return $this->webDriver;
}

// </editor-fold>
Expand Down Expand Up @@ -886,6 +902,15 @@
}
}

/**
* @throws DriverException
*/
private function actions(): WebDriverActions
{
// WebDriverActions are not reset after being performed - that's why we create a new instance each time.
return new WebDriverActions($this->getWebDriver());
}

/**
* @throws DriverException
*/
Expand Down Expand Up @@ -963,11 +988,12 @@
* $this->executeJsOnElement($element, 'return argument[0].childNodes.length');
* ```
*
* @phpstan-param TWebDriverElement $element
* @return mixed
* @throws DriverException
*/
private function executeJsOnElement(
WebDriverElement $element,
$element,
#[Language('JavaScript')]
string $script
) {
Expand Down Expand Up @@ -1040,37 +1066,14 @@
}
}

private function clickOnElement(WebDriverElement $element): void
{
$element->getLocationOnScreenOnceScrolledIntoView();
$element->click();
}

/**
* @phpstan-param TWebDriverElement $element
* @throws DriverException
*/
private function doubleClickOnElement(RemoteWebElement $element): void
private function clickOnElement($element): void
{
$element->getLocationOnScreenOnceScrolledIntoView();
$this->getWebDriver()->getMouse()->doubleClick($element->getCoordinates());
}

/**
* @throws DriverException
*/
private function rightClickOnElement(RemoteWebElement $element): void
{
$element->getLocationOnScreenOnceScrolledIntoView();
$this->getWebDriver()->getMouse()->contextClick($element->getCoordinates());
}

/**
* @throws DriverException
*/
private function mouseOverElement(RemoteWebElement $element): void
{
$element->getLocationOnScreenOnceScrolledIntoView();
$this->getWebDriver()->getMouse()->mouseMove($element->getCoordinates());
$element->click();
Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to be consistent with the other actions by using this:

$this->actions()->click($element)->perform();

but for some strange reason, it does not work well in firefox, failing many tests:
https://github.com/minkphp/webdriver-classic-driver/actions/runs/12611297356/job/35146807053

I thought it might be because of the geckdriver-specific fix in RemoteWebElement->click(), but it does not seem to be the case.

My last remaining guess is that DriverCommand::CLICK_ELEMENT also waits for the page to finish loading, whereas DriverCommand::ACTIONS(W3C)/DriverCommand::CLICK(JWP) do not wait.

It may be useful to figure out exactly why, it might be the reason behind various fragile tests we have.

Copy link
Member

Choose a reason for hiding this comment

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

You don't need to guess. Just do real-time debugging inside a WebDriver code (if you can reproduce a problem locally).

Copy link
Member Author

Choose a reason for hiding this comment

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

This happens within selenium (and possibly the webbrowser). DriverCommand::CLICK_ELEMENT and DriverCommand::ACTIONS(W3C)/DriverCommand::CLICK(JWP) produce a different request to selenium. Of course I'll dig deeper..

}

/**
Expand Down Expand Up @@ -1100,24 +1103,28 @@
}

/**
* @phpstan-return TWebDriverElement
* @throws DriverException
*/
private function findElement(
#[Language('XPath')]
string $xpath
): RemoteWebElement {
) {
try {
$finder = WebDriverBy::xpath($xpath);
return $this->getWebDriver()->findElement($finder);
$element = $this->getWebDriver()->findElement($finder);
assert($element instanceof WebDriverLocatable);
return $element;
} catch (\Throwable $e) {
throw new DriverException("Failed to find element: {$e->getMessage()}", 0, $e);
}
}

/**
* @phpstan-param TWebDriverElement $element
* @throws DriverException
*/
private function selectRadioValue(WebDriverElement $element, string $value): void
private function selectRadioValue($element, string $value): void
{
try {
(new WebDriverRadios($element))->selectByValue($value);
Expand All @@ -1133,9 +1140,10 @@
}

/**
* @phpstan-param TWebDriverElement $element
* @throws DriverException
*/
private function selectOptionOnElement(WebDriverElement $element, string $value, bool $multiple = false): void
private function selectOptionOnElement($element, string $value, bool $multiple = false): void
{
try {
$select = new WebDriverSelect($element);
Expand Down Expand Up @@ -1163,9 +1171,10 @@
*
* Note: this implementation does not trigger a change event after deselecting the elements.
*
* @phpstan-param TWebDriverElement $element
* @throws DriverException
*/
private function deselectAllOptions(WebDriverElement $element): void
private function deselectAllOptions($element): void
{
try {
(new WebDriverSelect($element))->deselectAll();
Expand All @@ -1180,10 +1189,11 @@
}

/**
* @phpstan-param TWebDriverElement $element
* @throws DriverException
*/
private function ensureInputType(
WebDriverElement $element,
$element,
#[Language('XPath')]
string $xpath,
string $type,
Expand Down Expand Up @@ -1223,10 +1233,11 @@
}

/**
* @phpstan-param TWebDriverElement $element
* @param mixed $value
* @throws DriverException
*/
private function setElementDomProperty(WebDriverElement $element, string $property, $value): void
private function setElementDomProperty($element, string $property, $value): void
{
$this->executeJsOnElement(
$element,
Expand All @@ -1235,13 +1246,14 @@
}

/**
* @phpstan-param TWebDriverElement $element
* @return mixed
* @throws DriverException
*/
private function getElementDomProperty(RemoteWebElement $element, string $property)
private function getElementDomProperty($element, string $property)
{
try {
return $this->getWebDriver()->isW3cCompliant()
return $this->isW3cCompliant()
? $element->getDomProperty($property)
: $this->executeJsOnElement($element, "return arguments[0]['$property']");
} catch (UnsupportedOperationException $e) {
Expand Down Expand Up @@ -1270,5 +1282,14 @@
return (string)$value;
}

private function isW3cCompliant(): bool
{
if (!method_exists($this->getWebDriver(), 'isW3cCompliant')) {
throw new DriverException('Base driver must implement an `isW3cCompliant` method that returns a boolean.');

Check warning on line 1288 in src/WebdriverClassicDriver.php

View check run for this annotation

Codecov / codecov/patch

src/WebdriverClassicDriver.php#L1288 

Added line #L1288 was not covered by tests
}

return (bool)$this->getWebDriver()->isW3cCompliant();
}

// </editor-fold>
}
Loading