fix: don't throttle duration change updates #4635

brandonocasey · 2017-09-28T21:20:13Z

Description

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.

Requirements Checklist

Feature implemented / Bug fixed
If necessary, more likely in a feature request than a bug fix
- Change has been verified in an actual browser (Chome, Firefox, IE)
- Example created (starter template on JSBin)

gkatsev

LGTM, though, there's a test failing.

brandonocasey · 2017-10-20T21:18:27Z

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 was a typo, I could arguable pull this into its own pr if we want, but this is one of the causes of the broken unit tests.

This is what's causing the issue on ie8? https://travis-ci.org/videojs/video.js/jobs/290639178#L1937-L1941

im actually not sure what is causing the failure on IE8, this must have just fixed IE9

brandonocasey · 2017-10-20T21:19:25Z

src/js/control-bar/time-controls/time-display.js

@@ -63,7 +63,7 @@ class TimeDisplay extends Component {
   * @private
   */
  updateTextNode_() {
-    if (this.textNode_) {
+    if (this.textNode_ && this.contentEl_ && this.contentEl_.contains(this.textNode_)) {
      this.contentEl_.removeChild(this.textNode_);


IE11 throws in request animation frames when trying to remove a DOM node from a container when the container does not have the node. Might only be an issue for our unit tests, but I fixed it here.

brandonocasey · 2017-10-30T15:43:27Z

src/js/component.js

-    const disposeFn = function() {
-      this.clearTimeout(timeoutId);
-    };
+    const disposeFn = () => this.clearTimeout(timeoutId);


This seemed a bit weird to me in a regular function we wouldn't have access to this.clearTimeout

we call the dispose handler with context as the component instance, so, it should have it.

at least, it should be. this is ok too.

brandonocasey · 2017-10-30T16:54:57Z

src/js/component.js

-    const disposeFn = function() {
-      this.clearInterval(intervalId);
-    };
+    const disposeFn = () => this.clearInterval(intervalId);


fixed here as well

gkatsev · 2017-10-31T18:16:50Z

src/js/component.js

-    const disposeFn = function() {
-      this.clearTimeout(timeoutId);
-    };
+    const disposeFn = () => this.clearTimeout(timeoutId);


we call the dispose handler with context as the component instance, so, it should have it.

gkatsev · 2017-10-31T18:18:38Z

src/js/component.js

-    const disposeFn = function() {
-      this.clearTimeout(timeoutId);
-    };
+    const disposeFn = () => this.clearTimeout(timeoutId);


at least, it should be. this is ok too.

gkatsev · 2017-10-31T18:19:16Z

src/js/control-bar/time-controls/time-display.js

+      return;
+    }
+
+    // ie8/9 don't support `el.contains` inside of setTimeout call.


gkatsev · 2017-10-31T18:22:34Z

src/js/control-bar/time-controls/time-display.js

+    }
+
+    // ie8/9 don't support `el.contains` inside of setTimeout call.
+    if (this.textNode_ && this.textNode_.parentNode === this.contentEl_) {


what about just removing everything inside of contentEl_? It'll also fix an issue where if the page is translated in chrome using google translate, the text node will be wrapped in another element and this method will fail.
This could also just happen in a separate PR.

gkatsev · 2017-10-31T18:23:10Z

src/js/component.js

@@ -1239,9 +1239,7 @@ class Component {
    fn = Fn.bind(this, fn);

    const timeoutId = window.setTimeout(fn, timeout);


I almost commented saying that this should be changed to this.setTimeout before realizing what this line is for 😆

brandonocasey closed this Oct 2, 2017

brandonocasey force-pushed the fix/duration-change branch from 802bee0 to f51d36b Compare October 2, 2017 15:23

brandonocasey reopened this Oct 2, 2017

brandonocasey force-pushed the fix/duration-change branch from 33f5a21 to a49eb16 Compare October 5, 2017 17:44

gkatsev reviewed Oct 18, 2017

View reviewed changes

brandonocasey force-pushed the fix/duration-change branch 2 times, most recently from b31b49d to 44ab963 Compare October 20, 2017 21:17

brandonocasey commented Oct 20, 2017

View reviewed changes

brandonocasey added the needs: LGTM Needs an additional approval label Oct 20, 2017

fix: duration change should not be throttled for duration display update

af4cf3f

brandonocasey force-pushed the fix/duration-change branch from 44ab963 to af4cf3f Compare October 23, 2017 19:25

test

b19efc5

brandonocasey force-pushed the fix/duration-change branch from 0d7f7f7 to b19efc5 Compare October 23, 2017 20:28

test2

e8ee563

brandonocasey commented Oct 30, 2017

View reviewed changes

brandonocasey added 3 commits October 30, 2017 11:51

test3

8b48161

revert

3006fe4

test4...

835f720

brandonocasey force-pushed the fix/duration-change branch from cc27517 to 835f720 Compare October 30, 2017 16:44

brandonocasey commented Oct 30, 2017

View reviewed changes

add a comment

6bf2b7d

gkatsev approved these changes Oct 31, 2017

View reviewed changes

gkatsev added confirmed and removed needs: LGTM Needs an additional approval labels Oct 31, 2017

code review

580db47

gkatsev approved these changes Oct 31, 2017

View reviewed changes

gkatsev merged commit 9cf9800 into master Oct 31, 2017

gkatsev deleted the fix/duration-change branch October 31, 2017 19:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: don't throttle duration change updates #4635

fix: don't throttle duration change updates #4635

brandonocasey commented Sep 28, 2017 •

edited

gkatsev left a comment

brandonocasey Oct 20, 2017

gkatsev Oct 23, 2017

brandonocasey Oct 23, 2017 •

edited

brandonocasey Oct 20, 2017

brandonocasey Oct 30, 2017

gkatsev Oct 31, 2017

gkatsev Oct 31, 2017

brandonocasey Oct 30, 2017

gkatsev Oct 31, 2017

gkatsev Oct 31, 2017

gkatsev Oct 31, 2017

gkatsev Oct 31, 2017

gkatsev Oct 31, 2017

		@@ -1239,9 +1239,7 @@ class Component {
		fn = Fn.bind(this, fn);

		const timeoutId = window.setTimeout(fn, timeout);

fix: don't throttle duration change updates #4635

fix: don't throttle duration change updates #4635

Conversation

brandonocasey commented Sep 28, 2017 • edited

Description

Requirements Checklist

gkatsev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonocasey Oct 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonocasey commented Sep 28, 2017 •

edited

brandonocasey Oct 23, 2017 •

edited