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

feat: playerresize event in all cases #4864

Merged
merged 32 commits into from
Jan 30, 2018
Merged

feat: playerresize event in all cases #4864

merged 32 commits into from
Jan 30, 2018

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Jan 8, 2018 •
edited
Loading

Use ResizeObserver when available for better and more performant resizing information, otherwise, fall back to a throttled resize event on an iframe that's the size of the player.
Allows a video.js user to disable this by setting resizeManager: false as an option since the component will not be initialized.
This probably needs to revert #4800 (e0ed0b5) because we end up getting two playerresize events with the dimension methods now.

TODO:

  • Revert feat: player resize event #4800
  • fix current tests
  • make RO polyfill possible
  • allow RO to be disabled
  • debounce calls instead of throttle
  • Write tests
  • documentation
  • make sure things are disposed properly
    • events
    • objects

@gkatsev
Copy link
Member Author

gkatsev commented Jan 9, 2018

One thing to consider as well here is that we're going to get multiple playerresize events when changing into fullscreen, depending on what the throttle is, it can be anywhere from 1 to 3 events. Currently, the ResizeObserver isn't throttled but the iframe resize event is throttled to 50ms. It's probable that we want to debounce rather than throttle.

@gkatsev
Revert "feat: playerresize event on Player dimension API calls (#4800)"
e7fa09b 
This reverts commit e0ed0b5.
@gkatsev
make sure that contentWindow is available before remove listener
5902483
@gkatsev
dispose iframe load handler properly
439ac9e
@gkatsev
dispose some players in tests when done
c98a945
@gkatsev
dispose all the things
5219730
@gkatsev
add .off to fake player in poster image tests
a063076
@gkatsev
add removeEventListener to tracks tests
818a574
@gkatsev
nullcheck contentWindow
8b1cee1
@gkatsev
use vjs event helpers
e0400e5
@gkatsev
can't delegate, use actual event helpers
e66acb1
@gkatsev
disable resize manager to IE8
a2494d5
@gkatsev
push to children
4fed939
@gkatsev
allow a ResizeObserver polyfill to be passed in.
69ad100
@gkatsev
test RM, can't test iframe because async
157adcf
@gkatsev
lint
b9c5967
@gkatsev
point at right player
18854a3
@gkatsev
update tests
6b7c278
@gkatsev
add debounce inspired by lodash and underscore
902bfde
@gkatsev
debounce handlers and don't create el if not necessary
4fa8e31
@gkatsev
fixup tests
1ffd0aa
@gkatsev
dispose debounce with player, use fake timers in test
a8f6ebb
@gkatsev
sinon fake timers
fec334b
@gkatsev
linter
a742ad4
@gkatsev gkatsev changed the title [WIP] feat: playerresize event in all cases feat: playerresize event in all cases Jan 10, 2018
ldayananda
ldayananda reviewed Jan 11, 2018
View reviewed changes
src/js/resize-manager.js Outdated
let RESIZE_OBSERVER_AVAILABLE = options.ResizeObserver || window.ResizeObserver;

if (options.ResizeObserver === null) {
RESIZE_OBSERVER_AVAILABLE = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't this set RESIZE_OBSERVER_AVAILABLE to false even when one is available through window.ResizeObserver?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, the idea is if you wanted to disable the ResizeObserver for whatever reason you could pass in null.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha. I'd suggest a comment over this block so it's clear for future readers 😄

src/js/player.js
* @event Player#playerresize
* @type {EventTarget~Event}
*/
this.trigger('playerresize');
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the event trigger would have worked in IE8 as well - since we're reverting this for the ResizeObserver and that's only available after IE8, is this a breaking change?

Copy link
Member Author

Choose a reason for hiding this comment

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

We are falling back to using an iframe for playerresize events when ResizeObserver isn't available, currently, that's basically everywhere, though, chrome will be shipping ResizeObserver soon.

test/unit/resize-manager.test.js Outdated
this.player.on('playerresize', function() {
playerresizeCalled++;
});
rm.resizeObserver_.observer();
Copy link
Contributor

Choose a reason for hiding this comment

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

observe?

Copy link
Contributor

Choose a reason for hiding this comment

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

actually looks like you don't need this line at all

Copy link
Member Author

Choose a reason for hiding this comment

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

I set this in the constructor of MyResizeObserver, calling it calls the observe handler which will trigger playerresize which we can then test. Maybe I should just make it a local variable to be more obvious what is going on.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see. I agree that making it a local variable will make it more clear

test/unit/resize-manager.test.js
assert.equal(playerresizeCalled, 1, 'playerresize was triggered');

rm.dispose();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

one good test to add would be a test that makes sure a resizeObserver is created when one is available on the window

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be nice but mocking out window is a pain :)

ldayananda
ldayananda previously approved these changes Jan 12, 2018
View reviewed changes
src/js/resize-manager.js Outdated
let RESIZE_OBSERVER_AVAILABLE = options.ResizeObserver || window.ResizeObserver;

if (options.ResizeObserver === null) {
RESIZE_OBSERVER_AVAILABLE = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha. I'd suggest a comment over this block so it's clear for future readers 😄

test/unit/resize-manager.test.js Outdated
this.player.on('playerresize', function() {
playerresizeCalled++;
});
rm.resizeObserver_.observer();
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see. I agree that making it a local variable will make it more clear

misteroneill
misteroneill previously approved these changes Jan 16, 2018
View reviewed changes
Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

One minor question that I don't think affects my approval of this PR. 👍

src/js/resize-manager.js
return super.createEl('iframe', {
className: 'vjs-resize-manager'
});
}
Copy link
Member

Choose a reason for hiding this comment

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

I believe the element will be created even when it's not needed. Should we suppress the creation of it when ResizeObserver is available?

Copy link
Member Author

Choose a reason for hiding this comment

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

This line in the constructor will make it so no element is created if ResizeObserver is used: https://github.com/videojs/video.js/pull/4864/files#diff-8947c14c969c2d82a892c5fac0d0efd0R19

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I didn't notice that. Carry on. 👍

@gkatsev gkatsev dismissed stale reviews from misteroneill and ldayananda January 17, 2018 20:13

Updated based on comments and added docs.

ldayananda
ldayananda approved these changes Jan 18, 2018
View reviewed changes
Copy link
Contributor

@ldayananda ldayananda left a comment

Choose a reason for hiding this comment

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

Made a few small typo corrections, but at this point LGTM

@gkatsev gkatsev added confirmed minor This PR can be added to a minor release. It should not be added to a patch release. labels Jan 30, 2018
@pagarwal123
Copy link

QA-LGTM

@gkatsev gkatsev merged commit 9ceb4e4 into master Jan 30, 2018
@gkatsev gkatsev deleted the resizer branch January 30, 2018 18:26
@gkatsev gkatsev mentioned this pull request Jul 23, 2018
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@misteroneill misteroneill misteroneill left review comments

@ldayananda ldayananda ldayananda approved these changes

Assignees
No one assigned
Labels
confirmed minor This PR can be added to a minor release. It should not be added to a patch release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gkatsev @pagarwal123 @misteroneill @ldayananda