Moodle
  1. Moodle
  2. MDL-29975

SCORM - improve pop-up generation

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.3
    • Component/s: SCORM
    • Labels:
    • Testing Instructions:
      Hide

      Create a new SCORM - on the SCORM editing page select to open in new window.

      Enter SCORM - check that pop-up is generated on submit of the form on view.php and that "view.php" stays in "parent" window. - not the ugly useless "empty" player.php page which displays a message about pop-ups.

      This needs testing in multiple browsers.

      Show
      Create a new SCORM - on the SCORM editing page select to open in new window. Enter SCORM - check that pop-up is generated on submit of the form on view.php and that "view.php" stays in "parent" window. - not the ugly useless "empty" player.php page which displays a message about pop-ups. This needs testing in multiple browsers.
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      master_MDL-29975
    • Rank:
      19519

      Description

      Now that the SCORM API loads in the pop-up window itself and not in the "parent" window - we can improve the way the pop-up is generated and ignore the "entry" page - this patch generates the pop-up using js when the user hits the "enter" button on the view page - keeping the original view page in the "parent" window.

        Activity

        Hide
        Dave Newgass added a comment -

        I will have to keep an eye on this one....

        Once solved, I will contact Remote-Learner UK and request an update.

        Cheers,
        Dave

        Show
        Dave Newgass added a comment - I will have to keep an eye on this one.... Once solved, I will contact Remote-Learner UK and request an update. Cheers, Dave
        Hide
        Dan Marsden added a comment -

        NOTE TO INTEGRATOR
        Master and 21_STABLE only - not 20_STABLE or lower.

        Show
        Dan Marsden added a comment - NOTE TO INTEGRATOR Master and 21_STABLE only - not 20_STABLE or lower.
        Hide
        Sam Hemelryk added a comment -

        Hi Dan,

        I've been mulling over these changes for a little while now. There are three things that I think need to be looked at, or discussed.

        1/ Really simple. I think the pop string needs review (I'm no good with English so I'll ask Helen, or MdR).
        Currently the combined string is:

        This SCORM package should open in a new window, if a new window does not appear, click here to launch the activity

        I'm pretty sure that the second comma should not be there.

        2/ This link is displayed regardless of whether JS is enabled or disabled. I think there should be CSS to hide this new notification until JS has been enabled. Use the jsenabled class that gets added to the body or something like.

        3/ This change adds a link with a target attribute causing XHTML validation to fail. We've worked in the past to reduce the number of target attributes used, in many cases the only remaining uses are where we know there isn't JS, otherwise we use a JS solution to add the target attribute on click.
        Had a quick grep for target and you'll see what I mean.

        Sending back at the mo so these can be looked at, yell out if there are any questions.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Dan, I've been mulling over these changes for a little while now. There are three things that I think need to be looked at, or discussed. 1/ Really simple. I think the pop string needs review (I'm no good with English so I'll ask Helen, or MdR). Currently the combined string is: This SCORM package should open in a new window, if a new window does not appear, click here to launch the activity I'm pretty sure that the second comma should not be there. 2/ This link is displayed regardless of whether JS is enabled or disabled. I think there should be CSS to hide this new notification until JS has been enabled. Use the jsenabled class that gets added to the body or something like. 3/ This change adds a link with a target attribute causing XHTML validation to fail. We've worked in the past to reduce the number of target attributes used, in many cases the only remaining uses are where we know there isn't JS, otherwise we use a JS solution to add the target attribute on click. Had a quick grep for target and you'll see what I mean. Sending back at the mo so these can be looked at, yell out if there are any questions. Cheers Sam
        Hide
        Dan Marsden added a comment -

        thanks Sam - I've been thinking about this a bit and I'm thinking that it makes more sense to dump the existing way this is done completely - it opens extra stupid windows....

        The pop-up shouldn't be triggered on player.php - it should be triggered on the onclick event on the button on the view.php page - as we're using the onclick from memory this gets around the pop-up blocker issue too.

        After the pop-up/new window has opened we should either redirect the "original" window back to the course homepage or handle an event when the "new window" has closed and refresh the "original" page to update the grade/user information returned.

        Show
        Dan Marsden added a comment - thanks Sam - I've been thinking about this a bit and I'm thinking that it makes more sense to dump the existing way this is done completely - it opens extra stupid windows.... The pop-up shouldn't be triggered on player.php - it should be triggered on the onclick event on the button on the view.php page - as we're using the onclick from memory this gets around the pop-up blocker issue too. After the pop-up/new window has opened we should either redirect the "original" window back to the course homepage or handle an event when the "new window" has closed and refresh the "original" page to update the grade/user information returned.
        Hide
        Dan Marsden added a comment -

        One of our devs - Matt Clarkson was looking at this and found we could do this pretty easily by using the onsubmit event like this:

        <form id="theform" method="post" action="<?php echo $CFG->wwwroot ?>/mod/scorm/player.php" onsubmit="window.open('','Popup','toolbar=no,location=no,status=no,menubar=no,scrollb     ars=yes,resizable=no,width='+screen.availWidth+',height='+screen.availHeight+',left=0,top=0'); this.target='Popup';">
        

        onsubmit creates a new window then changes the forms target to the new window - looks like a very easy thing to tidy up and apply - kudos to Matt!

        Show
        Dan Marsden added a comment - One of our devs - Matt Clarkson was looking at this and found we could do this pretty easily by using the onsubmit event like this: <form id= "theform" method= "post" action= "<?php echo $CFG->wwwroot ?>/mod/scorm/player.php" onsubmit= "window.open('','Popup','toolbar=no,location=no,status=no,menubar=no,scrollb ars=yes,resizable=no,width='+screen.availWidth+',height='+screen.availHeight+',left=0,top=0'); this .target='Popup';" > onsubmit creates a new window then changes the forms target to the new window - looks like a very easy thing to tidy up and apply - kudos to Matt!
        Hide
        Sam Hemelryk added a comment -

        Hi Dan,
        The onsubmit solution is a good one, hehe although we've also tried to eliminate onxxx calls within the code instead attaching the event using JS during page load. Would be 100% perfect if that could be worked out.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Dan, The onsubmit solution is a good one, hehe although we've also tried to eliminate onxxx calls within the code instead attaching the event using JS during page load. Would be 100% perfect if that could be worked out. Cheers Sam
        Hide
        Dan Marsden added a comment -

        Thanks Sam - I was thinking along those lines already

        Show
        Dan Marsden added a comment - Thanks Sam - I was thinking along those lines already
        Hide
        Ankit Agarwal added a comment -

        Hi Dan,
        Patch looks great.
        I had only one question...are we not allowed to access the pacakge if JS is disabled?
        Currently I get the msg "JavaScript is required to view this object, please enable JavaScript in your browser and try again.", if JS is disabled and the there is no option to enter the scorm.
        Is that how it is supposed to behave?

        Thanks

        Show
        Ankit Agarwal added a comment - Hi Dan, Patch looks great. I had only one question...are we not allowed to access the pacakge if JS is disabled? Currently I get the msg "JavaScript is required to view this object, please enable JavaScript in your browser and try again.", if JS is disabled and the there is no option to enter the scorm. Is that how it is supposed to behave? Thanks
        Hide
        Dan Marsden added a comment -

        thanks Ankit, yes that's correct - there's an admin setting in SCORM that controls it although I was tempted to remove the setting completely and make it always require JS - I might do that in Moodle 2.4

        SCORM just doesn't work without JS - and it can cause mass confusion if a student enters it with js disabled and expects their grade to be saved.

        Show
        Dan Marsden added a comment - thanks Ankit, yes that's correct - there's an admin setting in SCORM that controls it although I was tempted to remove the setting completely and make it always require JS - I might do that in Moodle 2.4 SCORM just doesn't work without JS - and it can cause mass confusion if a student enters it with js disabled and expects their grade to be saved.
        Hide
        Dan Poltawski added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Dan Marsden added a comment -

        rebased

        Show
        Dan Marsden added a comment - rebased
        Hide
        Dan Poltawski added a comment -

        Thanks Dan, i've integrated this now

        Show
        Dan Poltawski added a comment - Thanks Dan, i've integrated this now
        Hide
        Adrian Greeve added a comment -

        Tested on Chrome, Safari, IE 8 and Firefox. I had a couple of issue with strict error reporting enabled, but when I turned that off it worked fine.
        Thanks.

        Show
        Adrian Greeve added a comment - Tested on Chrome, Safari, IE 8 and Firefox. I had a couple of issue with strict error reporting enabled, but when I turned that off it worked fine. Thanks.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        This has been near becoming rejected, because it's not the best code you are able to produce.

        But, luckily, at the end, it has landed and has been spread to all repos out there.

        Many thanks and, don't forget it, keep improving your skills, you can!

        Closing, ciao

        Show
        Eloy Lafuente (stronk7) added a comment - This has been near becoming rejected, because it's not the best code you are able to produce. But, luckily, at the end, it has landed and has been spread to all repos out there. Many thanks and, don't forget it, keep improving your skills, you can! Closing, ciao

          People

          • Votes:
            3 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: