Repository navigation
Fix parcoords hover event handling in dcc.Graph - #4077
monsterlag wants to merge 1 commit into
Conversation
|
|
Thanks for the PR! We'll take a closer look and follow up. |
T4rk1n
left a comment
T4rk1n
left a comment
There was a problem hiding this comment.
The crash fix is right: wrapping the flat parcoords event in points stops the TypeError, and array-shaped events behave the same as before. My one real concern is the shape that reaches hoverData. It passes through curveNumber as the line index, but for every other trace type curveNumber is the trace index. Once this ships the shape is public and hard to change, so I'd like to settle that before merge. The two test notes are small. Can you also add a CHANGELOG entry under Fixed?
| * into the event object | ||
| */ | ||
| const data = gd.data; | ||
| // Parcoords sends a flat point, whose curveNumber is a line index. |
There was a problem hiding this comment.
With this change, hoverData.points[0].curveNumber is the parcoords line index. Everywhere else in Dash it's the trace index. A callback doing fig["data"][pt["curveNumber"]] breaks as soon as you hover line 2 of a one-trace figure (that's the exact case your test covers). Can you normalize it here instead: curveNumber: dataIndex, pointNumber: <line index>? Then it matches the other traces, and the customdata lookup below works without the hasPointsArray guard.
| { | ||
| "x": 190, | ||
| "y": 200, | ||
| "clientX": canvas.rect["x"] + 190, |
There was a problem hiding this comment.
Asserting exact x/y/clientX/clientY ties the test to the pixel layout, window size and Chrome's DPR handling, so it will flake when any of those shift. Can you assert just the index fields (curveNumber/dataIndex, or pointNumber after the change above) and check that the coordinate keys exist?
| if clear_on_unhover: | ||
| dash_dcc.wait_for_text_to_equal("#output", "null") | ||
| else: | ||
| assert json.loads(dash_dcc.find_element("#output").text) == hover_data |
There was a problem hiding this comment.
In the clear_on_unhover=False branch, this reads the output right after the mouse moves, before any unhover callback could fire. It would pass even if unhover did clear the data. Can you wait for something that proves the unhover went through first (for example, a second output fed by a callback on another prop), or drop the False param?
Note
AI-generated, not reviewed by a human. This pull request was written by an AI coding agent and then checked by a separate AI review agent, which re-ran the tests described below. No human has reviewed the code. It is submitted by @monsterlag, who runs these agents. If that does not fit how you want to receive contributions, please feel free to close it.
Fixes #3128.
Plotly.js emits a flat hover event for parallel coordinates, but Graph assumed every hover event had a
pointsarray. Hovering a line therefore raised a TypeError and did not updatehoverData.Wrap flat events in the usual
pointsrepresentation and filter their fields without treating the parcoords line index as a trace index. Leave array-shaped event handling unchanged.Add browser coverage for parcoords hover, both
clear_on_unhoversettings, and scatter and Sankey hover controls. The parcoords tests fail before the fix and pass after it.Testing
npm run lintand changed-file pylint/format checks: passed (existing ESLint warnings).python -m pytest tests/compliance/: 54 passed, 1 existing skip.python -m pytest tests/unit/ -k 'not Firefox' --headless: 728 passed, 6 skipped. Firefox smoke could not start in this container.Contributor Checklist