Skip to content

[Set Window Rect] Change parameter requirement from Number to Integer and range to "maximum safe integer" #1909

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

yezhizhen
Copy link
Contributor

@yezhizhen yezhizhen commented Jul 5, 2025

The spec says Number, which is "double-precision 64-bit binary format IEEE 754 value". But the wpt test below clearly requires integer.

https://github.com/web-platform-tests/wpt/blob/b281d07f3ead48995b9a2e94259d292e22cd5dd9/webdriver/tests/classic/set_window_rect/set.py#L41-L49

This PR adds definition of minimum safe integer and change the valid range of parameter.


Preview | Diff

index.html Outdated
@@ -4058,11 +4058,11 @@ <h4><dfn>Set Window Rect</dfn></h4>
<li><p>If <var>y</var> is <a>undefined</a>, let <var>y</var> be null.

<li><p>If <var>width</var> or <var>height</var> is neither null, nor
a <a>Number</a> from 0 to 2<sup>31</sup> − 1, return <a>error</a>
an integer from 0 to 2<sup>31</sup> − 1, return <a>error</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

Good find. But lets take one more thing into account. In WebDriver BiDi we actually use a range from 0 to maximum safe integer. We probably should do the same here as well even though those high values won't be able to get applied. The same applies for x and y.

Copy link
Contributor Author

@yezhizhen yezhizhen Jul 7, 2025

Choose a reason for hiding this comment

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

In WebDriver BiDi we actually use a range from 0 to maximum safe integer.

Can you show a reference of this case in BiDi? I cannot find it anywhere.

I only see usage of "maximum safe integer" in Add Cookie/Timeout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same applies for x and y.

https://github.com/mozilla-firefox/firefox/blob/dc64a7e82ff4e2e31b7dafaaa0a9599640a2c87c/testing/webdriver/src/command.rs#L850-L861

Also it seems x and y can be negative? How would we phrase it if upper bound is maximum safe integer?

Copy link
Contributor

Choose a reason for hiding this comment

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

The CDDL definition uses https://w3c.github.io/webdriver-bidi/#cddl-type-js-uint. That's basically the maximum safe value. There is also js-int for the full range. In WebDriver classic see https://w3c.github.io/webdriver/#dfn-maximum-safe-integer.

@yezhizhen yezhizhen requested a review from whimboo July 7, 2025 09:20
@yezhizhen yezhizhen changed the title [Set Window Rect] Change parameter requirement from Number to Integer [Set Window Rect] Change parameter requirement from Number to Integer and range to "maximum safe integer" Jul 8, 2025
@yezhizhen
Copy link
Contributor Author

web-platform-tests/wpt#53421 (comment)

Can you also review this one after? Thanks!!

@jgraham
Copy link
Member

jgraham commented Jul 8, 2025

I think this might actually be correct and BiDi wrong? It seems implausible that any implementation actually allows window dimensions larger than 32 bits (real life screen sizes typically fit into 13 bits). Also with high DPI screens it's possible that the width can be set to a non-integer number of CSS pixels (e.g. for a 2x DPI display a width of 800.5 CSS pixels would be 1601 device pixels).

@yezhizhen
Copy link
Contributor Author

yezhizhen commented Jul 8, 2025

Also with high DPI screens it's possible that the width can be set to a non-integer number of CSS pixels

Yeah this is true. I also had similar doubt when seeing the test..

@yezhizhen
Copy link
Contributor Author

Do you think we should change the test instead of the spec? @jgraham

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

Successfully merging this pull request may close these issues.

3 participants