Fix geo plot hover interaction for Firefox #5607

LucaVazz · 2021-04-23T09:20:27Z

Related to #5525.

This PR fixes the hover interaction for geo plots (i.e. choropleth) in Firefox and extends the test suite for choropleths to also test for translate3d transforms.

Background:

The previously used d3.mouse depends on the getScreenCTM function provided by the browser: https://github.com/plotly/d3/blob/v3.5.18/src/event/mouse.js#L27
In Firefox, this function is not working as expected compared to other browsers: https://bugzilla.mozilla.org/show_bug.cgi?id=1446011
D3 does not intend to add a workaround for this problematic behavior: d3.mouse returns different values on Chrome and Firefox d3/d3#2810
Therefore we have to use another mean to access the mouse pointer position

LucaVazz · 2021-04-23T10:02:32Z

Demo of the fix:
before (v1.58.4): https://codepen.io/lucavazz/pen/mdOpgMY
after fix: https://codepen.io/lucavazz/pen/KKabQpL

Tested with

Firefox 88.0 on Windows
Firefox Developer Edition 89.0b3 on Windows
Chrome 90.0.4430.85 on Windows
Chrome Dev 92.0.4484.7 on Windows
Edge 90.0.818.46 on Windows
Safari 14.0.1 on Mac
Safari on iPad 14.4.2
Firefox 88.1.1 on Android
Firefox Klar 8.15.1 on Android
Firefox (Fennec) 87.0.0-rc.1 on Android
Firefox 88.0.0-beta.6 on Android
Chrome 90.0.4430.82 on Android
Chrome Dev 92.0.4484.6 on Android

archmoj · 2021-04-29T21:51:45Z

Thanks very much for the information, investigation and PR.
A couple of questions:

Did you investigate why the proposed fix does not work (and appear to break hover) when using Firefox 85 and lower?
d3.mouse is used in few other traces and components namely parcats, parcoords and sliders. Could you please investigate if a similar issue existed there? If so, we need to make a centralize function e.g. in src/lib to handle the rest.

…ix-choropleth-interactions-for-firefox

LucaVazz · 2021-05-03T13:00:45Z

Hey, thanks for having a look!

1.

Indeed, I can sadly reproduce it not working for Firefox v <= 85 or the current Firefox ESR (78.10.0esr).
This seems to be due to this bug that was fixed for Firefox 86: https://bugzilla.mozilla.org/show_bug.cgi?id=1684973
The description matches what I observed in testing: Firefox <= 85 sets offsetX & offsetY relative to the hovered SVG element, while >= 86 sets it relative to the SVG root. Chrome and Safari behave like Firefox >= 86.

What would be the preferred way to proceed? We could test the user agent string and fallback to d3.mouse for Firefox <= 85 to have at least non-translated maps working for it.
Alternatively we could use layerX & layerY, which are even more non-standard but seem to work fine from a quick test in Firefox ESR. (see https://developer.mozilla.org/en-US/docs/Web/API/UIEvent/layerX )
What's Plotly's general stance on adding code based on the user agent string? Personally I try to avoid it, but it seems like its our only option if we want to get Firefox <= 85 to work with it.

2.

I'll have a look how well the change works with parcats, parcoords and sliders in a bit

archmoj · 2021-05-03T13:46:33Z

Given the fact that FireFox >=86 is pretty new, I think we still need to handle versions <=85 as well.
Here are pointers to few locations in plotly.js code that userAgent is used to detect certain fallbacks.

plotly.js/src/lib/index.js

Lines 708 to 716 in a5a5de9

    
           var IS_SAFARI_REGEX = /Version\/[\d\.]+.*Safari/; 
        
           lib.isSafari = function() { 
        
               return IS_SAFARI_REGEX.test(window.navigator.userAgent); 
        
           }; 
        
           var IS_IOS_REGEX = /iPad|iPhone|iPod/; 
        
           lib.isIOS = function() { 
        
               return IS_IOS_REGEX.test(window.navigator.userAgent); 
        
           };

plotly.js/src/lib/preserve_drawing_buffer.js

Lines 44 to 60 in a5a5de9

    
           function getUserAgent() { 
        
               // similar to https://github.com/juliangruber/is-mobile/blob/91ca39ccdd4cfc5edfb5391e2515b923a730fbea/index.js#L14-L17 
        
               var ua; 
        
               if(typeof navigator !== 'undefined') { 
        
                   ua = navigator.userAgent; 
        
               } 
        
               if( 
        
                   ua && 
        
                   ua.headers && 
        
                   typeof ua.headers['user-agent'] === 'string' 
        
               ) { 
        
                   ua = ua.headers['user-agent']; 
        
               } 
        
               return ua; 
        
           }

…#5525)

src/lib/index.js

test/jasmine/tests/choropleth_test.js

LucaVazz · 2021-05-03T16:31:36Z

Added functions for both steps into lib - at first just in the index.js, since I'd argue its not complicated enough to warrant its own file.

Updated the pen and retested including Firefox ESR (78.10.0esr) and works fine for all now.

However, I can't really make sense out of the test failures, when trying it locally it seems all to work.

archmoj · 2021-05-03T16:37:13Z

However, I can't really make sense out of the test failures, when trying it locally it seems all to work.

Some tests are flaky. I re-run the CircleCI and they should pass.

LucaVazz · 2021-05-03T16:59:47Z

Some tests are flaky. I re-run the CircleCI and they should pass.

Thanks, that seems to have helped with the jasmine tests.

What about the other tests / the build? Is this bad character also due to a flaky step...?

archmoj · 2021-05-03T17:06:58Z

Some tests are flaky. I re-run the CircleCI and they should pass.

Thanks, that seems to have helped with the jasmine tests.

What about the other tests / the build? Is this bad character also due to a flaky step...?

The bad character problem is really NOT related to your PR. So please do not worry about that.
In fact, the problem appears to be related to non-ASCII characters in d3-v3 geo or a memory issue with node or browserify.
Once we could merge #5112 and cleanup geo code from our d3 fork it should disappear 🤞

LucaVazz · 2021-05-03T17:31:22Z

Regarding the other trace types:

parcats: The fixed function enables the hover on the bands to work properly in a current Firefox version. In the ESR it fails / shows the hovertext in the top left corner still.
unfixed example: https://codepen.io/lucavazz/pen/NWdQeaV
parcoords: For this trace I'm unable to produce any hover interaction. Could you please provide an example for it?

I'll have a closer look at the parcats problem in Firefox ESR and the sliders in the coming days.
Would you prefer to land it all together in one PR?

archmoj · 2021-05-03T17:50:13Z

Regarding the other trace types:

parcats: The fixed function enables the hover on the bands to work properly in a current Firefox version. In the ESR it fails / shows the hovertext in the top left corner still.
unfixed example: https://codepen.io/lucavazz/pen/NWdQeaV

parcoords: For this trace I'm unable to produce any hover interaction. Could you please provide an example for it?

I'll have a closer look at the parcats problem in Firefox ESR and the sliders in the coming days.
Would you prefer to land it all together in one PR?

parcoords hover is incomplete. But I noticed a drag issue when trying to move the pink selector on the bottom plots on [this codepen].
If one makes a d3Mouse using your patch and use it instead of d3.mouse calls, I think that should fix that issue as well. Right?

archmoj · 2021-05-03T18:01:54Z

On another note. It appears there are similar bugs related to geo handling zoom here:

plotly.js/src/plots/geo/zoom.js

Line 135 in a5a5de9

mouse0 = d3.mouse(this);

(and if you search for d3.mouse) which again could possibly be addressed using your fix around d3.mouse.

…5607)

LucaVazz · 2021-05-05T09:13:28Z

parcats: Works reliably in current Firefox (and all other browsers), unfortunately not in Firefox ESR. After a day of investigating I didn't find any indication what is causing this further problem in Firefox ESR.
Example before fix: https://codepen.io/lucavazz/pen/NWdQeaV
Example after fix: https://codepen.io/lucavazz/pen/qBrWwZN
geo zoom: Unfortunately replacing all d3.mouse calls in the zoom handlers of geo does not fix the problem in Firefox.
As far as I can tell the code paths using the mouse position behave correctly when replacing d3.mouse with Lib.getPositionFromD3Event. However I don't understand the zoom logic enough to debug the problem.
parcoords and sliders: I wan't able to reliably reproduce a problem in Firefox with them. Furthermore, they are out of scope of the original problem.

Unfortunately I'm already over-stretching the time I've allocated for this issue. So I'd appreciate to focus this PR on the geo hover, take on the parcats hover as best effort and record the problems with parcoords and/or sliders into new tickets to be tackled in the future.

archmoj · 2021-05-05T20:49:26Z

@LucaVazz thanks very much for the hard work and great contribution.
There is now a conflict in src/lib/index.js with upstream/master.
Could you please resolve it?

…-choropleth-interactions-for-firefox

LucaVazz · 2021-05-06T13:21:44Z

Thank you as well @archmoj for your great help :)

I've merged master into the fix branch, hopefully its good to go now.

LucaVazz · 2021-05-11T16:19:15Z

@archmoj / @alexcjohnson Would this change by the way be a candidate for 2.0.0 or would it likely land in a following release?

archmoj · 2021-05-12T17:07:04Z

@archmoj / @alexcjohnson Would this change by the way be a candidate for 2.0.0 or would it likely land in a following release?

That's a bug we could fix either in 2.0.0 or 2.0.1.
After ff60f21 commit a number of tests start failing.
@LucaVazz would you please fix the problem?
Also it would be nice if you could merge upstream/master one more time into this branch to have more robust tests running on the CI.
Thanks!

archmoj · 2021-05-12T17:43:00Z

The failing tests are in parcats and legend.

npm run test-jasmine parcats legend

and using FireFox

npm run test-jasmine parcats legend -- --FF

both should pass.

…-choropleth-interactions-for-firefox

…lotly#5607)" This reverts commit ff60f21.

LucaVazz · 2021-05-21T09:42:42Z

@archmoj I've merged master and reverted the parcats change.
Unfortunately I wasn't able to resolve the failures for parcats, so I'd vote for keeping this PR focused on the fix for geo and to leave parcats in Firefox as-is for now - there seems to be another constraint that I'm unable to fix atm.

archmoj · 2021-05-21T13:24:25Z

@archmoj I've merged master and reverted the parcats change.
Unfortunately I wasn't able to resolve the failures for parcats, so I'd vote for keeping this PR focused on the fix for geo and to leave parcats in Firefox as-is for now - there seems to be another constraint that I'm unable to fix atm.

@LucaVazz, OK thanks very much.
I'll have a look :)

archmoj · 2021-05-21T16:31:47Z

src/lib/index.js

@@ -715,6 +715,18 @@ lib.isIOS = function() {
    return IS_IOS_REGEX.test(window.navigator.userAgent);
 };

+var FIREFOX_VERSION_REGEX = /Firefox\/(\d+)\.\d+/;
+lib.getFirefoxVersion = function() {
+    var match = FIREFOX_VERSION_REGEX.exec(window.navigator.userAgent);


It appears regex.exec was not used in any other places of our code.
Wondering is it is possible to convert this regex.exec to a regex.test?

Unfortunately not, as regex.test does only return true or false, but we need to evaluate the exact version of Firefox in the user agent string.
However, exec has the same compatibility as test: see mdn

src/lib/index.js

…-choropleth-interactions-for-firefox

…tly#5525)" This reverts commit c437006.

archmoj · 2021-05-26T13:34:52Z

src/lib/index.js

+ * @returns An array with two numbers, representing the x and y coordinates of the mouse pointer
+ *   at the event relative to the targeted node.
+ */
+lib.getPositionFromD3Event = function() {


Non-blocking suggestion to avoid if statement inside the function:

lib.getPositionFromD3Event = isProblematicFirefox ? function() { // layerX and layerY are non-standard, so we only fallback to them when we have to: return [ d3.event.layerX, d3.event.layerY ]; } : function() { return [ d3.event.offsetX, d3.event.offsetY ]; };

Personally I find ternary operators way harder to read - but I'd say that's just personal preference in the end :D

I think the point of @archmoj's comment was not so much about whether to use if or a ternary operator, but pulling the conditional out of the function and into the initial script evaluation, for better performance. I agree that ternaries can be confusing when used on such large expressions, but we can do something like:

if (isProblematicFirefox) { lib.getPositionFromD3Event = function() {...} } else { lib.getPositionFromD3Event = function() {...} }

archmoj · 2021-05-26T13:37:31Z

LGTM.
@alexcjohnson could you please provide a review for this PR as well?

alexcjohnson

💃 LGTM, with or without the one comment about inverting the function/conditional.

LucaVazz · 2021-06-03T14:52:48Z

Thanks for the review @alexcjohnson and thanks again for all the help in getting it integrated properly @archmoj

archmoj · 2021-06-03T15:07:07Z

Thanks @LucaVazz for the contribution.
Could you please provide an update on #5525 or if we could close it?

LucaVazz added 3 commits April 22, 2021 18:07

replace d3.mouse in geo hover to work around firefox bug (plotly#5525)

341963e

extend choropleth tests to include translate3d transform (plotly#5525)

c437006

fix coding style in changes

7b3929a

LucaVazz marked this pull request as ready for review April 23, 2021 10:03

archmoj added status: reviewable bug something broken community community contribution labels Apr 23, 2021

Merge branch 'master' of https://github.com/LucaVazz/plotly.js into f…

7c23483

…ix-choropleth-interactions-for-firefox

archmoj requested a review from alexcjohnson May 3, 2021 13:50

move mouse position getter to lib and work-around Firefox bug (plotly…

f0e8e98

…#5525)

archmoj reviewed May 3, 2021

View reviewed changes

src/lib/index.js Outdated Show resolved Hide resolved

archmoj reviewed May 3, 2021

View reviewed changes

test/jasmine/tests/choropleth_test.js Outdated Show resolved Hide resolved

use global isNan for IE compatibility

c80ce55

replace d3.mouse in parcats hover to work around firefox bug (plotly#…

ff60f21

…5607)

LucaVazz requested a review from archmoj May 5, 2021 14:55

Merge branch 'master' of https://github.com/plotly/plotly.js into fix…

37b46d0

…-choropleth-interactions-for-firefox

archmoj added status: has TODOs and removed status: reviewable labels May 19, 2021

LucaVazz added 2 commits May 21, 2021 11:27

Merge branch 'master' of https://github.com/plotly/plotly.js into fix…

ef1dcbf

…-choropleth-interactions-for-firefox

Revert "replace d3.mouse in parcats hover to work around firefox bug (p…

56b3599

…lotly#5607)" This reverts commit ff60f21.

archmoj added status: reviewable and removed status: has TODOs labels May 21, 2021

archmoj reviewed May 21, 2021

View reviewed changes

src/lib/index.js Outdated Show resolved Hide resolved

archmoj reviewed May 21, 2021

View reviewed changes

src/lib/index.js Outdated Show resolved Hide resolved

LucaVazz added 2 commits May 26, 2021 14:45

Merge branch 'master' of https://github.com/plotly/plotly.js into fix…

7b5c166

…-choropleth-interactions-for-firefox

adapt to review comments (plotly#5525)

344fb91

LucaVazz requested a review from archmoj May 26, 2021 12:47

Revert "extend choropleth tests to include translate3d transform (plo…

a9da031

…tly#5525)" This reverts commit c437006.

archmoj reviewed May 26, 2021

View reviewed changes

alexcjohnson approved these changes Jun 1, 2021

View reviewed changes

archmoj merged commit d36b491 into plotly:master Jun 1, 2021

LucaVazz deleted the fix-choropleth-interactions-for-firefox branch June 3, 2021 14:52

LucaVazz mentioned this pull request Jun 3, 2021

Interactions with choropleth plot inside elements with transform: translate don't work in Firefox #5525

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix geo plot hover interaction for Firefox #5607

Fix geo plot hover interaction for Firefox #5607

LucaVazz commented Apr 23, 2021

LucaVazz commented Apr 23, 2021 •

edited

Loading

archmoj commented Apr 29, 2021

LucaVazz commented May 3, 2021

archmoj commented May 3, 2021

LucaVazz commented May 3, 2021

archmoj commented May 3, 2021

LucaVazz commented May 3, 2021 •

edited

Loading

archmoj commented May 3, 2021

LucaVazz commented May 3, 2021 •

edited

Loading

archmoj commented May 3, 2021

archmoj commented May 3, 2021

LucaVazz commented May 5, 2021

archmoj commented May 5, 2021

LucaVazz commented May 6, 2021 •

edited

Loading

LucaVazz commented May 11, 2021

archmoj commented May 12, 2021

archmoj commented May 12, 2021 •

edited

Loading

LucaVazz commented May 21, 2021 •

edited

Loading

archmoj commented May 21, 2021

archmoj May 21, 2021

LucaVazz May 26, 2021

archmoj May 26, 2021

LucaVazz Jun 1, 2021

alexcjohnson Jun 1, 2021

archmoj commented May 26, 2021

alexcjohnson left a comment

LucaVazz commented Jun 3, 2021

archmoj commented Jun 3, 2021

Fix geo plot hover interaction for Firefox #5607

Fix geo plot hover interaction for Firefox #5607

Conversation

LucaVazz commented Apr 23, 2021

LucaVazz commented Apr 23, 2021 • edited Loading

archmoj commented Apr 29, 2021

LucaVazz commented May 3, 2021

1.

2.

archmoj commented May 3, 2021

LucaVazz commented May 3, 2021

archmoj commented May 3, 2021

LucaVazz commented May 3, 2021 • edited Loading

archmoj commented May 3, 2021

LucaVazz commented May 3, 2021 • edited Loading

archmoj commented May 3, 2021

archmoj commented May 3, 2021

LucaVazz commented May 5, 2021

archmoj commented May 5, 2021

LucaVazz commented May 6, 2021 • edited Loading

LucaVazz commented May 11, 2021

archmoj commented May 12, 2021

archmoj commented May 12, 2021 • edited Loading

LucaVazz commented May 21, 2021 • edited Loading

archmoj commented May 21, 2021

archmoj May 21, 2021

Choose a reason for hiding this comment

LucaVazz May 26, 2021

Choose a reason for hiding this comment

archmoj May 26, 2021

Choose a reason for hiding this comment

LucaVazz Jun 1, 2021

Choose a reason for hiding this comment

alexcjohnson Jun 1, 2021

Choose a reason for hiding this comment

archmoj commented May 26, 2021

alexcjohnson left a comment

Choose a reason for hiding this comment

LucaVazz commented Jun 3, 2021

archmoj commented Jun 3, 2021

LucaVazz commented Apr 23, 2021 •

edited

Loading

LucaVazz commented May 3, 2021 •

edited

Loading

LucaVazz commented May 3, 2021 •

edited

Loading

LucaVazz commented May 6, 2021 •

edited

Loading

archmoj commented May 12, 2021 •

edited

Loading

LucaVazz commented May 21, 2021 •

edited

Loading