Remove cancelable events from updateEvents to avoid two listeners issue

Author: camdecoster

src/__tests__/react-plotly.test.js

@@ -219,20 +219,30 @@ describe('<Plotly/>', () => {
         });
       });
 
-      // The drill-down traces all share plotly.js' sunburst click handler,
-      // which mutates `trace.level` — so onUpdate has to fire for each of them
-      // or consumers never see the new level. See issue #375.
+      // Cancelable — the consumer's handler must be the only listener or
+      // plotly could drop its `return false`. See the note in `events.js`.
       test.each(['SunburstClick', 'TreemapClick', 'IcicleClick'])(
-        'fires onUpdate for %s drill-downs',
+        'attaches only the consumer handler to %s',
         (eventName) => {
-          const onUpdate = jest.fn();
+          const handler = () => false;
 
-          return createPlot({onUpdate}).then((plot) => {
-            plot.gd.emit(getPlotlyEventName(eventName));
-            expect(onUpdate).toHaveBeenCalled();
-          });
+          return createPlot({[getPropName(eventName)]: handler, onUpdate: () => {}}).then(
+            (plot) => {
+              const listeners = [].concat(plot.gd.__ee__[getPlotlyEventName(eventName)] || []);
+              expect(listeners).toEqual([handler]);
+            }
+          );
         }
       );
+
+      test('fires onUpdate when a drill-down animation completes', () => {
+        const onUpdate = jest.fn();
+
+        return createPlot({onUpdate}).then((plot) => {
+          plot.gd.emit('plotly_animated');
+          expect(onUpdate).toHaveBeenCalled();
+        });
+      });
     });
 
     describe('StrictMode', () => {

src/events.js

@@ -4,11 +4,12 @@
 //   - events are attached as `'plotly_' + name.toLowerCase()`
 //   - react props are `'on' + name`
 //
-// `triggersUpdate` marks events that plotly.js emits *after* mutating the
-// figure itself (drill-downs, zooms, restyles). For those the wrapper has to
-// re-read the graph div and fire `onUpdate`. Keeping the flag on the same
-// record as the name is deliberate: a new drill-down event cannot be added
-// without deciding whether it updates the figure.
+// `triggersUpdate` marks events plotly.js emits *after* changing the figure;
+// the wrapper listens to those and fires `onUpdate`. Never set it on a
+// cancelable event — that adds a second listener alongside the consumer's,
+// and plotly keeps only the last listener's return value, so a consumer's
+// `return false` could be discarded. The drill-down clicks rely on
+// `plotly_animated` instead.
 //
 // The `on*` prop types in `factory.d.ts` are maintained by hand against this list
 export const events = [
@@ -29,7 +30,7 @@ export const events = [
   {name: 'Framework'},
   {name: 'Hover'},
   {name: 'HoverAnywhere'},
-  {name: 'IcicleClick', triggersUpdate: true},
+  {name: 'IcicleClick'},
   {name: 'LegendClick'},
   {name: 'LegendDoubleClick'},
   {name: 'Relayout', triggersUpdate: true},
@@ -41,10 +42,10 @@ export const events = [
   {name: 'SliderChange'},
   {name: 'SliderEnd'},
   {name: 'SliderStart'},
-  {name: 'SunburstClick', triggersUpdate: true},
+  {name: 'SunburstClick'},
   {name: 'Transitioning'},
   {name: 'TransitionInterrupted'},
-  {name: 'TreemapClick', triggersUpdate: true},
+  {name: 'TreemapClick'},
   {name: 'Unhover'},
   {name: 'WebGlContextLost'},
 ];