Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: don't throttle duration change updates #4635

Merged
merged 8 commits into from Oct 31, 2017
Merged

fix: don't throttle duration change updates #4635

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
af4cf3f
fix: duration change should not be throttled for duration display update
brandonocasey Oct 20, 2017
b19efc5
test
brandonocasey Oct 23, 2017
e8ee563
test2
brandonocasey Oct 30, 2017
8b48161
test3
brandonocasey Oct 30, 2017
3006fe4
revert
brandonocasey Oct 30, 2017
835f720
test4...
brandonocasey Oct 30, 2017
6bf2b7d
add a comment
brandonocasey Oct 30, 2017
580db47
code review
brandonocasey Oct 31, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 2 additions & 6 deletions src/js/component.js
View file
Open in desktop
Expand Up @@ -1239,9 +1239,7 @@ class Component {
fn = Fn.bind(this, fn);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const disposeFn = function() {
this.clearTimeout(timeoutId);
};
const disposeFn = () => this.clearTimeout(timeoutId);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


disposeFn.guid = `vjs-timeout-${timeoutId}`;

Expand Down Expand Up @@ -1302,9 +1300,7 @@ class Component {

const intervalId = window.setInterval(fn, interval);

const disposeFn = function() {
this.clearInterval(intervalId);
};
const disposeFn = () => this.clearInterval(intervalId);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed here as well


disposeFn.guid = `vjs-interval-${intervalId}`;

Expand Down
15 changes: 8 additions & 7 deletions src/js/control-bar/time-controls/duration-display.js
View file
Open in desktop
Expand Up @@ -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);
}

/**
Expand Down
11 changes: 8 additions & 3 deletions src/js/control-bar/time-controls/time-display.js
View file
Open in desktop
Expand Up @@ -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_)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}));

this.updateTextNode_();
Expand All @@ -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_);
}
Expand Down