[WebDriver: Set Window Rect] Make best-effort with partial input according to latest spec #53421
Conversation
The spec was changed in w3c/webdriver#1830 to allow partial parameter and change with best-effort. This makes some test assertion out-dated. This commit removes those out-dated test and move them to a new function `test_partial_input` to match the new expecation. Fixes: web-platform-tests#53409
wpt-pr-bot
requested review from
AutomatedTester,
jgraham,
juliandescottes,
sadym-chromium,
shs96c and
whimboo
yezhizhen
changed the title
| if is_wayland() and ('x' in rect or 'y' in rect): | ||
| fields = ("width", "height") | ||
| else: | ||
| fields = ("x", "y", "width", "height") | ||
|
|
||
| for field in fields: | ||
| if field in rect: | ||
| assert value[field] == rect[field] | ||
| else: | ||
| assert value[field] == original[field] |
There was a problem hiding this comment.
We won't need this loop here. Instead I would suggest to compare height and width directly if set in the rect. And do the same for x and y if it's not Wayland. That way we can safe us from setting up this extra logic.
There was a problem hiding this comment.
I have made the change accordingly.
But if neither x nor y is in rect, even for Wayland we should still make sure the coordinates stay the same?
There was a problem hiding this comment.
@whimboo Just a friendly ping.
There was a problem hiding this comment.
Yes, the application cannot move the window on its own. The solution you used is elegant. Thanks!
Signed-off-by: Euclid Ye <yezhizhenjiakang@gmail.com>
…accurately (#37848) `toolbar_height` is already part of `inner_size`, caused wrongly calculated `outer_size`. Even worse, it tried to `request_inner_size` with the already wrong `outer_size`. This PR make sure resize is accurate by first calculate the title/border height, and then compute the `inner_size` for `request_inner_size`. This is necessary because no direct `request_outer_size` is available. Testing: As manually tested, set window size WebDriver command no longer overshoot. This is also shared by [window.resizeTo](https://drafts.csswg.org/cssom-view/#dom-window-resizeto) JS method. WPT test would be necessary. (But that one is intermittent TIMEOUT. So created new one in #37856) WebDriver test will be postponed after web-platform-tests/wpt#53421 is merged and synced to Servo. Fixes: Task 3 of #37804 --------- Signed-off-by: Euclid Ye <yezhizhenjiakang@gmail.com>
|
Please lets wait with the review of the test changes until we have clarification on the spec PR. |
Which spec PR are we referring to? |
#37961) Previously, we only consider OS decoration height. But when testing #37960, I find that the decoration width is also non-zero. Testing: Need to wait W3C spec change web-platform-tests/wpt#53421 related to webdriver rectangle. When combined with #37960, this can fix at least `window_resizeTo.html`. Signed-off-by: Euclid Ye <yezhizhenjiakang@gmail.com>
There was a problem hiding this comment.
Which spec PR are we referring to?
Sorry, I had w3c/webdriver#1909 in mind but it's indeed de-coupled.
| if is_wayland() and ('x' in rect or 'y' in rect): | ||
| fields = ("width", "height") | ||
| else: | ||
| fields = ("x", "y", "width", "height") | ||
|
|
||
| for field in fields: | ||
| if field in rect: | ||
| assert value[field] == rect[field] | ||
| else: | ||
| assert value[field] == original[field] |
There was a problem hiding this comment.
Yes, the application cannot move the window on its own. The solution you used is elegant. Thanks!
| # Wayland doesn't return correct coordinates after changing window position. | ||
| if not is_wayland(): | ||
| assert value["x"] == rect.get("x", original["x"]) | ||
| assert value["y"] == rect.get("y", original["y"]) |
There was a problem hiding this comment.
@sadym-chromium i assume that we run Chrome by default with XWAYLAND so that it can still set the position of the window? So we probably should keep what we have here and not explicitely check the x and y coordinates which can be different between browsers.
The spec was changed in w3c/webdriver#1830 to allow partial parameter and change with best-effort. This makes some test assertion out-dated.
This commit removes those out-dated test and move them to a new function
test_partial_inputto match the new expectation.Fixes: #53409
The change is recommended in w3c/webdriver#1906 (comment)