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

Align interactions when plot created inside scaled and/or translated elements using CSS transform #5193

Merged
merged 63 commits into from
Nov 18, 2020
Merged

Conversation

alexhartstone
Copy link
Contributor

@alexhartstone alexhartstone commented Oct 6, 2020

Fixes #888.
This is the first commit of a few fixes for interactions with plots that are broken due when css transform scaling is applied.
The general aim is to undo the transform matrices applied to these elements for the interactions.

TODOs:

  • clicking the end of an axis in 2D cartesian to type a new number: the text entry box shows up in the wrong place
  • fixing 2D cartesian pan
  • fixing zoom & pan in ternary & polar
  • fixing scattermapbox hover & select
  • fixing choroplethmapbox hover
  • fixing the position of hover on sankey trace
  • fixing the position of hover on parcats trace
  • adding hover, select and drag-zoom tests for cartesian subplots
  • adding hover, select and drag-zoom tests for polar subplots
  • adding hover, select and drag-zoom tests for mapbox subplots i.e. choroplethmapbox and scattermapbox
  • adding hover, select and drag-zoom tests for geo subplots e.g. choropleth
  • adding hover, select and drag-zoom tests for ternary subplots
  • test using different browsers namely Firefox, Chrome and Safari

@alexhartstone alexhartstone marked this pull request as ready for review October 12, 2020 04:00
@archmoj archmoj added status: in progress bug something broken community community contribution and removed status: in progress labels Oct 17, 2020
@archmoj
Copy link
Contributor

archmoj commented Oct 21, 2020

Thanks very much @alexhartstone for submitting this PR.
I spend some time testing this on different plot types.
Please view/pick my commits on this dev branch: https://github.com/plotly/plotly.js/compare/pull-5193_ms that fixes few minor issues to start.

@archmoj
Copy link
Contributor

archmoj commented Oct 22, 2020

@alexhartstone FYI - I also added few more commits to that branch that may be of interest.

@archmoj
Copy link
Contributor

archmoj commented Oct 22, 2020

@alexhartstone just to let you know. I am investing more time on the dev-branch today. I will let you when I was able to complete my journey.

@archmoj
Copy link
Contributor

archmoj commented Oct 22, 2020

@alexhartstone please pick my commits from https://github.com/plotly/plotly.js/compare/pull-5193_ms or alternatively allow us as the maintainers access to push into your branch. Please find the instructions here https://github.com/plotly/plotly.js/blob/master/.github/PULL_REQUEST_TEMPLATE.md

With all those fixes the transforms e.g. scale, scaleX, scaleY, translate now works as expected on cartesian, geo, gl2d, gl3d, ternary and polar plots.
You should find a similar fix for the mapbox plot.
Also gl3d hovers does not scale according to the transform at the moment. It might be a desirable behaviour actually (and maybe we should try achieve the same with other subplots). However you should investigate why this is happening (if related to pixelRatio or not, etc).
Then you should start adding a number of tests for different subplots to ensure the hover, select and zoom works as expected.
Also it would be nice to catch the positioning issue when rotation is present.
Thanks!

@archmoj
Copy link
Contributor

archmoj commented Nov 18, 2020

@alexhartstone I just noticed that there is a problem with the position of hover on sankey trace.
Could you please fix that?

@archmoj
Copy link
Contributor

archmoj commented Nov 18, 2020

@alexhartstone Also the parcat hover position is not correct which is added to the list on the PR description.

@archmoj
Copy link
Contributor

archmoj commented Nov 18, 2020

@alexcjohnson This PR is now ready for review.
FYI - @alexhartstone will address minor issues with the position of hover in parcats and sankey as well.

@archmoj
Copy link
Contributor

archmoj commented Nov 18, 2020

@alexcjohnson This PR is now ready for review.
FYI - @alexhartstone will address minor issues with the position of hover in parcats and sankey as well.

Issues with parcats and sankey nodes were addressed in a2f34c9 and 9fe42b6.

So this looks ready to 💃

@archmoj
Copy link
Contributor

archmoj commented Nov 18, 2020

Notable changes to mapbox as a result of this PR i.e. no longer modifying event.clientX and Y to display hover.
mapbox-fix

@archmoj archmoj changed the title fix issue #888: interactions misaligned when plot is inside transform: scale()-ed and/or translate()-ed elements Align interactions when plot created inside transform: scale()-ed and/or translate()-ed elements Nov 18, 2020
@archmoj archmoj changed the title Align interactions when plot created inside transform: scale()-ed and/or translate()-ed elements Align interactions when plot created inside scaled and/or translated elements using CSS transform Nov 18, 2020
Co-authored-by: Alex Johnson <[email protected]>
Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

Beautiful - nice work @archmoj and @alexhartstone! Fantastic tests.

💃 pending confirmation that mapbox works right in FF looks like you've confirmed 🎉

@archmoj archmoj merged commit 4afc441 into plotly:master Nov 18, 2020
@archmoj archmoj deleted the 888-style-transforms branch November 18, 2020 19:43
@archmoj
Copy link
Contributor

archmoj commented Nov 18, 2020

@alexhartstone please open a new issue describing why recomputing transform matrix on events is required in your particular application with a demo like this codepen.
Thank you!

@alexhartstone
Copy link
Contributor Author

Awesome @archmoj @alexcjohnson thanks guys!

@nicolaskruchten
Copy link
Contributor

Thank you so much for taking the initiative on this @alexhartstone !

@nlevari
Copy link

nlevari commented Dec 14, 2020

@alexhartstone, Thanks you so much for this fix. I spent days trying to workaround, unscaling and rescaling.
Just updated to the last version and everything works.

@alexhartstone
Copy link
Contributor Author

@nlevari :) :) thanks for the nice comment! Much credit is due to @archmoj too for his work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken community community contribution feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: interactions misaligned when plot is inside transform: scale()-ed element
6 participants