Skip to content

Commit

Permalink
Fixed the dangling interaction issue (#1287)
Browse files Browse the repository at this point in the history 
Interactions do not get destroyed or deactivated automatically
even if a component it is attached to is detached from DOM. As
a result, event listeners were eagerly listening to mouse events
and expected a component to be mounted.

Cleaned up vz-line-chart a bit by factoring interaction out of
tooltip rendering. Interaction is supposed to be reused over
time.
  • Loading branch information
@stephanwlee
stephanwlee authored Jul 13, 2018
1 parent eea9ee8 commit 799f010
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 57 deletions.
12 changes: 10 additions & 2 deletions tensorboard/components/vz_line_chart/dragZoomInteraction.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,19 @@ export class DragZoomLayer extends Plottable.Components.SelectionBoxLayer {
this.xScale(xScale);
this.yScale(yScale);
this._dragInteraction = new Plottable.Interactions.Drag();
this._dragInteraction.attachTo(this);
this._doubleClickInteraction = new Plottable.Interactions.Click();
this._doubleClickInteraction.attachTo(this);
this.setupCallbacks();
this.unzoomMethod = unzoomMethod;

// Activate interaction only when the component is mounted onto DOM.
this.onDetach(() => {
this._doubleClickInteraction.detachFrom(this);
this._dragInteraction.detachFrom(this);
});
this.onAnchor(() => {
this._doubleClickInteraction.attachTo(this);
this._dragInteraction.attachTo(this);
});
}

/**
Expand Down
113 changes: 58 additions & 55 deletions tensorboard/components/vz_line_chart/vz-line-chart.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -335,12 +335,16 @@ Polymer({
this._chart.redraw();
}
},

attached: function() {
this._attached = true;
},

detached: function() {
if (this._chart) this._chart.destroy();
this._attached = false;
},

ready: function() {
this.scopeSubtree(this.$.tooltip, true);
this.scopeSubtree(this.$.chartdiv, true);
Expand Down Expand Up @@ -379,7 +383,6 @@ Polymer({
!this.tooltipColumns) {
return;
}
if (this._chart) this._chart.destroy();
var tooltip = d3.select(this.$.tooltip);
// We directly reference properties of `this` because this call is
// asynchronous, and values may have changed in between the call being
Expand All @@ -398,9 +401,11 @@ Polymer({
this.xAxisFormatter);
var div = d3.select(this.$.chartdiv);
chart.renderTo(div);
if (this._chart) this._chart.destroy();
this._chart = chart;
}, 350);
},

_reloadFromCache: function() {
if (this._chart) {
this._chart.setVisibleSeries(this._visibleSeriesCache);
Expand Down Expand Up @@ -461,7 +466,12 @@ class LineChart {
private outer: Plottable.Components.Table;
private colorScale: Plottable.Scales.Color;
private symbolFunction: vz_chart_helpers.SymbolFn;

private tooltipColumns: vz_chart_helpers.TooltipColumn[];
private tooltip: d3.Selection<any, any, any, any>;
private tooltipInteraction: Plottable.Interactions.Pointer;
private tooltipPointsComponent: Plottable.Component;

private dzl: DragZoomLayer;

private linePlot: Plottable.Plots.Line<number|Date>;
Expand Down Expand Up @@ -525,12 +535,12 @@ class LineChart {

this._defaultXRange = defaultXRange;
this._defaultYRange = defaultYRange;
this.tooltipColumns = tooltipColumns;

this.buildChart(
xComponentsCreationMethod,
yValueAccessor,
yScaleType,
tooltipColumns,
fillArea,
xAxisFormatter);
}
Expand All @@ -539,12 +549,9 @@ class LineChart {
xComponentsCreationMethod: () => vz_chart_helpers.XComponents,
yValueAccessor: Plottable.IAccessor<number>,
yScaleType: string,
tooltipColumns: vz_chart_helpers.TooltipColumn[],
fillArea: FillArea,
xAxisFormatter: (number) => string) {
if (this.outer) {
this.outer.destroy();
}
this.destroy();
const xComponents = xComponentsCreationMethod();
this.xAccessor = xComponents.accessor;
this.xScale = xComponents.scale;
Expand All @@ -564,10 +571,12 @@ class LineChart {
this.dzl = new DragZoomLayer(
this.xScale, this.yScale, this.resetYDomain.bind(this));

let center = this.buildPlot(
this.tooltipInteraction = this.createTooltipInteraction(this.dzl);
this.tooltipPointsComponent = new Plottable.Component();

const plot = this.buildPlot(
this.xScale,
this.yScale,
tooltipColumns,
fillArea);

this.gridlines =
Expand All @@ -578,13 +587,14 @@ class LineChart {
let yZeroLine = new Plottable.Components.GuideLineLayer('vertical');
yZeroLine.scale(this.xScale).value(0);

this.center = new Plottable.Components.Group(
[this.gridlines, xZeroLine, yZeroLine, center, this.dzl]);
this.center = new Plottable.Components.Group([
this.gridlines, xZeroLine, yZeroLine, plot,
this.dzl, this.tooltipPointsComponent]);
this.outer = new Plottable.Components.Table(
[[this.yAxis, this.center], [null, this.xAxis]]);
}

private buildPlot(xScale, yScale, tooltipColumns, fillArea):
private buildPlot(xScale, yScale, fillArea):
Plottable.Component {
if (fillArea) {
this.marginAreaPlot = new Plottable.Plots.Area<number|Date>();
Expand All @@ -608,7 +618,7 @@ class LineChart {
(d: vz_chart_helpers.Datum, i: number, dataset: Plottable.Dataset) =>
this.colorScale.scale(dataset.metadata().name));
this.linePlot = linePlot;
const group = this.setupTooltips(linePlot, tooltipColumns);
this.setupTooltips(linePlot);

let smoothLinePlot = new Plottable.Plots.Line<number|Date>();
smoothLinePlot.x(this.xAccessor, xScale);
Expand Down Expand Up @@ -660,7 +670,7 @@ class LineChart {
nanDisplay.symbol(Plottable.SymbolFactories.triangle);
this.nanDisplay = nanDisplay;

const groups = [nanDisplay, scatterPlot, smoothLinePlot, group];
const groups = [nanDisplay, scatterPlot, smoothLinePlot, linePlot];
if (this.marginAreaPlot) {
groups.push(this.marginAreaPlot);
}
Expand Down Expand Up @@ -805,53 +815,30 @@ class LineChart {
return this.smoothingEnabled ? this.smoothedAccessor : this.yValueAccessor;
}

private setupTooltips(
plot: Plottable.XYPlot<number|Date, number>,
tooltipColumns: vz_chart_helpers.TooltipColumn[]):
Plottable.Components.Group {
let pi = new Plottable.Interactions.Pointer();
pi.attachTo(plot);
// vz_chart_helpers.PointsComponent is a Plottable Component that will
// hold the little circles we draw over the closest data points
let pointsComponent = new Plottable.Component();
let group = new Plottable.Components.Group([plot, pointsComponent]);

let hideTooltips = () => {
this.tooltip.style('opacity', 0);
this.scatterPlot.attr('opacity', 1);
pointsComponent.content().selectAll('.point').remove();
};

let enabled = true;
let disableTooltips = () => {
enabled = false;
hideTooltips();
};
let enableTooltips = () => {
enabled = true;
};

this.dzl.interactionStart(disableTooltips);
this.dzl.interactionEnd(enableTooltips);
private createTooltipInteraction(dzl: DragZoomLayer):
Plottable.Interactions.Pointer {
const pi = new Plottable.Interactions.Pointer();
// Disable interaction while drag zooming.
dzl.interactionStart(() => {
pi.enabled(false);
this.hideTooltips();
});
dzl.interactionEnd(() => pi.enabled(true));

pi.onPointerMove((p: Plottable.Point) => {
if (!enabled) {
return;
}
// Line plot must be initialized to draw.
if (!this.linePlot) return;
let target: vz_chart_helpers.Point = {
x: p.x,
y: p.y,
datum: null,
dataset: null,
};


let bbox: SVGRect = (<any>this.gridlines.content().node()).getBBox();

// pts is the closets point to the tooltip for each dataset
let pts = plot.datasets()
let pts = this.linePlot.datasets()
.map((dataset) => this.findClosestPoint(target, dataset))
.filter(x => x != null);
.filter(Boolean);
let intersectsBBox = Plottable.Utils.DOM.intersectsBBox;
// We draw tooltips for points that are NaN, or are currently visible
let ptsForTooltips = pts.filter(
Expand All @@ -862,7 +849,7 @@ class LineChart {
(p) => !isNaN(this.yValueAccessor(p.datum, 0, p.dataset)));

let ptsSelection: any =
pointsComponent.content().selectAll('.point').data(
this.tooltipPointsComponent.content().selectAll('.point').data(
ptsToCircle,
(p: vz_chart_helpers.Point) => p.dataset.metadata().name);
if (pts.length !== 0) {
Expand All @@ -875,15 +862,30 @@ class LineChart {
'fill',
(p) => this.colorScale.scale(p.dataset.metadata().name));
ptsSelection.exit().remove();
this.drawTooltips(ptsForTooltips, target, tooltipColumns);
this.drawTooltips(ptsForTooltips, target, this.tooltipColumns);
} else {
hideTooltips();
this.hideTooltips();
}
});
pi.onPointerExit(() => this.hideTooltips());
return pi;
}

pi.onPointerExit(hideTooltips);
private hideTooltips(): void {
this.tooltip.style('opacity', 0);
this.scatterPlot.attr('opacity', 1);
this.tooltipPointsComponent.content().selectAll('.point').remove();
}

return group;
private setupTooltips(plot: Plottable.XYPlot<number|Date, number>): void {
plot.onDetach(() => {
this.tooltipInteraction.detachFrom(plot);
this.tooltipInteraction.enabled(false);
});
plot.onAnchor(() => {
this.tooltipInteraction.attachTo(plot);
this.tooltipInteraction.enabled(true);
});
}

private drawTooltips(
Expand Down Expand Up @@ -1173,7 +1175,8 @@ class LineChart {
}

public destroy() {
this.outer.destroy();
// Destroying outer destroys all subcomponents recursively.
if (this.outer) this.outer.destroy();
}
}

Expand Down

0 comments on commit 799f010

Please sign in to comment.