Moodle
  1. Moodle
  2. MDL-31381

Problem SCORM (new window) frame size and added padding version 2.2.1 (Build: 20120109)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.2.1
    • Fix Version/s: 2.2.2
    • Component/s: SCORM
    • Labels:
    • Testing Instructions:
      Hide

      install the demo scorm package attached (using either upload or url download funtion) in a running 2.2.1 build as a scorm activity, use the "new window" setting with size as 970, 770.
      check to make sure there isn't large amounts of white space around the package.

      • please check/test in multiple browser types and versions if possible.
      Show
      install the demo scorm package attached (using either upload or url download funtion) in a running 2.2.1 build as a scorm activity, use the "new window" setting with size as 970, 770. check to make sure there isn't large amounts of white space around the package. please check/test in multiple browser types and versions if possible.
    • Workaround:
      Hide

      we have not found one as of yet

      Show
      we have not found one as of yet
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:
      master_MDL-31381
    • Rank:
      37898

      Description

      Our scorm packages, that currently work as expected in version 2.0.3, are no longer being displayed correctly in this version. In 2.0.3 we can simply select open scorm in new window, set the dimensions (970, 770), and the window opens to that size and frames our scorm package skin perfectly. In this version we can't find a way to displaye the window as previously described. Our window in 2.2.1 is being displayed with added deadspace (and what seems to be a frame) at the bottom (causing the navigation section to be hidden) and a vertical scrollbar to appear. our issue is very similar to the issues described in this thread: http://moodle.org/mod/forum/discuss.php?d=185138#p846477

      You will find a screenshot attached as well as a demonstration scorm package for testing purposes.

        Issue Links

          Activity

          Hide
          Keith Murray added a comment -

          notice the vertical scroll bar and what seems to be a bottom frame with dead space

          Show
          Keith Murray added a comment - notice the vertical scroll bar and what seems to be a bottom frame with dead space
          Hide
          Dan Marsden added a comment -

          thanks Keith - great bug report with specific information - I've increased priority of this and put it into my stable backlog.

          Show
          Dan Marsden added a comment - thanks Keith - great bug report with specific information - I've increased priority of this and put it into my stable backlog.
          Hide
          Dan Marsden added a comment -

          Hi Keith,

          I decided to take a quick look at this and have knocked together some improvements - that SCORM package helped a lot thanks.

          The above patch makes 3 changes
          Decreases the max width of the TOC (if configured to show) so that the content can't be lost (small change) - I noticed that the TOC could be stretched over the top of the content and with the improvements to the width/height it was almost lost a little bit.
          Removed the 10px padding around the standard region-content when the "new window" option is being used.
          Improved the height calculation - the space near the bottom of the page was trying to copensate for a span that used to sit above the player which I removed recently - so by removing the space at the top, it just shifted the content up and I forgot about adjusting the footer size.

          I haven't pushed this through for integration yet as I have only tested this on a single browser (Ubuntu/FF 9) - when I get a bit more time I'll test this on a larger range of browsers and make sure I'm satisfied with the change before sending it up for integration.

          It would be good if you could apply this patch and see if it meets your needs - let me know how you go and which browsers you manage to test it in.

          thanks,

          Show
          Dan Marsden added a comment - Hi Keith, I decided to take a quick look at this and have knocked together some improvements - that SCORM package helped a lot thanks. The above patch makes 3 changes Decreases the max width of the TOC (if configured to show) so that the content can't be lost (small change) - I noticed that the TOC could be stretched over the top of the content and with the improvements to the width/height it was almost lost a little bit. Removed the 10px padding around the standard region-content when the "new window" option is being used. Improved the height calculation - the space near the bottom of the page was trying to copensate for a span that used to sit above the player which I removed recently - so by removing the space at the top, it just shifted the content up and I forgot about adjusting the footer size. I haven't pushed this through for integration yet as I have only tested this on a single browser (Ubuntu/FF 9) - when I get a bit more time I'll test this on a larger range of browsers and make sure I'm satisfied with the change before sending it up for integration. It would be good if you could apply this patch and see if it meets your needs - let me know how you go and which browsers you manage to test it in. thanks,
          Hide
          Mary Evans added a comment -

          Hi Dan,

          I'm helping Kieth with the CSS for this same problem and it was driving me crazy!
          Thanks for the patch I'll be checking it out later today.

          Cheers
          Mary

          Show
          Mary Evans added a comment - Hi Dan, I'm helping Kieth with the CSS for this same problem and it was driving me crazy! Thanks for the patch I'll be checking it out later today. Cheers Mary
          Hide
          Mary Evans added a comment - - edited

          Hi Dan,

          I just pulled this into my github to test it, and was a little concerned you added the CSS for this patch, which will be great for CORE themes, but not for those themes, like Kieth's, that use a different naming system for their pagelayout.

          However since pop-ups use layout/embedded.php which only has #page #content the CSS you have using #page-content .region-content will not work in CORE themes that use embedded.php.

          Either way if this were to be integrated into Moodle you would be better adding that CSS to the end of theme/base/style/core.css.

          I'm just about to test the "patch" now.

          Thanks

          Show
          Mary Evans added a comment - - edited Hi Dan, I just pulled this into my github to test it, and was a little concerned you added the CSS for this patch, which will be great for CORE themes, but not for those themes, like Kieth's, that use a different naming system for their pagelayout. However since pop-ups use layout/embedded.php which only has #page #content the CSS you have using #page-content .region-content will not work in CORE themes that use embedded.php. Either way if this were to be integrated into Moodle you would be better adding that CSS to the end of theme/base/style/core.css. I'm just about to test the "patch" now. Thanks
          Hide
          Dan Marsden added a comment -

          Hi Mary - I'd be surprised if the integrators let me squeeze in module specific code there! - can't see any module specific code in that file. If I get a +1 from an integrator I'm happy to put it in theme/base otherwise it will have to stay in mod/scorm/styles.css

          also if it did go into theme/base - wouldn't it be better to go into theme/base/style/pagelayout.css ? - as it's overriding css defined there?

          Show
          Dan Marsden added a comment - Hi Mary - I'd be surprised if the integrators let me squeeze in module specific code there! - can't see any module specific code in that file. If I get a +1 from an integrator I'm happy to put it in theme/base otherwise it will have to stay in mod/scorm/styles.css also if it did go into theme/base - wouldn't it be better to go into theme/base/style/pagelayout.css ? - as it's overriding css defined there?
          Hide
          Dan Marsden added a comment -

          Just had a quick chat with Eloy about theme base stuff - neither of us are specialists on the current theme stuff but it seems strange to put module specific stuff in base - Eloy did see this:
          a.autolink.glossary:hover

          {cursor: help;}

          but that must be a mistake or put there for some exceptional reason!

          IMO - modules own css should be pretty minimal - provide structural improvements but if they aren't generic enough that's a bug with the specific module - and if custom themes want to exclude the module css then they will need to be responsible for structuring appropriate css to support the modules.

          I think once we've tested this and I'm happy with it's performance on multiple browsers I'll push it into module.css - Mary if you want stuff to go into base theme you're welcome to discuss with HQ and create a new tracker issue to move css into base theme.

          thanks!

          btw - does this improve the display/fix the issue described?

          thanks!

          Show
          Dan Marsden added a comment - Just had a quick chat with Eloy about theme base stuff - neither of us are specialists on the current theme stuff but it seems strange to put module specific stuff in base - Eloy did see this: a.autolink.glossary:hover {cursor: help;} but that must be a mistake or put there for some exceptional reason! IMO - modules own css should be pretty minimal - provide structural improvements but if they aren't generic enough that's a bug with the specific module - and if custom themes want to exclude the module css then they will need to be responsible for structuring appropriate css to support the modules. I think once we've tested this and I'm happy with it's performance on multiple browsers I'll push it into module.css - Mary if you want stuff to go into base theme you're welcome to discuss with HQ and create a new tracker issue to move css into base theme. thanks! btw - does this improve the display/fix the issue described? thanks!
          Hide
          Mary Evans added a comment - - edited

          I was actually trying to draw your attention to the fact the pop-up uses a layout file called "embedded.php" which has minimal body content, just "page" and "content", both of which are ID selectors. The CSS you are proposing is irrelevant for this fix as #page-content .region-content do not exist in embedded.php! You would be better adding the following...

          #page-mod-scorm-player.pagelayout-popup #content {padding: 0; margin: 0;}
          

          With the above CSS the pop-up is better and has no padding in the page, but it has a 200px gap to the right of the SCORM content. I'll add a screen shot.

          Show
          Mary Evans added a comment - - edited I was actually trying to draw your attention to the fact the pop-up uses a layout file called "embedded.php" which has minimal body content, just "page" and "content", both of which are ID selectors. The CSS you are proposing is irrelevant for this fix as #page-content .region-content do not exist in embedded.php! You would be better adding the following... #page-mod-scorm-player.pagelayout-popup #content {padding: 0; margin: 0;} With the above CSS the pop-up is better and has no padding in the page, but it has a 200px gap to the right of the SCORM content. I'll add a screen shot.
          Hide
          Dan Marsden added a comment -

          hmm - I was testing that in the "standard" theme and it did exist! - didn't have a 200px gap on the right either.

          Which browser are you using? - are you including the mod/scorm/styles.css ?

          Show
          Dan Marsden added a comment - hmm - I was testing that in the "standard" theme and it did exist! - didn't have a 200px gap on the right either. Which browser are you using? - are you including the mod/scorm/styles.css ?
          Hide
          Dan Marsden added a comment -

          that css I added was to counteract this bit of css from base/style/pagelayout.css:
          #page-content .region-content

          {overflow:hidden;padding:10px;}

          maybe standard theme is handling pop-ups quite differently from the other themes? - I might need to look further there.

          Show
          Dan Marsden added a comment - that css I added was to counteract this bit of css from base/style/pagelayout.css: #page-content .region-content {overflow:hidden;padding:10px;} maybe standard theme is handling pop-ups quite differently from the other themes? - I might need to look further there.
          Hide
          Keith Murray added a comment -

          Dan,

          Would it help if I sent you the testing server we're running this on so you could see it live? I could get you whatever access level you would require.

          Sorry I didn't chime in earlier today; been out of office all day.

          thanks to both of you for all the hard work and I'll try the patch and update accordingly.

          Keith

          Show
          Keith Murray added a comment - Dan, Would it help if I sent you the testing server we're running this on so you could see it live? I could get you whatever access level you would require. Sorry I didn't chime in earlier today; been out of office all day. thanks to both of you for all the hard work and I'll try the patch and update accordingly. Keith
          Hide
          Mary Evans added a comment -

          After purging all caches...a million times! Cleared Moodledata cache manually.

          Re set SCORM Admin settings to 100 x 100

          Set SCORM Activity setting for pop-up to 1000 x 800 (with re-sizable window enabled)

          gave me a perfect pop-up window and no need to resize see screenshot.

          Brilliant! Thanks

          Mary

          Show
          Mary Evans added a comment - After purging all caches...a million times! Cleared Moodledata cache manually. Re set SCORM Admin settings to 100 x 100 Set SCORM Activity setting for pop-up to 1000 x 800 (with re-sizable window enabled) gave me a perfect pop-up window and no need to resize see screenshot. Brilliant! Thanks Mary
          Hide
          Mary Evans added a comment -

          Here's a copy of the embedded.php for reference.

          Show
          Mary Evans added a comment - Here's a copy of the embedded.php for reference.
          Hide
          Mary Evans added a comment -

          @Dan

          Sorry I did not see your comments.

          My mistake...with the embedded.php...Standard does use general.php and the CSS you added is correct in that respect..

          However in this instance the embedded.php is set in Kieth's themes and works OK with the CSS I added.

          So providing a theme uses a layout file (whatever it's name) with the correct Moodle pagelayout in it (which Kieth's theme doesn't for some reason that's why I pointed it to use embedded.php in base theme) the CSS is good.

          The main thing in all of this the Patch works!

          Had the custom theme used conventional Moodle layout then perhaps this would not have been so complicated!

          So sorry about that.

          Show
          Mary Evans added a comment - @Dan Sorry I did not see your comments. My mistake...with the embedded.php...Standard does use general.php and the CSS you added is correct in that respect.. However in this instance the embedded.php is set in Kieth's themes and works OK with the CSS I added. So providing a theme uses a layout file (whatever it's name) with the correct Moodle pagelayout in it (which Kieth's theme doesn't for some reason that's why I pointed it to use embedded.php in base theme) the CSS is good. The main thing in all of this the Patch works! Had the custom theme used conventional Moodle layout then perhaps this would not have been so complicated! So sorry about that.
          Hide
          Dan Marsden added a comment -

          great! - thanks Mary, can you let me know which browsers you have tested this with? - Screenshot looks like windows/FF

          also just to confirm - adding this line to mod/scorm/styles.css is what you did to improve support for pop-ups - you didn't add this in base or somewhere else?

          #page-mod-scorm-player.pagelayout-popup #content {padding: 0; margin: 0;}
          
          Show
          Dan Marsden added a comment - great! - thanks Mary, can you let me know which browsers you have tested this with? - Screenshot looks like windows/FF also just to confirm - adding this line to mod/scorm/styles.css is what you did to improve support for pop-ups - you didn't add this in base or somewhere else? #page-mod-scorm-player.pagelayout-popup #content {padding: 0; margin: 0;}
          Hide
          Mary Evans added a comment -

          Hi,
          I'll add you original PATCH and test this in a Moodle CORE theme Afterburner in IE9, FF, CHROME, OPERA
          and get back to you later today.

          Cheers
          Mary

          Show
          Mary Evans added a comment - Hi, I'll add you original PATCH and test this in a Moodle CORE theme Afterburner in IE9, FF, CHROME, OPERA and get back to you later today. Cheers Mary
          Hide
          Mary Evans added a comment -

          @Dan
          Tested OK using Afterburner in with
          Admin Scorm settings 100 x 100
          Course Activity settings for Pop-up 1000px x 800px

          IE9 OK
          IE7 (set in IE9 to IE7 using F12(Function Key) OK
          FireFox OK
          Chrome OK

          Not perfect but works...
          Opera (this had approx 150px gap right & bottom)

          Hope this helps?

          Mary

          Show
          Mary Evans added a comment - @Dan Tested OK using Afterburner in with Admin Scorm settings 100 x 100 Course Activity settings for Pop-up 1000px x 800px IE9 OK IE7 (set in IE9 to IE7 using F12(Function Key) OK FireFox OK Chrome OK Not perfect but works... Opera (this had approx 150px gap right & bottom) Hope this helps? Mary
          Hide
          Mary Evans added a comment -

          @Dan FYI

          For what it's worth it actually works better in an embedded.php page! I just changed Afterburner/config.php under $THEME->layouts

              // Pages that appear in pop-up windows - no navigation, no blocks, no header.
              'popup' => array(
                  'file' => 'embedded.php',
                  'regions' => array(),
                  'options' => array('nofooter'=>true, 'nonavbar'=>true, 'nocustommenu'=>true, 'nologininfo'=>true),
              ),
          
          Show
          Mary Evans added a comment - @Dan FYI For what it's worth it actually works better in an embedded.php page! I just changed Afterburner/config.php under $THEME->layouts // Pages that appear in pop-up windows - no navigation, no blocks, no header. 'popup' => array( 'file' => 'embedded.php', 'regions' => array(), 'options' => array('nofooter'=> true , 'nonavbar'=> true , 'nocustommenu'=> true , 'nologininfo'=> true ), ),
          Hide
          Dan Marsden added a comment -

          great - thanks Mary,

          and just to confirm it still needs that change you mentioned above to page-mod-scorm-player.pagelayout-popup #content

          thanks

          Show
          Dan Marsden added a comment - great - thanks Mary, and just to confirm it still needs that change you mentioned above to page-mod-scorm-player.pagelayout-popup #content thanks
          Hide
          Mary Evans added a comment - - edited

          Hi Dan,

          NO!!! because the fix you are applying here, which will be for ALL core themes, is SPOT ON.

          However, should the day come when the Moodle developers decide that pop-ups need the embedded.php instead of general.php for ALL themes, then the CSS I suggested would be needed.

          However, it's just that in Kieth's theme, because general.php was not conventional Moodle pagelayout, I switched the pop-up to use base themes embedded.php, that's why I needed to change the CSS.

          Anyway...this is good to go so if you hurry you might get it integrated today!!!

          Cheers.

          Show
          Mary Evans added a comment - - edited Hi Dan, NO!!! because the fix you are applying here, which will be for ALL core themes, is SPOT ON. However, should the day come when the Moodle developers decide that pop-ups need the embedded.php instead of general.php for ALL themes, then the CSS I suggested would be needed. However, it's just that in Kieth's theme, because general.php was not conventional Moodle pagelayout, I switched the pop-up to use base themes embedded.php, that's why I needed to change the CSS. Anyway...this is good to go so if you hurry you might get it integrated today!!! Cheers.
          Hide
          Dan Marsden added a comment -

          NOTE TO INTEGRATOR: this is for 22Stable and master only - not 21Stable. - thanks to Mary/Keith for help with testing.

          Show
          Dan Marsden added a comment - NOTE TO INTEGRATOR: this is for 22Stable and master only - not 21Stable. - thanks to Mary/Keith for help with testing.
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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
          Sam Hemelryk added a comment -

          Thanks everyone, this has been integrated just now

          Show
          Sam Hemelryk added a comment - Thanks everyone, this has been integrated just now
          Hide
          Ankit Agarwal added a comment -

          Hi,
          I tested this on FF,IE and chrome. on 22 everything appeared perfectly fine. However on master there was some empty space on the right side. I have attached screen shots of both cases.

          Dan an you please confirm should this be considered as a pass or fail?

          Although when loading the package I got an error "SCORM API NOT FOUND do you want to continue?", But I don't think that is anything to do with this fix.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi, I tested this on FF,IE and chrome. on 22 everything appeared perfectly fine. However on master there was some empty space on the right side. I have attached screen shots of both cases. Dan an you please confirm should this be considered as a pass or fail? Although when loading the package I got an error "SCORM API NOT FOUND do you want to continue?", But I don't think that is anything to do with this fix. Thanks
          Hide
          Dan Marsden added a comment -

          I'd pass it and create a new bug to fix the space in master - I hadn't noticed it in my testing but there's obviously something extra we need there to improve it further in master - you're right that SCORM package isn't valid.

          Show
          Dan Marsden added a comment - I'd pass it and create a new bug to fix the space in master - I hadn't noticed it in my testing but there's obviously something extra we need there to improve it further in master - you're right that SCORM package isn't valid.
          Hide
          Ankit Agarwal added a comment -

          Thanks Dan.
          I have created an issue and linked here.
          Passing this test.
          Thanks

          Show
          Ankit Agarwal added a comment - Thanks Dan. I have created an issue and linked here. Passing this test. Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Some changes to Moodle should be milestones in the project by themselves.

          This is not the case and your fix is not so important, but your collaboration is highly appreciated, thanks!

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Some changes to Moodle should be milestones in the project by themselves. This is not the case and your fix is not so important, but your collaboration is highly appreciated, thanks! Closing as fixed, ciao

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: