From 33a51ed4bbb9eff20e061227f0cda3769f175184 Mon Sep 17 00:00:00 2001 From: J Copperfield Date: Wed, 29 Nov 2017 10:22:52 +0100 Subject: [PATCH 1/7] Fix issue #4989 - tooltip in 'index' mode doesn't animate smoothly. --- src/core/core.tooltip.js | 32 ++++++++++++++++---------------- test/specs/core.tooltip.tests.js | 4 ++++ 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/src/core/core.tooltip.js b/src/core/core.tooltip.js index 0072580dfbf..de6fbf6d058 100644 --- a/src/core/core.tooltip.js +++ b/src/core/core.tooltip.js @@ -852,27 +852,27 @@ module.exports = function(Chart) { // Remember Last Actives changed = !helpers.arrayEquals(me._active, me._lastActive); - // If tooltip didn't change, do not handle the target event - if (!changed) { - return false; - } + // Only handle target event on tooltip change + if (changed) { + me._lastActive = me._active; - me._lastActive = me._active; + if (options.enabled || options.custom) { + me._eventPosition = { + x: e.x, + y: e.y + }; - if (options.enabled || options.custom) { - me._eventPosition = { - x: e.x, - y: e.y - }; + var model = me._model; + me.update(true); - var model = me._model; - me.update(true); - me.pivot(); - - // See if our tooltip position changed - changed |= (model.x !== me._model.x) || (model.y !== me._model.y); + // See if our tooltip position changed + changed |= (model.x !== me._model.x) || (model.y !== me._model.y); + } } + // always call me.pivot before returning to + // reset me._start for smooth animations issue #4989 + me.pivot(); return changed; } }); diff --git a/test/specs/core.tooltip.tests.js b/test/specs/core.tooltip.tests.js index 8878d9d9dbc..8f2d7c9772c 100755 --- a/test/specs/core.tooltip.tests.js +++ b/test/specs/core.tooltip.tests.js @@ -808,19 +808,23 @@ describe('Core.Tooltip', function() { var tooltip = chart.tooltip; spyOn(tooltip, 'update'); + spyOn(tooltip, 'pivot'); /* Manually trigger rather than having an async test */ // First dispatch change event, should update tooltip node.dispatchEvent(firstEvent); expect(tooltip.update).toHaveBeenCalledWith(true); + expect(tooltip.pivot).toHaveBeenCalledWith(); // Reset calls tooltip.update.calls.reset(); + tooltip.pivot.calls.reset(); // Second dispatch change event (same event), should not update tooltip node.dispatchEvent(firstEvent); expect(tooltip.update).not.toHaveBeenCalled(); + expect(tooltip.pivot).toHaveBeenCalledWith(); }); describe('positioners', function() { From ce679dcca43aea68afcf3702e4988c0731ec4368 Mon Sep 17 00:00:00 2001 From: J Copperfield Date: Thu, 30 Nov 2017 22:05:20 +0100 Subject: [PATCH 2/7] Change: different approach for smooth tooltip animation in 'index' mode, when target doesn't change. --- src/core/core.controller.js | 6 +++++- src/core/core.tooltip.js | 7 +------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/core/core.controller.js b/src/core/core.controller.js index 5fd4b2d6aa6..1340035eb5b 100644 --- a/src/core/core.controller.js +++ b/src/core/core.controller.js @@ -821,7 +821,11 @@ module.exports = function(Chart) { me._bufferedRequest = null; var changed = me.handleEvent(e); - changed |= tooltip && tooltip.handleEvent(e); + // for smooth tooltip animations issue #4989 + // the tooltip should be the source of change + if (tooltip) { + changed = tooltip.handleEvent(e); + } plugins.notify(me, 'afterEvent', [e]); diff --git a/src/core/core.tooltip.js b/src/core/core.tooltip.js index de6fbf6d058..e96d46e8784 100644 --- a/src/core/core.tooltip.js +++ b/src/core/core.tooltip.js @@ -864,15 +864,10 @@ module.exports = function(Chart) { var model = me._model; me.update(true); - - // See if our tooltip position changed - changed |= (model.x !== me._model.x) || (model.y !== me._model.y); + me.pivot(); } } - // always call me.pivot before returning to - // reset me._start for smooth animations issue #4989 - me.pivot(); return changed; } }); From 982f0b42b75626aefe6ad4fe34d5deeac58d0aae Mon Sep 17 00:00:00 2001 From: J Copperfield Date: Thu, 30 Nov 2017 22:13:08 +0100 Subject: [PATCH 3/7] Fix: jslint error --- src/core/core.tooltip.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/core/core.tooltip.js b/src/core/core.tooltip.js index e96d46e8784..9b09d760443 100644 --- a/src/core/core.tooltip.js +++ b/src/core/core.tooltip.js @@ -862,7 +862,6 @@ module.exports = function(Chart) { y: e.y }; - var model = me._model; me.update(true); me.pivot(); } From 482e59f8dbbe6be2a696c968711a0d28401ff0e0 Mon Sep 17 00:00:00 2001 From: J Copperfield Date: Thu, 30 Nov 2017 22:24:32 +0100 Subject: [PATCH 4/7] Fix: remove spyOn pivot from test --- test/specs/core.tooltip.tests.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/test/specs/core.tooltip.tests.js b/test/specs/core.tooltip.tests.js index 8f2d7c9772c..8878d9d9dbc 100755 --- a/test/specs/core.tooltip.tests.js +++ b/test/specs/core.tooltip.tests.js @@ -808,23 +808,19 @@ describe('Core.Tooltip', function() { var tooltip = chart.tooltip; spyOn(tooltip, 'update'); - spyOn(tooltip, 'pivot'); /* Manually trigger rather than having an async test */ // First dispatch change event, should update tooltip node.dispatchEvent(firstEvent); expect(tooltip.update).toHaveBeenCalledWith(true); - expect(tooltip.pivot).toHaveBeenCalledWith(); // Reset calls tooltip.update.calls.reset(); - tooltip.pivot.calls.reset(); // Second dispatch change event (same event), should not update tooltip node.dispatchEvent(firstEvent); expect(tooltip.update).not.toHaveBeenCalled(); - expect(tooltip.pivot).toHaveBeenCalledWith(); }); describe('positioners', function() { From 38574c38fec3905949a66824785e635e2811f8dd Mon Sep 17 00:00:00 2001 From: J Copperfield Date: Sun, 3 Dec 2017 12:29:34 +0100 Subject: [PATCH 5/7] Add: setAnimating-flag in transition used to set on tooltip.transition to keep track of tooltip animation. --- src/core/core.controller.js | 6 ++++-- src/core/core.element.js | 9 ++++++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/core/core.controller.js b/src/core/core.controller.js index 1340035eb5b..969f3f72c2e 100644 --- a/src/core/core.controller.js +++ b/src/core/core.controller.js @@ -555,7 +555,7 @@ module.exports = function(Chart) { } } - me.tooltip.transition(easingValue); + me.tooltip.transition(easingValue, true); }, /** @@ -824,7 +824,9 @@ module.exports = function(Chart) { // for smooth tooltip animations issue #4989 // the tooltip should be the source of change if (tooltip) { - changed = tooltip.handleEvent(e); + changed = tooltip.animating + ? tooltip.handleEvent(e) + : changed | tooltip.handleEvent(e); } plugins.notify(me, 'afterEvent', [e]); diff --git a/src/core/core.element.js b/src/core/core.element.js index 2ca60692b97..48837073971 100644 --- a/src/core/core.element.js +++ b/src/core/core.element.js @@ -72,7 +72,7 @@ helpers.extend(Element.prototype, { return me; }, - transition: function(ease) { + transition: function(ease, setAnimating) { var me = this; var model = me._model; var start = me._start; @@ -80,6 +80,9 @@ helpers.extend(Element.prototype, { // No animation -> No Transition if (!model || ease === 1) { + if (setAnimating) { + me.animating = false; + } me._view = model; me._start = null; return me; @@ -95,6 +98,10 @@ helpers.extend(Element.prototype, { interpolate(start, view, model, ease); + if (setAnimating && !me.animating) { + me.animating = true; + } + return me; }, From f7d4d546a60abaa77bac53f119afb9dd9598d7e0 Mon Sep 17 00:00:00 2001 From: J Copperfield Date: Sun, 3 Dec 2017 18:00:46 +0100 Subject: [PATCH 6/7] Decrease code complexity --- src/core/core.element.js | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/core/core.element.js b/src/core/core.element.js index 48837073971..c768e8e7bf6 100644 --- a/src/core/core.element.js +++ b/src/core/core.element.js @@ -35,12 +35,10 @@ function interpolate(start, view, model, ease) { if (type === typeof origin) { if (type === 'string') { c0 = color(origin); - if (c0.valid) { - c1 = color(target); - if (c1.valid) { - view[key] = c1.mix(c0, ease).rgbString(); - continue; - } + c1 = color(target); + if (c0.valid && c1.valid) { + view[key] = c1.mix(c0, ease).rgbString(); + continue; } } else if (type === 'number' && isFinite(origin) && isFinite(target)) { view[key] = origin + (target - origin) * ease; @@ -80,11 +78,9 @@ helpers.extend(Element.prototype, { // No animation -> No Transition if (!model || ease === 1) { - if (setAnimating) { - me.animating = false; - } me._view = model; me._start = null; + delete me.animating; return me; } @@ -96,12 +92,12 @@ helpers.extend(Element.prototype, { start = me._start = {}; } - interpolate(start, view, model, ease); - if (setAnimating && !me.animating) { me.animating = true; } + interpolate(start, view, model, ease); + return me; }, From badc4d954207ddf2b0379fc221993f2b8e51cf07 Mon Sep 17 00:00:00 2001 From: J Copperfield Date: Sun, 3 Dec 2017 19:04:12 +0100 Subject: [PATCH 7/7] Revert transition and complexity changes Add: use 'tooltip._start' as workaround check for tooltip animation status --- src/core/core.controller.js | 6 ++++-- src/core/core.element.js | 17 +++++++---------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/src/core/core.controller.js b/src/core/core.controller.js index 969f3f72c2e..e873e3a977f 100644 --- a/src/core/core.controller.js +++ b/src/core/core.controller.js @@ -555,7 +555,7 @@ module.exports = function(Chart) { } } - me.tooltip.transition(easingValue, true); + me.tooltip.transition(easingValue); }, /** @@ -823,8 +823,10 @@ module.exports = function(Chart) { var changed = me.handleEvent(e); // for smooth tooltip animations issue #4989 // the tooltip should be the source of change + // Animation check workaround: + // tooltip._start will be null when tooltip isn't animating if (tooltip) { - changed = tooltip.animating + changed = tooltip._start ? tooltip.handleEvent(e) : changed | tooltip.handleEvent(e); } diff --git a/src/core/core.element.js b/src/core/core.element.js index c768e8e7bf6..2ca60692b97 100644 --- a/src/core/core.element.js +++ b/src/core/core.element.js @@ -35,10 +35,12 @@ function interpolate(start, view, model, ease) { if (type === typeof origin) { if (type === 'string') { c0 = color(origin); - c1 = color(target); - if (c0.valid && c1.valid) { - view[key] = c1.mix(c0, ease).rgbString(); - continue; + if (c0.valid) { + c1 = color(target); + if (c1.valid) { + view[key] = c1.mix(c0, ease).rgbString(); + continue; + } } } else if (type === 'number' && isFinite(origin) && isFinite(target)) { view[key] = origin + (target - origin) * ease; @@ -70,7 +72,7 @@ helpers.extend(Element.prototype, { return me; }, - transition: function(ease, setAnimating) { + transition: function(ease) { var me = this; var model = me._model; var start = me._start; @@ -80,7 +82,6 @@ helpers.extend(Element.prototype, { if (!model || ease === 1) { me._view = model; me._start = null; - delete me.animating; return me; } @@ -92,10 +93,6 @@ helpers.extend(Element.prototype, { start = me._start = {}; } - if (setAnimating && !me.animating) { - me.animating = true; - } - interpolate(start, view, model, ease); return me;