when window gains focus, update mouse coordinate #11794

mrmiywj · 2016-06-19T08:18:31Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #11130 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because these cannot be tested automated.

This change is

mrmiywj · 2016-06-19T08:20:06Z

cc @paulrouget
Since my modify to servo/glutin has not been merged in crate.io. This PR cannot compiled successfully.

KiChjang · 2016-06-19T08:26:48Z

You can update glutin by modifying the Cargo.toml file under ports/glutin and running ./mach update-cargo -p glutin

paulrouget · 2016-06-20T08:39:37Z

ports/glutin/window.rs

+                                self.mouse_pos.set(Point2D::new(x, y));
+                                self.handle_mouse(mouse_button, element_state, x, y);
+                                self.event_queue.borrow_mut().push(
+                                WindowEvent::MouseWindowMoveEventClass(Point2D::typed(x as f32, y as f32)));


You need a move event, but also a click event. So you'll need to call handle_mouse here as well.

Hmm, I'm wrong here. But I do think the solution should be:

set the new position

push the MouseWindowMoveEventClass into event_queue

call handle_mouse

For the move event, should I also call handle_mouse? I though handle_mouse is only for the click event.

I believe the mousemove event should happen when the window gains focus.
See my latest comment.

Manishearth · 2016-06-20T08:52:15Z

ports/glutin/Cargo.toml

@@ -17,7 +17,7 @@ log = "0.3.5"
 msg = {path = "../../components/msg"}
 net_traits = {path = "../../components/net_traits"}
 script_traits = {path = "../../components/script_traits"}
-servo-glutin = "0.4"
+servo-glutin = { git = "https://github.com/servo/glutin"}


servo/glutin#98

Just change this line to "0.4.24". Remove the git link

Manishearth · 2016-06-20T08:52:58Z

r? @paulrouget

Manishearth · 2016-06-20T08:53:07Z

@bors-servo delegate=paulrouget

bors-servo · 2016-06-20T08:53:08Z

✌️ @paulrouget can now approve this pull request

paulrouget · 2016-06-20T09:20:37Z

STR:

load the above example
click anywhere. see the red div being moved exactly where the click occurs
move the mouse anywhere above the window. Do not click. Let's call this new position P1
with Alt-Tab (or Cmd-Tab), bring another non-Servo window above the Servo window (make it so the servo window loses focus)
move the mouse 10 pixels to the right. Let's call this new position P2
with Alt-Tab (or Cmd-Tab), bring back the Servo window. Do not move mouse.
Click
the red div moves to P1, not to P2. Which is wrong.

Ideally, we should get a mouse move event when 6) happens.

<style>
  div {
    width: 0px;
    height: 0px;
    position: absolute;
    border: 2px solid red;
    top: 0;
    left: 0;
  }
</style>
<div></div>
<script>
  var div = document.querySelector("div");
  window.addEventListener("click", e => {
    div.style.top = e.clientY - 2 + "px";
    div.style.left = e.clientX - 2 + "px";
  });
</script>

mrmiywj · 2016-06-20T11:06:06Z

@paulrouget
Hi, I've updated my PR. I do think it's enough.
But unfortunately, it cannot be compiled on my RMBP.

Updating git repository `https://github.com/servo/glutin`
error: no matching package named `servo-glutin` found (required by `glutin_app`)
location searched: https://github.com/servo/glutin
version required: *
Build completed in 0:00:07

Is there anything wrong with my modification to Cargo.toml?

jdm · 2016-06-20T12:31:14Z

The change to gluten will need to be published, and then we'll need to use the version from crates.io instead of git.

mrmiywj · 2016-06-20T12:35:53Z

@jdm
So what do I need to do now? My PR is merged by servo/glutin, which is forked.

Manishearth · 2016-06-20T12:37:15Z

./mach update-cargo -p servo-glutin , and commit the lockfile changes

mrmiywj · 2016-06-20T12:40:05Z

@Manishearth

➜  servo git:(update-mouse-coordinate-when-focus) ✗ ./mach update-cargo -p servo-glutin
components/servo
    Updating registry `https://github.com/rust-lang/crates.io-index`
    Updating git repository `https://github.com/servo/glutin`
error: no matching package named `servo-glutin` found (required by `glutin_app`)
location searched: https://github.com/servo/glutin
version required: *
ports/cef
    Updating registry `https://github.com/rust-lang/crates.io-index`
    Updating git repository `https://github.com/servo/glutin`
error: no matching package named `servo-glutin` found (required by `glutin_app`)
location searched: https://github.com/servo/glutin
version required: *
ports/geckolib
error: package id specification `servo-glutin` matched no packages

It also complains. QAQ

Manishearth · 2016-06-20T12:41:58Z

