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

Fixes shifted mouse events when CSS transform-scale applied #8017

Conversation

adamcbrewer
Copy link

@adamcbrewer adamcbrewer commented Mar 11, 2019

This fixes 6079 and part of 7701 where mouse and touch events are incorrect when CSS transforms are applied to the map or any parent element wrapping it.

NOTE: The PR is a draft until someone can confirm this could be a sensible fix.

  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page

@adamcbrewer adamcbrewer changed the title Fixed shifted mouse events when CSS transform-scale applied Fixes shifted mouse events when CSS transform-scale applied Mar 11, 2019
@adamcbrewer adamcbrewer marked this pull request as ready for review March 27, 2019 16:22
@adamcbrewer
Copy link
Author

For some reason simulated mouse events are failing in test/unit/ui/marker.test.js, but I'm not sure what could be causing the issue. Is this an existing issue or a dependancy one? Pretty sure it's failing on simulate.mousedown(el); before any equality checks are even performed.

@adamcbrewer adamcbrewer force-pushed the feature/css-transforms-and-mouse-events branch from db026a7 to 4d7fadc Compare June 26, 2019 07:38
@adamcbrewer
Copy link
Author

I've updated to v1.0.0, but I still need someone to review these changes before looking into fixing the unit tests

@adamcbrewer adamcbrewer force-pushed the feature/css-transforms-and-mouse-events branch from 4d7fadc to fffc21b Compare July 3, 2019 19:10
@adamcbrewer
Copy link
Author

@mourner not sure who to ask, but would be great if someone from the team could review this proposal. I might need some help with the tests, but I don't want to spent time on fixing them if this isn't something that's likely to make it in.

@ryanhamley
Copy link
Contributor

ryanhamley commented Aug 6, 2019

Hi @adamcbrewer thanks for the PR and for being so patient. I think that this approach probably is not the right way to solve this issue. The cssTransformPoint function seems like it would put us on the hook to implement and support all current CSS transform functions plus any new ones that are added in the future. In effect, GL JS would have to maintain an implementation of CSS transform. Limiting it to certain transforms seems like a half-measure that leads to a bad user experience. Additionally, this approach requires the developer to implement the transform in two places, the CSS and the map constructor.

I think that @jingsam was on the right track with their example in #7701 (comment) even if that particular implementation has some edge cases such as the ones you mentioned in that issue. The map shouldn't need to know what, if any, transforms have been applied to it. Using offsetX and offsetY seems like the most promising approach; we'll just have to finesse how we handle panning and the mouse entering other elements. I'm not sure yet what the best way to achieve that is, but I think this is where I'd personally focus my efforts.

@peterqliu any thoughts on the best way forward on this issue?

@peterqliu
Copy link
Contributor

peterqliu commented Aug 7, 2019

🤔 to take a step back, using transforms on the map verges on an antipattern to me, and is at best an edge case to support. Scaling is best done by resetting map dimensions directly.

That said, I think this feature is limited in its upside, and in doing so introduces leaky abstractions (support for only certain transforms, on elements no higher in the DOM hierarchy) with the potential to add confusion and break expectations of intended behavior.

I second @ryanhamley that the offsetX route is most promising, if we can iron out mouseexit weirdness without adding significant complexity.

@jingsam
Copy link
Contributor

jingsam commented Aug 7, 2019

@peterqliu transform map does have its use cases. For example, If we create a dashboard that have maps, when presenting it on super big screen (I mean 4K X 2, or 1K X 6), it would cause two major problem:

  1. poor performance: too many request to fulfill the map and memory consumption is huge.
    2.too small symbol: the distance from a viewer to a big screen is far from desktop PC, so the text and icon is difficult to distinguish.

On this case, we scale the map using transform, perfectly solve the problem.

offsetX and offsetY are indeed a promising solution. However, touch events do not support offsetX and offsetY, so we should find another way on the mobile.

@adamcbrewer
Copy link
Author

Thanks for the reviews @peterqliu and @ryanhamley. I actually agree that it's an antipattern and also trying to maintain a set of css transforms is something to avoid.

I really liked the offsetX and offsetY solution by @jingsam as well, but as he mentioned there are additional considerations (such as mobile) and also the ones I mentioned in this comment for 7701.

