servoshell: Make fn request_resize resize window w.r.t. outer_size accurately#37848
Merged
xiaochengh merged 2 commits intoservo:mainfrom Jul 4, 2025
Merged
servoshell: Make fn request_resize resize window w.r.t. outer_size accurately#37848xiaochengh merged 2 commits intoservo:mainfrom
fn request_resize resize window w.r.t. outer_size accurately#37848xiaochengh merged 2 commits intoservo:mainfrom
Conversation
Signed-off-by: Euclid Ye <[email protected]>
|
🔨 Triggering try run (#16046257080) for Linux (WPT) |
|
Test results for linux-wpt from try job (#16046257080): Flaky unexpected result (23)
Stable unexpected results that are known to be intermittent (18)
|
|
✨ Try run (#16046257080) succeeded. |
Signed-off-by: Euclid Ye <[email protected]>
yezhizhen
force-pushed
the
fix-set-innersize
branch
from
ae976ac to
7e1e81d
Compare
yezhizhen
changed the title
fn request_resize resize accuratelyfn request_resize resize window w.r.t. outer_size accurately
yezhizhen
commented
Jul 3, 2025
Comment on lines
-29
to
+31
| /// Request a new inner size for the window, not including external decorations. | ||
| fn request_resize(&self, webview: &WebView, inner_size: DeviceIntSize) | ||
| /// Request a new outer size for the window, including external decorations. | ||
| /// This should be the same as `window.outerWidth` and `window.outerHeight`` | ||
| fn request_resize(&self, webview: &WebView, outer_size: DeviceIntSize) |
Member
Author
There was a problem hiding this comment.
This function is used for only two things:
- window.resizeTo
- WebDriver SetWindowRect
Both would request outer_size instead of inner_size
xiaochengh
reviewed
Jul 4, 2025
Comment on lines
+470
to
+471
| let title_height = | ||
| self.winit_window.outer_size().height - self.winit_window.inner_size().height; |
Contributor
There was a problem hiding this comment.
Should we calculate toolbar height in the same way as before? Since hidpi scale factor might be playing a role here.
Member
Author
There was a problem hiding this comment.
Not needed because inner_size already includes toolbar height.
Member
Author
There was a problem hiding this comment.
title_height is what is marked red.
Contributor
There was a problem hiding this comment.
Then what does inner_size stand for? Window.innerHeight should not include toolbar, but viewport only.
Member
Author
There was a problem hiding this comment.
inner_size is the terminology used by winit. It is everything except the title and border from OS.
Window.innerHeight should not include toolbar, but viewport only.
Yes. This is already done correctly, except that they didn't include Scrollbar. But that is another story..
xiaochengh
approved these changes
Jul 4, 2025
Comment on lines
+470
to
+471
| let title_height = | ||
| self.winit_window.outer_size().height - self.winit_window.inner_size().height; |
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jul 4, 2025
…& Fix wrong usage of `innerHeight` & add new test (#37856) 1. rename original `window_resizeTo.html` to `window_resize_event.html` to reflect the purpose. Also change {innerWidth, innerHeight} to {outerWidth, outerHeight} to match spec. 2. Add a new test `window_resizeTo.html` for #37848 Testing: new test always fails because of #37824, which gives inaccurate outerHeight. Signed-off-by: Euclid Ye <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
toolbar_heightis already part ofinner_size, caused wrongly calculatedouter_size. Even worse, it tried torequest_inner_sizewith the already wrongouter_size.This PR make sure resize is accurate by first calculate the title/border height, and then compute the
inner_sizeforrequest_inner_size. This is necessary because no directrequest_outer_sizeis available.Testing: As manually tested, set window size WebDriver command no longer overshoot. This is also shared by 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