Modified: trunk/Websites/perf.webkit.org/ChangeLog (213115 => 213116)
--- trunk/Websites/perf.webkit.org/ChangeLog 2017-02-28 03:24:37 UTC (rev 213115)
+++ trunk/Websites/perf.webkit.org/ChangeLog 2017-02-28 03:30:33 UTC (rev 213116)
@@ -1,5 +1,23 @@
2017-02-27 Ryosuke Niwa <rn...@webkit.org>
+ Arrow key shouldn't move the indicator beyond the visible points
+ https://bugs.webkit.org/show_bug.cgi?id=168956
+
+ Reviewed by Joseph Pecoraro.
+
+ The bug was caused by moveLockedIndicatorWithNotification using the full sampled time series view
+ instead of the one constrained by the domain. Since the time series chart expands the visible domain
+ to include at least one point before the start time and one point after the end tiem to draw lines
+ extending beyond the visible region (otherwise it looks as though the graph ends there), we need to
+ use a view constrained by the start time and the end time before looking for a next/previous point.
+
+ * browser-tests/time-series-chart-tests.js: Added test cases for moveLockedIndicatorWithNotification.
+ * public/v3/components/interactive-time-series-chart.js:
+ (InteractiveTimeSeriesChart.prototype.moveLockedIndicatorWithNotification): Fixed the bug. Also
+ enqueue itself to render instead of relying on a parent component to do it.
+
+2017-02-27 Ryosuke Niwa <rn...@webkit.org>
+
A Locked indicator should be visually distinct from an unlocked indicator
https://bugs.webkit.org/show_bug.cgi?id=168868
<rdar://problem/29666054>
Modified: trunk/Websites/perf.webkit.org/browser-tests/time-series-chart-tests.js (213115 => 213116)
--- trunk/Websites/perf.webkit.org/browser-tests/time-series-chart-tests.js 2017-02-28 03:24:37 UTC (rev 213115)
+++ trunk/Websites/perf.webkit.org/browser-tests/time-series-chart-tests.js 2017-02-28 03:30:33 UTC (rev 213116)
@@ -1581,6 +1581,197 @@
});
});
+ describe('moveLockedIndicatorWithNotification', () => {
+ it('should move the locked indicator to the right when forward boolean is true', () => {
+ const context = new BrowsingContext();
+ return context.importScripts(scripts, 'ComponentBase', 'InteractiveTimeSeriesChart', 'MeasurementSet', 'MockRemoteAPI').then(() => {
+ const chart = createInteractiveChartWithSampleCluster(context);
+
+ chart.setDomain(sampleCluster.startTime, sampleCluster.endTime);
+ chart.fetchMeasurementSets();
+ let indicatorChangeCount = 0;
+ chart.listenToAction('indicatorChange', () => indicatorChangeCount++);
+ respondWithSampleCluster(context.symbols.MockRemoteAPI.requests[0]);
+
+ let canvas;
+ return waitForComponentsToRender(context).then(() => {
+ expect(indicatorChangeCount).to.be(0);
+
+ canvas = chart.content().querySelector('canvas');
+
+ const rect = canvas.getBoundingClientRect();
+ const x = rect.left + 1;
+ const y = rect.top + rect.height / 2;
+ canvas.dispatchEvent(new MouseEvent('click', {target: canvas, clientX: x, clientY: y, composed: true, bubbles: true}));
+
+ CanvasTest.fillCanvasBeforeRedrawCheck(canvas);
+ return waitForComponentsToRender(context);
+ }).then(() => {
+ expect(CanvasTest.hasCanvasBeenRedrawn(canvas)).to.be(true);
+ expect(indicatorChangeCount).to.be(1);
+
+ let indicator = chart.currentIndicator();
+ let currentView = chart.sampledTimeSeriesData('current');
+ expect(indicator.view).to.be(currentView);
+ expect(indicator.point).to.be(currentView.firstPoint());
+ expect(indicator.isLocked).to.be(true);
+
+ chart.moveLockedIndicatorWithNotification(true);
+
+ CanvasTest.fillCanvasBeforeRedrawCheck(canvas);
+ return waitForComponentsToRender(context);
+ }).then(() => {
+ expect(CanvasTest.hasCanvasBeenRedrawn(canvas)).to.be(true);
+ expect(indicatorChangeCount).to.be(2);
+
+ let indicator = chart.currentIndicator();
+ let currentView = chart.sampledTimeSeriesData('current');
+ expect(indicator.view).to.be(currentView);
+ expect(indicator.point).to.not.be(currentView.firstPoint());
+ expect(indicator.point).to.be(currentView.nextPoint(currentView.firstPoint()));
+ expect(currentView.previousPoint(indicator.point)).to.be(currentView.firstPoint());
+ expect(indicator.isLocked).to.be(true);
+ expect(indicatorChangeCount).to.be(2);
+ });
+ });
+ });
+
+ it('should move the locked indicator to the left when forward boolean is false', () => {
+ const context = new BrowsingContext();
+ return context.importScripts(scripts, 'ComponentBase', 'InteractiveTimeSeriesChart', 'MeasurementSet', 'MockRemoteAPI').then(() => {
+ const chart = createInteractiveChartWithSampleCluster(context);
+
+ chart.setDomain(sampleCluster.startTime, sampleCluster.endTime);
+ chart.fetchMeasurementSets();
+ let indicatorChangeCount = 0;
+ chart.listenToAction('indicatorChange', () => indicatorChangeCount++);
+ respondWithSampleCluster(context.symbols.MockRemoteAPI.requests[0]);
+
+ let canvas;
+ return waitForComponentsToRender(context).then(() => {
+ expect(indicatorChangeCount).to.be(0);
+
+ canvas = chart.content().querySelector('canvas');
+
+ const rect = canvas.getBoundingClientRect();
+ const x = rect.right - 1;
+ const y = rect.top + rect.height / 2;
+ canvas.dispatchEvent(new MouseEvent('click', {target: canvas, clientX: x, clientY: y, composed: true, bubbles: true}));
+
+ CanvasTest.fillCanvasBeforeRedrawCheck(canvas);
+ return waitForComponentsToRender(context);
+ }).then(() => {
+ expect(CanvasTest.hasCanvasBeenRedrawn(canvas)).to.be(true);
+ expect(indicatorChangeCount).to.be(1);
+
+ let indicator = chart.currentIndicator();
+ let currentView = chart.sampledTimeSeriesData('current');
+ expect(indicator.view).to.be(currentView);
+ expect(indicator.point).to.be(currentView.lastPoint());
+ expect(indicator.isLocked).to.be(true);
+
+ chart.moveLockedIndicatorWithNotification(false);
+
+ CanvasTest.fillCanvasBeforeRedrawCheck(canvas);
+ return waitForComponentsToRender(context);
+ }).then(() => {
+ expect(CanvasTest.hasCanvasBeenRedrawn(canvas)).to.be(true);
+ expect(indicatorChangeCount).to.be(2);
+
+ let indicator = chart.currentIndicator();
+ let currentView = chart.sampledTimeSeriesData('current');
+ expect(indicator.view).to.be(currentView);
+ expect(indicator.point).to.not.be(currentView.firstPoint());
+ expect(indicator.point).to.be(currentView.previousPoint(currentView.lastPoint()));
+ expect(currentView.nextPoint(indicator.point)).to.be(currentView.lastPoint());
+ expect(indicator.isLocked).to.be(true);
+ expect(indicatorChangeCount).to.be(2);
+ });
+ });
+ });
+
+ it('should not move the locked indicator when there are no points within the domain', () => {
+ const context = new BrowsingContext();
+ return context.importScripts(scripts, 'ComponentBase', 'InteractiveTimeSeriesChart', 'MeasurementSet', 'MockRemoteAPI').then(() => {
+ const chart = createInteractiveChartWithSampleCluster(context);
+
+ // The domain inclues points 2, 3
+ chart.setDomain(posixTime('2016-01-05T20:00:00Z'), posixTime('2016-01-06T00:00:00Z'));
+ chart.fetchMeasurementSets();
+ let indicatorChangeCount = 0;
+ chart.listenToAction('indicatorChange', () => indicatorChangeCount++);
+ respondWithSampleCluster(context.symbols.MockRemoteAPI.requests[0]);
+
+ let canvas;
+ let currentView;
+ return waitForComponentsToRender(context).then(() => {
+ expect(indicatorChangeCount).to.be(0);
+
+ canvas = chart.content().querySelector('canvas');
+
+ const rect = canvas.getBoundingClientRect();
+ const x = rect.right - 1;
+ const y = rect.top + rect.height / 2;
+ canvas.dispatchEvent(new MouseEvent('click', {target: canvas, clientX: x, clientY: y, composed: true, bubbles: true}));
+
+ CanvasTest.fillCanvasBeforeRedrawCheck(canvas);
+ return waitForComponentsToRender(context);
+ }).then(() => {
+ expect(CanvasTest.hasCanvasBeenRedrawn(canvas)).to.be(true);
+ expect(indicatorChangeCount).to.be(1);
+
+ currentView = chart.sampledTimeSeriesData('current');
+ expect(currentView.length()).to.be(4); // points 0 and 4 are added to draw lines extending beyond the domain.
+ expect([...currentView].map((point) => point.id)).to.be.eql([1000, 1002, 1003, 1004]);
+
+ const indicator = chart.currentIndicator();
+ expect(indicator.view).to.be(currentView);
+ expect(indicator.point.id).to.be(1003);
+ expect(indicator.isLocked).to.be(true);
+
+ chart.moveLockedIndicatorWithNotification(true);
+
+ CanvasTest.fillCanvasBeforeRedrawCheck(canvas);
+ return waitForComponentsToRender(context);
+ }).then(() => {
+ expect(CanvasTest.hasCanvasBeenRedrawn(canvas)).to.be(false);
+
+ expect(indicatorChangeCount).to.be(1);
+ const indicator = chart.currentIndicator();
+ expect(indicator.view).to.be(currentView);
+ expect(indicator.point.id).to.be(1003);
+ expect(indicator.isLocked).to.be(true);
+
+ chart.moveLockedIndicatorWithNotification(false);
+
+ CanvasTest.fillCanvasBeforeRedrawCheck(canvas);
+ return waitForComponentsToRender(context);
+ }).then(() => {
+ expect(CanvasTest.hasCanvasBeenRedrawn(canvas)).to.be(true);
+
+ expect(indicatorChangeCount).to.be(2);
+ const indicator = chart.currentIndicator();
+ expect(indicator.view).to.be(currentView);
+ expect(indicator.point.id).to.be(1002);
+ expect(indicator.isLocked).to.be(true);
+
+ chart.moveLockedIndicatorWithNotification(false);
+
+ CanvasTest.fillCanvasBeforeRedrawCheck(canvas);
+ return waitForComponentsToRender(context);
+ }).then(() => {
+ expect(CanvasTest.hasCanvasBeenRedrawn(canvas)).to.be(false);
+
+ expect(indicatorChangeCount).to.be(2);
+ const indicator = chart.currentIndicator();
+ expect(indicator.view).to.be(currentView);
+ expect(indicator.point.id).to.be(1002);
+ expect(indicator.isLocked).to.be(true);
+ });
+ });
+ });
+
+ });
});
})();
\ No newline at end of file
Modified: trunk/Websites/perf.webkit.org/public/v3/components/interactive-time-series-chart.js (213115 => 213116)
--- trunk/Websites/perf.webkit.org/public/v3/components/interactive-time-series-chart.js 2017-02-28 03:24:37 UTC (rev 213115)
+++ trunk/Websites/perf.webkit.org/public/v3/components/interactive-time-series-chart.js 2017-02-28 03:30:33 UTC (rev 213116)
@@ -74,8 +74,9 @@
return false;
console.assert(!this._selectionTimeRange);
- const newPoint = forward ? indicator.view.nextPoint(indicator.point) : indicator.view.previousPoint(indicator.point);
- if (!newPoint)
+ const constrainedView = indicator.view.viewTimeRange(this._startTime, this._endTime);
+ const newPoint = forward ? constrainedView.nextPoint(indicator.point) : constrainedView.previousPoint(indicator.point);
+ if (!newPoint || this._indicatorID == newPoint.id)
return false;
this._indicatorID = newPoint.id;
@@ -82,6 +83,7 @@
this._lastMouseDownLocation = null;
this._forceRender = true;
+ this.enqueueToRender();
this._notifyIndicatorChanged();
}
