From 9cf98006d54b04d6b36389d0fba25cad1e36aa6c Mon Sep 17 00:00:00 2001 From: Brandon Casey Date: Tue, 31 Oct 2017 15:24:01 -0400 Subject: [PATCH] fix: don't throttle duration change updates (#4635) Right now the durationchange event is throttled with the other two events, timeupdate and loadedmetadata. This means that only one of those events will trigger an update if they all occur within 25ms of each other. This functionality makes sense for timeupdate or loadedmetadata as those should not indicate a time update (even though they often do). For durationchange however it will always indicate a change in the duration, and we want to always update the display when it happens. Here is a scenario of how we could show duration incorrectly right now: User is playing a video that has a postroll ad at the end. After the postroll ad their will be a timeupdate, and then a durationchange to signify that we are back in content. Then ended will fire, and no more events will happen. The player will still show the duration of the ad, as the durationchange was eaten by the timeupdate that happened at nearly the same time. Also, fix a potential issue where if the page was translated using google translate, the time displays stopped updating. --- src/js/component.js | 8 ++------ .../control-bar/time-controls/duration-display.js | 15 ++++++++------- src/js/control-bar/time-controls/time-display.js | 11 ++++++++--- 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/src/js/component.js b/src/js/component.js index 40f523bedc..d379e4eb9b 100644 --- a/src/js/component.js +++ b/src/js/component.js @@ -1242,9 +1242,7 @@ class Component { fn = Fn.bind(this, fn); const timeoutId = window.setTimeout(fn, timeout); - const disposeFn = function() { - this.clearTimeout(timeoutId); - }; + const disposeFn = () => this.clearTimeout(timeoutId); disposeFn.guid = `vjs-timeout-${timeoutId}`; @@ -1305,9 +1303,7 @@ class Component { const intervalId = window.setInterval(fn, interval); - const disposeFn = function() { - this.clearInterval(intervalId); - }; + const disposeFn = () => this.clearInterval(intervalId); disposeFn.guid = `vjs-interval-${intervalId}`; diff --git a/src/js/control-bar/time-controls/duration-display.js b/src/js/control-bar/time-controls/duration-display.js index db62c943b1..2f8384a420 100644 --- a/src/js/control-bar/time-controls/duration-display.js +++ b/src/js/control-bar/time-controls/duration-display.js @@ -23,14 +23,15 @@ class DurationDisplay extends TimeDisplay { constructor(player, options) { super(player, options); - this.on(player, [ - 'durationchange', + // we do not want to/need to throttle duration changes, + // as they should always display the changed duration as + // it has changed + this.on(player, 'durationchange', this.updateContent); - // Also listen for timeupdate (in the parent) and loadedmetadata because removing those - // listeners could have broken dependent applications/libraries. These - // can likely be removed for 7.0. - 'loadedmetadata' - ], this.throttledUpdateContent); + // Also listen for timeupdate (in the parent) and loadedmetadata because removing those + // listeners could have broken dependent applications/libraries. These + // can likely be removed for 7.0. + this.on(player, 'loadedmetadata', this.throttledUpdateContent); } /** diff --git a/src/js/control-bar/time-controls/time-display.js b/src/js/control-bar/time-controls/time-display.js index c90dbc7aea..9a25c318e2 100644 --- a/src/js/control-bar/time-controls/time-display.js +++ b/src/js/control-bar/time-controls/time-display.js @@ -48,7 +48,7 @@ class TimeDisplay extends Component { 'aria-live': 'off' }, Dom.createEl('span', { className: 'vjs-control-text', - textContent: this.localize(this.contentText_) + textContent: this.localize(this.controlText_) })); this.updateTextNode_(); @@ -63,9 +63,14 @@ class TimeDisplay extends Component { * @private */ updateTextNode_() { - if (this.textNode_) { - this.contentEl_.removeChild(this.textNode_); + if (!this.contentEl_) { + return; + } + + while (this.contentEl_.firstChild) { + this.contentEl_.removeChild(this.contentEl_.firstChild); } + this.textNode_ = document.createTextNode(this.formattedTime_ || '0:00'); this.contentEl_.appendChild(this.textNode_); }