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

Correct mouse/touch position for maps scaled with CSS transform #218

Closed
wants to merge 0 commits into from

Conversation

khwstolle
Copy link

@khwstolle khwstolle commented Jul 22, 2021

When you scale the GL container using a CSS transform, i.e. transform: scale(N), interacting with the map becomes difficult because the DOM.touchPos and DOM.mousePos functions do not correct for this scaling factor N.

This issue has been raised before (Nov. 2019) in the Mapbox GL repository, but the PRs [1, 2] were never accepted due to the development team at the time waiting for a more robust solution to be made. Sadly, this solution never came to be. For this reason, this piece of code has been something that I had to manually paste into my own version of GL every time I required the functionality. I would like to re-evaluate whether this could be added to the Maplibre GL project at this time for practical reasons.

Full disclosure: this code is based on a rejected PR in the Mapbox GL repository [2] of which I am not the author. This PR was submitted on Dec 3, 2019 for version 1.x.

@HarelM
Copy link
Collaborator

HarelM commented Jul 23, 2021

Thanks for opening a PR!
Is there a way for you to write a small unit-test to make sure this behavior is preserved?
Also reading the code it seems that you are talking about scaling when the code is using offset.
Can you explain the reasoning for that?

@github-actions
Copy link
Contributor

Bundle size report:

Size Change: +57 B
Total Size Before: 206 kB
Total Size After: 206 kB

Output file Before After Change
maplibre-gl.js 197 kB 197 kB +57 B
maplibre-gl.css 9.46 kB 9.46 kB 0 B
ℹ️ View Details
Source file Before After Change
src/util/dom.js 885 B 950 B +65 B

@HarelM
Copy link
Collaborator

HarelM commented Sep 4, 2021

I would like to merge this if possible but for that I need a few more things:

  1. Open an issue describing the problem
  2. Add a unit test to make sure this is working as expected
  3. Resolve conflicts (probably related to the fact that we migrated to typescript)
  4. Add a changelog entry

Thanks!!

@khwstolle
Copy link
Author

khwstolle commented Sep 18, 2021

Hello @HarelM, I've been away for a while hence why there has been no progress. I will update the PR and make an issue in the next two weeks.

Edit: It appears that I have found no time in my schedule to finish the TypeScript conversion and test implementation. If somebody would like to pick this up, then please do. The required changes are trivial and require minimal work.

@HarelM
Copy link
Collaborator

HarelM commented Sep 18, 2021

Cool, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants