Skip to content

Commit cfd13b1

Browse files
FarhanAliRazaFarhan Ali Raza
andauthored
ENG-11596 Fix bar hover tooltip rewriting and jumping when the exact pick reply lands (#499)
* Fix bar hover tooltip rewriting and jumping when the exact pick reply lands Hover is two-phase (§16): an instant local readout from the shipped f32, then the kernel's exact f64 pick_result replaces it. For bars the exact row was wrong on both seams, so the replacement was a visible rewrite — the tooltip changed its data and position a round-trip after every hover, on both orientations. Kernel: _append_rect_trace built the conventional Trace(x, y) columns as x-midpoint / y1-edge regardless of orientation. A horizontal bar of value 42 replied x: 21 (half the value) and y: 1.4 (band position + half the bar width). The columns now follow the orientation: band center on the position axis, value end on the value axis. Histogram/box/violin pass no orientation and are unchanged. Client: the pick_result handler rendered the kernel row raw, so a category code replaced its label ("B" became "1"), and the finite code then dragged the tooltip anchor away from the cursor. The handler now maps x/y through the same _sourceDisplayValue path the local row uses. The _cpuRect local readout (variable-width bars ship as plain rects) is also orientation-aware now, matching the corrected kernel convention. Pinned by a Figure.pick readout test (vertical, horizontal, stacked with negatives) and a headless-Chromium probe per orientation that hovers a bar, injects the real pick_result row, and asserts the tooltip text and position are identical before and after. Spec updated: the orientation- aware center/value convention (chart-kind-contract.md) and the invisible-refinement requirement on pick_result (wire-protocol.md). * Treat an exact pick reply for a dropped trace as a miss A pick_result can land after a data update removed its trace from gpuTraces; with no mapping source the raw kernel row would render the category code and drag the anchor — the same rewrite/jump this branch fixes. Hide the tooltip instead: the mark no longer exists. --------- Co-authored-by: Farhan Ali Raza <farhan@reflex.dev>
1 parent f5ff03d commit cfd13b1

6 files changed

Lines changed: 209 additions & 4 deletions

File tree

‎js/src/52_tooltip.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,10 +73,16 @@ Object.assign(ChartView.prototype, {
7373
const x1 = this._decodeValue(r.x1, r.x1Meta, hit.index);
7474
const y0 = this._decodeValue(r.y0, r.y0Meta, hit.index);
7575
const y1 = this._decodeValue(r.y1, r.y1Meta, hit.index);
76+
// Center/value follows the mark's orientation, matching the kernel's
77+
// conventional columns (§16): a horizontal bar reads its value from x1
78+
// and its category center from y — never the x midpoint (half the value).
79+
const horizontal = g.trace.style && g.trace.style.orientation === "horizontal";
7680
const [x, xKind] = this._sourceDisplayValue(
77-
g, "x", x0 + (x1 - x0) / 2, r.x0Meta.kind,
81+
g, "x", horizontal ? x1 : x0 + (x1 - x0) / 2, r.x0Meta.kind,
82+
);
83+
const [y, yKind] = this._sourceDisplayValue(
84+
g, "y", horizontal ? y0 + (y1 - y0) / 2 : y1, r.y1Meta.kind,
7885
);
79-
const [y, yKind] = this._sourceDisplayValue(g, "y", y1, r.y1Meta.kind);
8086
row.x = x;
8187
row.y = y;
8288
if (xKind !== undefined) row.x_kind = xKind;

‎js/src/54_kernel.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -857,6 +857,23 @@ Object.assign(ChartView.prototype, {
857857
// them (fields the kernel row lacks are exactly the sibling-sourced
858858
// ones) so _applySharedTooltipFields finds nothing missing and skips
859859
// its O(n) sibling scans. It stays as the mismatch fallback.
860+
// The kernel row carries raw axis coordinates; a category axis's code
861+
// must become its label here, exactly as _localRow did for the instant
862+
// readout — otherwise the exact reply visibly swaps "B" for "1" (and
863+
// the finite code below would drag the anchor away from the cursor).
864+
// A reply for a trace no longer in gpuTraces (dropped by a data update
865+
// while the pick was in flight) has no mapping source and describes a
866+
// mark that no longer exists — a miss.
867+
const rowG = this.gpuTraces.find((t) => t.trace.id === msg.row.trace);
868+
if (!rowG) { this._hideTooltip(); return; }
869+
for (const channel of ["x", "y"]) {
870+
if (typeof msg.row[channel] !== "number") continue;
871+
const [value, kind] = this._sourceDisplayValue(
872+
rowG, channel, msg.row[channel], msg.row[`${channel}_kind`],
873+
);
874+
msg.row[channel] = value;
875+
if (kind === undefined) delete msg.row[`${channel}_kind`];
876+
}
860877
const local = this._lastRow;
861878
if (local && local.trace === msg.row.trace && local.index === msg.row.index) {
862879
for (const [key, value] of Object.entries(local)) {

‎python/xy/_figure.py‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1401,8 +1401,16 @@ def _append_rect_trace(
14011401
x1c = self.store.ingest(x1)
14021402
y0c = self.store.ingest(y0)
14031403
y1c = self.store.ingest(y1)
1404-
xc = self.store.ingest(x0c.values + (x1c.values - x0c.values) / 2.0)
1405-
yc = self.store.ingest(y1c.values)
1404+
# Conventional center/value columns follow the orientation: the
1405+
# pick readout (§16) reports the category-band center and the value
1406+
# end, so a horizontal bar must not read back its x midpoint (half
1407+
# the value) or its y1 edge (category position + half width).
1408+
if orientation == "horizontal":
1409+
xc = self.store.ingest(x1c.values)
1410+
yc = self.store.ingest(y0c.values + (y1c.values - y0c.values) / 2.0)
1411+
else:
1412+
xc = self.store.ingest(x0c.values + (x1c.values - x0c.values) / 2.0)
1413+
yc = self.store.ingest(y1c.values)
14061414
style: dict[str, Any] = {"color": color, "opacity": opacity, "role": role}
14071415
if orientation is not None:
14081416
style["orientation"] = orientation

‎spec/api/chart-kind-contract.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -421,6 +421,11 @@ usually wrong). Each has an explicit trigger:
421421
`MARK_KINDS` at that call site.
422422
- **Trace shape & autorange**: `Trace(x, y)` remains the conventional center/value
423423
pair, while rectangle and segment marks carry explicit `x0/x1/y0/y1` columns.
424+
For oriented bar-like marks the convention follows the orientation: the
425+
position axis holds the band center and the value axis holds the value end
426+
(`x1` for horizontal bars, `y1` for vertical), because the exact pick readout
427+
(§16) projects these columns and must agree with the client's instant local
428+
row — a horizontal bar must never read back its x midpoint (half the value).
424429
`Figure._range_columns()` already includes those geometry extents, so error
425430
bars, boxes, violins, contours, and other multi-column marks do not autorange
426431
to their midpoint only. *Trigger: a future mark whose extent is not expressible

‎spec/design/wire-protocol.md‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,17 @@ and `size_value` when those channels exist. A heatmap row is
230230
use their own client-side sequence (`_pickSeq`), not the view `seq` — sharing
231231
one counter let a hover invalidate an in-flight `tier_update`.
232232

233+
`x`/`y` are raw axis coordinates from the conventional center/value columns
234+
(orientation-aware for bars: band center on the position axis, value end on
235+
the value axis). The client maps a category axis's code to its label before
236+
rendering or re-anchoring — the exact reply must land as an invisible
237+
refinement of the instant local readout, never a visible rewrite (a category
238+
code replacing its label, or a finite code dragging the tooltip anchor away
239+
from the cursor). A reply naming a trace the client no longer holds (dropped
240+
by a data update while the pick was in flight) has no mapping source and
241+
describes a mark that no longer exists; the client treats it as a miss and
242+
hides the tooltip.
243+
233244
**`selection`** — `{type, traces, total}` plus one u32 buffer per trace. Each
234245
entry is `{id, count, buf, drill_seq}`. Masks speak **shipped-vertex
235246
positions** (`fig.to_shipped_indices`), so `count` is the wire mask length,

‎tests/test_ui_issue_regressions.py‎

Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1129,3 +1129,161 @@ def test_categorical_tick_bounds_follow_anchor_rotation_and_extra_axis_side(
11291129
assert extra, result
11301130
assert all(row["side"] == "right" for row in extra), result
11311131
assert all(row["pinnedRight"] for row in extra), result
1132+
1133+
1134+
def test_bar_pick_reports_band_center_and_value_for_both_orientations() -> None:
1135+
"""The exact pick row must agree with the instant client readout: the
1136+
category-band center on the position axis and the value end on the value
1137+
axis. Horizontal bars used to report the rect x midpoint (half the value)
1138+
and the y1 edge (band position + half width), so the kernel's exact reply
1139+
visibly rewrote the tooltip a round-trip after hover."""
1140+
vertical = xy.bar_chart(xy.bar(x=["A", "B", "C"], y=[10.0, 42.0, 7.0]))
1141+
assert vertical.pick(0, 1) == {
1142+
"trace": 0,
1143+
"index": 1,
1144+
"x": 1.0,
1145+
"y": 42.0,
1146+
"x_kind": "float",
1147+
"y_kind": "float",
1148+
}
1149+
horizontal = xy.bar_chart(
1150+
xy.bar(x=["A", "B", "C"], y=[10.0, 42.0, 7.0], orientation="horizontal")
1151+
)
1152+
assert horizontal.pick(0, 1) == {
1153+
"trace": 0,
1154+
"index": 1,
1155+
"x": 42.0,
1156+
"y": 1.0,
1157+
"x_kind": "float",
1158+
"y_kind": "float",
1159+
}
1160+
# Stacked horizontal: the value end is the signed cumulative edge and the
1161+
# band center stays the category code, including on the negative branch.
1162+
stacked = xy.bar_chart(
1163+
xy.bar(
1164+
x=["A", "B"],
1165+
y=[[1.0, -2.0], [3.0, 4.0]],
1166+
mode="stacked",
1167+
orientation="horizontal",
1168+
)
1169+
)
1170+
assert stacked.pick(1, 0) == {
1171+
"trace": 1,
1172+
"index": 0,
1173+
"x": 4.0,
1174+
"y": 0.0,
1175+
"x_kind": "float",
1176+
"y_kind": "float",
1177+
}
1178+
assert stacked.pick(0, 1) == {
1179+
"trace": 0,
1180+
"index": 1,
1181+
"x": -2.0,
1182+
"y": 1.0,
1183+
"x_kind": "float",
1184+
"y_kind": "float",
1185+
}
1186+
1187+
1188+
@pytest.mark.parametrize("orientation", ["vertical", "horizontal"])
1189+
def test_bar_tooltip_survives_the_exact_pick_reply_unchanged(
1190+
tmp_path: Path, orientation: str
1191+
) -> None:
1192+
"""Hovering a bar shows the instant local readout; the kernel's exact
1193+
pick_result then replaces it. Both must say the same thing: the exact row
1194+
used to arrive as raw axis coordinates (category code, and for horizontal
1195+
bars half the value), so the tooltip visibly changed content and jumped to
1196+
a new anchor a round-trip after every hover."""
1197+
chart = xy.bar_chart(
1198+
xy.bar(x=["A", "B", "C"], y=[10.0, 42.0, 7.0], orientation=orientation),
1199+
width=520,
1200+
height=320,
1201+
)
1202+
pick_row = chart.pick(0, 1)
1203+
hover_xy = (21.0, 1.0) if orientation == "horizontal" else (1.0, 21.0)
1204+
script = (
1205+
_PRELUDE
1206+
+ f"""
1207+
const pickRow = {json.dumps(pick_row)};
1208+
const [hx, hy] = {json.dumps(hover_xy)};
1209+
"""
1210+
+ """
1211+
const rect = view.canvas.getBoundingClientRect();
1212+
// _projectDataPoint is root-relative; the hover canvas covers the plot.
1213+
const [lx, ly] = view._projectDataPoint("x", "y", hx, hy);
1214+
const cssX = lx - view.plot.x;
1215+
const cssY = ly - view.plot.y;
1216+
const hit = view._hoverAt(cssX, cssY);
1217+
if (!hit || hit.index !== 1) throw new Error("expected to hover bar index 1");
1218+
const clientX = rect.left + cssX;
1219+
const clientY = rect.top + cssY;
1220+
view._hoverTarget = hit;
1221+
view._lastHoverXY = {clientX, clientY};
1222+
view._showTooltip(hit, clientX, clientY);
1223+
const snapshot = () => ({
1224+
text: view.tooltip.textContent,
1225+
left: view.tooltip.style.left,
1226+
top: view.tooltip.style.top,
1227+
display: view.tooltip.style.display,
1228+
});
1229+
const before = snapshot();
1230+
view._onKernelMsg({type: "pick_result", row: pickRow}, []);
1231+
const after = snapshot();
1232+
document.body.setAttribute("data-xy-issue-probe", JSON.stringify({
1233+
before, after, exactRow: view._lastRow,
1234+
}));
1235+
"""
1236+
+ _POSTLUDE
1237+
)
1238+
result = _probe(chart, script, tmp_path, f"bar tooltip exact reply {orientation}")
1239+
1240+
expected = "x42yB" if orientation == "horizontal" else "xBy42"
1241+
assert result["before"]["text"] == expected, result
1242+
assert result["after"] == result["before"], result
1243+
# The exact row reads like the local one: category label, full value.
1244+
label_field = "y" if orientation == "horizontal" else "x"
1245+
value_field = "x" if orientation == "horizontal" else "y"
1246+
assert result["exactRow"][label_field] == "B", result
1247+
assert result["exactRow"][value_field] == 42.0, result
1248+
1249+
1250+
def test_pick_reply_for_a_dropped_trace_is_a_miss(tmp_path: Path) -> None:
1251+
"""A pick_result can land after a data update removed its trace. With no
1252+
trace to map against, the raw kernel row must not render: the category
1253+
code would replace the label and drag the anchor — the exact rewrite/jump
1254+
this fix removes. A reply for a trace that no longer exists is a miss."""
1255+
chart = xy.bar_chart(
1256+
xy.bar(x=["A", "B", "C"], y=[10.0, 42.0, 7.0]),
1257+
width=520,
1258+
height=320,
1259+
)
1260+
pick_row = chart.pick(0, 1)
1261+
script = (
1262+
_PRELUDE
1263+
+ f"""
1264+
const pickRow = {json.dumps(pick_row)};
1265+
"""
1266+
+ """
1267+
const rect = view.canvas.getBoundingClientRect();
1268+
const [lx, ly] = view._projectDataPoint("x", "y", 1.0, 21.0);
1269+
const cssX = lx - view.plot.x;
1270+
const cssY = ly - view.plot.y;
1271+
const hit = view._hoverAt(cssX, cssY);
1272+
if (!hit || hit.index !== 1) throw new Error("expected to hover bar index 1");
1273+
const clientX = rect.left + cssX;
1274+
const clientY = rect.top + cssY;
1275+
view._hoverTarget = hit;
1276+
view._lastHoverXY = {clientX, clientY};
1277+
view._showTooltip(hit, clientX, clientY);
1278+
// Simulate a data update dropping the trace while the pick is in flight.
1279+
view.gpuTraces = view.gpuTraces.filter((t) => t.trace.id !== pickRow.trace);
1280+
view._onKernelMsg({type: "pick_result", row: pickRow}, []);
1281+
document.body.setAttribute("data-xy-issue-probe", JSON.stringify({
1282+
display: view.tooltip.style.display,
1283+
text: view.tooltip.textContent,
1284+
}));
1285+
"""
1286+
+ _POSTLUDE
1287+
)
1288+
result = _probe(chart, script, tmp_path, "pick reply dropped trace")
1289+
assert result["display"] == "none", result

0 commit comments

Comments
 (0)