Oh, undo your check to that cargo.toml too. Mention servo-glutin 0.4 or 0.4.24

mrmiywj · 2016-06-20T12:50:22Z

Sorry for that maybe I cannot understand what you said.
Maybe u said is that I should specify the servo-glutin version based on my commit? Like this:
servo-glutin = { git = "https://github.com/servo/glutin", version = "0.4.24"}
But it also complained...

➜  servo git:(update-mouse-coordinate-when-focus) ✗ ./mach update-cargo -p servo-glutin
components/servo
    Updating registry `https://github.com/rust-lang/crates.io-index`
    Updating git repository `https://github.com/servo/glutin`
error: no matching package named `servo-glutin` found (required by `glutin_app`)
location searched: https://github.com/servo/glutin
version required: ^0.4.24
ports/cef
    Updating registry `https://github.com/rust-lang/crates.io-index`
    Updating git repository `https://github.com/servo/glutin`
error: no matching package named `servo-glutin` found (required by `glutin_app`)
location searched: https://github.com/servo/glutin
version required: ^0.4.24
ports/geckolib
error: package id specification `servo-glutin` matched no packages

Or just like the Cargo.toml in master? Like:
servo-glutin="0.4.24"
But it will use the repo on crate.io, not our own repo, I think.
It also complained...

➜  servo git:(update-mouse-coordinate-when-focus) ✗ ./mach update-cargo -p servo-glutin
components/servo
    Updating registry `https://github.com/rust-lang/crates.io-index`
    Updating cocoa v0.4.3 -> v0.4.4
    Updating servo-glutin v0.4.23 -> v0.4.24
ports/cef
    Updating registry `https://github.com/rust-lang/crates.io-index`
error: failed to select a version for `cocoa` (required by `servo-glutin`):
all possible versions conflict with previously selected versions of `cocoa`
  version 0.4.3 in use by cocoa v0.4.3
  possible versions to select: 0.4.4
ports/geckolib
error: package id specification `servo-glutin` matched no packages

jdm · 2016-06-20T13:03:03Z

You may need to run ./mach cargo-update -p cocoa first.

mrmiywj · 2016-06-20T13:19:49Z

I've updated this PR.
But if this PR is not merged by the crate.io. I do think it will not compile successfully...

jdm · 2016-06-20T13:40:14Z

Looks like there's a change missing to Servo's code:

/home/travis/build/servo/servo/ports/glutin/window.rs:268:29: 268:39 error: this pattern has 2 fields, but the corresponding variant has 1 field [E0023]
/home/travis/build/servo/servo/ports/glutin/window.rs:268                             Some(x, y) => {
                                                                                      ^~~~~~~~~~
/home/travis/build/servo/servo/ports/glutin/window.rs:268:29: 268:39 help: run `rustc --explain E0023` to see a detailed explanation
error: aborting due to previous error
Build failed, waiting for other jobs to finish...
error: Could not compile `glutin_app`.

I think it's supposed to be Some((x, y)).

KiChjang · 2016-07-05T15:33:01Z

ports/glutin/window.rs

@@ -279,7 +288,8 @@ impl Window {
                    MouseScrollDelta::PixelDelta(dx, dy) => (dx, dy),
                };
                let phase = glutin_phase_to_touch_event_type(phase);
-                self.scroll_window(dx, dy, phase);
+                self.scroll_window(
+dx, dy, phase);


Revert this change please.

KiChjang · 2016-07-05T15:34:08Z

ports/glutin/window.rs

-                        let mouse_pos = self.mouse_pos.get();
-                        self.handle_mouse(mouse_button, element_state, mouse_pos.x, mouse_pos.y);
-                   }
+                    mouse_button == MouseButton::Right {


I think the problem is here, the mouse_button here should match up with the last line's mouse_button columns, and the match expression on the next line should be de-indented once.

mrmiywj · 2016-07-05T15:44:13Z

@KiChjang Is the indent correct now?

KiChjang · 2016-07-05T15:55:53Z

ports/glutin/window.rs

+                               self.handle_mouse(mouse_button, element_state, mouse_pos.x, mouse_pos.y);
+                           }
+                       }
+                }


The last one is correct, but you can see that the one above it is indented too much to the right. De-indenting once should fix it.


        when window gains focus, update mouse coordinate

mrmiywj · 2016-07-05T16:10:28Z

I think it's due to the two lines of if conditions, which may mistake my emacs auto-intending.

KiChjang · 2016-07-05T16:31:59Z

@bors-servo r+

Thank you!

bors-servo · 2016-07-05T16:32:00Z

📌 Commit 6bcca8e has been approved by KiChjang

bors-servo · 2016-07-05T17:18:50Z

