Skip to content
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

MiniBrowser method is_in_browser_rect returns true iff position is in toolbar #32254

Open
Nopey opened this issue May 7, 2024 · 2 comments
Open
Labels
C-untriaged New issues that haven't been triaged yet

Comments

@Nopey
Copy link
Contributor

Nopey commented May 7, 2024

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 #31655, all relevant context should be here.

@Nopey Nopey added the C-untriaged New issues that haven't been triaged yet label May 7, 2024
@Nopey
Copy link
Contributor Author

Nopey commented May 7, 2024

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.

@MunishMummadi
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-untriaged New issues that haven't been triaged yet
Projects
None yet
Development

No branches or pull requests

2 participants