I agree that handling transforms isn't the responsibility of GL JS, but in the comment above the offsetX and offsetY solution only works well if the mouse is still over the scaled node containing the css scaled/transformed map. If someone pans a map and drags the mouse outside of the scaled area it's almost completely unusable and you might get a lot of people creating issues for it 😄

Perhaps there's a way to kill all the events if the map has been scaled and the mouse/pointer has left the map area? Maybe supporting we find some way of supporting offsetX and offsetY, but create an online example for handling css transform cases?

@peterqliu
Copy link
Contributor

peterqliu commented Aug 7, 2019

@jingsam tangent regarding big screens: you make a great point that big screens tend to also be farther away, where we can see less detail. This hints at the actual best practice: reducing screen resolution as they get bigger, as users can't see as finely anyway. We see this pattern in display resolutions between phones, tablets, and laptops.

Handling this in the display settings is better than in CSS, because we can build the map once for all screens, without adding conditional logic to apply different transform scales for different screen sizes.

@jingsam
Copy link
Contributor

jingsam commented Aug 7, 2019

@peterqliu I agree with you that adjusting screen resolution is a feasible way. However, sometimes we share this big screen with other applications. Adjusting its resolution maybe break other application appearance. Also, our dashboard need to deploy to different big screen, like 4k, 6k, 8k. Adjusting resolution maybe a little trivial. In our practice, we calculate the ratio between screen and dashboard size and scale the dashboard to autofit to the big screen. That's why we depend on transform feature a lot. In out fork of mapbox-gl, we use offsetX, offsetY in mouse events and fallback to the original way in touch events.

@ryanhamley
Copy link
Contributor

you make a great point that big screens tend to also be farther away, where we can see less detail. This hints at the actual best practice: reducing screen resolution as they get bigger, as users can't see as finely anyway. We see this pattern in display resolutions between phones, tablets, and laptops.

@peterqliu @jingsam Given this paradigm, is it even a big deal if offsetX and offsetY don't work on mobile devices? The assumption would be that transforms should only really be necessary on large screens which by definition are not mobile devices. This doesn't solve the issue of mouse events outside the transformed element but it would simplify the implementation a bit by avoiding the concerns with mobile. Or am I missing a mobile use case that would require a transform of some kind?

@ryanhamley
Copy link
Contributor

Also, would being able to set the devicePixelRatio help with larger screen resolutions instead of scaling the map? #7333

@jingsam
Copy link
Contributor

jingsam commented Aug 8, 2019

@ryanhamley I think we could use offsetX, offsetY in mouse events. It has better performance as it is native supported. Also, it compatible to all kinds of transform. When mouse moves outside of the transformed map, I assume that the mouse is no longer interact with the map.

@ryanhamley thanks for your advice. Using devicePixelRatio maybe a tricky way to go around this problem. I guess that will increase the burden of GPU. I will test against it.

@adamcbrewer
Copy link
Author

@jingsam just to comment on what you said:

When mouse moves outside of the transformed map, I assume that the mouse is no longer interact with the map.

In fact Mapbox does allow panning the map outside the map bounds as long as the mouse/pointer is still held down. This is where I've noticed the issues with offsetX and offsetY where once you're moving over elements with different transforms the behaviour becomes very erratic to say the least.

So really if we use offsetX and offsetY we need to ask

  1. should we allow panning outside of the map area (if the user is still interacting) and
  2. if so we should have at least show an example of this case, or at least how to kill the events once pointer has 'left' the map area

@adamcbrewer
Copy link
Author

I think this PR should be closed and a new one created if offsetX and offsetY are going to be the way forward.

@ryanhamley
Copy link
Contributor

agreed @adamcbrewer are you interested in working on a new PR for this? i'm not sure when someone on the GL JS team would get to it

@ryanhamley ryanhamley closed this Oct 3, 2019
@adamcbrewer
Copy link
Author

@ryanhamley I'd be happy to, but it would be good to get some thoughts on my previous comment: #8017 (comment)

In fact Mapbox does allow panning the map outside the map bounds as long as the mouse/pointer is still held down. This is where I've noticed the issues with offsetX and offsetY where once you're moving over elements with different transforms the behaviour becomes very erratic to say the least.

So really if we use offsetX and offsetY we need to ask:

  1. should we allow panning outside of the map area (if the user is still interacting) and
  2. if so we should have at least show an example of this case, or at least how to kill the events once pointer has 'left' the map area"

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.

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