Moodle
  1. Moodle
  2. MDL-39227

SCORM navigation panel is hidden when bootstrap theme is active

    Details

    • Testing Instructions:
      Hide
      1. Apply the clean/bootstrap theme to Moodle
      2. Open a SCORM package.
      3. Ensure you can access and move the SCORM navigation panel
      4. Test a couple of other themes including at least standard and one based upon canvas checking for regressions.
      Show
      Apply the clean/bootstrap theme to Moodle Open a SCORM package. Ensure you can access and move the SCORM navigation panel Test a couple of other themes including at least standard and one based upon canvas checking for regressions.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-39227-master
    • Story Points:
      5
    • Rank:
      49835
    • Sprint:
      FRONTEND Sprint 1

      Description

      1. Try accessing a scorm with bootstrap theme.
      2. No navigation panel is shown.

        Issue Links

          Activity

          Hide
          Ankit Agarwal added a comment -

          Found out what is happening. The div has style="z-index: 2; visibility: visible; left: -195px; top: 259px;" on it, the -195px takes it away from the visible scorm area and hides it behind the navigation block.

          Show
          Ankit Agarwal added a comment - Found out what is happening. The div has style="z-index: 2; visibility: visible; left: -195px; top: 259px;" on it, the -195px takes it away from the visible scorm area and hides it behind the navigation block.
          Hide
          Jasmin Klindzic added a comment -

          Similar thing happens on the default theme on qa.moodle.net - partial visibility of the drop down menu.

          Show
          Jasmin Klindzic added a comment - Similar thing happens on the default theme on qa.moodle.net - partial visibility of the drop down menu.
          Hide
          Andrew Nicols added a comment -

          Just checking - is this actually a JavaScript issue? It looks at face value to be CSS?

          Show
          Andrew Nicols added a comment - Just checking - is this actually a JavaScript issue? It looks at face value to be CSS?
          Hide
          Mary Evans added a comment -

          Since the rename, I can't get the custommenu in the top navigation to work.
          I thought it was because I had added jQuery in a copy of Clean theme I am customising, but checked just now in Bootstrap itself and the top navbar isn't working there either!

          Show
          Mary Evans added a comment - Since the rename, I can't get the custommenu in the top navigation to work. I thought it was because I had added jQuery in a copy of Clean theme I am customising, but checked just now in Bootstrap itself and the top navbar isn't working there either!
          Hide
          Jason Fowler added a comment -

          Mary, this isn't moodle navigation, this is the SCORM navigation panel.

          Show
          Jason Fowler added a comment - Mary, this isn't moodle navigation, this is the SCORM navigation panel.
          Hide
          Jason Fowler added a comment - - edited

          Andrew, I think the CSS is set inline by Javascript. If this is true, it is indeed a JS issue.

          Show
          Jason Fowler added a comment - - edited Andrew, I think the CSS is set inline by Javascript. If this is true, it is indeed a JS issue.
          Hide
          Jason Fowler added a comment -
          scorm_nav_panel = new Y.YUI2.widget.Panel('scorm_navpanel', { visible:true, draggable:true, close:false, xy: [250, 450],
                                                                              autofillheight: "body"} );
          

          line 507 in mod/scorm/module.js

          the X value needs recalculating if bootstrap is the current theme.

          Show
          Jason Fowler added a comment - scorm_nav_panel = new Y.YUI2.widget.Panel('scorm_navpanel', { visible: true , draggable: true , close: false , xy: [250, 450], autofillheight: "body" } ); line 507 in mod/scorm/module.js the X value needs recalculating if bootstrap is the current theme.
          Hide
          Jason Fowler added a comment -

          can't really change the value based on the theme, and seeing as it is set as an inline style, the only real option is to increase the value.

          This has been done, Ankit, could you please peer review this?

          Show
          Jason Fowler added a comment - can't really change the value based on the theme, and seeing as it is set as an inline style, the only real option is to increase the value. This has been done, Ankit, could you please peer review this?
          Hide
          Ankit Agarwal added a comment - - edited

          Hi Jason,

          1. There are two issues reported in this report. Jasmin reported a similar issue, if you feel that, that issue is not scope of this bug, please create a separate issue for it, as the associated QA is failed for that.
          2. The issue is still present, I still have left: -192px; on the BS theme effectively hiding the navigation panel.

          Sorry but my understanding of how exactly the panel's styles are rendered is limited. If you can comment a bit here about how and where the styles are generated for the panel, would be great.
          Thanks

          Show
          Ankit Agarwal added a comment - - edited Hi Jason, There are two issues reported in this report. Jasmin reported a similar issue, if you feel that, that issue is not scope of this bug, please create a separate issue for it, as the associated QA is failed for that. The issue is still present, I still have left: -192px; on the BS theme effectively hiding the navigation panel. Sorry but my understanding of how exactly the panel's styles are rendered is limited. If you can comment a bit here about how and where the styles are generated for the panel, would be great. Thanks
          Hide
          Jason Fowler added a comment -

          The patch for this issue should fix all themes having this issue Ankit. Have you tried flushing your cache before testing the issue?

          Show
          Jason Fowler added a comment - The patch for this issue should fix all themes having this issue Ankit. Have you tried flushing your cache before testing the issue?
          Hide
          Ankit Agarwal added a comment -

          nvm, its working for bs theme now. Anyways it effectively brings the panel in content area where it overlaps with other stuff for other themes. Have a look at arialist and boxxie theme for example.

          Show
          Ankit Agarwal added a comment - nvm, its working for bs theme now. Anyways it effectively brings the panel in content area where it overlaps with other stuff for other themes. Have a look at arialist and boxxie theme for example.
          Hide
          Jason Fowler added a comment -

          I'm not seeing that behaviour at all Ankit. The code works on my environments, and the logic is sound. Pushing for integration, unless you have any objections...

          Show
          Jason Fowler added a comment - I'm not seeing that behaviour at all Ankit. The code works on my environments, and the logic is sound. Pushing for integration, unless you have any objections...
          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
          Michael de Raadt added a comment -

          This issue should be able to be tested as part of MDLQA-5332.

          Show
          Michael de Raadt added a comment - This issue should be able to be tested as part of MDLQA-5332 .
          Hide
          Ankit Agarwal added a comment -

          I have reported the issue Jasmin mentioned as MDL-39508, as the current patch doesn't fix that.

          Show
          Ankit Agarwal added a comment - I have reported the issue Jasmin mentioned as MDL-39508 , as the current patch doesn't fix that.
          Hide
          Damyon Wiese added a comment -

          Hi Jason,

          This solution is not acceptable.

          To pass the QA tests, we can fix the code that positions the navigation box so it is visible, but the navigation controls really need to be redone as they are currently draggable, but only visible when displayed over the navigation table of contents which is just broken IMO.

          The problem with your solution is that it randomly positions the box but you have no way of knowing if the position is correct based on the current theme etc.

          A better (temporary solution) would be to get the position of the table of contents and position the navigation relative to that (in js).

          Ugly version that works:

          -            scorm_nav_panel = new Y.YUI2.widget.Panel('scorm_navpanel', { visible:true, draggable:true, close:false, xy: [450, 450],
          -                                                                    autofillheight: "body"} );
          +            var left = scorm_layout_widget.getUnitByPosition('left');
          +            navposition = Y.YUI2.util.Dom.getXY(left);
          +            navposition[1] += 200;
          +            scorm_nav_panel = new Y.YUI2.widget.Panel('scorm_navpanel', { visible:true, draggable:true, close:false, xy: navposition,
          +                                                                    autofillheight: "body", iframe: true} );
          
          
          Show
          Damyon Wiese added a comment - Hi Jason, This solution is not acceptable. To pass the QA tests, we can fix the code that positions the navigation box so it is visible, but the navigation controls really need to be redone as they are currently draggable, but only visible when displayed over the navigation table of contents which is just broken IMO. The problem with your solution is that it randomly positions the box but you have no way of knowing if the position is correct based on the current theme etc. A better (temporary solution) would be to get the position of the table of contents and position the navigation relative to that (in js). Ugly version that works: - scorm_nav_panel = new Y.YUI2.widget.Panel('scorm_navpanel', { visible: true , draggable: true , close: false , xy: [450, 450], - autofillheight: "body" } ); + var left = scorm_layout_widget.getUnitByPosition('left'); + navposition = Y.YUI2.util.Dom.getXY(left); + navposition[1] += 200; + scorm_nav_panel = new Y.YUI2.widget.Panel('scorm_navpanel', { visible: true , draggable: true , close: false , xy: navposition, + autofillheight: "body" , iframe: true } );
          Hide
          Jason Fowler added a comment -

          I am not sure how well that works, because even with the way I did it, the values given were not those it ended up with after everything had been done. there was something hinky going on after all that was set...

          Show
          Jason Fowler added a comment - I am not sure how well that works, because even with the way I did it, the values given were not those it ended up with after everything had been done. there was something hinky going on after all that was set...
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Mary Evans added a comment -

          I'm just adding CSS in canvas theme to fix problem with this navigation dropdown menu in standard themes who have Canvas as a parent theme. I've just tested bootstrapbase and found that it looks OK. So not sure why you saw it differently.

          Hope this helps?

          Show
          Mary Evans added a comment - I'm just adding CSS in canvas theme to fix problem with this navigation dropdown menu in standard themes who have Canvas as a parent theme. I've just tested bootstrapbase and found that it looks OK. So not sure why you saw it differently. Hope this helps?
          Hide
          Jason Fowler added a comment -

          This issue is about the little panel that can be dragged and dropped Mary.

          MDL-39508 is about the drop down. This is why we need to report issues separately, rather than bunch them together. Too many cross-discussions that get confusing.

          Show
          Jason Fowler added a comment - This issue is about the little panel that can be dragged and dropped Mary. MDL-39508 is about the drop down. This is why we need to report issues separately, rather than bunch them together. Too many cross-discussions that get confusing.
          Hide
          Jason Fowler added a comment -

          Okay, all tested using the method you recommended Damyon. It works fine. Put that in my patch and requesting peer review now.

          Show
          Jason Fowler added a comment - Okay, all tested using the method you recommended Damyon. It works fine. Put that in my patch and requesting peer review now.
          Hide
          Ankit Agarwal added a comment -

          Looks much better now. Feel free to submit for integration.
          Thanks

          Show
          Ankit Agarwal added a comment - Looks much better now. Feel free to submit for integration. Thanks
          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
          Damyon Wiese added a comment -

          One more thing Jason - setting this CSS allows the nav to pop out of the toc div (looks much better):

          #toctree

          { overflow: visible; }
          Show
          Damyon Wiese added a comment - One more thing Jason - setting this CSS allows the nav to pop out of the toc div (looks much better): #toctree { overflow: visible; }
          Hide
          Jason Fowler added a comment -

          Reopened to work on suggestions from Damyon

          Show
          Jason Fowler added a comment - Reopened to work on suggestions from Damyon
          Hide
          Dan Marsden added a comment -

          grrr... shouldn't there be a +1 from a component maintainer before stuff lands in integration?

          Show
          Dan Marsden added a comment - grrr... shouldn't there be a +1 from a component maintainer before stuff lands in integration?
          Hide
          Dan Marsden added a comment -

          ... I only saw this just now due to the new auto watcher changes.

          Show
          Dan Marsden added a comment - ... I only saw this just now due to the new auto watcher changes.
          Hide
          Jason Fowler added a comment -

          This is a minor tweak to the positioning of an element within the SCORM system to make it function correctly. I don't know if that needs a +1 from the maintainer, but if it does, I was unaware that SCORM had a maintainer, I was under the impression that Ankit was the closest thing we had to a maintainer for SCORM.

          Show
          Jason Fowler added a comment - This is a minor tweak to the positioning of an element within the SCORM system to make it function correctly. I don't know if that needs a +1 from the maintainer, but if it does, I was unaware that SCORM had a maintainer, I was under the impression that Ankit was the closest thing we had to a maintainer for SCORM.
          Hide
          Dan Marsden added a comment -

          I'm not sure how to respond to that.... perhaps you should have a conversation with Michael/Martin about how appropriate that comment was.

          Show
          Dan Marsden added a comment - I'm not sure how to respond to that.... perhaps you should have a conversation with Michael/Martin about how appropriate that comment was.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Damyon Wiese added a comment -

          Dan - yes this should have a +1 from you - originally this was just a fix for a regression caused by bootstrap but is growing in scope.

          Show
          Damyon Wiese added a comment - Dan - yes this should have a +1 from you - originally this was just a fix for a regression caused by bootstrap but is growing in scope.
          Hide
          Damyon Wiese added a comment -

          Jason - the component leads are all listed against the components in Jira.

          Show
          Damyon Wiese added a comment - Jason - the component leads are all listed against the components in Jira.
          Hide
          Jason Fowler added a comment - - edited

          Sorry Dan, didn't mean any offence by my previous comment, I simply wasn't aware that this would need a +1 from the maintainer, and wasn't aware that there was a maintainer for this component.

          Damyon - I've never been able to see a definitive list on JIRA for component leads, every time I get linked to one, it shows a few then stops ...

          Show
          Jason Fowler added a comment - - edited Sorry Dan, didn't mean any offence by my previous comment, I simply wasn't aware that this would need a +1 from the maintainer, and wasn't aware that there was a maintainer for this component. Damyon - I've never been able to see a definitive list on JIRA for component leads, every time I get linked to one, it shows a few then stops ...
          Hide
          Jason Fowler added a comment -

          Dan - can you please have a look at the patch and let me know if it's okay?

          Show
          Jason Fowler added a comment - Dan - can you please have a look at the patch and let me know if it's okay?
          Hide
          Mary Evans added a comment -

          @Jason,

          Can this not be fixed with CSS? Surely this is just a z-index thing isn't it? From memory we have had this problem with standard Moodle 2.x themes in the past, and they got fixed. If it works OK in other Moodle themes then it should work OK in Bootstrapbase themes too, like Clean theme and any other that are cloned from it. Providing of course that the CSS from Base theme was carried over to Bootstrapbase. So the fact it is not working is more than likely that Base CSS did not make it to Bootstrapbase.

          @Dan, didn't you sort this out in Afterburner?

          Show
          Mary Evans added a comment - @Jason, Can this not be fixed with CSS? Surely this is just a z-index thing isn't it? From memory we have had this problem with standard Moodle 2.x themes in the past, and they got fixed. If it works OK in other Moodle themes then it should work OK in Bootstrapbase themes too, like Clean theme and any other that are cloned from it. Providing of course that the CSS from Base theme was carried over to Bootstrapbase. So the fact it is not working is more than likely that Base CSS did not make it to Bootstrapbase. @Dan, didn't you sort this out in Afterburner?
          Hide
          Mary Evans added a comment -

          @Jason,

          The images above show the the side panel. This is visible in the Clean theme, although it is closed, but opens (slides out) when you click the >> arrow in the header of the side panel strip on the left.

          The second image shows the dropdown menu hidden behind the SCORM Player window. I have fixed this for ALL Moodle themes in MDL-39508

          Show
          Mary Evans added a comment - @Jason, The images above show the the side panel. This is visible in the Clean theme, although it is closed, but opens (slides out) when you click the >> arrow in the header of the side panel strip on the left. The second image shows the dropdown menu hidden behind the SCORM Player window. I have fixed this for ALL Moodle themes in MDL-39508
          Hide
          Jason Fowler added a comment -

          Thanks Mary, I've looked through the issue, and the CSS on the item is set inline, hence the javascript to fix it. The patch I have pushed solves the issue. There is a z-index issue, but there is also a positioning issue. and the code in the patch fixes it all.

          Show
          Jason Fowler added a comment - Thanks Mary, I've looked through the issue, and the CSS on the item is set inline, hence the javascript to fix it. The patch I have pushed solves the issue. There is a z-index issue, but there is also a positioning issue. and the code in the patch fixes it all.
          Hide
          Dan Marsden added a comment -

          patch looks ok to me - except that navigation panel is just nasty and would be nice to completely rewrite/replace with something else sometime - Jeremy posted a hack that moves it to under the player window using in-line JS here:
          https://moodle.org/mod/forum/discuss.php?d=159769#p991576
          (although the code isn't reusable I like the location and display of the nav bar a lot better)

          Show
          Dan Marsden added a comment - patch looks ok to me - except that navigation panel is just nasty and would be nice to completely rewrite/replace with something else sometime - Jeremy posted a hack that moves it to under the player window using in-line JS here: https://moodle.org/mod/forum/discuss.php?d=159769#p991576 (although the code isn't reusable I like the location and display of the nav bar a lot better)
          Hide
          Jason Fowler added a comment -

          The frontend team has been discussing a re-write of the scorm layout. Nothing concrete yet, just the idea that it should happen, to make it mobile friendly. In the mean time though, this patch fixes the issue at hand, so I will push it for integration.

          Show
          Jason Fowler added a comment - The frontend team has been discussing a re-write of the scorm layout. Nothing concrete yet, just the idea that it should happen, to make it mobile friendly. In the mean time though, this patch fixes the issue at hand, so I will push it for integration.
          Hide
          Dan Marsden added a comment -

          Hopefully the frontend team don't have their head in the sand and are aware of the existing SCORM project to implement an html5 version of the player.

          Show
          Dan Marsden added a comment - Hopefully the frontend team don't have their head in the sand and are aware of the existing SCORM project to implement an html5 version of the player.
          Hide
          Jason Fowler added a comment -

          We are well aware of that, and were discussing it again this morning. That's the GSOC project. We are deferring any work until that has been completed.

          Show
          Jason Fowler added a comment - We are well aware of that, and were discussing it again this morning. That's the GSOC project. We are deferring any work until that has been completed.
          Hide
          Jason Fowler added a comment -

          Sorry, that came across wrong.

          We have been discussing the idea of some changes, mostly CSS, to allow it to fit on smaller screens and to allow for a reflow when a screen is rotated, but we are also expecting that the project you mentioned, to rewrite it in HTML5, will negate the need for that. I'm not sure if the watch list thing is sorted, so I can locate the issues and link you to them, as they would benefit greatly from your input.

          Show
          Jason Fowler added a comment - Sorry, that came across wrong. We have been discussing the idea of some changes, mostly CSS, to allow it to fit on smaller screens and to allow for a reflow when a screen is rotated, but we are also expecting that the project you mentioned, to rewrite it in HTML5, will negate the need for that. I'm not sure if the watch list thing is sorted, so I can locate the issues and link you to them, as they would benefit greatly from your input.
          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
          Sam Hemelryk added a comment -

          Thanks all, this has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks all, this has been integrated now.
          Hide
          Mark Nelson added a comment -

          Passing this. However, dragging the navigation panel at high speeds will often cause the mouse to move away from the panel and the panel to remain stationary (except when in Opera - strange), but after comparing this to moving blocks in Moodle the behaviour is similar.

          I will raise a separate issue about not being able to move the navigation block outside of the SCORM area.

          Thanks.

          Show
          Mark Nelson added a comment - Passing this. However, dragging the navigation panel at high speeds will often cause the mouse to move away from the panel and the panel to remain stationary (except when in Opera - strange), but after comparing this to moving blocks in Moodle the behaviour is similar. I will raise a separate issue about not being able to move the navigation block outside of the SCORM area. Thanks.
          Hide
          Marina Glancy added a comment -

          Thanks for your awesome work! This has now become a part of Moodle.

          Closing as fixed!

          Show
          Marina Glancy added a comment - Thanks for your awesome work! This has now become a part of Moodle. Closing as fixed!

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Agile