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

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

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

merged 63 commits into from Nov 18, 2020

Conversation

alexhartstone
Copy link
Contributor

@alexhartstone alexhartstone commented Oct 6, 2020 •
edited by archmoj

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
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 •
edited

@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
@archmoj @alexcjohnson
integrate fullLayout inverseTransform and m shorthand
9cc0a15 
Co-authored-by: Alex Johnson <johnson.alex.c@gmail.com>
@archmoj @alexcjohnson
refactor polar fix
95a81fc 
Co-authored-by: Alex Johnson <johnson.alex.c@gmail.com>
alexcjohnson
alexcjohnson approved these changes Nov 18, 2020
View reviewed changes
Copy link
Contributor

@alexcjohnson alexcjohnson left a comment •
edited

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
Member

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

@archmoj archmoj mentioned this pull request Nov 27, 2020
Merged
@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

@LucaVazz LucaVazz mentioned this pull request Feb 25, 2021
Open
@cderv cderv mentioned this pull request Sep 17, 2021
Closed
@cderv cderv mentioned this pull request Sep 14, 2022
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@archmoj archmoj archmoj left review comments

@alexcjohnson alexcjohnson alexcjohnson approved these changes

@antoinerg antoinerg Awaiting requested review from antoinerg

Assignees
No one assigned
Labels
status: reviewable type: bug type: community type: feature
Projects
None yet
Milestone
v1.58.0
Development

Successfully merging this pull request may close these issues.

bug: interactions misaligned when plot is inside transform: scale()-ed element
6 participants
@alexhartstone @archmoj @alexcjohnson @prateek-ag @nicolaskruchten @nlevari