Uploaded image for project: '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
    • Sprint:
      FRONTEND Sprint 1
    • Story Points (Obsolete):
      5
    • Sprint:
      FRONTEND Sprint 1

      Description

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

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            ankit_frenz 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_frenz 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
            jasmink Jasmin Klindzic added a comment -

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

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

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

            Show
            dobedobedoh Andrew Nicols added a comment - Just checking - is this actually a JavaScript issue? It looks at face value to be CSS?
            Hide
            lazydaisy 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
            lazydaisy 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
            phalacee Jason Fowler added a comment -

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

            Show
            phalacee Jason Fowler added a comment - Mary, this isn't moodle navigation, this is the SCORM navigation panel.
            Hide
            phalacee 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
            phalacee 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
            phalacee 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
            phalacee 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
            phalacee 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
            phalacee 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_frenz 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_frenz 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
            phalacee 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
            phalacee 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_frenz 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_frenz 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
            phalacee 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
            phalacee 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
            poltawski 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
            poltawski 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
            salvetore Michael de Raadt added a comment -

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

            Show
            salvetore Michael de Raadt added a comment - This issue should be able to be tested as part of MDLQA-5332 .
            Hide
            ankit_frenz 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_frenz 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 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 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
            phalacee 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
            phalacee 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 CiBoT added a comment -

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

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

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

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            lazydaisy 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
            lazydaisy 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
            phalacee 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
            phalacee 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
            phalacee 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
            phalacee 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_frenz Ankit Agarwal added a comment -

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

            Show
            ankit_frenz Ankit Agarwal added a comment - Looks much better now. Feel free to submit for integration. Thanks
            Hide
            stronk7 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
            stronk7 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 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 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
            phalacee Jason Fowler added a comment -

            Reopened to work on suggestions from Damyon

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

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

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

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

            Show
            danmarsden Dan Marsden added a comment - ... I only saw this just now due to the new auto watcher changes.
            Hide
            phalacee 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
            phalacee 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
            danmarsden 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
            danmarsden 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 CiBoT added a comment -

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

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            damyon 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 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 Damyon Wiese added a comment -

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

            Show
            damyon Damyon Wiese added a comment - Jason - the component leads are all listed against the components in Jira.
            Hide
            phalacee 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
            phalacee 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
            phalacee Jason Fowler added a comment -

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

            Show
            phalacee Jason Fowler added a comment - Dan - can you please have a look at the patch and let me know if it's okay?
            Hide
            lazydaisy 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
            lazydaisy 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
            lazydaisy 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
            lazydaisy 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
            phalacee 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
            phalacee 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
            danmarsden 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
            danmarsden 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
            phalacee 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
            phalacee 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
            danmarsden 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
            danmarsden 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
            phalacee 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
            phalacee 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
            phalacee 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
            phalacee 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
            poltawski 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
            poltawski 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
            samhemelryk Sam Hemelryk added a comment -

            Thanks all, this has been integrated now.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks all, this has been integrated now.
            Hide
            markn 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
            markn 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 Marina Glancy added a comment -

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

            Closing as fixed!

            Show
            marina 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:
                  Fix Release Date:
                  8/Jul/13

                  Agile