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.

Sign up for GitHub

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

Jump to bottom

Ensure DragRotate stops when the window looses focus. Fixes #3389 #4390

Merged
merged 3 commits into from
Mar 16, 2017
Merged

Ensure DragRotate stops when the window looses focus. Fixes #3389 #4390

merged 3 commits into from
Mar 16, 2017

Conversation

andrewharvey
Copy link
Collaborator

@andrewharvey andrewharvey commented Mar 9, 2017 •
edited
Loading

Launch Checklist

The depends on #4389. Without it you might not observe this PR fixing the issue.

  • briefly describe the changes in this PR

Ensure DragRotate stops when the window looses focus. Fixes #3389

  • write tests for all new functionality

I couldn't find any existing unit tests for DragRotate.

  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page

Manually tested on Linux with Chrome and Firefox (with #4389 applied).

mourner
mourner reviewed Mar 9, 2017
View reviewed changes
src/ui/handler/drag_rotate.js Outdated
*/
_onBlur(e) {
this._onUp(e);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there's nothing else going on, can we just pass this._onUp instead of adding an identical method?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tip. I've changed it. Note that you'll need to apply #4389 before this one.

@andrewharvey andrewharvey changed the title Ensure DragRotate stops when the window looses focus. Fixes #3389 [wip] Ensure DragRotate stops when the window looses focus. Fixes #3389 Mar 9, 2017
@andrewharvey
Copy link
Collaborator Author

This is incomplete, I need to apply the same logic to the DragPanHandler!

@andrewharvey andrewharvey changed the title [wip] Ensure DragRotate stops when the window looses focus. Fixes #3389 Ensure DragRotate stops when the window looses focus. Fixes #3389 Mar 9, 2017
@andrewharvey
Copy link
Collaborator Author

This is incomplete, I need to apply the same logic to the DragPanHandler!

I've updated it to apply the same logic to DragPan to fix the same behaviour when dragging.

  • BoxZoom has the same issue, but I think it's less of a problem as it leaves you in a state where you're finishing off the box zoom you started before changing tabs and it's clear that you need to click to finish it. However I'm not fussed either way if it remains this way or we apply the same logic to cancel the box zoom when the window looses focus. Thoughts?

  • Same for TouchZoomRotate however I'm not sure if it would be an issue there as loosing focus while you're still interacting with the map is less common. If it does happen it might mean that if you went to DragPan it would still be stuck in TouchZoomRotate. Do you think I should just go ahead and apply the same logic to de-activate on window blur anyway? I don't have a desktop touch device to more easily test some of these scenarios though.

@andrewharvey andrewharvey mentioned this pull request Mar 13, 2017
Closed
@lucaswoj
Copy link
Contributor

BoxZoom has the same issue, but I think it's less of a problem as it leaves you in a state where you're finishing off the box zoom you started before changing tabs and it's clear that you need to click to finish it. However I'm not fussed either way if it remains this way or we apply the same logic to cancel the box zoom when the window looses focus. Thoughts?

Same for TouchZoomRotate however I'm not sure if it would be an issue there as loosing focus while you're still interacting with the map is less common. If it does happen it might mean that if you went to DragPan it would still be stuck in TouchZoomRotate. Do you think I should just go ahead and apply the same logic to de-activate on window blur anyway? I don't have a desktop touch device to more easily test some of these scenarios though.

I see no harm in following up on these edge cases if/when they are reported. What's here is a huge improvement over what we have now! 🚢

@lucaswoj
Copy link
Contributor

I'm not able to repro the bug in master right now. Can you confirm that the bug is still present in master and verify that the repro steps in the bug report #4389 still apply?

  1. Use the control key change the camera on the map
  2. Quickly change windows with command + tab
  3. Go back to chrome
  4. Map remains in change camera mode even though the control key has been released

@andrewharvey
Copy link
Collaborator Author

I see no harm in following up on these edge cases if/when they are reported. What's here is a huge improvement over what we have now!

Agreed.

I'm not able to repro the bug in master right now. Can you confirm that the bug is still present in master and verify that the repro steps in the bug report #4389 still apply?

@lucaswoj I presume you mean #3389 (changing windows). I can't test that as it was reported on OSX. I presumed it was the same as changing tabs in Chrome which I can test, and this PR addresses, but maybe not.

@lucaswoj
Copy link
Contributor

I can sometimes repro changing tabs 👍 LGTM!

@lucaswoj lucaswoj merged commit e7368ca into mapbox:master Mar 16, 2017
andrewharvey added a commit to andrewharvey/mapbox-gl-js that referenced this pull request Mar 17, 2017
@andrewharvey
Ensure DragRotate stops when the window looses focus. Fixes mapbox#3389…
59d75b5 
… (mapbox#4390)

* Ensure DragRotate stops when the window looses focus. Fixes mapbox#3389

* simplify approach to de-activate DragRotate when the window loosese focus

* de-activate DragPan when window loosese focus
@andrewharvey andrewharvey mentioned this pull request Apr 22, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mourner mourner mourner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Map remains in change camera mode (control key) after changing windows
3 participants
@andrewharvey @lucaswoj @mourner