Skip to content

Fixed dom event coordinates for scaled container #10096

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
Merged
merged 2 commits into from
Feb 12, 2021
Merged

Fixed dom event coordinates for scaled container #10096

merged 2 commits into from
Feb 12, 2021
+24 −14

Conversation

kawsndriy
Copy link
Contributor

@kawsndriy kawsndriy commented Nov 13, 2020 •
edited
Loading

When map container belongs to a CSS transformed tree, the getBoundingClientRect() based solutions report incorrect
coordinates.

This change accounts for scale during DOM events processing and fixes #6079

This solution would not work for the cases when css transform includes rotation or skew. But should serve as a decent band-aid for the case of simple scaling.

Launch Checklist

The following items have not been done. Please let me know if these are blockers for the merge.

  • include before/after visuals or gifs if this PR includes visual changes
  • write tests for all new functionality
  • post benchmark scores
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog></changelog>

Sorry, something went wrong.

@kawsndriy
Fixed dom event coordinates for scaled container
5a8a684 
When map container belongs to a CSS transformed tree,
the `getBoundingClientRect()` based solutions report incorrect
coordinates.

This change accounts for scale during DOM events
processing and fixes mapbox#6079

This solution would not work for the cases when css transform includes
rotation or skew. But should serve as a decent band-aid for the case of
simple scaling.
@kawsndriy
Copy link
Contributor Author

I'm trying to fix the unit tests, but on my osx I'm getting Failed to initialize WebGL error when running yarn test-unit ui/map.test.js - not sure yet how to fix this. Tips are highly appreciated.

@kawsndriy
Copy link
Contributor Author

Ah, I think my webgl issues are due to node 12.13 version. As mentioned in https://github.com/stackgl/headless-gl:

Note [macOS only]: due to an inadvertant low-level breaking change in libuv's process handling code, this package doesn't return a gl context when running nodejs version 12.13.1 through to 12.16.1, and 13.0.0 through to 13.6.0 on macOS. A fix has been released in Node.js versions 12.16.2 and 13.7.0. Other platforms are unaffected.

Checking on 12.16.3 now.

@kawsndriy
Copy link
Contributor Author

@ryanhamley can you please check this one? It avoids using offsetX/offsetY properties, so the map panning experience remains the same

@edgars-vasiljevs
Copy link

thanks @kawsndriy, fix works great.

@kawsndriy
Copy link
Contributor Author

Friends, can you please check this out?

@ryanhamley
Copy link
Contributor

ryanhamley commented Nov 30, 2020 •
edited
Loading

Hi @kawsndriy sorry for the delay. We're working on some deadlines right now and most other work has been put on the back burner. I will try to give this a thorough review on Thursday or Friday of this week.

@CLAassistant
Copy link

CLAassistant commented Dec 8, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@ryanhamley
Copy link
Contributor

ryanhamley commented Jan 12, 2021 •
edited
Loading

@kawsndriy if you still want to contribute this, you'll need to sign our new Contributor License Agreement. I'm going to close this as stale but if you sign the CLA, we can re-open it.

@ryanhamley ryanhamley closed this Jan 12, 2021
@rgazelot
Copy link

I was waiting for excited news about this issue :'(

@ryanhamley
Copy link
Contributor

No judgement on this PR. We'll be happy to re-open and review it once the CLA is signed or if anyone else wants to take up this work.

@kawsndriy
Copy link
Contributor Author

CLA signed. Please consider reopening @ryanbaumann

@ryanhamley
Copy link
Contributor

ryanhamley commented Jan 20, 2021 •
edited
Loading

@kawsndriy are you sure it worked? i didn't get an email notifying me of it and the link above still shows it not being sign. Maybe try again? Either way, I'll reopen this in anticipation of getting that resolved. Edit: Nevermind. It took a bit of time for the CLA to update. Thanks for signing!

@ryanhamley ryanhamley reopened this Jan 20, 2021
ryanhamley
ryanhamley approved these changes Feb 12, 2021
View reviewed changes
Copy link
Contributor

@ryanhamley ryanhamley left a comment

Choose a reason for hiding this comment

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

This looks good! Thanks for the contribution. I think it's fine that it only handles scale at this point since that's likely to be the most common transform used on maps.

@ryanhamley ryanhamley merged commit c8490fa into mapbox:main Feb 12, 2021
@ryanhamley ryanhamley mentioned this pull request Feb 12, 2021
Open
@adamcbrewer
Copy link

So happy to see this go in, thanks again for the work @kawsndriy

@karimnaaji karimnaaji added this to the v2.2 milestone Mar 1, 2021
blq
blq reviewed Apr 11, 2021
View reviewed changes
src/util/dom.js
// Note: `el.offsetWidth === rect.width` eliminates the `0/0` case.
const scaling = el.offsetWidth === rect.width ? 1 : el.offsetWidth / rect.width;
return new Point(
(e.clientX - rect.left) * scaling,
Copy link

Choose a reason for hiding this comment

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

el.clientLeft/Top is not used anymore in this version, is that correct?

@ghost ghost mentioned this pull request Apr 12, 2021
Closed
@steff-o steff-o mentioned this pull request Jan 24, 2022
Closed
@andresgutgon andresgutgon mentioned this pull request Jun 7, 2022
Merged
@andresgutgon
Copy link

Thanks @kawsndriy this PR pointed me in the right direction 🙌

@huanglii huanglii mentioned this pull request Dec 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blq blq

@ryanhamley ryanhamley

@shiftonetothree shiftonetothree

@Rmkasun-8 Rmkasun-8

Assignees
No one assigned
Labels
bug 🐞
Projects
None yet
Milestone
v2.2
Development

Successfully merging this pull request may close these issues.

Mouse events are shifted after a css transform scale on the map container