⌛ Testing commit 6bcca8e with merge d99ff8d...


        Auto merge of #11794 - mrmiywj:update-mouse-coordinate-when-focus, r=…

…KiChjang when window gains focus, update mouse coordinate  ---  - [ ] `./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).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because these cannot be tested automated.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11794)

bors-servo · 2016-07-05T18:31:18Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

highfive assigned Manishearth Jun 19, 2016

highfive added the S-awaiting-review label Jun 19, 2016

paulrouget reviewed Jun 20, 2016
View changes

Manishearth reviewed Jun 20, 2016
View changes

highfive assigned paulrouget and unassigned Manishearth Jun 20, 2016

nox added S-needs-code-changes and removed S-awaiting-review labels Jun 20, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Jun 20, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Jun 20, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Jun 20, 2016

KiChjang added S-needs-code-changes and removed S-awaiting-review S-needs-rebase labels Jul 5, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Jul 5, 2016

KiChjang reviewed Jul 5, 2016
View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Jul 5, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Jul 5, 2016

KiChjang reviewed Jul 5, 2016
View changes

KiChjang added S-needs-code-changes and removed S-awaiting-review labels Jul 5, 2016

when window gains focus, update mouse coordinate

Loading status checks…

6bcca8e

highfive added S-awaiting-review and removed S-needs-code-changes labels Jul 5, 2016

highfive assigned KiChjang and unassigned paulrouget Jul 5, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 5, 2016

bors-servo merged commit 6bcca8e into servo:master Jul 5, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jul 5, 2016

servo / servo

when window gains focus, update mouse coordinate #11794

when window gains focus, update mouse coordinate #11794

mrmiywj commented Jun 19, 2016 •

edited by larsbergstrom

mrmiywj commented Jun 19, 2016

KiChjang commented Jun 19, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Jun 20, 2016

Manishearth commented Jun 20, 2016

bors-servo commented Jun 20, 2016

paulrouget commented Jun 20, 2016

mrmiywj commented Jun 20, 2016

jdm commented Jun 20, 2016

mrmiywj commented Jun 20, 2016

Manishearth commented Jun 20, 2016

mrmiywj commented Jun 20, 2016

Manishearth commented Jun 20, 2016 •

edited

mrmiywj commented Jun 20, 2016

jdm commented Jun 20, 2016

mrmiywj commented Jun 20, 2016

jdm commented Jun 20, 2016 •

edited

This comment has been minimized.

This comment has been minimized.

mrmiywj commented Jul 5, 2016

This comment has been minimized.

mrmiywj commented Jul 5, 2016

KiChjang commented Jul 5, 2016

bors-servo commented Jul 5, 2016

bors-servo commented Jul 5, 2016

bors-servo commented Jul 5, 2016

servo / servo

Join GitHub today

when window gains focus, update mouse coordinate #11794

when window gains focus, update mouse coordinate #11794

Conversation

mrmiywj commented Jun 19, 2016 • edited by larsbergstrom

mrmiywj commented Jun 19, 2016

KiChjang commented Jun 19, 2016

This comment has been minimized.

paulrouget Jun 20, 2016

This comment has been minimized.

mrmiywj Jun 20, 2016

This comment has been minimized.

paulrouget Jun 20, 2016

This comment has been minimized.

Manishearth Jun 20, 2016

This comment has been minimized.

Manishearth Jun 20, 2016 • edited

Manishearth commented Jun 20, 2016

Manishearth commented Jun 20, 2016

bors-servo commented Jun 20, 2016

paulrouget commented Jun 20, 2016

mrmiywj commented Jun 20, 2016

jdm commented Jun 20, 2016

mrmiywj commented Jun 20, 2016

Manishearth commented Jun 20, 2016

mrmiywj commented Jun 20, 2016

Manishearth commented Jun 20, 2016 • edited

mrmiywj commented Jun 20, 2016

jdm commented Jun 20, 2016

mrmiywj commented Jun 20, 2016

jdm commented Jun 20, 2016 • edited

This comment has been minimized.

KiChjang Jul 5, 2016

This comment has been minimized.

KiChjang Jul 5, 2016

mrmiywj commented Jul 5, 2016

This comment has been minimized.

KiChjang Jul 5, 2016

mrmiywj commented Jul 5, 2016

KiChjang commented Jul 5, 2016

bors-servo commented Jul 5, 2016

bors-servo commented Jul 5, 2016

bors-servo commented Jul 5, 2016

mrmiywj commented Jun 19, 2016 •

edited by larsbergstrom

Manishearth Jun 20, 2016 •

edited

Manishearth commented Jun 20, 2016 •

edited

jdm commented Jun 20, 2016 •

edited