diff --git a/src/components/sliders/draw.js b/src/components/sliders/draw.js index 6679dcb408f..5a40ee579d3 100644 --- a/src/components/sliders/draw.js +++ b/src/components/sliders/draw.js @@ -72,10 +72,18 @@ module.exports = function draw(gd) { computeLabelSteps(sliderOpts); Plots.manageCommandObserver(gd, sliderOpts, sliderOpts.steps, function(data) { - if(sliderOpts.active === data.index) return; - if(sliderOpts._dragging) return; - - setActive(gd, gSlider, sliderOpts, data.index, false, true); + // NB: Same as below. This is *not* always the same as sliderOpts since + // if a new set of steps comes in, the reference in this callback would + // be invalid. We need to refetch it from the slider group, which is + // the join data that creates this slider. So if this slider still exists, + // the group should be valid, *to the best of my knowledge.* If not, + // we'd have to look it up by d3 data join index/key. + var opts = gSlider.data()[0]; + + if(opts.active === data.index) return; + if(opts._dragging) return; + + setActive(gd, gSlider, opts, data.index, false, true); }); drawSlider(gd, d3.select(this), sliderOpts); @@ -225,6 +233,15 @@ function findDimensions(gd, sliderOpts) { } function drawSlider(gd, sliderGroup, sliderOpts) { + // This is related to the other long notes in this file regarding what happens + // when slider steps disappear. This particular fix handles what happens when + // the *current* slider step is removed. The drawing functions will error out + // when they fail to find it, so the fix for now is that it will just draw the + // slider in the first position but will not execute the command. + if(sliderOpts.active >= sliderOpts.steps.length) { + sliderOpts.active = 0; + } + // These are carefully ordered for proper z-ordering: sliderGroup .call(drawCurrentValue, sliderOpts) @@ -251,9 +268,9 @@ function drawCurrentValue(sliderGroup, sliderOpts, valueOverride) { switch(sliderOpts.currentvalue.xanchor) { case 'right': - // This is anchored left and adjusted by the width of the longest label - // so that the prefix doesn't move. The goal of this is to emphasize - // what's actually changing and make the update less distracting. + // This is anchored left and adjusted by the width of the longest label + // so that the prefix doesn't move. The goal of this is to emphasize + // what's actually changing and make the update less distracting. x0 = sliderOpts.inputAreaLength - constants.currentValueInset - sliderOpts.currentValueMaxWidth; textAnchor = 'left'; break; @@ -402,11 +419,21 @@ function setActive(gd, sliderGroup, sliderOpts, index, doCallback, doTransition) } } -function attachGripEvents(item, gd, sliderGroup, sliderOpts) { +function attachGripEvents(item, gd, sliderGroup) { var node = sliderGroup.node(); var $gd = d3.select(gd); + // NB: This is *not* the same as sliderOpts itself! These callbacks + // are in a closure so this array won't actually be correct if the + // steps have changed since this was initialized. The sliderGroup, + // however, has not changed since that *is* the slider, so it must + // be present to receive mouse events. + function getSliderOpts() { + return sliderGroup.data()[0]; + } + item.on('mousedown', function() { + var sliderOpts = getSliderOpts(); gd.emit('plotly_sliderstart', {slider: sliderOpts}); var grip = sliderGroup.select('.' + constants.gripRectClass); @@ -420,11 +447,13 @@ function attachGripEvents(item, gd, sliderGroup, sliderOpts) { sliderOpts._dragging = true; $gd.on('mousemove', function() { + var sliderOpts = getSliderOpts(); var normalizedPosition = positionToNormalizedValue(sliderOpts, d3.mouse(node)[0]); handleInput(gd, sliderGroup, sliderOpts, normalizedPosition, false); }); $gd.on('mouseup', function() { + var sliderOpts = getSliderOpts(); sliderOpts._dragging = false; grip.call(Color.fill, sliderOpts.bgcolor); $gd.on('mouseup', null); diff --git a/src/plots/plots.js b/src/plots/plots.js index 3ad048addd5..e0f353b58db 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -1546,6 +1546,23 @@ plots.computeFrame = function(gd, frameName) { return result; }; +/* + * Recompute the lookup table that maps frame name -> frame object. addFrames/ + * deleteFrames already manages this data one at a time, so the only time this + * is necessary is if you poke around manually in `gd._transitionData._frames` + * and create and haven't updated the lookup table. + */ +plots.recomputeFrameHash = function(gd) { + var hash = gd._transitionData._frameHash = {}; + var frames = gd._transitionData._frames; + for(var i = 0; i < frames.length; i++) { + var frame = frames[i]; + if(frame && frame.name) { + hash[frame.name] = frame; + } + } +}; + /** * Extend an object, treating container arrays very differently by extracting * their contents and merging them separately. diff --git a/test/jasmine/tests/sliders_test.js b/test/jasmine/tests/sliders_test.js index 0e7c8a0bbcb..5a1274769a2 100644 --- a/test/jasmine/tests/sliders_test.js +++ b/test/jasmine/tests/sliders_test.js @@ -191,6 +191,7 @@ describe('sliders initialization', function() { Plotly.plot(gd, [{x: [1, 2, 3]}], { sliders: [{ + transition: {duration: 0}, steps: [ {method: 'restyle', args: [], label: 'first'}, {method: 'restyle', args: [], label: 'second'}, @@ -209,6 +210,59 @@ describe('sliders initialization', function() { }); }); +describe('ugly internal manipulation of steps', function() { + 'use strict'; + var gd; + + beforeEach(function(done) { + gd = createGraphDiv(); + + Plotly.plot(gd, [{x: [1, 2, 3]}], { + sliders: [{ + transition: {duration: 0}, + steps: [ + {method: 'restyle', args: [], label: 'first'}, + {method: 'restyle', args: [], label: 'second'}, + ] + }] + }).then(done); + }); + + afterEach(function() { + Plotly.purge(gd); + destroyGraphDiv(); + }); + + it('adds and removes slider steps gracefully', function(done) { + expect(gd._fullLayout.sliders[0].active).toEqual(0); + + // Set the active index higher than it can go: + Plotly.relayout(gd, {'sliders[0].active': 2}).then(function() { + // Confirm nothing changed + expect(gd._fullLayout.sliders[0].active).toEqual(0); + + // Add an option manually without calling API functions: + gd.layout.sliders[0].steps.push({method: 'restyle', args: [], label: 'first'}); + + // Now that it's been added, restyle and try again: + return Plotly.relayout(gd, {'sliders[0].active': 2}); + }).then(function() { + // Confirm it's been changed: + expect(gd._fullLayout.sliders[0].active).toEqual(2); + + // Remove the option: + gd.layout.sliders[0].steps.pop(); + + // And redraw the plot: + return Plotly.redraw(gd); + }).then(function() { + // The selected option no longer exists, so confirm it's + // been fixed during the process of updating/drawing it: + expect(gd._fullLayout.sliders[0].active).toEqual(0); + }).catch(fail).then(done); + }); +}); + describe('sliders interactions', function() { 'use strict';