Merge pull request #1176 from plotly/fix-command-api-bug

Add check for failed binding comparison

Author: etpinard

src/plots/command.js

@@ -145,11 +145,12 @@ exports.manageCommandObserver = function(gd, container, commandList, onchange) {
  *   3. the same property must be affected by all commands
  */
 exports.hasSimpleAPICommandBindings = function(gd, commandList, bindingsByValue) {
+    var i;
     var n = commandList.length;
 
     var refBinding;
 
-    for(var i = 0; i < n; i++) {
+    for(i = 0; i < n; i++) {
         var binding;
         var command = commandList[i];
         var method = command.method;
@@ -200,7 +201,11 @@ exports.hasSimpleAPICommandBindings = function(gd, commandList, bindingsByValue)
         binding = bindings[0];
         var value = binding.value;
         if(Array.isArray(value)) {
-            value = value[0];
+            if(value.length === 1) {
+                value = value[0];
+            } else {
+                return false;
+            }
         }
         if(bindingsByValue) {
             bindingsByValue[value] = i;

test/jasmine/tests/command_test.js

@@ -155,12 +155,17 @@ describe('Plots.hasSimpleAPICommandBindings', function() {
             args: [{'marker.color': 20}, [2, 1]]
         }]);
 
-        expect(isSimple).toEqual({
+        // See https://github.com/plotly/plotly.js/issues/1169 for an example of where
+        // this logic was a little too sophisticated. It's better to bail out and omit
+        // functionality than to get it wrong.
+        expect(isSimple).toEqual(false);
+
+        /* expect(isSimple).toEqual({
             type: 'data',
             prop: 'marker.color',
             traces: [ 1, 2 ],
             value: [ 10, 10 ]
-        });
+        });*/
     });
 });
 
@@ -508,6 +513,14 @@ describe('component bindings', function() {
         }).catch(fail).then(done);
     });
 
+    it('does not update the component if the value is not present', function(done) {
+        expect(gd.layout.sliders[0].active).toBe(0);
+
+        Plotly.restyle(gd, 'marker.color', 'black').then(function() {
+            expect(gd.layout.sliders[0].active).toBe(0);
+        }).catch(fail).then(done);
+    });
+
     it('udpates bound components when the computed value changes', function(done) {
         expect(gd.layout.sliders[0].active).toBe(0);
 

test/jasmine/tests/updatemenus_test.js

@@ -462,6 +462,52 @@ describe('update menus interactions', function() {
         });
     });
 
+    it('should update correctly on failed binding comparisons', function(done) {
+
+        // See https://github.com/plotly/plotly.js/issues/1169
+        // for more info.
+
+        var data = [{
+            y: [1, 2, 3],
+            visible: true
+        }, {
+            y: [2, 3, 1],
+            visible: false
+        }, {
+            y: [3, 1, 2],
+            visible: false
+        }];
+
+        var layout = {
+            updatemenus: [{
+                buttons: [{
+                    label: 'a',
+                    method: 'restyle',
+                    args: ['visible', [true, false, false]]
+                }, {
+                    label: 'b',
+                    method: 'restyle',
+                    args: ['visible', [false, true, false]]
+                }, {
+                    label: 'c',
+                    method: 'restyle',
+                    args: ['visible', [false, false, true]]
+                }]
+            }]
+        };
+
+        Plotly.newPlot(gd, data, layout).then(function() {
+            return click(selectHeader(0));
+        })
+        .then(function() {
+            return click(selectButton(1));
+        })
+        .then(function() {
+            assertActive(gd, [1]);
+        })
+        .then(done);
+    });
+
     it('should change color on mouse over', function(done) {
         var INDEX_0 = 2,
             INDEX_1 = gd.layout.updatemenus[1].active;