From ba67a0a4fbd512cb14f4bda80b2c64dc27433cdd Mon Sep 17 00:00:00 2001 From: Mukilan Thiyagarajan Date: Tue, 24 Sep 2024 17:42:23 +0530 Subject: [PATCH] servoshell: fix issues related to HiDPI (#33529) The current implementation has 3 main issues related to HiDPI: 1. When the window moves from a screen with scale factor of 1.5 to one with 1 and back to 1.5, the minibrowser toolbar actually ends up being scaled by a factor of 2.25 instead of 1.5. This is because we currently use the [set_pixels_per_point] method on egui's Context, but calling this with a value of `ppp` will modify egui's internal 'zoom factor' to be: ``` zoom_factor = ppp / native_points_per_pixel. ``` where `native_points_per_pixel` is the window system scale factor. The idea is egui can calculate the final scale factor for translating its logical points to physical pixels as: ``` points_per_pixel = zoom_factor * native_points_per_pixel ``` where zoom_factor is a factor used for Ctrl+Plus, Ctrl+Minus behaviour. The problem is when we handle the ScaleFactorChanged winit event due to window moving between screens, the `native_points_per_pixel` still has the value of the previous screen's native scaling factor and not the current screen's factor. This seems to be the case even if we pass the ScaleFactorChanged event to egui before we call `set_pixels_per_point`. 2. The egui logic for handing Ctrl+Plus, Ctrl+Minus and Ctrl+0 doesn't interact well with servoshell's device-pixel-ratio CLI argument which allows the user to override the HiDPI factor. For example, Ctrl+0 will cause egui to reset the zoom_factor to 1.0 instead of the override we wanted. Another issue is egui's Ctrl+Plus/Ctrl+Minus will scale the minibrowser in increments of 0.10 whereas Servo's own page zoom doesn't (it keeps multiplying by 1.1, so the actual increments are 0.1, 0.21. 0.33 etc) 3. The inital window size calculation on Linux currently assumes a scale factor of 1.0. This means the window doesn't have the expected default logical size of 1024*740 on HiDPI systems. On a screen with HiDPI factor of 1.5, the logical window size ends up being 682x493. This change addresses all 3 issues: For 1, switch to the `set_zoom_factor` method of egui context to avoid the issue with scaling by incorrect native_points_per_pixel. To allow for the device-pixel-ratio override to work, we calculate the actual zoom_factor as `device-pixel-ratio / window's scaling factor`. For 2, disable egui's handling of Ctrl+Plus, Ctrl+Minus, Ctrl-0 shortcuts. It is unclear whether the current behaviour of scaling both the toolbar and the web page was intentional, or just an accident. This behaviour is also different from other browser where page zoom doesn't scale the GUI, so it doesn't seem like a regression to me. For 3, use LogicalSize type of winit which lets the physical size calulation to be handled by winit using the windows's actual HiDPI factor instead of hardcoded 1.0. [set_pixels_per_point]: https://github.com/emilk/egui/blob/1603f0581882c4ca299db1d6efdba9148ebcddb6/crates/egui/src/context.rs#L1886 Signed-off-by: Mukilan Thiyagarajan --- ports/servoshell/desktop/app.rs | 11 ++++++---- ports/servoshell/desktop/headed_window.rs | 26 ++++++----------------- ports/servoshell/desktop/minibrowser.rs | 7 ++++++ 3 files changed, 20 insertions(+), 24 deletions(-) diff --git a/ports/servoshell/desktop/app.rs b/ports/servoshell/desktop/app.rs index 528ee96ba01..d87daf3124d 100644 --- a/ports/servoshell/desktop/app.rs +++ b/ports/servoshell/desktop/app.rs @@ -249,15 +249,18 @@ impl App { // Intercept any ScaleFactorChanged events away from EguiGlow::on_window_event, so // we can use our own logic for calculating the scale factor and set egui’s // scale factor to that value manually. - let effective_scale_factor = window.hidpi_factor().get(); + let desired_scale_factor = window.hidpi_factor().get(); + let effective_egui_zoom_factor = desired_scale_factor / scale_factor as f32; + info!( - "window scale factor changed to {}, setting scale factor to {}", - scale_factor, effective_scale_factor + "window scale factor changed to {}, setting egui zoom factor to {}", + scale_factor, effective_egui_zoom_factor ); + minibrowser .context .egui_ctx - .set_pixels_per_point(effective_scale_factor); + .set_zoom_factor(effective_egui_zoom_factor); // Request a winit redraw event, so we can recomposite, update and paint // the minibrowser, and present the new frame. diff --git a/ports/servoshell/desktop/headed_window.rs b/ports/servoshell/desktop/headed_window.rs index 73be7c7602c..911e60d9caf 100644 --- a/ports/servoshell/desktop/headed_window.rs +++ b/ports/servoshell/desktop/headed_window.rs @@ -24,7 +24,7 @@ use servo::webrender_api::units::{DeviceIntPoint, DeviceIntRect, DeviceIntSize}; use servo::webrender_api::ScrollLocation; use servo::webrender_traits::RenderingContext; use surfman::{Connection, Context, Device, SurfaceType}; -use winit::dpi::{LogicalPosition, PhysicalPosition, PhysicalSize}; +use winit::dpi::{LogicalPosition, LogicalSize, PhysicalPosition, PhysicalSize}; use winit::event::{ElementState, KeyEvent, MouseButton, MouseScrollDelta, TouchPhase}; use winit::keyboard::{Key as LogicalKey, ModifiersState, NamedKey}; #[cfg(any(target_os = "linux", target_os = "windows"))] @@ -57,19 +57,6 @@ pub struct Window { modifiers_state: Cell, } -#[cfg(not(target_os = "windows"))] -fn window_creation_scale_factor() -> Scale { - Scale::new(1.0) -} - -#[cfg(target_os = "windows")] -fn window_creation_scale_factor() -> Scale { - use windows_sys::Win32::Graphics::Gdi::{GetDC, GetDeviceCaps, LOGPIXELSY}; - - let ppi = unsafe { GetDeviceCaps(GetDC(std::ptr::null_mut()), LOGPIXELSY as i32) }; - Scale::new(ppi as f32 / 96.0) -} - impl Window { pub fn new( win_size: Size2D, @@ -85,15 +72,11 @@ impl Window { // #9996. let visible = opts.output_file.is_none() && !no_native_titlebar; - let win_size: DeviceIntSize = (win_size.to_f32() * window_creation_scale_factor()).to_i32(); - let width = win_size.to_untyped().width; - let height = win_size.to_untyped().height; - let window_builder = winit::window::WindowBuilder::new() .with_title("Servo".to_string()) .with_decorations(!no_native_titlebar) .with_transparent(no_native_titlebar) - .with_inner_size(PhysicalSize::new(width as f64, height as f64)) + .with_inner_size(LogicalSize::new(win_size.width, win_size.height)) .with_visible(visible); let winit_window = window_builder @@ -128,7 +111,10 @@ impl Window { .window_handle() .expect("could not get window handle from window"); let native_widget = connection - .create_native_widget_from_window_handle(window_handle, Size2D::new(width, height)) + .create_native_widget_from_window_handle( + window_handle, + inner_size.to_i32().to_untyped(), + ) .expect("Failed to create native widget"); let surface_type = SurfaceType::Widget { native_widget }; let rendering_context = RenderingContext::create(&connection, &adapter, surface_type) diff --git a/ports/servoshell/desktop/minibrowser.rs b/ports/servoshell/desktop/minibrowser.rs index ea71d8ca72c..77d0e676068 100644 --- a/ports/servoshell/desktop/minibrowser.rs +++ b/ports/servoshell/desktop/minibrowser.rs @@ -88,6 +88,13 @@ impl Minibrowser { // Adapted from https://github.com/emilk/egui/blob/9478e50d012c5138551c38cbee16b07bc1fcf283/crates/egui_glow/examples/pure_glow.rs #[allow(clippy::arc_with_non_send_sync)] let context = EguiGlow::new(events_loop.as_winit(), Arc::new(gl), None); + + // Disable the builtin egui handlers for the Ctrl+Plus, Ctrl+Minus and Ctrl+0 + // shortcuts as they don't work well with servoshell's `device-pixel-ratio` CLI argument. + context + .egui_ctx + .options_mut(|options| options.zoom_with_keyboard = false); + let widget_surface_fbo = match rendering_context.context_surface_info() { Ok(Some(info)) => NonZeroU32::new(info.framebuffer_object).map(NativeFramebuffer), Ok(None) => panic!("Failed to get widget surface info from surfman!"),