Hit testing is offset in GL aquarium

[jdm]

https://webglsamples.org/aquarium/aquarium.html

When I try to click the numbers, there is a clear vertical offset (like 100px or more) that I need to compensate for to click the numbers I intend.

[cbrewster]

I think hit testing is broken in general, I see this behavior on every site. I think it may be related to hidpi screens; servo thinks the mouse is in the center of the window when its actually on the far right.

[gw3583]

I can also reproduce this on Linux - I'll investigate today.

[gw3583]

It looks like something has changed to do with how / where DPI scaling is applied, either in Servo or in Glutin / Winit. Specifically, in the hit test method in compositor.rs, we receive a DevicePoint but debug logs suggest that it is actually not a true device point, but has the DPI scale un-applied.

As a quick hack, I applied the following patch:

diff --git a/components/compositing/compositor.rs b/components/compositing/compositor.rs
index 0e17b41baf..702a54d12c 100644
--- a/components/compositing/compositor.rs
+++ b/components/compositing/compositor.rs
@@ -678,10 +678,7 @@ impl<Window: WindowMethods> IOCompositor<Window> {
     }
 
     fn hit_test_at_point(&self, point: DevicePoint) -> HitTestResult {
-        let dppx = self.page_zoom * self.hidpi_factor();
-        let scaled_point = (point / dppx).to_untyped();
-
-        let world_cursor = webrender_api::WorldPoint::from_untyped(&scaled_point);
+        let world_cursor = webrender_api::WorldPoint::new(point.x, point.y);
         self.webrender_api.hit_test(
             self.webrender_document,
             None,

This appears to fix hit testing on my Linux machine in the tests I've tried thus far.

@jdm or @cbrewster Would you be able to check if this also fixes it on a Mac?

If it does, the proper fix will involve working out what changed and either restoring the previous behavior (so we are getting a true DevicePoint in this method) or modifying the method and signatures to reflect that it is no longer a DevicePoint. We'll still need to handle other scale factors, such as page zoom, which I've ignored in the patch above for now.

[jdm]

@gw3583 Yes, that patch fixes the issue for me as well.

[atouchet]

This might be due to the winit/glutin update in #21087. There were some HiDPI changes there: https://github.com/tomaka/winit/blob/master/CHANGELOG.md

[gw3583]

@jdm OK, I confirmed the issue is from #21087. It looks like winit now returns events in logical units, rather than device units. That patch will need to be re-worked in order to handle this - it looks like it gets logical / device units confused in a few places, by constructing those types directly from the scalar values, I think.

[jdm]

Thanks for investigating!

[jdm]

diff --git a/ports/servo/glutin_app/window.rs b/ports/servo/glutin_app/window.rs
index 514b1f8c07..75c5ac29d0 100644
--- a/ports/servo/glutin_app/window.rs
+++ b/ports/servo/glutin_app/window.rs
@@ -493,12 +493,14 @@ impl Window {
             },
             Event::WindowEvent {
                 event: winit::WindowEvent::CursorMoved {
-                    position: LogicalPosition { x, y },
+                    position,
                     ..
                 },
                 ..
             } => {
-                self.mouse_pos.set(TypedPoint2D::new(x as i32, y as i32));
+                let pos = position.to_physical(self.hidpi_factor().get() as f64);
+                let (x, y): (i32, i32) = pos.into();
+                self.mouse_pos.set(TypedPoint2D::new(x, y));
                 self.event_queue.borrow_mut().push(
                     WindowEvent::MouseWindowMoveEventClass(TypedPoint2D::new(x as f32, y as f32)));

This change fixed the hit testing for me.