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.

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

issue-1470: fullscreen hook + inview control #163

Merged
merged 8 commits into from
Jun 29, 2017
Merged

issue-1470: fullscreen hook + inview control #163

merged 8 commits into from
Jun 29, 2017

Conversation

oliverfoster
Copy link
Member

@oliverfoster oliverfoster self-assigned this Mar 22, 2017
@oliverfoster oliverfoster requested a review from moloko March 22, 2017 10:28
Copy link
Contributor

@moloko moloko left a comment

Choose a reason for hiding this comment

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

it's partly helping; the problem is that inview is triggered not only when the media component goes into fullscreen but also when it exits it.

defering the unlock call doesn't work unfortunately...

doing something like

_.delay(function() {
   $.inview.unlock("mediaelement");
}, 500);

Works for my purposes but whether it's good enough a solution to go into Adapt or not is another question... I suppose it's not a totally unreasonable thing to do, seems like unlikely that the user is going to be able to do anything within .5 of second of exiting fullscreen

@moloko
Copy link
Contributor

moloko commented Mar 22, 2017

Even a 250ms delay seems to be working for me

@oliverfoster
Copy link
Member Author

oliverfoster commented Mar 23, 2017

did a straight defer work? ( 0ms )
tom just pointed out you said it didn't

@oliverfoster
Copy link
Member Author

oliverfoster commented Mar 23, 2017

i think half a second is plenty of time to allow the dom + css rendering to settle before unlocking inview. it's a perfectly reasonable thing to do as there is no alternative.

@moloko
Copy link
Contributor

moloko commented Mar 23, 2017

@oliverfoster fair enough. also although using 250ms delay worked fine for me yesterday, it didn't today...! 500ms does seem to be the 'sweet spot'.

'libraries/mediaelement-and-player'
], function(Adapt) {

var rawPrototype = $.extend({}, mejs.MediaElementPlayer.prototype);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this file could use a quick summary of what it's doing somewhere e.g. 'fixes a bug (adaptlearning/adapt_framework#1478) where the media player going into/coming out of full-screen mode would trigger inview on components below it; we therefore need to switch off inview when entering full screen mode and switch it back on again shortly after exiting it'

moloko
moloko previously approved these changes Mar 23, 2017
@moloko
Copy link
Contributor

moloko commented Apr 3, 2017

One problem I've discovered with this approach is it only works when you use the MEP fullscreen control to go in and out of full screen mode.

It doesn't handle Safari-for-iPhone automatically sending the video into fullscreen mode on play, or exiting fullscreen mode using either the 'Done' button or the 'Leave fullscreen mode' icon in the iOS media player on both iPhone and iPad.

Equally in Firefox you can right-click on the video itself (whilst the video playing) and select 'Full screen' from the context menu - it won't detect that either.

My solution for iOS was to do this:

detectFullscreenMode: function() {

	if($('html').hasClass('OS-ios')) {
		this.media.addEventListener('webkitbeginfullscreen', function (){
			$.inview.lock("mediaelement");
		});
		this.media.addEventListener('webkitendfullscreen', function (){
			$.inview.unlock("mediaelement");
		});
	}

	return rawPrototype.detectFullscreenMode.apply(this, arguments);
}

Maybe we should investigate something along these lines for the other browsers?

@oliverfoster
Copy link
Member Author

investigation sounds good. i'll add that code to my pr.
how many of the fullscreen modes mess up the inview stuff?

@moloko
Copy link
Contributor

moloko commented Apr 4, 2017

how many of the fullscreen modes mess up the inview stuff?

All of them though this particular instance was a bit different, I had a full-width video followed immediately by an articleReveal that contained a block containing a text and graphic component. If the video went fullscreen via any mechanism, it would trigger inview on the text component (probably the graphic too but it was set to 'optional').

There's good overview of the various browsers' fullscreen events over at MDN... it looks like most browser support the event but require the event name to be prefixed.

@moloko
Copy link
Contributor

moloko commented Apr 4, 2017

as I'm sure you'll notice my code sample contains no attempt to remove the event listeners; since I was working on a single page course that only contained one component it wasn't necessary for that project...

@moloko
Copy link
Contributor

moloko commented Apr 5, 2017

Just realised that if you do as the browser's full screen mode suggests and 'press Esc to exit' that that won't be detected (and inview will never get unlocked) unless we change over to using browser fullscreen event detection...

moloko added 4 commits May 12, 2017 11:28
… when the user activates/deactivates fullscreen mode using the browser's own fullscreen controls rather than the mediaelementjs control
* This function works out the correct prefix for the current browser.
*/
getVendorPrefix: function() {
var $html = $('html');
Copy link
Member

Choose a reason for hiding this comment

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

Adapt.device.browsercould be used here.

@moloko moloko dismissed their stale review May 12, 2017 23:48

cannot review as I have contributed code

@oliverfoster
Copy link
Member Author

+1 Matt's changes

@moloko moloko merged commit de80875 into master Jun 29, 2017
@guywillis guywillis deleted the issue/1480 branch November 13, 2019 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants