Cache the last mousemove event #1304

rreusser · 2017-01-16T15:11:06Z

This PR attempts to solve #1274 by caching the mousemove event globally and then using it to re-set the hoverdata when the plot is clicked.

I don't think this is really satisfactory, but I didn't manage to come up with a more pleasant solution. The problem is that animations may change the data, but the hoverdata is only set when the mouse moves, so that if the data changes and you click before moving the mouse, hoverdata won't be correct. The next best solution seems to be to use the click event data to compute the hover data, except clientX and clientY aren't available on the click event; they're only available on the mousemove event. So the solution here is to simply use the most recent mouse move event, if available. This could work, but I'm concerned about what it means to only consider a single mousemove event. Does clientX mean the same thing against all subplots that might call this?

I don't think it's a good idea to merge this, but submitting it for feedback so we can come up with a better solution.

cc @etpinard @alexcjohnson @bpostlethwaite

rreusser · 2017-01-16T15:27:35Z

Instead of totally rethinking this, the other option is to just be more careful about scoping, caching and reusing the last hover data. The more I look at it and think about it, the more I think this might actually be a decent option. As long as it's robust, it seems much simpler than refactoring things to really just accomplish the same thing.

etpinard · 2017-01-16T22:19:42Z

This patch looks solid. I'm a little concerned about graphs with multiple cartesian subplots. @rreusser have you tried manually testing multi-subplots mocks?

Moreover, by the looks of it #1274 should only affect cartesian subplots. Is this that case?

rreusser · 2017-01-16T22:46:07Z

Yeah, multiple subplots and different trace types were my main concern here. Wanted to get feedback on if this is even reasonable or if there's a magic shortcut before going too far with it. This will probably require some refinement to ensure it's fully robust and only affects what it should affect.

bpostlethwaite · 2017-01-20T18:36:02Z

@alexcjohnson can you continue to review this one for us?

alexcjohnson · 2017-01-20T19:29:11Z

thanks @bpostlethwaite

@rreusser I like the concept of caching the event - but I think rather than re-evaluating it on click, I think it needs to happen after any change to the plot. That way if animation actually changes which point you're over, this will be captured by the displayed hover info (and therefore by the click too).

Note that this issue has existed forever (though never bubbled up into an issue...) in the case that something else that wasn't directly initiated by the user changes the plot while the mouse is over it - such as streaming, or restyle/relayout/whatever calls coming from some larger javascript app that the plot is embedded in. Would be great if whatever we do here can solve those cases too.

Streaming: eg https://plot.ly/~streaming-demos/4/double-pendulum-simulation/ - mouse over the history, the hovertext will remain even when the history has disappeared from that point.
JS app: here's one using postMessage: http://codepen.io/plotly/pen/ogpGbL?editors=101 - same deal, and same happens if a plot is inserted directly in the page rather than through postMessage to an iframe.

rreusser · 2017-01-20T19:46:30Z

Thanks for the context, @alexcjohnson. I think the path forward then is:

understand whether hover is attached to the whole plot or to specific subplots so that I can be sure to get subplot behavior correct, and
figure out the right hook for reevaluating. In order to get called with minimal additional effort and at the right times, I feel like maybe it should be called at the end of the calc step for baseplot=cartesian type traces. (this could get exposed as module.rehover or something, but I hate always adding more surface area to the module APIs (though it generally feels maintainable and straightforward))

rreusser · 2017-01-30T15:59:13Z

I've very slightly modified this to cache the last hover handler call instead. Whenever it runs calcdata, it now executes the last hover callback. I still need to figure out whether hoverdata is being updated at the correct time or not.

The gif below shows that it updates the hover to reflect new data even when the mouse hasn't moved. It's not shown here, but if you click repeatedly, the correct (that is, current) curve and point are returned in the click handler.

Live example here: https://rreusser.github.io/demos/plotly-unsupported/outdated-hover.html

rreusser · 2017-01-30T16:19:37Z

In order to finish this, I the main thing I need is hints regarding the cases in which it's still likely to fail. Here's what I can come up with:

is the end of calcdata the correct place to trigger this? It seemed to make sense since that's identically the same as the data changing, which is when this needs a recheck.
does it need to be unset? like if the visibility of a trace is toggled? Or perhaps it should be globally unset at the beginning of each replot?
other corner cases related to plot types or otherwise?

cc @alexcjohnson @etpinard

alexcjohnson · 2017-01-30T17:59:56Z

It seems a little funny to me to have something as presentational as hover take place during the calc stage. And there may be some changes that affect the hover label but don't recalc - like changing trace color? I think Plots.style gets called near the end of all of these paths, though that also seems like a little bit of a nonintuitive place to put it. Maybe checking for rehover actually warrants being an explicit step that Plotly.plot, Plotly.restyle, Plotly.relayout, and Plotly.update all tack onto their queues?

Another thing I worry about is whether maindrag always still exists and represents the actual dragger at the end of the update. We're probably not handling its creation and update using regular d3 idioms... until we do, might rehover fail if you do something like change a subplot size in the update - like Plotly.relayout(gd, {xaxis.domain: [0, 0.5]})? In a case like that I'd be fine if we just drop the hover label entirely - something like try { hover(...) } except { unhover(...) }.

Anyway the general behavior your demo shows (hover label jumps while animation is smooth) seems totally reasonable.

rreusser · 2017-01-30T18:43:54Z

Agreed. Pardon the burden, but that's exactly the feedback I was hoping for while it was still only a five-line patch. It would be possible to unpack values from the event so everything is definitely current when used later, but I think some of the references (maindrag?) were only accessible in the closure, which is where the out-of-date reference issue becomes tricky to resolve.

I'll iterate on this and see if I can make it a bit more satisfying. Thanks!

Conflicts: src/plot_api/plot_api.js src/plots/plots.js

rreusser · 2017-02-13T18:29:24Z

@alexcjohnson I've taken another shot at refining this by explicitly appending it to the async queue where needed. I've also added matching of the subplot and unsetting the name of the hovered subplot when the mouse leaves maindrag. This prevents some corner cases like when it would reapply the hover inappropriately after the mouse has fully left the plot.

I think it's doing a bit of unnecessary work by applying the hover more often than necessary. It's possible this could be optimized by more carefully relying on flags like docalc etc., but I'm hesitant to get too elaborate. Cost = dev time, code complexity, extra processing, Benefit = calling it more often than necessary ensures it's more likely to be correct and not missed.

alexcjohnson · 2017-02-13T19:05:46Z

src/plots/plots.js

-    var seq = [plots.previousPromises, interruptPreviousTransitions, prepareTransitions, executeTransitions];
+    function rehover() {
+        plots.rehover(gd);
+    }


This wrapper isn't necessary, as all the items get gd passed as an arg.

Ahh, thanks. Wondered about that but assumed the opposite.

alexcjohnson · 2017-02-13T19:10:56Z

src/plots/cartesian/graph_interact.js

+                // When the mouse leaves this maindrag, unset the hovered subplot.
+                // This may cause problems if it leaves the subplot directly *onto*
+                // another subplot, but that's a tiny corner case at the moment.
+                gd._fullLayout._hoversubplot = null;


Inset plots seem like a not-so-tiny corner case. But does this actually cause problems? Presumably mouseout happens before mouseover in that case?

Yes, which happens first was the question. If mouseover and then mouseout, it would end up unset, but the only time that could happen is if you move exactly one pixel outside of the inset region, which is where it started to seem like a small corner case. The second pixel moved will immediately fix things.

alexcjohnson · 2017-02-13T19:12:03Z

src/plots/cartesian/graph_interact.js

+                gd._fullLayout._hoversubplot = plotinfo.id;
+
+                gd._fullLayout._rehover();
+
                fx.hover(gd, evt, subplot);
                fullLayout._lasthover = maindrag;
                fullLayout._hoversubplot = subplot;


Are this fx.hover and fullLayout._hoversubplot = subplot still necessary then?

Oops! Good catch. I change this back and forth and accidentally left both. Fixed in the commit below.

Oops. Yeah, you're right. There's still an extra item or two there. Fixing (and looking at the full diff when doing so this time)

rreusser · 2017-02-13T19:15:56Z

@alexcjohnson Thanks for the quick review. I made the changes you pointed out. I can add a test that catches the straightforward cases if you think the proposed fix is starting to look reasonable.

It now looks like this, which removes unneeded lines and is a bit more precise with fullLayout vs. gd._fullLayout references:

            maindrag.onmousemove = function(evt) {
                // This is on `gd._fullLayout`, *not* fullLayout because the reference
                // changes by the time this is called again.
                gd._fullLayout._rehover = function() {
                    if(gd._fullLayout._hoversubplot === subplot) {
                        fx.hover(gd, evt, subplot);
                    }
                };

                fx.hover(gd, evt, subplot);

                // Note that we have *not* used the cached fullLayout variable here
                // since that may be outdated when this is called as a callback later on
                gd._fullLayout._lasthover = maindrag;
                gd._fullLayout._hoversubplot = subplot;
            };

alexcjohnson · 2017-02-13T19:27:31Z

Yes, this looks great! 🔒 it down with some tests and we should be ready to roll.

It's possible this could be optimized by more carefully relying on flags like docalc etc., but I'm hesitant to get too elaborate

Agreed 100%

rreusser · 2017-02-14T18:15:18Z

Added test to verify it's updated when data changes (and nothing else, i.e. no replot, no mouse event). Used animation to accomplish this. That doesn't exhaustively test every case, but it at least verifies the fix is occurring.

alexcjohnson · 2017-02-14T19:30:33Z

Could you add a test where an update causes the hover label to disappear, then another update causes one to appear again? Maybe a relayout changing an axis range, so we're also testing a route that app developers are likely to use when altering the data in their plots, and similar to the route used by streaming?

rreusser · 2017-02-15T19:09:21Z

I've expanded the test so that it now:

shows label
animates data (no replot) and confirms point in other trace now hovered
relayout to move data offscreen & verify hover label removed
relayout to restore original axis limits & verify hover label restored

alexcjohnson · 2017-02-15T19:18:09Z

Awesome, thanks! 💃

Cache the last mousemove event

05e3687

etpinard added status: in progress bug something broken labels Jan 16, 2017

Rehover

5881ad5

rreusser added 3 commits February 13, 2017 12:40

Manually trigger rehover behavior

18b36a6

Merge branch 'master' into graph-interact-cache-event

99e718b

Conflicts: src/plot_api/plot_api.js src/plots/plots.js

Fix bad merge

7df021e

alexcjohnson reviewed Feb 13, 2017

View reviewed changes

Simplify rehover calling slightly

bfaf905

rreusser added 2 commits February 13, 2017 14:25

Fix mousemove actions

77aa54c

Fix small typo in comment

9ad97e4

rreusser added 2 commits February 14, 2017 13:12

Test label updates

bfa5500

Lint obv

6711827

rreusser added the status: reviewable label Feb 14, 2017

rreusser removed the status: in progress label Feb 14, 2017

Expand hover label test to verify is hidden/redisplayed on relayout

554d7ed

rreusser merged commit 09ff6d7 into master Feb 15, 2017

rreusser deleted the graph-interact-cache-event branch February 15, 2017 19:34

etpinard mentioned this pull request Feb 15, 2017

Hoverdata undefined when animating. #1274

Closed

etpinard added this to the 1.24.0 milestone Feb 15, 2017

This was referenced Mar 6, 2017

Latest release leads to infinite loops when calling restyle on plotly_hover #1446

Closed

Clear gd._hoverdata before emitting plotly_unhover #1448

Merged

Adding traces on hover can cause infinite loops #1467

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache the last mousemove event #1304

Cache the last mousemove event #1304

rreusser commented Jan 16, 2017 •

edited

Loading

rreusser commented Jan 16, 2017 •

edited

Loading

etpinard commented Jan 16, 2017 •

edited

Loading

rreusser commented Jan 16, 2017 •

edited

Loading

bpostlethwaite commented Jan 20, 2017

alexcjohnson commented Jan 20, 2017

rreusser commented Jan 20, 2017 •

edited

Loading

rreusser commented Jan 30, 2017 •

edited

Loading

rreusser commented Jan 30, 2017

alexcjohnson commented Jan 30, 2017

rreusser commented Jan 30, 2017 •

edited

Loading

rreusser commented Feb 13, 2017

alexcjohnson Feb 13, 2017

rreusser Feb 13, 2017

alexcjohnson Feb 13, 2017

rreusser Feb 13, 2017

alexcjohnson Feb 13, 2017

rreusser Feb 13, 2017

rreusser Feb 13, 2017

rreusser commented Feb 13, 2017 •

edited

Loading

alexcjohnson commented Feb 13, 2017

rreusser commented Feb 14, 2017

alexcjohnson commented Feb 14, 2017

rreusser commented Feb 15, 2017

alexcjohnson commented Feb 15, 2017

Cache the last mousemove event #1304

Cache the last mousemove event #1304

Conversation

rreusser commented Jan 16, 2017 • edited Loading

rreusser commented Jan 16, 2017 • edited Loading

etpinard commented Jan 16, 2017 • edited Loading

rreusser commented Jan 16, 2017 • edited Loading

bpostlethwaite commented Jan 20, 2017

alexcjohnson commented Jan 20, 2017

rreusser commented Jan 20, 2017 • edited Loading

rreusser commented Jan 30, 2017 • edited Loading

rreusser commented Jan 30, 2017

alexcjohnson commented Jan 30, 2017

rreusser commented Jan 30, 2017 • edited Loading

rreusser commented Feb 13, 2017

alexcjohnson Feb 13, 2017

Choose a reason for hiding this comment

rreusser Feb 13, 2017

Choose a reason for hiding this comment

alexcjohnson Feb 13, 2017

Choose a reason for hiding this comment

rreusser Feb 13, 2017

Choose a reason for hiding this comment

alexcjohnson Feb 13, 2017

Choose a reason for hiding this comment

rreusser Feb 13, 2017

Choose a reason for hiding this comment

rreusser Feb 13, 2017

Choose a reason for hiding this comment

rreusser commented Feb 13, 2017 • edited Loading

alexcjohnson commented Feb 13, 2017

rreusser commented Feb 14, 2017

alexcjohnson commented Feb 14, 2017

rreusser commented Feb 15, 2017

alexcjohnson commented Feb 15, 2017

rreusser commented Jan 16, 2017 •

edited

Loading

rreusser commented Jan 16, 2017 •

edited

Loading

etpinard commented Jan 16, 2017 •

edited

Loading

rreusser commented Jan 16, 2017 •

edited

Loading

rreusser commented Jan 20, 2017 •

edited

Loading

rreusser commented Jan 30, 2017 •

edited

Loading

rreusser commented Jan 30, 2017 •

edited

Loading

rreusser commented Feb 13, 2017 •

edited

Loading