chore(bidi): pointerMove action needs to floor fractional values for x and y position #34191
Conversation
This comment has been minimized.
This comment has been minimized.
| @@ -77,8 +77,8 @@ export class RawMouseImpl implements input.RawMouse { | |||
|
|
|||
| async move(x: number, y: number, button: types.MouseButton | 'none', buttons: Set<types.MouseButton>, modifiers: Set<types.KeyboardModifier>, forClick: boolean): Promise<void> { | |||
| // Bidi throws when x/y are not integers. | |||
| x = Math.round(x); | |||
| y = Math.round(y); | |||
| x = Math.floor(x); | |||
There was a problem hiding this comment.
There is already a block that fixes the same problem for Firefox, accounting for all kinds of oddly shaped elements: https://github.com/microsoft/playwright/blob/main/packages/playwright-core/src/server/dom.ts#L269. Perhaps we should generalize that check through PageDelegate.shouldClickAtIntegerCoordinates()?
There was a problem hiding this comment.
This referenced command in the DOM module is using Content Quads to calculate the position. This is a feature that is not yet supported in WebDriver BiDi. So I feel it might be inappropriate to be used right now. If you agree I could at least add a comment to my changes to reference that other method for future adaption once quads are supported.
There was a problem hiding this comment.
Even though content quads are not supported in BiDi yet, there is a polyfill in BidiPage for it. Otherwise, click won't work in Playwright at all. Therefore, we can still assume there are some content quads available and perform smart integer rounding, wdyt?
There was a problem hiding this comment.
Searching for an integer point in a quad is a separate issue. This PR is good per se as it aligns the logic with what we have in ffInput.ts where we use floor rather than round. Let's land it.
This comment has been minimized.
This comment has been minimized.
|
@whimboo Do you plan to update this PR or should we close it? |
Ideally yes, but I'm lacking time at the moment. Could someone from your side assist and finish this patch? I would appreciate. |
| @@ -77,8 +77,8 @@ export class RawMouseImpl implements input.RawMouse { | |||
|
|
|||
| async move(x: number, y: number, button: types.MouseButton | 'none', buttons: Set<types.MouseButton>, modifiers: Set<types.KeyboardModifier>, forClick: boolean): Promise<void> { | |||
| // Bidi throws when x/y are not integers. | |||
| x = Math.round(x); | |||
| y = Math.round(y); | |||
| x = Math.floor(x); | |||
There was a problem hiding this comment.
Searching for an integer point in a quad is a separate issue. This PR is good per se as it aligns the logic with what we have in ffInput.ts where we use floor rather than round. Let's land it.
…x and y position. For elements that are only 1 pixel in size, rounding can cause the click to occur outside the element’s rect. For example, a 1px div at (8,8) would not be reached if the click is rounded to (9,9).
yury-s
force-pushed
the
bidi-perform-action-move-fractional
branch
from
8af1764 to
c062552
Compare
|
I've pushed rebased version to whimboo:bidi-perform-action-move-fractional |
|
On a closer look, the problem is specific to Firefox implementation of Bidi as Chromium does not throw on fractional coordinates and handles them just fine (as underlying CDP implementation does, I guess). I see that in the spec the coordinates are typed as |
Test results for "tests 1"11 flaky37801 passed, 655 skipped Merge workflow run. |
For elements that are only 1 pixel in size, rounding can cause the click to occur outside the element’s rect.
For example, a 1px div at (8,8) would not be reached if the click is rounded to (9,9).
This will fix the test
page/page-click.spec.ts :: should click the aligned 1x1 div.@yury-s can you please review? Thanks.