servo icon indicating copy to clipboard operation
servo copied to clipboard

MiniBrowser method is_in_browser_rect returns true iff position is in toolbar

Open Nopey opened this issue 9 months ago • 2 comments

Describe the bug: The method is_in_browser_rect on MiniBrowser returns true if (and only if) the position is inside the toolbar. If this behavior is not a bug, perhaps the function should be renamed to something like fn is_in_browser_toolbar.

The code (from minibrowser.rs):

    /// Return true iff the given position is in the Servo browser rect.
    fn is_in_browser_rect(&self, position: Point2D<f32, DeviceIndependentPixel>) -> bool {
        position.y < self.toolbar_height.get()
    }

It is used above in fn on_window_event, where MouseWheel and MouseInput events are only marked as consumed if fn is_in_browser_rect returns true.

Impact is unknown to me, as I don't know how the consumed flag effects the further handling of events; does website JavaScript receive (only) unconsumed events?

To Reproduce: Modify fn is_in_browser_rect to debug-print its result, and then observe its behavior by clicking around on the servo window.

Platform: Occurs on Windows 10, while using a mouse.

CC @MunishMummadi Some prior discussion occured on https://github.com/servo/servo/issues/31655, all relevant context should be here.

Nopey avatar May 07 '24 21:05 Nopey

In the prior discussion, Munish wrote:

based on your suggestion. we can change it to something like this and by fetching the window_size in the beginning of the function and using it in is_in_browser_rect

    fn is_in_browser_rect(
        &self,
        position: Point2D<f32, DeviceIndependentPixel>,
        window_size: LogicalSize<f32>,
    ) -> bool {
        position.y < self.toolbar_height.get()
            && position.x >= 0.0
            && position.x <= window_size.width
    }

Is my understanding correct or you are suggesting something else. even with the above change I am not able to get the functionality @Nopey

Let's not add checks for the position's X coordinate, as I'd expect events that occur entirely outside the browser window are already correctly handled (hopefully, by not even being sent to our browser's event loop)

Switching the current code's position.y < ... to position.y >= ... should be sufficient.

Nopey avatar May 07 '24 22:05 Nopey

i withdraw my conclusion. I think checking the y-coordinate should be enough.

MunishMummadi avatar May 07 '24 22:05 MunishMummadi