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

      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.

        Gliffy Diagrams

          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: