-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Compositor: Resize PinchZoom::unscaled_viewport when viewport size changes
#40396
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
Conversation
mrobinson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is correct. Scroll can cause a pan of the viewport.
|
A scroll on the viewport should be converted into a pan because scrolling causes the pinch zoom viewport to the move to the edge edge of the un-pinch-zoomed viewport. Once the pinch zoom viewport is at the edge, a scroll happens. |
|
You can see why this is problem if you run Servo on desktop, pinch zoom, and then scroll with the mouse wheel. Without the pan here, you cannot scroll to the edge of the page. |
Make sense. |
…call it on window size changes - Added a `resize` method to the `PinchZoom` struct to update the unscaled viewport size based on the new webview rectangle. - Modified the `WebViewRenderer` to invoke the `resize` method of `PinchZoom` whenever the window size changes, ensuring the pinch zoom functionality adapts to the new dimensions. Signed-off-by: kongbai1996 <1782765876@qq.com>
There is indeed an issue here, but currently, the unscaled_viewport_size of PinchZoom does not update with the resize. I have added this logic. |
|
@kongbai1996 Is seems the PR contents and description have completely changed here? |
yes |
I also got confused earlier. |
|
🔨 Triggering try run (#19134577325) for Linux (WPT) |
|
Test results for linux-wpt from try job (#19134577325): Flaky unexpected result (39)
Stable unexpected results that are known to be intermittent (27)
Stable unexpected results (1)
|
|
|
|
This one is quite flaky today. #40457 |
I ran it locally 10 times and it passed all of them. |
|
Can this be merged? Or do we need to rerun the WPT tests? |
Our of curiosity, is this something that actually happens in your use case? When I was thinking about this, before, I was thinking that any changes to size or HiDPI scale of the view should simply reset pinch zoom entirely. |
|
What does this PR actually achieve? Can you show two videos, showing what's different after this PR? |
The size of |
|
So it is performance improvement, no behaviour change? @kongbai1996 I believe this also changes some visual behaviour? |
yezhizhen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense!
Signed-off-by: kongbai1996 <1782765876@qq.com>
If unscaled_viewport_size is not updated, it can also cause the issue where the page cannot be scrolled to the bottom after zooming. |
PinchZoom::unscaled_viewport when viewport size changes
mrobinson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a chance this isn't going to work how you want it to if there is already some PinchZoom applied to the pinch zoom viewport. If possible for your usecase please just clear the entire pinch zoom (reset transform to identity and reset the zoom_factor to 1) when resizing the viewport.
To play safe yeah. @kongbai1996 tested for a while without resetting, seems ok. |
Implement resize method for PinchZoom and update webview_renderer to call it on window size changes
Testing: No new WPT test cases passed.
Fixes: N/A