<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[213116] trunk/Websites/perf.webkit.org</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/213116">213116</a></dd>
<dt>Author</dt> <dd>rniwa@webkit.org</dd>
<dt>Date</dt> <dd>2017-02-27 19:30:33 -0800 (Mon, 27 Feb 2017)</dd>
</dl>
<h3>Log Message</h3>
<pre>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.</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkWebsitesperfwebkitorgChangeLog">trunk/Websites/perf.webkit.org/ChangeLog</a></li>
<li><a href="#trunkWebsitesperfwebkitorgbrowserteststimeseriescharttestsjs">trunk/Websites/perf.webkit.org/browser-tests/time-series-chart-tests.js</a></li>
<li><a href="#trunkWebsitesperfwebkitorgpublicv3componentsinteractivetimeserieschartjs">trunk/Websites/perf.webkit.org/public/v3/components/interactive-time-series-chart.js</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkWebsitesperfwebkitorgChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Websites/perf.webkit.org/ChangeLog (213115 => 213116)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -1,5 +1,23 @@
</span><span class="cx"> 2017-02-27 Ryosuke Niwa <rniwa@webkit.org>
</span><span class="cx">
</span><ins>+ 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>
+
</ins><span class="cx"> A Locked indicator should be visually distinct from an unlocked indicator
</span><span class="cx"> https://bugs.webkit.org/show_bug.cgi?id=168868
</span><span class="cx"> <rdar://problem/29666054>
</span></span></pre></div>
<a id="trunkWebsitesperfwebkitorgbrowserteststimeseriescharttestsjs"></a>
<div class="modfile"><h4>Modified: trunk/Websites/perf.webkit.org/browser-tests/time-series-chart-tests.js (213115 => 213116)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -1581,6 +1581,197 @@
</span><span class="cx"> });
</span><span class="cx"> });
</span><span class="cx">
</span><ins>+ 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);
+ });
+ });
+ });
+
+ });
</ins><span class="cx"> });
</span><span class="cx">
</span><span class="cx"> })();
</span><span class="cx">\ No newline at end of file
</span></span></pre></div>
<a id="trunkWebsitesperfwebkitorgpublicv3componentsinteractivetimeserieschartjs"></a>
<div class="modfile"><h4>Modified: trunk/Websites/perf.webkit.org/public/v3/components/interactive-time-series-chart.js (213115 => 213116)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -74,8 +74,9 @@
</span><span class="cx"> return false;
</span><span class="cx"> console.assert(!this._selectionTimeRange);
</span><span class="cx">
</span><del>- const newPoint = forward ? indicator.view.nextPoint(indicator.point) : indicator.view.previousPoint(indicator.point);
- if (!newPoint)
</del><ins>+ 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)
</ins><span class="cx"> return false;
</span><span class="cx">
</span><span class="cx"> this._indicatorID = newPoint.id;
</span><span class="lines">@@ -82,6 +83,7 @@
</span><span class="cx"> this._lastMouseDownLocation = null;
</span><span class="cx"> this._forceRender = true;
</span><span class="cx">
</span><ins>+ this.enqueueToRender();
</ins><span class="cx"> this._notifyIndicatorChanged();
</span><span class="cx"> }
</span><span class="cx">
</span></span></pre>
</div>
</div>
</body>
</html>