Skip to content

[1.15] Fix precision loss (int vs float mixup) #655

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

Open
wants to merge 5 commits into
base: 1.13
Choose a base branch
from
Open

[1.15] Fix precision loss (int vs float mixup) #655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b797ba0
fix precision loss (int vs float mixup)
divinity76 Oct 25, 2024
3c6ca8b
doc
divinity76 Oct 25, 2024
381b33f
Merge branch '1.13' into patch-11
divinity76 May 28, 2025
b1a1b4b
StyleCI
divinity76 May 28, 2025
436b052
Update MouseApiTest.php
divinity76 May 28, 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
20 changes: 10 additions & 10 deletions src/Input/Mouse.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ class Mouse
*/
protected $page;

protected $x = 0;
protected $y = 0;
protected $x = 0.0;
protected $y = 0.0;

protected $button = self::BUTTON_NONE;

Expand All @@ -45,16 +45,16 @@ public function __construct(Page $page)
}

/**
* @param int $x
* @param int $y
* @param float $x
* @param float $y
* @param array|null $options
*
* @throws \HeadlessChromium\Exception\CommunicationException
* @throws \HeadlessChromium\Exception\NoResponseAvailable
*
* @return $this
*/
public function move(int $x, int $y, ?array $options = null)
public function move(float $x, float $y, ?array $options = null)
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change, when the caller has strict types enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

commented on wrong line, but yes. the actual api break is on line 34, 35 and 338.
strict_types have an exception for implicit int->float, so line 57 is not an api break: https://3v4l.org/gS1b7
thus function move can be upgraded to support float without breaking api,
but function getPosition() and getMaximumDistance() cannot.

Would you prefer to only fix the

Implicit conversion from float -0.666748046875 to int loses precision

issue, at least for the remainder of v1?

Copy link
Member

Choose a reason for hiding this comment

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

Why is getMaximumDistance a breaking change if it's a private method? And getPosition returns an array with no defined types, not even in the docblock.

Copy link
Contributor Author

@divinity76 divinity76 May 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@enricodias

Why is getMaximumDistance a breaking change if it's a private method?

sorry you're right, getMaximumDistance is not a breaking change, my bad.

And getPosition returns an array with no defined types, not even in the docblock.

that's kinda a api-break because code that expect $page->mouse()->getPosition()['y'] to be int may be disappointed. (it broke tests/MouseApiTest.php line 85 test case)

IMO it's a minor API break tho. getPosition()=>["x"=>int,"y"=>int] becomes ["x"=>float,"y"=>float].

{
$this->page->assertNotClosed();

Expand Down Expand Up @@ -329,13 +329,13 @@ public function findElement(Selector $selector, int $position = 1): self
/**
* Get the maximum distance to scroll a page.
*
* @param int $distance Distance to scroll, positive or negative
* @param int $current Current position
* @param int $maximum Maximum possible distance
* @param float $distance Distance to scroll, positive or negative
* @param float $current Current position
* @param float $maximum Maximum possible distance
*
* @return int allowed distance to scroll
* @return float allowed distance to scroll
*/
private function getMaximumDistance(int $distance, int $current, int $maximum): int
private function getMaximumDistance(float $distance, float $current, float $maximum): float
{
$result = $current + $distance;

Expand Down
14 changes: 7 additions & 7 deletions tests/MouseApiTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,23 +82,23 @@ public function testScroll(): void
$windowScrollY = $page->evaluate('window.scrollY')->getReturnValue();

self::assertSame(100, $windowScrollY);
self::assertSame(100, $page->mouse()->getPosition()['y']);
self::assertSame(100.0, $page->mouse()->getPosition()['y']);

// scrolling 100px up should revert the last action
$page->mouse()->scrollUp(100);

$windowScrollY = $page->evaluate('window.scrollY')->getReturnValue();

self::assertSame(0, $windowScrollY);
self::assertSame(0, $page->mouse()->getPosition()['y']);
self::assertSame(0.0, $page->mouse()->getPosition()['y']);

// try to scroll more than possible
$page->mouse()->scrollDown(10000);

$windowScrollY = $page->evaluate('window.scrollY')->getReturnValue();

self::assertLessThan(10000, $windowScrollY);
self::assertLessThan(10000, $page->mouse()->getPosition()['y']);
self::assertLessThan(10000.0, $page->mouse()->getPosition()['y']);
}

/**
Expand Down Expand Up @@ -265,10 +265,10 @@ public function testGetPosition(): void
$x = $page->mouse()->getPosition()['x'];
$y = $page->mouse()->getPosition()['y'];

self::assertGreaterThanOrEqual(1, $x); // 8
self::assertLessThanOrEqual(51, $x);
self::assertGreaterThanOrEqual(1.0, $x); // 8
self::assertLessThanOrEqual(51.0, $x);

self::assertGreaterThanOrEqual(1, $y); // 87
self::assertLessThanOrEqual(107, $y);
self::assertGreaterThanOrEqual(1.0, $y); // 87
self::assertLessThanOrEqual(107.0, $y);
}
}