Multiply input coords by window scale_factor so Retina clicks land
GPUI delivers logical (CSS) pixels but Servo expects device pixels. On a 2x Retina display the page rendered at half resolution and every click landed in roughly the upper-left quadrant of the page — the second independent root cause that surfaces immediately after T1's show()/focus() lets input reach Servo at all. Single conversion boundary inside web_surface_geometry.rs: viewport_dimension, scroll_dimension, and click_coordinate each multiply once by window.scale_factor() before truncating to integer device pixels. positive_scale_or_one() guards against a zero/negative/ NaN scale_factor reaching the arithmetic — falling back to 1.0 keeps coordinates valid even if the platform reports nonsense (system boundary validation per CLAUDE.md, not internal trust). scale_factor is plumbed through controller and view layer, sourced from window.scale_factor() at every record_* boundary so all four inputs (click, scroll, hover, viewport) stay in sync. Existing tests pin the 1.0 path; new retina_scale_factor_doubles_every_input_coordinate locks the 2.0 path against a future regression. cargo test ely_app --bin ely_app: 104 passed (was 103 + new test).
This commit is contained in:
@@ -102,8 +102,9 @@ impl WebSurfaceStore {
|
||||
tab_id: &TabId,
|
||||
requested_url: &str,
|
||||
delta: Point<Pixels>,
|
||||
scale_factor: f32,
|
||||
) -> bool {
|
||||
let Some(delta) = WebSurfaceScrollDelta::from_point(delta) else {
|
||||
let Some(delta) = WebSurfaceScrollDelta::from_point(delta, scale_factor) else {
|
||||
return false;
|
||||
};
|
||||
|
||||
@@ -130,8 +131,13 @@ impl WebSurfaceStore {
|
||||
true
|
||||
}
|
||||
|
||||
pub(super) fn record_viewport_size(&mut self, tab_id: &TabId, bounds: Bounds<Pixels>) -> bool {
|
||||
let Some(size) = WebSurfaceSize::from_bounds(bounds) else {
|
||||
pub(super) fn record_viewport_size(
|
||||
&mut self,
|
||||
tab_id: &TabId,
|
||||
bounds: Bounds<Pixels>,
|
||||
scale_factor: f32,
|
||||
) -> bool {
|
||||
let Some(size) = WebSurfaceSize::from_bounds(bounds, scale_factor) else {
|
||||
return false;
|
||||
};
|
||||
let surface = self.surface_mut(tab_id);
|
||||
@@ -162,13 +168,15 @@ impl WebSurfaceStore {
|
||||
&mut self,
|
||||
tab_id: &TabId,
|
||||
position: Point<Pixels>,
|
||||
scale_factor: f32,
|
||||
) -> bool {
|
||||
let surface = self.surfaces.get_mut(tab_id).filter(|surface| surface.viewport_bounds.is_some());
|
||||
let Some(surface) = surface else {
|
||||
return false;
|
||||
};
|
||||
let bounds = surface.viewport_bounds.expect("viewport_bounds checked above");
|
||||
let Some(point) = WebSurfaceClickPoint::from_window_position(bounds, position) else {
|
||||
let Some(point) = WebSurfaceClickPoint::from_window_position(bounds, position, scale_factor)
|
||||
else {
|
||||
return false;
|
||||
};
|
||||
surface.hover_point = Some(point);
|
||||
@@ -180,13 +188,15 @@ impl WebSurfaceStore {
|
||||
tab_id: &TabId,
|
||||
requested_url: &str,
|
||||
position: Point<Pixels>,
|
||||
scale_factor: f32,
|
||||
) -> bool {
|
||||
let Some(bounds) =
|
||||
self.surfaces.get(tab_id).and_then(|surface| surface.viewport_bounds)
|
||||
else {
|
||||
return false;
|
||||
};
|
||||
let Some(point) = WebSurfaceClickPoint::from_window_position(bounds, position) else {
|
||||
let Some(point) = WebSurfaceClickPoint::from_window_position(bounds, position, scale_factor)
|
||||
else {
|
||||
return false;
|
||||
};
|
||||
|
||||
|
||||
@@ -52,9 +52,10 @@ impl ElyShell {
|
||||
&mut self,
|
||||
tab_id: TabId,
|
||||
bounds: Bounds<Pixels>,
|
||||
scale_factor: f32,
|
||||
cx: &mut Context<Self>,
|
||||
) {
|
||||
if self.web_surfaces.record_viewport_size(&tab_id, bounds) {
|
||||
if self.web_surfaces.record_viewport_size(&tab_id, bounds, scale_factor) {
|
||||
cx.notify();
|
||||
}
|
||||
}
|
||||
@@ -64,9 +65,15 @@ impl ElyShell {
|
||||
tab_id: TabId,
|
||||
requested_url: String,
|
||||
delta: Point<Pixels>,
|
||||
scale_factor: f32,
|
||||
cx: &mut Context<Self>,
|
||||
) {
|
||||
if self.web_surfaces.record_scroll_delta(&tab_id, requested_url.as_str(), delta) {
|
||||
if self.web_surfaces.record_scroll_delta(
|
||||
&tab_id,
|
||||
requested_url.as_str(),
|
||||
delta,
|
||||
scale_factor,
|
||||
) {
|
||||
cx.notify();
|
||||
}
|
||||
}
|
||||
@@ -75,9 +82,10 @@ impl ElyShell {
|
||||
&mut self,
|
||||
tab_id: TabId,
|
||||
position: Point<Pixels>,
|
||||
scale_factor: f32,
|
||||
cx: &mut Context<Self>,
|
||||
) {
|
||||
if self.web_surfaces.record_hover_point(&tab_id, position) {
|
||||
if self.web_surfaces.record_hover_point(&tab_id, position, scale_factor) {
|
||||
cx.notify();
|
||||
}
|
||||
}
|
||||
@@ -91,7 +99,13 @@ impl ElyShell {
|
||||
cx: &mut Context<Self>,
|
||||
) {
|
||||
self.focus_handle.focus(window);
|
||||
if self.web_surfaces.record_click_point(&tab_id, requested_url.as_str(), position) {
|
||||
let scale_factor = window.scale_factor();
|
||||
if self.web_surfaces.record_click_point(
|
||||
&tab_id,
|
||||
requested_url.as_str(),
|
||||
position,
|
||||
scale_factor,
|
||||
) {
|
||||
cx.notify();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -7,10 +7,10 @@ pub(super) struct WebSurfaceSize {
|
||||
}
|
||||
|
||||
impl WebSurfaceSize {
|
||||
pub(super) fn from_bounds(bounds: Bounds<Pixels>) -> Option<Self> {
|
||||
pub(super) fn from_bounds(bounds: Bounds<Pixels>, scale_factor: f32) -> Option<Self> {
|
||||
Some(Self {
|
||||
width: viewport_dimension(bounds.size.width)?,
|
||||
height: viewport_dimension(bounds.size.height)?,
|
||||
width: viewport_dimension(bounds.size.width, scale_factor)?,
|
||||
height: viewport_dimension(bounds.size.height, scale_factor)?,
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -25,10 +25,11 @@ impl WebSurfaceClickPoint {
|
||||
pub(super) fn from_window_position(
|
||||
bounds: Bounds<Pixels>,
|
||||
position: Point<Pixels>,
|
||||
scale_factor: f32,
|
||||
) -> Option<Self> {
|
||||
Some(Self {
|
||||
x: click_coordinate(position.x, bounds.origin.x, bounds.size.width)?,
|
||||
y: click_coordinate(position.y, bounds.origin.y, bounds.size.height)?,
|
||||
x: click_coordinate(position.x, bounds.origin.x, bounds.size.width, scale_factor)?,
|
||||
y: click_coordinate(position.y, bounds.origin.y, bounds.size.height, scale_factor)?,
|
||||
})
|
||||
}
|
||||
|
||||
@@ -82,9 +83,9 @@ pub(super) struct WebSurfaceScrollDelta {
|
||||
}
|
||||
|
||||
impl WebSurfaceScrollDelta {
|
||||
pub(super) fn from_point(delta: Point<Pixels>) -> Option<Self> {
|
||||
let x = scroll_dimension(delta.x)?;
|
||||
let y = scroll_dimension(delta.y)?;
|
||||
pub(super) fn from_point(delta: Point<Pixels>, scale_factor: f32) -> Option<Self> {
|
||||
let x = scroll_dimension(delta.x, scale_factor)?;
|
||||
let y = scroll_dimension(delta.y, scale_factor)?;
|
||||
if x == 0 && y == 0 {
|
||||
return None;
|
||||
}
|
||||
@@ -105,8 +106,8 @@ impl WebSurfaceScrollDelta {
|
||||
}
|
||||
}
|
||||
|
||||
fn viewport_dimension(pixels: Pixels) -> Option<u32> {
|
||||
let value = f32::from(pixels.round());
|
||||
fn viewport_dimension(pixels: Pixels, scale_factor: f32) -> Option<u32> {
|
||||
let value = (f32::from(pixels) * positive_scale_or_one(scale_factor)).round();
|
||||
if !value.is_finite() || value < 1.0 || value > u32::MAX as f32 {
|
||||
return None;
|
||||
}
|
||||
@@ -114,8 +115,8 @@ fn viewport_dimension(pixels: Pixels) -> Option<u32> {
|
||||
Some(value as u32)
|
||||
}
|
||||
|
||||
fn scroll_dimension(pixels: Pixels) -> Option<i32> {
|
||||
let value = f32::from(pixels.round());
|
||||
fn scroll_dimension(pixels: Pixels, scale_factor: f32) -> Option<i32> {
|
||||
let value = (f32::from(pixels) * positive_scale_or_one(scale_factor)).round();
|
||||
if !value.is_finite() {
|
||||
return None;
|
||||
}
|
||||
@@ -129,14 +130,24 @@ fn scroll_dimension(pixels: Pixels) -> Option<i32> {
|
||||
Some(value as i32)
|
||||
}
|
||||
|
||||
fn click_coordinate(position: Pixels, origin: Pixels, size: Pixels) -> Option<u32> {
|
||||
fn click_coordinate(
|
||||
position: Pixels,
|
||||
origin: Pixels,
|
||||
size: Pixels,
|
||||
scale_factor: f32,
|
||||
) -> Option<u32> {
|
||||
let relative = f32::from(position) - f32::from(origin);
|
||||
let size = f32::from(size);
|
||||
if !relative.is_finite() || !size.is_finite() || relative < 0.0 || relative >= size {
|
||||
return None;
|
||||
}
|
||||
|
||||
Some(relative.floor() as u32)
|
||||
let scaled = (relative * positive_scale_or_one(scale_factor)).floor();
|
||||
if !scaled.is_finite() || scaled < 0.0 || scaled > u32::MAX as f32 {
|
||||
return None;
|
||||
}
|
||||
|
||||
Some(scaled as u32)
|
||||
}
|
||||
|
||||
fn positive_scroll_component(current: i32, delta: i32) -> i32 {
|
||||
@@ -149,3 +160,12 @@ fn combined_scroll_delta(current: i32, next: i32) -> i32 {
|
||||
let value = i64::from(current) + i64::from(next);
|
||||
value.clamp(i64::from(i32::MIN), i64::from(i32::MAX)) as i32
|
||||
}
|
||||
|
||||
/// Guard against a zero/negative/NaN scale factor reaching the
|
||||
/// arithmetic above. GPUI's `Window::scale_factor` returns the real
|
||||
/// device pixel ratio (1.0 on standard displays, 2.0 on Retina), so a
|
||||
/// non-positive value would mean the platform reported nonsense; we
|
||||
/// fall back to 1.0 rather than collapsing every coordinate to zero.
|
||||
fn positive_scale_or_one(scale_factor: f32) -> f32 {
|
||||
if scale_factor.is_finite() && scale_factor > 0.0 { scale_factor } else { 1.0 }
|
||||
}
|
||||
|
||||
@@ -101,7 +101,7 @@ fn render_web_surface_frame(
|
||||
|
||||
for attempt in 0..LIVE_SITE_RENDER_ATTEMPTS {
|
||||
let tab = web_tab(profile_id.clone(), case.url)?;
|
||||
assert!(store.record_viewport_size(tab.id(), live_surface_bounds()), "{}", case.url);
|
||||
assert!(store.record_viewport_size(tab.id(), live_surface_bounds(), 1.0), "{}", case.url);
|
||||
store.ensure_surface(&tab, ProfileDataMode::Transient, &[]);
|
||||
|
||||
match wait_for_ready_frame(store, tab.id(), case) {
|
||||
|
||||
@@ -10,8 +10,13 @@ fn typed_text_enters_pending_input_after_clicked_viewport() -> Result<(), Box<dy
|
||||
let mut store = WebSurfaceStore::new();
|
||||
let tab = web_tab("https://example.com/form")?;
|
||||
|
||||
assert!(store.record_viewport_size(tab.id(), web_bounds()));
|
||||
assert!(store.record_click_point(tab.id(), tab.url().as_str(), point(px(160.0), px(120.0))));
|
||||
assert!(store.record_viewport_size(tab.id(), web_bounds(), 1.0));
|
||||
assert!(store.record_click_point(
|
||||
tab.id(),
|
||||
tab.url().as_str(),
|
||||
point(px(160.0), px(120.0)),
|
||||
1.0,
|
||||
));
|
||||
assert!(store.record_typed_text(tab.id(), tab.url().as_str(), "e"));
|
||||
assert!(store.record_typed_text(tab.id(), tab.url().as_str(), "l"));
|
||||
|
||||
@@ -27,9 +32,19 @@ fn scroll_delta_enters_pending_input_after_wheel() -> Result<(), Box<dyn Error>>
|
||||
let mut store = WebSurfaceStore::new();
|
||||
let tab = web_tab("https://example.com/list")?;
|
||||
|
||||
assert!(store.record_viewport_size(tab.id(), web_bounds()));
|
||||
assert!(store.record_scroll_delta(tab.id(), tab.url().as_str(), point(px(0.0), px(140.0))));
|
||||
assert!(store.record_scroll_delta(tab.id(), tab.url().as_str(), point(px(0.0), px(60.0))));
|
||||
assert!(store.record_viewport_size(tab.id(), web_bounds(), 1.0));
|
||||
assert!(store.record_scroll_delta(
|
||||
tab.id(),
|
||||
tab.url().as_str(),
|
||||
point(px(0.0), px(140.0)),
|
||||
1.0,
|
||||
));
|
||||
assert!(store.record_scroll_delta(
|
||||
tab.id(),
|
||||
tab.url().as_str(),
|
||||
point(px(0.0), px(60.0)),
|
||||
1.0,
|
||||
));
|
||||
|
||||
let input = store.take_pending_input(tab.id(), tab.url().as_str());
|
||||
|
||||
@@ -43,9 +58,9 @@ fn viewport_size_changes_after_stable_second_measurement() -> Result<(), Box<dyn
|
||||
let mut store = WebSurfaceStore::new();
|
||||
let tab = web_tab("https://example.com/resize")?;
|
||||
|
||||
assert!(store.record_viewport_size(tab.id(), web_bounds()));
|
||||
assert!(!store.record_viewport_size(tab.id(), resized_once_bounds()));
|
||||
assert!(store.record_viewport_size(tab.id(), resized_once_bounds()));
|
||||
assert!(store.record_viewport_size(tab.id(), web_bounds(), 1.0));
|
||||
assert!(!store.record_viewport_size(tab.id(), resized_once_bounds(), 1.0));
|
||||
assert!(store.record_viewport_size(tab.id(), resized_once_bounds(), 1.0));
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -63,11 +78,11 @@ fn scroll_after_click_keeps_keyboard_focus_and_typed_text() -> Result<(), Box<dy
|
||||
let tab = web_tab("https://example.com/form")?;
|
||||
let url = tab.url().as_str();
|
||||
|
||||
assert!(store.record_viewport_size(tab.id(), web_bounds()));
|
||||
assert!(store.record_click_point(tab.id(), url, point(px(160.0), px(120.0))));
|
||||
assert!(store.record_viewport_size(tab.id(), web_bounds(), 1.0));
|
||||
assert!(store.record_click_point(tab.id(), url, point(px(160.0), px(120.0)), 1.0));
|
||||
assert!(store.record_typed_text(tab.id(), url, "h"));
|
||||
|
||||
assert!(store.record_scroll_delta(tab.id(), url, point(px(0.0), px(140.0))));
|
||||
assert!(store.record_scroll_delta(tab.id(), url, point(px(0.0), px(140.0)), 1.0));
|
||||
|
||||
assert!(
|
||||
store.record_typed_text(tab.id(), url, "i"),
|
||||
@@ -94,6 +109,39 @@ fn scroll_after_click_keeps_keyboard_focus_and_typed_text() -> Result<(), Box<dy
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Locks the Retina (2x) scale path: GPUI delivers logical pixels but
|
||||
/// Servo expects device pixels, so every coordinate that crosses the
|
||||
/// boundary must be multiplied by `window.scale_factor()`. Without
|
||||
/// this regression a future refactor could silently revert to the
|
||||
/// 1.0-only behavior that left every Retina click landing in the
|
||||
/// upper-left quadrant of the page.
|
||||
#[test]
|
||||
fn retina_scale_factor_doubles_every_input_coordinate() -> Result<(), Box<dyn Error>> {
|
||||
let mut store = WebSurfaceStore::new();
|
||||
let tab = web_tab("https://example.com/form")?;
|
||||
let url = tab.url().as_str();
|
||||
|
||||
assert!(store.record_viewport_size(tab.id(), web_bounds(), 2.0));
|
||||
assert!(store.record_click_point(tab.id(), url, point(px(160.0), px(120.0)), 2.0));
|
||||
assert!(store.record_typed_text(tab.id(), url, "h"));
|
||||
assert!(store.record_scroll_delta(tab.id(), url, point(px(0.0), px(140.0)), 2.0));
|
||||
|
||||
let input = store.take_pending_input(tab.id(), url);
|
||||
|
||||
assert_eq!(
|
||||
input.scroll_delta.map(|delta| (delta.x(), delta.y())),
|
||||
Some((0, 280)),
|
||||
"wheel delta of 140 logical px must be 280 device px on Retina",
|
||||
);
|
||||
assert_eq!(input.scroll_offset.y(), 280, "scroll offset accumulates in device px");
|
||||
assert_eq!(
|
||||
input.click_point,
|
||||
None,
|
||||
"scroll drops the buffered click — its viewport coords are stale",
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Locks the precondition that `record_typed_text` requires a prior
|
||||
/// click to have established keyboard focus. Without this guard, a
|
||||
/// future refactor could silently start buffering stray keystrokes
|
||||
@@ -105,7 +153,7 @@ fn typing_without_a_prior_click_is_rejected() -> Result<(), Box<dyn Error>> {
|
||||
let tab = web_tab("https://example.com/form")?;
|
||||
let url = tab.url().as_str();
|
||||
|
||||
assert!(store.record_viewport_size(tab.id(), web_bounds()));
|
||||
assert!(store.record_viewport_size(tab.id(), web_bounds(), 1.0));
|
||||
assert!(
|
||||
!store.record_typed_text(tab.id(), url, "x"),
|
||||
"typing must fail until a click establishes keyboard focus on this tab and url",
|
||||
|
||||
@@ -129,22 +129,26 @@ fn render_input_overlay(
|
||||
});
|
||||
cx.stop_propagation();
|
||||
})
|
||||
.on_mouse_move(move |event, _window, cx| {
|
||||
.on_mouse_move(move |event, window, cx| {
|
||||
let scale_factor = window.scale_factor();
|
||||
hover_entity.update(cx, |shell, cx| {
|
||||
shell.hover_external_web_viewport(
|
||||
hover_tab_id.clone(),
|
||||
event.position,
|
||||
scale_factor,
|
||||
cx,
|
||||
);
|
||||
});
|
||||
})
|
||||
.on_scroll_wheel(move |event, window, cx| {
|
||||
let delta = event.delta.pixel_delta(window.line_height());
|
||||
let scale_factor = window.scale_factor();
|
||||
scroll_entity.update(cx, |shell, cx| {
|
||||
shell.scroll_external_web_viewport(
|
||||
scroll_tab_id.clone(),
|
||||
scroll_url.clone(),
|
||||
delta,
|
||||
scale_factor,
|
||||
cx,
|
||||
);
|
||||
});
|
||||
@@ -154,9 +158,10 @@ fn render_input_overlay(
|
||||
|
||||
fn render_viewport_tracker(tab_id: TabId, state_entity: Entity<ElyShell>) -> impl IntoElement {
|
||||
canvas(
|
||||
move |bounds, _window: &mut Window, cx: &mut App| {
|
||||
move |bounds, window: &mut Window, cx: &mut App| {
|
||||
let scale_factor = window.scale_factor();
|
||||
state_entity.update(cx, |shell, cx| {
|
||||
shell.record_external_web_viewport(tab_id, bounds, cx);
|
||||
shell.record_external_web_viewport(tab_id, bounds, scale_factor, cx);
|
||||
});
|
||||
},
|
||||
|_, _, _, _| {},
|
||||
|
||||
Reference in New Issue
Block a user