Skip to content

Reset forced minimum tick spacing on every calc pass - #7950

Open
Jaybhade wants to merge 2 commits into
plotly:mainfrom
Jaybhade:fix-stale-forced-min-tick-spacing
Open

Reset forced minimum tick spacing on every calc pass#7950
Jaybhade wants to merge 2 commits into
plotly:mainfrom
Jaybhade:fix-stale-forced-min-tick-spacing

Conversation

@Jaybhade

@Jaybhade Jaybhade commented Aug 9, 2026

Copy link
Copy Markdown

Closes #7968

The bug

Plotly.react can draw an axis with different ticks than Plotly.newPlot of the exact same figure. Same for restyle, addTraces and friends — anything that updates a graph div in place.

var boxFig = {
    data: [{type: 'box', x: [1, 1, 2, 2, 3, 3], y: [1, 2, 3, 4, 5, 6]}],
    layout: {width: 700, height: 400}
};
var scatterFig = {data: [{y: [1, 2, 3]}], layout: {width: 700, height: 400}};

// x ticks: 1, 2, 3
Plotly.newPlot(gd, boxFig);

// x ticks: 0.5, 1, 1.5, 2, 2.5, 3, 3.5
Plotly.newPlot(gd, scatterFig).then(function() { return Plotly.react(gd, boxFig); });

Box, violin, candlestick and ohlc traces ask the position axis for a minimum tick spacing, so that each box gets a tick of its own instead of ticks at meaningless in-between positions. In the second case that forcing is silently lost.

It is most obvious on a date axis. Five daily candles at width: 1000, reached by reacting from a line chart over the same dates:

newPlot:  Jan 1 | Jan 2 | Jan 3 | Jan 4 | Jan 5
react:    12:00 Dec 31, 2023 | 00:00 Jan 1, 2024 | 12:00 | 00:00 Jan 2, 2024 | 12:00 | ...

Every second tick falls in the gap between two candles, and the axis now starts on the day before the data.

Cause

Axes.minDtick keeps its state in ax._minDtick / ax._forceTick0 and distinguishes three cases: undefined (nothing forced yet — adopt this trace's spacing), a positive number (a forcing is in effect), and 0 (forcing cancelled, e.g. by non-grouped bars or by scatter/heatmap, and sticky so a later trace can't reinstate it).

The reset back to undefined between passes was at the bottom of setConvert, but it has never had any effect on a real axis: setConvert runs while supplyDefaults builds the new _fullLayout, where those keys don't exist yet, and relinkPrivateKeys then copies the old values back onto it. So whichever figure the graph div held first decides the forcing forever after. Plotly.newPlot isn't affected because it starts from an empty _fullLayout.

doCalcdata already handles the identical relink staleness for shared color axes ("clear relinked cmin/cmax values in shared axes to start aggregation from scratch"). This moves the reset into ax.clearCalc() — the axis' own per-calc-pass reset, which doCalcdata runs for every axis, and always before any crossTraceCalc.

One existing expectation changed

plot_api_test.js"updates box position and axis type when it falls back to name" asserted that a single box restyled to x0: 12.3 ends up with ticks ['12', '12.5']. That was the stale value. Plotly.newPlot of that same figure produces a single tick at 12.3, on master as well as here, so the change makes restyle agree with newPlot:

master this PR
newPlot box x0: 12.3 12.3 12.3
restyle to x0: 12.3 12, 12.5 12.3
react to x0: 12.3 12, 12.5 12.3

Testing

  • New regression test in axes_test.js"should not carry over the forced minimum tick spacing of the previous figure". It fails on master (Expected 0 to be 1) and passes here.
  • npm run test-jasmine -- --nowatch: 7000 specs, and the failure set is identical to master's on this machine (a batch of font-metric, WebGL, drag/touch and @flaky tests fail either way; the two extra failures in my run pass when their suites are run alone).
  • npm run lint and npm run test-syntax are clean.
  • I don't have the docker setup for test-image / test-export. Those baselines shouldn't be able to move: they're all single newPlot calls, which never have a previous _fullLayout to relink from.

setConvert cleared ax._minDtick / ax._forceTick0 so each calc pass would
start over, but the cleanup never had any effect: setConvert runs while
supplyDefaults builds the new _fullLayout, where the keys do not exist
yet, and relinkPrivateKeys then copies the old values back onto it.

Axes.minDtick treats 0 as "forcing cancelled", so the 0 written by
whichever figure was drawn first survived every later update and vetoed
the forcing for every figure after it. Reacting from a scatter to a box
plot lost the one-tick-per-box spacing, while newPlot of the same figure
kept it.

Move the reset into ax.clearCalc, the axis' own per-calc-pass reset,
which doCalcdata runs for every axis before any cross-trace calc.
@robertclaus

Copy link
Copy Markdown

@Jaybhade could you open an issue to document this? @emilykl will be assigned as a reviewer.

@Jaybhade

Copy link
Copy Markdown
Author

Opened #7968 with the repro and the cause. Thanks @robertclaus — happy to rebase or split anything up if @emilykl wants it structured differently.

