fix: decuple brush cursor from chart rendering (opensearch-project#331)

This PR remove the unnecessary style recalculation caused by chancing the cursor style on the document body from the chart_state. This was noticeable on this PR elastic/kibana#36517 (review) where the style reflow calculation tooks up to 75ms. This PR decouple the chart container from the chart renderer, allowing to set correctly the cursor changing the class without re-rendering the chart component, as the actual implementation. The cursor style now depends also on the status of the highlighted geometries. If one geometry is highlighted but no onElementClick or onElementOver listener is available, the cursor will be default if the brush is disabled, or crosshair if the brush is Enabled. The pointer is shown only if we have one between onElementClick or onElementOver enabled and we are over a geometry
AMoo-Miki · Aug 21, 2019 · b5b8dde · b5b8dde
1 parent 0b38770
commit b5b8dde
Show file tree

Hide file tree

Showing 7 changed files with 149 additions and 100 deletions.
diff --git a/packages/osd-charts/src/chart_types/xy_chart/store/chart_state.test.ts b/packages/osd-charts/src/chart_types/xy_chart/store/chart_state.test.ts
@@ -902,21 +902,63 @@ describe('Chart Store', () => {
       store.cursorPosition.x = -1;
       store.cursorPosition.y = -1;
       store.onBrushEndListener = brushEndListener;
-      expect(store.isCrosshairCursorVisible.get()).toBe(false);
+      expect(store.chartCursor.get()).toBe('default');
     });
 
     test('when cursor is within chart bounds and brush enabled', () => {
       store.cursorPosition.x = 10;
       store.cursorPosition.y = 10;
       store.onBrushEndListener = brushEndListener;
-      expect(store.isCrosshairCursorVisible.get()).toBe(true);
+      expect(store.chartCursor.get()).toBe('crosshair');
     });
 
     test('when cursor is within chart bounds and brush disabled', () => {
       store.cursorPosition.x = 10;
       store.cursorPosition.y = 10;
       store.onBrushEndListener = undefined;
-      expect(store.isCrosshairCursorVisible.get()).toBe(false);
+      expect(store.chartCursor.get()).toBe('default');
+    });
+    test('when cursor is within chart bounds and brush enabled but over one geom', () => {
+      store.cursorPosition.x = 10;
+      store.cursorPosition.y = 10;
+      store.onBrushEndListener = brushEndListener;
+      const geom1: IndexedGeometry = {
+        color: 'red',
+        geometryId: {
+          specId: getSpecId('specId1'),
+          seriesKey: [2],
+        },
+        value: {
+          x: 0,
+          y: 1,
+          accessor: 'y1',
+        },
+        x: 0,
+        y: 0,
+        width: 0,
+        height: 0,
+        seriesStyle: {
+          rect: {
+            opacity: 1,
+          },
+          rectBorder: {
+            strokeWidth: 1,
+            visible: false,
+          },
+          displayValue: {
+            fill: 'black',
+            fontFamily: '',
+            fontSize: 2,
+            offsetX: 0,
+            offsetY: 0,
+            padding: 2,
+          },
+        },
+      };
+      store.highlightedGeometries.replace([geom1]);
+      expect(store.chartCursor.get()).toBe('crosshair');
+      store.onElementClickListener = jest.fn();
+      expect(store.chartCursor.get()).toBe('pointer');
     });
   });
   test('should set tooltip type to follow when single value x scale', () => {

diff --git a/packages/osd-charts/src/chart_types/xy_chart/store/chart_state.ts b/packages/osd-charts/src/chart_types/xy_chart/store/chart_state.ts
@@ -238,17 +238,16 @@ export class ChartStore {
   legendPosition = observable.box<Position>(Position.Right);
   showLegendDisplayValue = observable.box(true);
 
-  /**
-   * determine if crosshair cursor should be visible based on cursor position and brush enablement
-   */
-  isCrosshairCursorVisible = computed(() => {
+  chartCursor = computed(() => {
     const { x: xPos, y: yPos } = this.cursorPosition;
 
     if (yPos < 0 || xPos < 0) {
-      return false;
+      return 'default';
     }
-
-    return this.isBrushEnabled();
+    if (this.highlightedGeometries.length > 0 && (this.onElementClickListener || this.onElementOverListener)) {
+      return 'pointer';
+    }
+    return this.isBrushEnabled() ? 'crosshair' : 'default';
   });
 
   /**
@@ -466,13 +465,6 @@ export class ChartStore {
     } else {
       this.tooltipData.replace(tooltipValues);
     }
-
-    // TODO move this into the renderer
-    if (oneHighlighted) {
-      document.body.style.cursor = 'pointer';
-    } else {
-      document.body.style.cursor = 'default';
-    }
   });
 
   legendItemTooltipValues = computed(() => {
@@ -549,8 +541,6 @@ export class ChartStore {
     this.highlightedGeometries.clear();
     this.tooltipData.clear();
 
-    document.body.style.cursor = 'default';
-
     if (this.onCursorUpdateListener && this.isActiveChart.get()) {
       this.onCursorUpdateListener();
     }

diff --git a/packages/osd-charts/src/components/_container.scss b/packages/osd-charts/src/components/_container.scss
@@ -11,11 +11,26 @@
   display: flex;
   height: 100%;
 
-  &--isBrushEnabled {
-    cursor: crosshair;
-  }
-
   &--column {
     flex-direction: column;
   }
 }
+
+.echChartCursorContainer {
+  position: absolute;
+  top: 0;
+  bottom: 0;
+  right: 0;
+  left: 0;
+  box-sizing: border-box;
+}
+
+.echChartResizer {
+  z-index: -10000000;
+  position: absolute;
+  bottom: 0;
+  top: 0;
+  left: 0;
+  right: 0;
+  box-sizing: border-box;
+}
diff --git a/packages/osd-charts/src/components/chart.tsx b/packages/osd-charts/src/components/chart.tsx
@@ -9,7 +9,7 @@ import { ChartResizer } from './chart_resizer';
 import { Crosshair } from './crosshair';
 import { Highlighter } from './highlighter';
 import { Legend } from './legend/legend';
-import { ReactiveChart as ReactChart } from './react_canvas/reactive_chart';
+import { ChartContainer } from './react_canvas/chart_container';
 import { Tooltips } from './tooltips';
 import { isHorizontal } from '../chart_types/xy_chart/utils/axis_utils';
 import { Position } from '../chart_types/xy_chart/utils/specs';
@@ -94,8 +94,8 @@ export class Chart extends React.Component<ChartProps, ChartState> {
             <ChartResizer />
             <Crosshair />
             {// TODO reenable when SVG rendered is aligned with canvas one
-            renderer === 'svg' && <ReactChart />}
-            {renderer === 'canvas' && <ReactChart />}
+            renderer === 'svg' && <ChartContainer />}
+            {renderer === 'canvas' && <ChartContainer />}
             <Tooltips />
             <AnnotationTooltip />
             <Highlighter />

diff --git a/packages/osd-charts/src/components/chart_resizer.tsx b/packages/osd-charts/src/components/chart_resizer.tsx
@@ -35,20 +35,7 @@ class Resizer extends React.Component<ResizerProps> {
   };
 
   render() {
-    return (
-      <div
-        ref={this.containerRef}
-        style={{
-          zIndex: -10000000,
-          position: 'absolute',
-          bottom: 0,
-          top: 0,
-          left: 0,
-          right: 0,
-          boxSizing: 'border-box',
-        }}
-      />
-    );
+    return <div ref={this.containerRef} className="echChartResizer" />;
   }
 
   private handleResize = (entries: ResizeObserverEntry[]) => {

diff --git a/packages/osd-charts/src/components/react_canvas/chart_container.tsx b/packages/osd-charts/src/components/react_canvas/chart_container.tsx
@@ -0,0 +1,43 @@
+import React from 'react';
+import { inject, observer } from 'mobx-react';
+import { ChartStore } from '../../chart_types/xy_chart/store/chart_state';
+import { ReactiveChart } from './reactive_chart';
+interface ReactiveChartProps {
+  chartStore?: ChartStore; // FIX until we find a better way on ts mobx
+}
+
+class ChartContainerComponent extends React.Component<ReactiveChartProps> {
+  static displayName = 'ChartContainer';
+
+  render() {
+    const { chartInitialized } = this.props.chartStore!;
+    if (!chartInitialized.get()) {
+      return null;
+    }
+    const { setCursorPosition } = this.props.chartStore!;
+    return (
+      <div
+        className="echChartCursorContainer"
+        style={{
+          cursor: this.props.chartStore!.chartCursor.get(),
+        }}
+        onMouseMove={({ nativeEvent: { offsetX, offsetY } }) => {
+          setCursorPosition(offsetX, offsetY);
+        }}
+        onMouseLeave={() => {
+          setCursorPosition(-1, -1);
+        }}
+        onMouseUp={() => {
+          if (this.props.chartStore!.isBrushing.get()) {
+            return;
+          }
+          this.props.chartStore!.handleChartClick();
+        }}
+      >
+        <ReactiveChart />
+      </div>
+    );
+  }
+}
+
+export const ChartContainer = inject('chartStore')(observer(ChartContainerComponent));
diff --git a/packages/osd-charts/src/components/react_canvas/reactive_chart.tsx b/packages/osd-charts/src/components/react_canvas/reactive_chart.tsx
@@ -1,4 +1,3 @@
-import classNames from 'classnames';
 import { inject, observer } from 'mobx-react';
 import React from 'react';
 import { Layer, Rect, Stage } from 'react-konva';
@@ -352,7 +351,6 @@ class Chart extends React.Component<ReactiveChartProps, ReactiveChartState> {
       chartRotation,
       chartTransform,
       debug,
-      setCursorPosition,
       isChartEmpty,
     } = this.props.chartStore!;
 
@@ -373,74 +371,48 @@ class Chart extends React.Component<ReactiveChartProps, ReactiveChartState> {
       };
     }
 
-    const className = classNames({
-      'echChart--isBrushEnabled': this.props.chartStore!.isCrosshairCursorVisible.get(),
-    });
-
     return (
-      <div
+      <Stage
+        width={parentDimensions.width}
+        height={parentDimensions.height}
         style={{
-          position: 'absolute',
-          top: 0,
-          bottom: 0,
-          right: 0,
-          left: 0,
-          boxSizing: 'border-box',
-        }}
-        onMouseMove={({ nativeEvent: { offsetX, offsetY } }) => {
-          setCursorPosition(offsetX, offsetY);
-        }}
-        onMouseLeave={() => {
-          setCursorPosition(-1, -1);
+          width: '100%',
+          height: '100%',
         }}
-        onMouseUp={() => {
-          if (this.props.chartStore!.isBrushing.get()) {
-            return;
-          }
-          this.props.chartStore!.handleChartClick();
-        }}
-        className={className}
+        {...brushProps}
       >
-        <Stage
-          width={parentDimensions.width}
-          height={parentDimensions.height}
-          style={{
-            width: '100%',
-            height: '100%',
-          }}
-          {...brushProps}
+        <Layer hitGraphEnabled={false} listening={false}>
+          {this.renderGrids()}
+        </Layer>
+        <Layer hitGraphEnabled={false} listening={false}>
+          {this.renderAxes()}
+        </Layer>
+
+        <Layer
+          x={chartDimensions.left + chartTransform.x}
+          y={chartDimensions.top + chartTransform.y}
+          rotation={chartRotation}
+          hitGraphEnabled={false}
+          listening={false}
         >
-          <Layer hitGraphEnabled={false} listening={false}>
-            {this.renderGrids()}
-          </Layer>
-          <Layer hitGraphEnabled={false} listening={false}>
-            {this.renderAxes()}
-          </Layer>
-
-          <Layer
-            x={chartDimensions.left + chartTransform.x}
-            y={chartDimensions.top + chartTransform.y}
-            rotation={chartRotation}
-            hitGraphEnabled={false}
-            listening={false}
-          >
-            {this.sortAndRenderElements()}
-          </Layer>
+          {this.sortAndRenderElements()}
+        </Layer>
 
+        {debug && (
           <Layer hitGraphEnabled={false} listening={false}>
-            {debug && this.renderDebugChartBorders()}
+            {this.renderDebugChartBorders()}
           </Layer>
-          {isBrushEnabled && (
-            <Layer hitGraphEnabled={false} listening={false}>
-              {this.renderBrushTool()}
-            </Layer>
-          )}
-
+        )}
+        {isBrushEnabled && (
           <Layer hitGraphEnabled={false} listening={false}>
-            {this.renderBarValues()}
+            {this.renderBrushTool()}
           </Layer>
-        </Stage>
-      </div>
+        )}
+
+        <Layer hitGraphEnabled={false} listening={false}>
+          {this.renderBarValues()}
+        </Layer>
+      </Stage>
     );
   }