Skip to content

Commit

Permalink
fix(external pointer): avoid recursive-loops on pointer events (#503)
Browse files Browse the repository at this point in the history 
This commit fix the issue with the recursive loop created when feeding chart with its own cursor position.

BREAKING CHANGE: The `onCursorUpdate` Settings property is changed to a more generic
`onPointerUpdate`. The same apply for the event type `CursorEvent` that is now `PointerEvent` and can assume a `PointerOverEvent` or `PointOutEvent` shape (see TS types)

fix #504
  • Loading branch information
@markov00
markov00 authored Jan 2, 2020
1 parent 48db8c8 commit c170f0d
Show file tree
Hide file tree
Showing 13 changed files with 222 additions and 128 deletions.
45 changes: 25 additions & 20 deletions .playground/playgroud.tsx
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import React from 'react';
import { Chart, Settings, TooltipType, BarSeries } from '../src';
import { Chart, Settings, TooltipType, AreaSeries, PointerEvent } from '../src';
import { KIBANA_METRICS } from '../src/utils/data_samples/test_dataset_kibana';
export class Playground extends React.Component {
chartRef: React.RefObject<Chart> = React.createRef();
chartRef2: React.RefObject<Chart> = React.createRef();
onSnapshot = () => {
if (!this.chartRef.current) {
return;
Expand All @@ -27,39 +28,43 @@ export class Playground extends React.Component {
document.body.removeChild(link);
}
};
onPointerUpdate = (event: PointerEvent) => {
if (this.chartRef && this.chartRef.current) {
this.chartRef.current.dispatchExternalPointerEvent(event);
}
if (this.chartRef2 && this.chartRef2.current) {
this.chartRef2.current.dispatchExternalPointerEvent(event);
}
};
render() {
return (
<>
<button onClick={this.onSnapshot}>Snapshot</button>
<div className="chart">
<Chart size={[200, 100]}>
<Settings tooltip={{ type: TooltipType.Crosshairs }} showLegend />
<BarSeries
id="lines"
xAccessor={0}
yAccessors={[1]}
data={KIBANA_METRICS.metrics.kibana_os_load[0].data.slice(0, 5)}
<Chart ref={this.chartRef}>
<Settings
tooltip={{ type: TooltipType.VerticalCursor }}
showLegend
onPointerUpdate={this.onPointerUpdate}
/>
<BarSeries
id="lines2"
<AreaSeries
id="lines"
xAccessor={0}
yAccessors={[1]}
stackAccessors={[0]}
data={KIBANA_METRICS.metrics.kibana_os_load[0].data.slice(0, 5)}
/>
</Chart>
</div>
<div className="chart">
<Chart>
<Settings tooltip={{ type: TooltipType.Crosshairs }} showLegend />
<BarSeries
id="lines"
xAccessor={0}
yAccessors={[1]}
stackAccessors={[0]}
data={KIBANA_METRICS.metrics.kibana_os_load[0].data.slice(0, 5)}
<Chart ref={this.chartRef2}>
<Settings
tooltip={{ type: TooltipType.VerticalCursor }}
showLegend
onPointerUpdate={this.onPointerUpdate}
/>
<BarSeries
id="lines2"
<AreaSeries
id="lines"
xAccessor={0}
yAccessors={[1]}
stackAccessors={[0]}
Expand Down
113 changes: 82 additions & 31 deletions src/chart_types/xy_chart/state/chart_state.interactions.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import { upsertSpec, specParsed } from '../../../state/actions/specs';
import { updateParentDimensions } from '../../../state/actions/chart_settings';
import { onPointerMove } from '../../../state/actions/mouse';
import { ChartTypes } from '../..';
import { createOnPointerMoveCaller } from './selectors/on_pointer_move_caller';
import { onExternalPointerEvent } from '../../../state/actions/events';

const SPEC_ID = 'spec_1';
const GROUP_ID = 'group_1';
Expand Down Expand Up @@ -250,23 +252,29 @@ function mouseOverTestSuite(scaleType: ScaleType) {
let store: Store<GlobalChartState>;
let onOverListener: jest.Mock<undefined>;
let onOutListener: jest.Mock<undefined>;
let onPointerUpdateListener: jest.Mock<undefined>;
const spec = scaleType === ScaleType.Ordinal ? ordinalBarSeries : linearBarSeries;
beforeEach(() => {
store = initStore(spec);
onOverListener = jest.fn((): undefined => undefined);
onOutListener = jest.fn((): undefined => undefined);
onPointerUpdateListener = jest.fn((): undefined => undefined);
const settingsWithListeners: SettingsSpec = {
...settingSpec,
onElementOver: onOverListener,
onElementOut: onOutListener,
onPointerUpdate: onPointerUpdateListener,
};
store.dispatch(upsertSpec(settingsWithListeners));
store.dispatch(specParsed());
const onElementOutCaller = createOnElementOutCaller();
const onElementOverCaller = createOnElementOverCaller();
const onPointerMoveCaller = createOnPointerMoveCaller();
store.subscribe(() => {
onElementOutCaller(store.getState());
onElementOverCaller(store.getState());
const state = store.getState();
onElementOutCaller(state);
onElementOverCaller(state);
onPointerMoveCaller(state);
});
const tooltipData = getTooltipValuesAndGeometriesSelector(store.getState());
expect(tooltipData.tooltipValues).toEqual([]);
Expand All @@ -279,36 +287,79 @@ function mouseOverTestSuite(scaleType: ScaleType) {
expect(seriesGeoms.scales.yScales).not.toBeUndefined();
});

test.skip('set cursor from external source', () => {
// store.setCursorValue(0);
// expect(store.externalCursorShown.get()).toBe(true);
// expect(store.cursorBandPosition).toEqual({
// height: 100,
// left: 10,
// top: 10,
// visible: true,
// width: 50,
// });
// store.setCursorValue(1);
// expect(store.externalCursorShown.get()).toBe(true);
// expect(store.cursorBandPosition).toEqual({
// height: 100,
// left: 60,
// top: 10,
// visible: true,
// width: 50,
// });
// store.setCursorValue(2);
// expect(store.externalCursorShown.get()).toBe(true);
// // equal to the latest except the visiblility
// expect(store.cursorBandPosition).toEqual({
// height: 100,
// left: 60,
// top: 10,
// visible: false,
// width: 50,
// });
test('avoid call pointer update listener if moving over the same element', () => {
store.dispatch(onPointerMove({ x: chartLeft + 10, y: chartTop + 10 }, 0));
expect(onPointerUpdateListener).toBeCalledTimes(1);

const tooltipData1 = getTooltipValuesAndGeometriesSelector(store.getState());
expect(tooltipData1.tooltipValues.length).toBe(2);
// avoid calls
store.dispatch(onPointerMove({ x: chartLeft + 12, y: chartTop + 12 }, 1));
expect(onPointerUpdateListener).toBeCalledTimes(1);

const tooltipData2 = getTooltipValuesAndGeometriesSelector(store.getState());
expect(tooltipData2.tooltipValues.length).toBe(2);
expect(tooltipData1).toEqual(tooltipData2);
});

test('call pointer update listener on move', () => {
store.dispatch(onPointerMove({ x: chartLeft + 10, y: chartTop + 10 }, 0));
expect(onPointerUpdateListener).toBeCalledTimes(1);
expect(onPointerUpdateListener.mock.calls[0][0]).toEqual({
chartId: 'chartId',
scale: scaleType,
type: 'Over',
unit: undefined,
value: 0,
});

// avoid multiple calls for the same value
store.dispatch(onPointerMove({ x: chartLeft + 50, y: chartTop + 10 }, 1));
expect(onPointerUpdateListener).toBeCalledTimes(2);
expect(onPointerUpdateListener.mock.calls[1][0]).toEqual({
chartId: 'chartId',
scale: scaleType,
type: 'Over',
unit: undefined,
value: 1,
});

store.dispatch(onPointerMove({ x: chartLeft + 200, y: chartTop + 10 }, 1));
expect(onPointerUpdateListener).toBeCalledTimes(3);
expect(onPointerUpdateListener.mock.calls[2][0]).toEqual({
chartId: 'chartId',
type: 'Out',
});
});

test('handle only external pointer update', () => {
store.dispatch(
onExternalPointerEvent({
chartId: 'chartId',
scale: scaleType,
type: 'Over',
unit: undefined,
value: 0,
}),
);
let cursorBandPosition = getCursorBandPositionSelector(store.getState());
expect(cursorBandPosition).toBeDefined();
expect(cursorBandPosition && cursorBandPosition.visible).toBe(false);

store.dispatch(
onExternalPointerEvent({
chartId: 'differentChart',
scale: scaleType,
type: 'Over',
unit: undefined,
value: 0,
}),
);
cursorBandPosition = getCursorBandPositionSelector(store.getState());
expect(cursorBandPosition).toBeDefined();
expect(cursorBandPosition && cursorBandPosition.visible).toBe(true);
});

test.skip('can determine which tooltip to display if chart & annotation tooltips possible', () => {
// const annotationDimensions = [{ rect: { x: 49, y: -1, width: 3, height: 99 } }];
// const rectAnnotationSpec: RectAnnotationSpec = {
Expand Down
8 changes: 4 additions & 4 deletions src/chart_types/xy_chart/state/selectors/get_cursor_band.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { Point } from '../../../../utils/point';
import { Scale } from '../../../../utils/scales/scales';
import { isLineAreaOnlyChart } from '../utils';
import { getCursorBandPosition } from '../../crosshair/crosshair_utils';
import { SettingsSpec, CursorEvent } from '../../../../specs/settings';
import { SettingsSpec, PointerEvent } from '../../../../specs/settings';
import { getSettingsSpecSelector } from '../../../../state/selectors/get_settings_specs';
import { computeChartDimensionsSelector } from './compute_chart_dimensions';
import { BasicSeriesSpec } from '../../utils/specs';
Expand All @@ -15,7 +15,7 @@ import { getOrientedProjectedPointerPositionSelector } from './get_oriented_proj
import { isTooltipSnapEnableSelector } from './is_tooltip_snap_enabled';
import { getGeometriesIndexKeysSelector } from './get_geometries_index_keys';
import { GlobalChartState } from '../../../../state/chart_state';
import { isValidExternalPointerEvent } from '../../../../utils/events';
import { isValidPointerOverEvent } from '../../../../utils/events';
import { getChartIdSelector } from '../../../../state/selectors/get_chart_id';

const getExternalPointerEventStateSelector = (state: GlobalChartState) => state.externalEvents.pointer;
Expand Down Expand Up @@ -59,7 +59,7 @@ export const getCursorBandPositionSelector = createCachedSelector(

function getCursorBand(
orientedProjectedPoinerPosition: Point,
externalPointerEvent: CursorEvent | null,
externalPointerEvent: PointerEvent | null,
chartDimensions: Dimensions,
settingsSpec: SettingsSpec,
xScale: Scale | undefined,
Expand All @@ -75,7 +75,7 @@ function getCursorBand(
}
let pointerPosition = orientedProjectedPoinerPosition;
let xValue;
if (externalPointerEvent && isValidExternalPointerEvent(externalPointerEvent, xScale)) {
if (isValidPointerOverEvent(xScale, externalPointerEvent)) {
const x = xScale.pureScale(externalPointerEvent.value);

if (x == null || x > chartDimensions.width + chartDimensions.left) {
Expand Down
8 changes: 4 additions & 4 deletions src/chart_types/xy_chart/state/selectors/get_elements_at_cursor_pos.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ import { getComputedScalesSelector } from './get_computed_scales';
import { getGeometriesIndexKeysSelector } from './get_geometries_index_keys';
import { getGeometriesIndexSelector } from './get_geometries_index';
import { IndexedGeometry } from '../../../../utils/geometry';
import { CursorEvent } from '../../../../specs';
import { PointerEvent } from '../../../../specs';
import { computeChartDimensionsSelector } from './compute_chart_dimensions';
import { Dimensions } from '../../../../utils/dimensions';
import { GlobalChartState } from '../../../../state/chart_state';
import { isValidExternalPointerEvent } from '../../../../utils/events';
import { isValidPointerOverEvent } from '../../../../utils/events';
import { getChartIdSelector } from '../../../../state/selectors/get_chart_id';

const getExternalPointerEventStateSelector = (state: GlobalChartState) => state.externalEvents.pointer;
Expand All @@ -32,14 +32,14 @@ function getElementAtCursorPosition(
scales: ComputedScales,
geometriesIndexKeys: any,
geometriesIndex: Map<any, IndexedGeometry[]>,
externalPointerEvent: CursorEvent | null,
externalPointerEvent: PointerEvent | null,
{
chartDimensions,
}: {
chartDimensions: Dimensions;
},
): IndexedGeometry[] {
if (externalPointerEvent && isValidExternalPointerEvent(externalPointerEvent, scales.xScale)) {
if (isValidPointerOverEvent(scales.xScale, externalPointerEvent)) {
const x = scales.xScale.pureScale(externalPointerEvent.value);

if (x == null || x > chartDimensions.width + chartDimensions.left) {
Expand Down
13 changes: 8 additions & 5 deletions src/chart_types/xy_chart/state/selectors/get_tooltip_values_highlighted_geoms.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ import { formatTooltip } from '../../tooltip/tooltip';
import { getTooltipHeaderFormatterSelector } from './get_tooltip_header_formatter';
import { isPointOnGeometry } from '../../rendering/rendering';
import { GlobalChartState } from '../../../../state/chart_state';
import { CursorEvent } from '../../../../specs';
import { isValidExternalPointerEvent } from '../../../../utils/events';
import { PointerEvent, isPointerOutEvent } from '../../../../specs';
import { isValidPointerOverEvent } from '../../../../utils/events';
import { getChartRotationSelector } from '../../../../state/selectors/get_chart_rotation';
import { getChartIdSelector } from '../../../../state/selectors/get_chart_id';
import { hasSingleSeriesSelector } from './has_single_series';
Expand Down Expand Up @@ -61,7 +61,7 @@ function getTooltipAndHighlightFromXValue(
scales: ComputedScales,
xMatchingGeoms: IndexedGeometry[],
tooltipType: TooltipType,
externalPointerEvent: CursorEvent | null,
externalPointerEvent: PointerEvent | null,
tooltipHeaderFormatter?: TooltipValueFormatter,
): TooltipAndHighlightedGeoms {
if (!scales.xScale || !scales.yScales) {
Expand All @@ -72,7 +72,7 @@ function getTooltipAndHighlightFromXValue(
}
let x = orientedProjectedPointerPosition.x;
let y = orientedProjectedPointerPosition.y;
if (externalPointerEvent && isValidExternalPointerEvent(externalPointerEvent, scales.xScale)) {
if (isValidPointerOverEvent(scales.xScale, externalPointerEvent)) {
x = scales.xScale.pureScale(externalPointerEvent.value);
y = 0;
} else if (projectedPointerPosition.x === -1 || projectedPointerPosition.y === -1) {
Expand Down Expand Up @@ -108,7 +108,10 @@ function getTooltipAndHighlightFromXValue(

// check if the pointer is on the geometry (avoid checking if using external pointer event)
let isHighlighted = false;
if (!externalPointerEvent && isPointOnGeometry(x, y, indexedGeometry)) {
if (
(!externalPointerEvent || isPointerOutEvent(externalPointerEvent)) &&
isPointOnGeometry(x, y, indexedGeometry)
) {
isHighlighted = true;
highlightedGeometries.push(indexedGeometry);
}
Expand Down
Loading

0 comments on commit c170f0d

Please sign in to comment.