Arrow key shouldn't move the indicator beyond the visible points
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 28 Feb 2017 03:30:33 +0000 (03:30 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 28 Feb 2017 03:30:33 +0000 (03:30 +0000)
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.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@213116 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/browser-tests/time-series-chart-tests.js
Websites/perf.webkit.org/public/v3/components/interactive-time-series-chart.js

index bec7e7d..f135e2c 100644 (file)
@@ -1,5 +1,23 @@
 2017-02-27  Ryosuke Niwa  <rniwa@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  <rniwa@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>
index db7b49f..4677246 100644 (file)
@@ -1581,6 +1581,197 @@ describe('InteractiveTimeSeriesChart', () => {
         });
     });
 
+    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
index 1a61a55..ddf6da6 100644 (file)
@@ -74,14 +74,16 @@ class InteractiveTimeSeriesChart extends TimeSeriesChart {
             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;
         this._lastMouseDownLocation = null;
         this._forceRender = true;
 
+        this.enqueueToRender();
         this._notifyIndicatorChanged();
     }