@CAOShurong CAOShurong left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exact-head review at b4ec95cd7 (branch of #7950, closes #7968)

I verified this change at the exact head — source-level trace plus a dynamic probe.

Why the bug exists (traced on current main): the delete ax._minDtick / ax._forceTick0 at the tail of setConvert (src/plots/cartesian/set_convert.js) can never take effect on an updated graph div, because setConvert runs inside supplyDefaults, which builds a fresh _fullLayout where those keys don't exist yet — and relinkPrivateKeys(newFullLayout, oldFullLayout) (src/plots/plots.js:404, src/lib/relink_private.js) then copies the old values back. So the first figure's forcing sticks for every subsequent react/restyle. newPlot escapes because it starts from an empty _fullLayout.

Why the fix works: ax.clearCalc() is invoked from setupAxisCategories (src/plots/plots.js:2989) during doCalcdata, which runs after the relink — so deleting there actually clears state. This mirrors how calc already resets _minDtick = 0 for shared color axes. The move is minimal and correct.

Checks run:

  • Diffed PR head against main: only the reset relocation + comment/doc updates in set_convert.js; test additions assert _minDtick === 1 after react back to the box figure.
  • Loaded both bundles headless under node with a DOM shim and ran the newPlot(box) → newPlot(scatter) → react(box) sequence probing xaxis._minDtick: 1 → 0 → 1, i.e. the forcing survives the update path with this head (matches the new test's expectation).
  • Checked the two other clearCalc() call sites (src/plots/plots.js, rangeslider draw.js) — both want categories cleared together with tick forcing, so no regression path found.
  • Full CI on this head is green including all jasmine shards; the updated expectation in plot_api_test.js (['12', '12.5'] → ['12.3']) is the correct consequence: a single box now forces a tick at its own position instead of leaving stale midpoints.

One thing worth a maintainer glance: clearCalc() is also called from the rangeslider component (src/components/rangeslider/draw.js) on every range-slider redraw, so _minDtick/_forceTick0 will be dropped and re-derived on the next calc pass for zoomed slider states. I traced the tick pipeline and don't see user-visible impact, but flagging since that call site predates this change and wasn't covered by a test here.

Nice find on the relink ordering — this also explains why the forcing leak was sticky rather than transient.

@Jaybhade

Copy link
Copy Markdown
Author

Thanks for the trace — the relink ordering is exactly it.

On the rangeslider call site: it can't drop the forcing, because it never touches a real axis. draw.js builds a local mockFigure, runs Plots.supplyDefaults(mockFigure) on it, and then clears the axes of that layout:

Plots.supplyDefaults(mockFigure);

var xa = mockFigure._fullLayout.xaxis;
var ya = mockFigure._fullLayout[oppAxisName];

xa.clearCalc();
xa.setScale();

mockFigure is a fresh object literal each pass, so its _fullLayout is built from scratch with no relink from the graph div — _minDtick/_forceTick0 were never on it, and the two new deletes are no-ops there.

Instrumented at this head (b4ec95cd7), logging every clearCalc receiver on a box + rangeslider figure:

--- relayout rangeslider.thickness
   axis=xaxis  isTheRealAxisObject=false  _minDtick before delete=undefined
   axis=yaxis  isTheRealAxisObject=false  _minDtick before delete=undefined
   axis=xaxis  isTheRealAxisObject=false  _minDtick before delete=undefined
   axis=yaxis  isTheRealAxisObject=false  _minDtick before delete=undefined
   => real xaxis._minDtick after: 1, dtick: 1

--- relayout xaxis.range (slider redraw)
   axis=xaxis  isTheRealAxisObject=false  _minDtick before delete=undefined
   axis=yaxis  isTheRealAxisObject=false  _minDtick before delete=undefined
   => real xaxis._minDtick after: 1, dtick: 1

--- relayout rangeslider.range (zoomed)
   axis=xaxis  isTheRealAxisObject=true   _minDtick before delete=1
   axis=yaxis  isTheRealAxisObject=true   _minDtick before delete=undefined
   axis=xaxis  isTheRealAxisObject=false  _minDtick before delete=undefined
   axis=yaxis  isTheRealAxisObject=false  _minDtick before delete=undefined
   => real xaxis._minDtick after: 1, dtick: 1

Every rangeslider-originated call lands on a mock axis. The one real-axis call, in the third case, is the intended new site — that relayout runs a full doCalcdata, so it's setupAxisCategoriesclearCalc, and box's calc re-derives _minDtick = 1 in the same pass before ticks are computed. Note the ordering there: the real axes are cleared first, the mock ones after, which is why a zoomed slider state can't outlive the value it depends on.

And black-box, branch vs. master, on the same figure — ten paths that redraw the slider, printing _minDtick/_forceTick0/dtick/tick0 and the resulting tick labels:

step _minDtick _forceTick0 dtick tick0 ticks
newPlot(box + rangeslider) 1 1 1 1 1, 2, 3
relayout rangeslider.thickness 1 1 1 1 1, 2, 3
relayout xaxis.range 1 1 1 1 1, 2
relayout rangeslider.range 1 1 1 1 1, 2
relayout rangeslider.autorange=false 1 1 1 1 1, 2
relayout title 1 1 1, 2
restyle marker.color 1 1 1, 2
react back to the box figure 1 1 1 1 1, 2, 3
relayout rangeslider.visible=false 1 1 1 1 1, 2, 3
relayout rangeslider.visible=true 1 1 1 1 1, 2, 3

Identical on both, row for row — so nothing observable changed at that call site. (The rows are the two steps that don't re-run calc; dtick/tick0 are recomputed lazily there, and the forcing they'd be derived from is intact.)

Happy to add a guard test for it if a maintainer would rather have the rangeslider path pinned, but it would be asserting a no-op, so I left it out to keep the diff to the one reset.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: forced minimum tick spacing (box/violin/candlestick/ohlc) leaks between figures on the same graph div

5 participants