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

When the Servo window gains focus, it should update the mouse coordinate #11130

Closed
paulrouget opened this issue May 11, 2016 · 16 comments
Closed

When the Servo window gains focus, it should update the mouse coordinate #11130

paulrouget opened this issue May 11, 2016 · 16 comments

Comments

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented May 11, 2016

This is important for browserhtml. We need this to build automated tests.

in ports/glutin, the mouse coordinate is updated when receiving a glutin mousemove event.

When Servo starts, as the mouse moves, mouse_pos is updated. Then Servo loses focus. User moves the mouse. When Servo gains focus, the mouse is likely not at mouse_pos anymore. That means:

  1. mouseover is not triggered and :hover styles are not applied for element below the mouse
  2. if a click occurs before the mouse is moved, the clicked is triggered for the element that was under the latest known coordinate (right before losing focus)

We should update mouse_pos when gaining focus, and fire a mousemove event if it changed.

There is a Focused(bool) glutin event, but it doesn't come with new mouse coordinates.

@tschneidereit
Copy link
Contributor

@tschneidereit tschneidereit commented May 11, 2016

In addition, a mouseout event (with the current coordinates) should be fired upon losing focus, and any :hover states reset.

@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented May 11, 2016

In addition, a mouseout event (with the current coordinates) should be fired upon losing focus, and any :hover states reset.

That will happen automatically. When I'm talking about mousemove, I'm talking about WindowEvent::MouseWindowMoveEventClass.

@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented May 11, 2016

If someone can tell me how to get the mouse coordinates from a didBecomeKey notification, i can take care of the rest (is NSEvent::locationInWindow(nil) supposed to work?).

/cc @pcwalton

@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented May 11, 2016

We get a NSAppKitDefined nsevent before getting the focus notification. So maybe we can use that to get a nsevent instance and do nsevent.locationInWindow().

@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented May 12, 2016

First thing first, a click should not rely on the previous mousemove coordinate, but on the actual coordinate.

In Glutin:

In src/events.rs, we need to change MouseInput(ElementState, MouseButton) to MouseInput(ElementState, MouseButton, Option<(i32, i32)>). Option<(i32, i32)> will hold the new coordinates.

In src/api/cocoa/mod.rs, MouseInput events should include the coordinates (see how we retrieve the coordinates in NSRightMouseDragged). For other platforms, we can just use None for now.

In Servo:

In ports/glutin/window.rs, for Event::MouseInput events, if coordinates are present, self.mouse_pos should be updated and a WindowEvent::MouseWindowMoveEventClass should also be fired in addition to any event handle_mouse will eventually fire.

@nox nox changed the title When the Servo window gains focus, it should udpate the mouse coordinate When the Servo window gains focus, it should update the mouse coordinate May 12, 2016
@mrmiywj
Copy link
Contributor

@mrmiywj mrmiywj commented May 14, 2016

@paulrouget
I'll pick this up.

@metajack metajack added the C-assigned label May 22, 2016
@metajack
Copy link
Contributor

@metajack metajack commented May 22, 2016

@mrmiywj Have at it!

@jdm
Copy link
Member

@jdm jdm commented May 31, 2016

@mrmiywj Have you made any progress on this?

@mrmiywj
Copy link
Contributor

@mrmiywj mrmiywj commented Jun 5, 2016

@jdm
Yep, I've finished my finals and will go on work on servo.

@mrmiywj
Copy link
Contributor

@mrmiywj mrmiywj commented Jun 5, 2016

@paulrouget
I read through all your solutions, does it mean that I need to modify glutin's source code?

@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Jun 7, 2016

@mrmiywj Yes. I think the proper way of doing it is what I described here: #11130 (comment) - that would partially address this issue.

@mrmiywj
Copy link
Contributor

@mrmiywj mrmiywj commented Jun 7, 2016

@paulrouget
But, will the Glutin's maintainer approve our modification of its APIs?

@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Jun 7, 2016

But, will the Glutin's maintainer approve our modification of its APIs?

This should not a be a problem.

@mrmiywj
Copy link
Contributor

@mrmiywj mrmiywj commented Jun 12, 2016

@paulrouget
I've pushed a PR to glutin to fix this issue. Could you review it?

bors-servo added a commit to servo/glutin that referenced this issue Jun 16, 2016
return coordinates in MouseInput events

Return mouse coordinates in MouseInput event.
Due to this [servo issue](servo/servo#11130)

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/glutin/97)
<!-- Reviewable:end -->
@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Jun 16, 2016

@mrmiywj thanks for landing the first part. Now we need a glutin update in Servo with the changes I'm describing in the second part of #11130 (comment)

pcwalton added a commit to pcwalton/servo that referenced this issue Jun 30, 2016
Large framerate improvement in browser.html.

This takes the Glutin upgrade intended for servo#11130 but does not address
the underlying issue. A FIXME has been added.
bors-servo added a commit that referenced this issue Jun 30, 2016
Update Glutin to pick up borderless window performance fixes on Mac.

Large framerate improvement in browser.html.

This takes the Glutin upgrade intended for #11130 but does not address
the underlying issue. A FIXME has been added.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11940)
<!-- Reviewable:end -->
pcwalton added a commit to pcwalton/servo that referenced this issue Jun 30, 2016
Large framerate improvement in browser.html.

This takes the Glutin upgrade intended for servo#11130 but does not address
the underlying issue. A FIXME has been added.
bors-servo added a commit that referenced this issue Jun 30, 2016
Update Glutin to pick up borderless window performance fixes on Mac.

Large framerate improvement in browser.html.

This takes the Glutin upgrade intended for #11130 but does not address
the underlying issue. A FIXME has been added.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11940)
<!-- Reviewable:end -->
cbrewster added a commit to cbrewster/servo that referenced this issue Jul 1, 2016
Large framerate improvement in browser.html.

This takes the Glutin upgrade intended for servo#11130 but does not address
the underlying issue. A FIXME has been added.
bors-servo added a commit that referenced this issue Jul 1, 2016
Update Glutin to pick up borderless window performance fixes on Mac.

Updated cocoa to unblock servo-glutin from being updated on cef.

> Large framerate improvement in browser.html.
>
> This takes the Glutin upgrade intended for #11130 but does not address
> the underlying issue. A FIXME has been added.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11979)
<!-- Reviewable:end -->
@jdm jdm added the C-has open PR label Jul 4, 2016
bors-servo added a commit that referenced this issue Jul 5, 2016
…KiChjang

when window gains focus, update mouse coordinate

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #11130 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because these cannot be tested automated.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11794)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants
You can’t perform that action at this time.