fix: enable browser fullscreen mode for the scorm x-block #23
Conversation
…orrect rendering in the MFE application
|
@regisb Could you take a look at this PR, please? |
|
Yes, this is on my todo-list. |
| } else { | ||
| // Find scorm-xblock content | ||
| const element = $('.scorm-xblock').get(0); | ||
| if (element.requestFullscreen) { |
There was a problem hiding this comment.
I was not aware that there existed a native fullscreen API: https://developer.mozilla.org/en-US/docs/Web/API/Element/requestFullscreen
Could we ditch the ad-hoc fullscreen implementation that was until now part of the Scorm xblock implementation? Ideally, I'd like to keep only the new, native implementation.
There was a problem hiding this comment.
Unfortunately, fullscreen can be triggered only by the user. So we need to keep an old methods too, also it contains useful CSS styles and classes, we keep using to. 'Display in fullscreen mode on launch' feature are not working correctly in the MFEs, I added a note about it.
There was a problem hiding this comment.
See my review comment: #23 (review)
| @@ -12,11 +60,13 @@ function ScormXBlock(runtime, element, settings) { | |||
| function enterFullscreen() { | |||
| $(element).find(".scorm-xblock").addClass("fullscreen-enabled"); | |||
| triggerResize(); | |||
| toggleScormFullScreen(); | |||
There was a problem hiding this comment.
I'm a little bothered by the fact that the fullscreen logic is now split between the enterFullscreen/exitFullscreen/toggleScormFullScreen functions.
Also: shouldn't the triggerResize() function be called after fullscreen toggle?
There was a problem hiding this comment.
Place triggerResize() function after fullscreen toggle, thx.
I'm a little bothered by the fact that the fullscreen logic is now split between the enterFullscreen/exitFullscreen/toggleScormFullScreen functions.
Answered above about this, the main problem, that it's prohibited to call native fullscreen by default only user allow to trigger it. And for now, I don't how any other proper solution to fix all this issue when we use scorm xblock in the MFE, sorry
There was a problem hiding this comment.
Thanks again for your PR @bydawen, and sorry about the review delay.
This PR was made to make it possible to display scorm XBlocks in fullscreen when the learning MFE is enabled. I verified that it works as expected in Nutmeg.
The "native browser fullscreen" implementation makes the previous "fullscreen document" implementation kinda obsolete. However, we would still need to keep it to preserve the "fullscreen by default" feature. The problem is that this "fullscreen by default" feature will not work in the learning MFE -- because "fullscreen document" doesn't either. Since Nutmeg, we expect the learning MFE to be used by all platforms. The previous learning courseware is considered legacy. This means that "fullscreen by default" just does not work for most people. As a consequence I propose the following:
- Deprecate the "fullscreen by default" feature.
- Get rid of the "fullscreen document" implementation.
@bydawen I understand that the proposed changes may be outside of the scope of your PR. If you'd like to implement them in this PR, that's great! If not, just tell me and I'll do the changes myself.
Hello @regisb i'm sorry but may i ask you to do it? For now i'm not be able to do your proposal. I hope for your understanding. |
Sure thing, no trouble at all. |
Fix to prevent incorrect rendering fullscreen mode when scorm x-block in MFE application.