Attach handlers to ownerDocument, not document #3230
Conversation
Attaching to ownerDocument means that handlers will work if the player is rendered inside an iframe.
|
Seems like a good change. We'll need to make sure this is tested across browsers like IE8 and on mobile. |
gkatsev
added
confirmed
minor
|
It seems So although we should probably test it doesn't break anything, it LGTM |
gkatsev
added
confirmed
and removed
needs: LGTM
|
Verified in IE8, the only browser that could have potentially given us problems with this. |
|
thanks @andyearnshaw! |
|
Just noticed this PR - @gkatsev I'm a little concerned that the changes happen for the sliders but not for, e.g., |
|
@OwenEdwards yeah, those should probably change as well. Didn't realize that more things bind to the document. For normal users of videojs, they shouldn't be any difference since the element's owner document and @andyearnshaw would you be up for going through and fixing the other references? |
|
@gkatsev sure, I'll try and set some time aside this week. |
|
Sounds good, thanks! |
|
Quick update, unfortunately we've had to switch back to JWPlayer because the MailOnline's VAST/VPAID plugin doesn't support non-linear content. It's a real shame because JWPlayer's has similar issues to the ones I've contributed fixes for here. The downside is that I haven't had much time to fix these other references, but it's on my list of things to do. |
* upstream/stable: (77 commits) v5.9.2 @gkatsev grouped text track errors in the console, if we can. closes videojs#3259 v5.9.1 @gkatsev fixed text track tests for older IEs. closes videojs#3269 revert 75116d4 adding chrome to travis (videojs#3254) @forbesjo added back the background color to the poster. closes videojs#3267 @gkatsev fixed removeRemoteTextTracks not working with return value from addRemoteTextTracks. closes videojs#3253 @gkatsev made the first emulated text track enabled by default. closes videojs#3248 @mister-ben blacklisted Chrome for Android for playback rate support. closes videojs#3246 @benjipott updated IS_CHROME to not be true on MS Edge. closes videojs#3232 v5.9.0 @andyearnshaw updated document event handlers to use el.ownerDocument. closes videojs#3230 @chrisauclair added ARIA region and label to player element. closes videojs#3227 @MCGallaspy added vttjs to the self-hosting guide. closes videojs#3229 @forbesjo added chrome for PR tests. closes videojs#3235 @OwenEdwards improved handling of deprecated use of Button component. closes videojs#3236 v5.8.8 @seescode fixed dragging on mute toggle changing the volume. Fixes videojs#3036. Closes videojs#3228 @seescode fixed css failing on IE8 due to incorrect ie8 hack. Fixes videojs#3140. Closes videojs#3226. @vtytar fixed auto-setup failing if taking too long to load. Fixes videojs#2386. Closes videojs#3233. ...
Description
Currently, video.js has problems if the main script is loaded into a window but the player will be rendered into another window (e.g. an iframe). Everywhere an event handler attaches to
documentassumes the document where the script was loaded, rather than where the player resides. This is mostly apparent with the Slider component, that attaches mousemove events on mousedown and removes them on mouseup.Specific Changes proposed
This PR makes some headway towards context independence for the player. It attaches events for the Slider and volume controls to the
ownerDocumentof the component's element instead of the global document object.This allows for a use case we have with React components that may or may not render a video in an iframe, saving us from potentially reloading the script many times as the component updates. A proper test suite for context independence might be required in the future if this is considered to be an important use case.
Requirements Checklist