Details

    • Testing Instructions:
      Hide

      Testing should be completed with the JS console open in at least IE/Firefox/Chrome:

      • Ensure that you're using the Standard theme
      • Ensure that the doctonewwindow setting is off
      • Open a course
      • Turn editing on
      • At the bottom of the page, click the 'Moodle docs for this page' button
        • Confirm that the page opens in the current window
      • Return to the course
      • Open the Activity Chooser
      • Select an activity
      • Scroll to the bottom of its help
      • Click the 'More help' button
        • Confirm that the page opens as a popup
      • Turn the 'doctonewwindow' setting on
      • Refresh the course page
      • At the bottom of the page, click the 'Moodle docs for this page' button
        • Confirm that the page opens as a popup
      • Open the Activity Chooser again
      • Find and select a 'More help' button
        • Confirm that the page opens as a popup
      Show
      Testing should be completed with the JS console open in at least IE/Firefox/Chrome: Ensure that you're using the Standard theme Ensure that the doctonewwindow setting is off Open a course Turn editing on At the bottom of the page, click the 'Moodle docs for this page' button Confirm that the page opens in the current window Return to the course Open the Activity Chooser Select an activity Scroll to the bottom of its help Click the 'More help' button Confirm that the page opens as a popup Turn the 'doctonewwindow' setting on Refresh the course page At the bottom of the page, click the 'Moodle docs for this page' button Confirm that the page opens as a popup Open the Activity Chooser again Find and select a 'More help' button Confirm that the page opens as a popup
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-35836-master

      Description

      The doctonewwindow setting alter Moodle help links such that they open in a new popup. It appears as though each link is made using an on click event, which we know to be pretty inefficient.

      It should be relatively easy to modify them such that event delegation is used on all such help links. The links that are opened are the same link as on the anchor.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Andrew Nicols added a comment -

            Raising priority to Major - this is disabled in default installations.

            Show
            Andrew Nicols added a comment - Raising priority to Major - this is disabled in default installations.
            Hide
            Andrew Nicols added a comment -

            This changes the doctonewwindow check to add a new class to the hyperlink instead of generating an individual onclick event.
            Additionally, we always run the setup part of this code (which just delegates any a.helplinkpopup to run a function) so that it can be used at any time, even if doctonewwindow is disabled. We then make use of this in the module chooser which forced a popup when clicking help regardless of the setting of doctonewwindow.

            Show
            Andrew Nicols added a comment - This changes the doctonewwindow check to add a new class to the hyperlink instead of generating an individual onclick event. Additionally, we always run the setup part of this code (which just delegates any a.helplinkpopup to run a function) so that it can be used at any time, even if doctonewwindow is disabled. We then make use of this in the module chooser which forced a popup when clicking help regardless of the setting of doctonewwindow.
            Hide
            Andrew Nicols added a comment -

            This should really help those users with doctonewwindow enabled. It should also increase performance of the page with the activity chooser (there were onclick events for each activities help).

            Longer term, it would be good to look at a more generic commoneventshandler YUI module to replace this and handle standard popups like this without being directly related to the help JS.

            This will need to be backported for 2.3 - I'll create a branch once Peer Review has taken place.

            Show
            Andrew Nicols added a comment - This should really help those users with doctonewwindow enabled. It should also increase performance of the page with the activity chooser (there were onclick events for each activities help). Longer term, it would be good to look at a more generic commoneventshandler YUI module to replace this and handle standard popups like this without being directly related to the help JS. This will need to be backported for 2.3 - I'll create a branch once Peer Review has taken place.
            Hide
            Paul Nicholls added a comment -

            Looks good to me - works as expected, no errors in JS console.

            Show
            Paul Nicholls added a comment - Looks good to me - works as expected, no errors in JS console.
            Hide
            Rajesh Taneja added a comment -

            Hello Andrew,

            Patch looks good, although you might want to consider adding class to helplink by default rather then from JS.

            • As we are doing this for performance, it might be nice to avoid any dom change (adding helplinkpopup class). Might be nice to alter doc_link api to accept forcepopup as optional value and make forcepopup true in get_module_metadata.
            Show
            Rajesh Taneja added a comment - Hello Andrew, Patch looks good, although you might want to consider adding class to helplink by default rather then from JS. As we are doing this for performance, it might be nice to avoid any dom change (adding helplinkpopup class). Might be nice to alter doc_link api to accept forcepopup as optional value and make forcepopup true in get_module_metadata.
            Hide
            Andrew Nicols added a comment -

            Thanks Rajesh,

            That's a great idea - I've made the change as per your suggestion.

            I've rebased on weeklies.

            Submitting for Integration review.

            Show
            Andrew Nicols added a comment - Thanks Rajesh, That's a great idea - I've made the change as per your suggestion. I've rebased on weeklies. Submitting for Integration review.
            Hide
            Aparup Banerjee 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
            Aparup Banerjee 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
            Andrew Nicols added a comment -

            Rebased on latest integration/master

            Show
            Andrew Nicols added a comment - Rebased on latest integration/master
            Hide
            Dan Poltawski added a comment - - edited

            Integrated to 23 and master, thanks

            Show
            Dan Poltawski added a comment - - edited Integrated to 23 and master, thanks
            Hide
            Adrian Greeve added a comment -

            Tested on the 2.3 and master integration branches on firefox, chrome and IE.
            No errors were found while testing.
            Test passed.

            Show
            Adrian Greeve added a comment - Tested on the 2.3 and master integration branches on firefox, chrome and IE. No errors were found while testing. Test passed.
            Hide
            Dan Poltawski added a comment -

            Hurray!

            You did it, congratulations! You have on Mojito credit to redeem after the release of Moodle 2.4

            Show
            Dan Poltawski added a comment - Hurray! You did it, congratulations! You have on Mojito credit to redeem after the release of Moodle 2.4

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: