Details

    • Testing Instructions:
      Hide

      I'd advise having your browsers inspector up for these tests

      For master:

      • Open a course
      • Turn editing on
      • Turn off the activity chooser
      • Click the help icon next to the 'Add a resource...' dropdown
      • Confirm:
        • that the Network activity tab in your browser inspector showed a page fetch to help.php
        • that the correct help text is displayed
        • that the correct help title is displayed
      • Click the close button to close the tooltip
        • Confirm that the popup closed
      • Re-open the same help
      • Click elsewhere on the page not a link
        • Confirm that the popup closed
      • Re-open the same help
      • Click on another help icon
      • Confirm:
        • that the Network activity tab in your browser inspector showed a new page fetch to help.php
        • that the correct help text is displayed
        • that the correct help title is displayed
      • Disable JavaScript in your browser
      • Refresh the page
      • Click on the help icon again
        • Confirm that the help opens correctly (not a JS popup)*
      Show
      I'd advise having your browsers inspector up for these tests For master: Open a course Turn editing on Turn off the activity chooser Click the help icon next to the 'Add a resource...' dropdown Confirm: that the Network activity tab in your browser inspector showed a page fetch to help.php that the correct help text is displayed that the correct help title is displayed Click the close button to close the tooltip Confirm that the popup closed Re-open the same help Click elsewhere on the page not a link Confirm that the popup closed Re-open the same help Click on another help icon Confirm: that the Network activity tab in your browser inspector showed a new page fetch to help.php that the correct help text is displayed that the correct help title is displayed Disable JavaScript in your browser Refresh the page Click on the help icon again Confirm that the help opens correctly (not a JS popup)*
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-36752-master
    • Rank:
      46259

      Description

      Although MDL-35819 will completely rewrite tooltip help in Moodle, it cannot be backported and is highly unlikely to hit Moodle 2.4

      This change will improve the (client-side) efficiency of the help tooltip in Moodle. It works in a similar way to the change in MDL-35819 in that we move to an event delegation model rather than individual listeners for each clickable help icon.

        Activity

        Hide
        Andrew Nicols added a comment -

        Increasing priority of this because it will have some performance improvements for many users - particularly those with courses in editing mode with lots of sections.

        Show
        Andrew Nicols added a comment - Increasing priority of this because it will have some performance improvements for many users - particularly those with courses in editing mode with lots of sections.
        Hide
        Frédéric Massart added a comment -

        Hi Andrew,

        thanks for providing this solution. I like that improvement which will hidhly reduce the number of instances to create by using a generic selector. We should do that way more often! Here are few notes about your patch:

        • I think you should not set the new class 'helpicon' on the link. I think the id was not well chosen before as it means 'icon' where as it's a link, which may or may not contain text. Also, I think for the sake of selectors and making it consistent for themers, you might not need to set a new class but use span.helplink as a selector. This class is already set and used. (I've been working with CSS those days, and we definitely should think more of consistency about those selectors, I also thought that we could create a more generic class for the purpose of javascript selectors, such helplink-has-popup, but I don't think this is required for this issue. Maybe using another attribute could be interesting too, as in 'rel=helppopup', anyway, those were just thoughts for the future.)
        • Do we need to pass array(array()) to setup() as it does not take any argument any more?
        • Just for my information, I wrote a small JS thingy the other day, and Raj noticed that I was using Y.one(document.body). So if the script runs before body is set, it would fail. I know it should never happen, but would you consider it safer to do: body = Y.one('body'); if (!body) return;?
        • this.helplink = event.target.ancestor('a.helpicon', true); I am scared that this would point to the wrong thing, what if I click the link and not the icon? Also, I guess this would be solved if the selector is span.helplink
        • if (Y.one('html').get('dir') == 'rtl'). I recently introduced the function javascript-static:right_to_left() which is the equivalent to the PHP function, perhaps you could make use of it. I know you didn't change anything on that line, but at least now you know about that. (It might only be available on the integration thought, and it has not been backported MDL-36613)
        • Can you confirm that not using the full URL any more would work?

        Thanks
        Fred

        Show
        Frédéric Massart added a comment - Hi Andrew, thanks for providing this solution. I like that improvement which will hidhly reduce the number of instances to create by using a generic selector. We should do that way more often! Here are few notes about your patch: I think you should not set the new class 'helpicon' on the link. I think the id was not well chosen before as it means 'icon' where as it's a link, which may or may not contain text. Also, I think for the sake of selectors and making it consistent for themers, you might not need to set a new class but use span.helplink as a selector. This class is already set and used. (I've been working with CSS those days, and we definitely should think more of consistency about those selectors, I also thought that we could create a more generic class for the purpose of javascript selectors, such helplink-has-popup, but I don't think this is required for this issue. Maybe using another attribute could be interesting too, as in 'rel=helppopup', anyway, those were just thoughts for the future.) Do we need to pass array(array()) to setup() as it does not take any argument any more? Just for my information, I wrote a small JS thingy the other day, and Raj noticed that I was using Y.one(document.body). So if the script runs before body is set, it would fail. I know it should never happen, but would you consider it safer to do: body = Y.one('body'); if (!body) return;? this.helplink = event.target.ancestor('a.helpicon', true); I am scared that this would point to the wrong thing, what if I click the link and not the icon? Also, I guess this would be solved if the selector is span.helplink if (Y.one('html').get('dir') == 'rtl'). I recently introduced the function javascript-static:right_to_left() which is the equivalent to the PHP function, perhaps you could make use of it. I know you didn't change anything on that line, but at least now you know about that. (It might only be available on the integration thought, and it has not been backported MDL-36613 ) Can you confirm that not using the full URL any more would work? Thanks Fred
        Hide
        Andrew Nicols added a comment -

        Hi Fred,

        Thanks for the feedback.

        • have you any better suggestions - I'm happy to use something else. We should be able to use the existing class on the span.
        • We don't need to pass the empty array - I'll remove those
        • useful to know that. I'm never sure which to use, but I guess Y.one('body') is just as easy.
        • Cheers for the RTL info - I'll keep an eye on that
        • I can't confirm about the full URL - it was added beack when the tooltip opened in a new window rather than as a tooltip. I don't know of any reason it would be required as we're using the same target on the anchor. If it's broken on the anchor, it will be broken everywhere anyway. IMO, we shouldn't support people giving different help for the AJAX and non-AJAX versions.
        • Taking the current code as a start point:
        <span class="helplink">
          <a href="http://kumquat.lancs.ac.uk/help.php?component=moodle&amp;identifier=shortnamecourse&amp;lang=en" title="Help with Course short name" aria-haspopup="true" class="helpicon">
            <img src="http://kumquat.lancs.ac.uk/theme/image.php?theme=standard&amp;component=core&amp;image=help" alt="Help with Course short name" class="iconhelp">
          </a>
        </span>
        
        • If someone clicks on the span, then nothing will happen and in my opinion, you wouldn't expect it to. There's no reason it should.
        • If someone clicks on the anchor, then
          event.target.ancestor('a.helpicon', true)

          will return the anchor (the true argument ensures that ancestor includes the target in the search)

        • If someone clicks on the img, then
          event.target.ancestor('a.helpicon', true)

          will return the anchor

        I guess we could change the selector to "span.helplink a" but it's the same logic really as we'd still need to place the event delegation on the anchor and not the span (we can't cancel the default click action of the anchor if we listen on the span)

        I'll knock up a patch with the above changes.

        Show
        Andrew Nicols added a comment - Hi Fred, Thanks for the feedback. have you any better suggestions - I'm happy to use something else. We should be able to use the existing class on the span. We don't need to pass the empty array - I'll remove those useful to know that. I'm never sure which to use, but I guess Y.one('body') is just as easy. Cheers for the RTL info - I'll keep an eye on that I can't confirm about the full URL - it was added beack when the tooltip opened in a new window rather than as a tooltip. I don't know of any reason it would be required as we're using the same target on the anchor. If it's broken on the anchor, it will be broken everywhere anyway. IMO, we shouldn't support people giving different help for the AJAX and non-AJAX versions. Taking the current code as a start point: <span class= "helplink" > <a href= "http: //kumquat.lancs.ac.uk/help.php?component=moodle&amp;identifier=shortnamecourse&amp;lang=en" title= "Help with Course short name" aria-haspopup= " true " class= "helpicon" > <img src= "http: //kumquat.lancs.ac.uk/theme/image.php?theme=standard&amp;component=core&amp;image=help" alt= "Help with Course short name" class= "iconhelp" > </a> </span> If someone clicks on the span, then nothing will happen and in my opinion, you wouldn't expect it to. There's no reason it should. If someone clicks on the anchor, then event.target.ancestor('a.helpicon', true ) will return the anchor (the true argument ensures that ancestor includes the target in the search) If someone clicks on the img, then event.target.ancestor('a.helpicon', true ) will return the anchor I guess we could change the selector to "span.helplink a" but it's the same logic really as we'd still need to place the event delegation on the anchor and not the span (we can't cancel the default click action of the anchor if we listen on the span) I'll knock up a patch with the above changes.
        Hide
        Andrew Nicols added a comment -

        Sam - I've added you as a watcher on here to ask a question about the fullurl section of http://git.moodle.org/gw?p=moodle.git;a=blob;f=lib/javascript-static.js;h=fa0dfc03e0cba5e061a3a093e00d25e79d911cca;hb=HEAD#l1511

        At present, if it's missing a protocol, then it rewrites the URL - I guess this is because the help icons used to open as a popup window rather than a YUI overlay, and that a relative link would not work in such situations but just wanted to confirm with you.

        Show
        Andrew Nicols added a comment - Sam - I've added you as a watcher on here to ask a question about the fullurl section of http://git.moodle.org/gw?p=moodle.git;a=blob;f=lib/javascript-static.js;h=fa0dfc03e0cba5e061a3a093e00d25e79d911cca;hb=HEAD#l1511 At present, if it's missing a protocol, then it rewrites the URL - I guess this is because the help icons used to open as a popup window rather than a YUI overlay, and that a relative link would not work in such situations but just wanted to confirm with you.
        Hide
        Andrew Nicols added a comment -

        I've just made the changes discussed which simplify things more. I'd appreciate a second peer review though if you don't mind.

        I'm fairly sure that the fullurl conversion was because window.open wouldn't handle a relative URL correctly. Y.io should do this correctly though.

        Show
        Andrew Nicols added a comment - I've just made the changes discussed which simplify things more. I'd appreciate a second peer review though if you don't mind. I'm fairly sure that the fullurl conversion was because window.open wouldn't handle a relative URL correctly. Y.io should do this correctly though.
        Hide
        Sam Hemelryk added a comment -

        Hi Andrew, I'd bet that was the case, simple fixing of relative link issues.

        I couldn't imagine any other reason for it but if you need to investigate further David Mudrak would probably be the person to talk to, he's been involved in other tasks associated with help texts and probably has a better idea of their history than I do.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Andrew, I'd bet that was the case, simple fixing of relative link issues. I couldn't imagine any other reason for it but if you need to investigate further David Mudrak would probably be the person to talk to, he's been involved in other tasks associated with help texts and probably has a better idea of their history than I do. Cheers Sam
        Hide
        Frédéric Massart added a comment -

        Hi Andrew, thanks for clarifying those and sorry about my misunderstanding of .ancestor(), it does make perfect sense. I don't think it's required to change the class of the span, but I like the idea of the selector being span.helplink a. If that URL thing is sorted it, feel free to push for integration.

        Cheers!

        Show
        Frédéric Massart added a comment - Hi Andrew, thanks for clarifying those and sorry about my misunderstanding of .ancestor(), it does make perfect sense. I don't think it's required to change the class of the span, but I like the idea of the selector being span.helplink a. If that URL thing is sorted it, feel free to push for integration. Cheers!
        Hide
        Andrew Nicols added a comment -

        I'm going to submit this for integration.

        I've tested that Y.io can handle absolute links without the protocol, and I think that the trip down memory lane suggests the original reason behind these fully qualified links and that this case no longer applies.

        I've added David as a watcher in case he wants to add anything here.

        Show
        Andrew Nicols added a comment - I'm going to submit this for integration. I've tested that Y.io can handle absolute links without the protocol, and I think that the trip down memory lane suggests the original reason behind these fully qualified links and that this case no longer applies. I've added David as a watcher in case he wants to add anything here.
        Hide
        Dan Poltawski added a comment -

        I'm feeling generous today , but this should really have gone into 2.2 as well as 2.3 I think.

        Andrew: could you clarify the testing instructions for 23, as you've only mentioned master.

        Show
        Dan Poltawski added a comment - I'm feeling generous today , but this should really have gone into 2.2 as well as 2.3 I think. Andrew: could you clarify the testing instructions for 23, as you've only mentioned master.
        Hide
        Dan Poltawski added a comment -

        Hi Andrew,

        Looks like there is a problem with forum ratings for this, see:
        https://moodle.org/mod/forum/discuss.php?d=216609

        Click on the forum rating help item.

        Show
        Dan Poltawski added a comment - Hi Andrew, Looks like there is a problem with forum ratings for this, see: https://moodle.org/mod/forum/discuss.php?d=216609 Click on the forum rating help item.
        Hide
        Dan Poltawski added a comment -

        Failing this issue.

        Raj: it might be good if you can test this even if we don't have a fix for that issue, though.

        Show
        Dan Poltawski added a comment - Failing this issue. Raj: it might be good if you can test this even if we don't have a fix for that issue, though.
        Hide
        Andrew Nicols added a comment -

        These issues relate to the change that Fred and I discussed to make the selector for this more generic - the original thought was to add a selector to the anchor and act on that, but we changed it to reduce the number of selectors to:

        span.helplink a
        

        Unfortunately, it seems that there are a couple of places where this may also match:

        • mod/glossary/view.php
        • lib/outputrenderers.php (help_icon_scale)

        There may also be cases in third-party code where developers have made use of the same selector for their help links.

        The least disruptive fixes IMHO would be to:

        • add an additional tooltip selector to span.helplink and change the JS selector to "span.helplink.tooltip a"; or
        • add back a selector to the anchor making the JS selector "span.helplink a.tooltip".

        These options would allow third-party devs to do nothing and would require us not to make any further changes to affected locations. This is good, but it does mean that we have more selectors - which isn't bad really.

        A more disruptive approach would be to:

        • add a selector to those locations which are broken and test for this selector early in the display function

        This would force us, and any other third-party devs to do something which is arguably better in the long run. They would either have to add the selector, or to make their code compatible with the popup (e.g. to respect the ajax flag and not to whatever custom event they're currently doing).

        In the long run, we should try and convert the core locations to use the same interface as the help popups as this will help us to improve UI consistency - and we should encourage any third-parties to do the same.

        It would be good to have some thoughts on these options - all are pretty trivial to implement in core.

        Show
        Andrew Nicols added a comment - These issues relate to the change that Fred and I discussed to make the selector for this more generic - the original thought was to add a selector to the anchor and act on that, but we changed it to reduce the number of selectors to: span.helplink a Unfortunately, it seems that there are a couple of places where this may also match: mod/glossary/view.php lib/outputrenderers.php (help_icon_scale) There may also be cases in third-party code where developers have made use of the same selector for their help links. The least disruptive fixes IMHO would be to: add an additional tooltip selector to span.helplink and change the JS selector to "span.helplink.tooltip a"; or add back a selector to the anchor making the JS selector "span.helplink a.tooltip". These options would allow third-party devs to do nothing and would require us not to make any further changes to affected locations. This is good, but it does mean that we have more selectors - which isn't bad really. A more disruptive approach would be to: add a selector to those locations which are broken and test for this selector early in the display function This would force us, and any other third-party devs to do something which is arguably better in the long run. They would either have to add the selector, or to make their code compatible with the popup (e.g. to respect the ajax flag and not to whatever custom event they're currently doing). In the long run, we should try and convert the core locations to use the same interface as the help popups as this will help us to improve UI consistency - and we should encourage any third-parties to do the same. It would be good to have some thoughts on these options - all are pretty trivial to implement in core.
        Hide
        Andrew Nicols added a comment -

        Having thought about this for a bit, I've gone for the "span.helplink a.tooltip" option because this is the least disruptive and allows us to apply the fix to all stable branches.

        Longer term, we need to look at updating the scales code to use the same tooltip as the helplink code.

        I've pushed a fix for this to:

        https://git.luns.net.uk/moodle.git MDL-36752-master-2
        https://git.luns.net.uk/moodle.git MDL-36752-MOODLE_23_STABLE-2

        I've also pushed the Moodle 2.2 branch to
        https://git.luns.net.uk/moodle.git MDL-36752-MOODLE_22_STABLE
        (complete with the same patch)

        Show
        Andrew Nicols added a comment - Having thought about this for a bit, I've gone for the "span.helplink a.tooltip" option because this is the least disruptive and allows us to apply the fix to all stable branches. Longer term, we need to look at updating the scales code to use the same tooltip as the helplink code. I've pushed a fix for this to: https://git.luns.net.uk/moodle.git MDL-36752 -master-2 https://git.luns.net.uk/moodle.git MDL-36752 -MOODLE_23_STABLE-2 I've also pushed the Moodle 2.2 branch to https://git.luns.net.uk/moodle.git MDL-36752 -MOODLE_22_STABLE (complete with the same patch)
        Hide
        Dan Poltawski added a comment -

        I've pulled this in. I haven't pulled in the 2.2 branch though.

        Show
        Dan Poltawski added a comment - I've pulled this in. I haven't pulled in the 2.2 branch though.
        Hide
        Dan Poltawski added a comment -

        I've also pulled the latest integration to moodle.org and has fixed that ugly problem.

        Show
        Dan Poltawski added a comment - I've also pulled the latest integration to moodle.org and has fixed that ugly problem.
        Hide
        Rajesh Taneja added a comment -

        Thanks Andrew,

        Works Great for me.
        Although there is one accessibly problem with this help popup. I can't get to select close button with keyboard. Probably will be nice to have a close option on Esc as well.

        Show
        Rajesh Taneja added a comment - Thanks Andrew, Works Great for me. Although there is one accessibly problem with this help popup. I can't get to select close button with keyboard. Probably will be nice to have a close option on Esc as well.
        Hide
        Andrew Nicols added a comment -

        Hi, Raj

        These aren't regressions with this patch. I should have thought that the close button accessibility would have been improved with MDL-28235. I can tab to the close button, but the tab order is certainly whacky but this is not an effect of this patch.

        The addition of the escape button would also be a further improvement.

        Both of these improvements should be addressed by MDL-35819 in 2.5 (hopefully).

        Show
        Andrew Nicols added a comment - Hi, Raj These aren't regressions with this patch. I should have thought that the close button accessibility would have been improved with MDL-28235 . I can tab to the close button, but the tab order is certainly whacky but this is not an effect of this patch. The addition of the escape button would also be a further improvement. Both of these improvements should be addressed by MDL-35819 in 2.5 (hopefully).
        Hide
        Rajesh Taneja added a comment -

        Thanks Andrew.

        Show
        Rajesh Taneja added a comment - Thanks Andrew.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Y E S !

        Closing as fixed, many thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Y E S ! Closing as fixed, many thanks!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: