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 bec7e7dc6e42c6734455d8dd9581e5c77b372a6b..f135e2c06e73b8a6531456f9de8bf4cac9ea355e 100644 (file)
@@ -1,3 +1,21 @@
+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
index db7b49f0c6975bf1b5ef228ac3865c043b9a1f7a..4677246daadf652089b5c211e3f745eb0772dcfd 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 1a61a557608c115ff2ddd52e164e989714d27cd7..ddf6da69ae5f3f51d86a34b0e78073f0c5691624 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();
     }