Moodle
  1. Moodle
  2. MDL-35819

Rewrite help popup to use moodle-core-notification and restyle

    Details

    • Testing Instructions:
      Hide

      I'd advise having your browsers inspector up for these tests in case of JS issues.

      Note: I've added caching of help strings, however this caching is disabled when you're in developer mode. This is intended to make it easier for people trying to write modules to work on help strings without having to constantly refresh their browser to see how they work/look in situ.
      Some of these tests ask you to confirm that the text was not re-fetched. Obviously if you have developer mode enabled then you will get these help texts returned and the test will look like it's failing (it's not really).

      • Open a course
      • Turn editing on
      • Turn off the activity chooser
      • Click the help icon next to the 'Add a resource...' dropdown
      • Confirm:
        • that text was shown with a spinner to say that the dialogue was loading (you may need to add a sleep(3) into help.php for this)
        • 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
        • that you can drag the dialogue around the screen by the title bar
        • *that the [x] in the top right corner was focused by defualt
      • Click the [x] in the top corner with the keyboard to close the help
        • Confirm that the popup closed
      • Re-open the same help
      • Click the [x] in the top corner with the mouse to close the help
        • Confirm that the popup closed
      • Re-open the same help
      • Confirm:
        • that the dialogue did not inform you that it was loading
        • that the Network activity tab in your browser inspector didn't show a new page fetch
        • that the help loaded relatively quickly
      • Click elsewhere on the page not a link
        • Confirm that the popup closed
      • Re-open the same help
      • Press the escape key
        • Confirm that the popup closed
      • Re-open the same help
      • Click on the help for the 'Add an activity...' dropdown
      • 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
      • Navigate to Site administration -> Users -> Permissions -> Define roles
      • Click on the help on the 'Role' column
      • 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
        • that the 'More help' link is shown correctly in the bottom right-hand corner of the popup
        • that you can tab from the [x] to the 'More help' link
      • Click on the 'More help' link
        • If you have doctonewwindow set (and MDL-35836 has hit master now) then this will open in a popup
        • If yo don't have doctonewwindow set, the page will load in the same window
      • 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 in case of JS issues. Note: I've added caching of help strings, however this caching is disabled when you're in developer mode. This is intended to make it easier for people trying to write modules to work on help strings without having to constantly refresh their browser to see how they work/look in situ. Some of these tests ask you to confirm that the text was not re-fetched. Obviously if you have developer mode enabled then you will get these help texts returned and the test will look like it's failing (it's not really). Open a course Turn editing on Turn off the activity chooser Click the help icon next to the 'Add a resource...' dropdown Confirm: that text was shown with a spinner to say that the dialogue was loading (you may need to add a sleep(3) into help.php for this) 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 that you can drag the dialogue around the screen by the title bar *that the [x] in the top right corner was focused by defualt Click the [x] in the top corner with the keyboard to close the help Confirm that the popup closed Re-open the same help Click the [x] in the top corner with the mouse to close the help Confirm that the popup closed Re-open the same help Confirm: that the dialogue did not inform you that it was loading that the Network activity tab in your browser inspector didn't show a new page fetch that the help loaded relatively quickly Click elsewhere on the page not a link Confirm that the popup closed Re-open the same help Press the escape key Confirm that the popup closed Re-open the same help Click on the help for the 'Add an activity...' dropdown 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 Navigate to Site administration -> Users -> Permissions -> Define roles Click on the help on the 'Role' column 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 that the 'More help' link is shown correctly in the bottom right-hand corner of the popup that you can tab from the [x] to the 'More help' link Click on the 'More help' link If you have doctonewwindow set (and MDL-35836 has hit master now) then this will open in a popup If yo don't have doctonewwindow set, the page will load in the same window 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_23_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
    • Rank:
      44580

      Description

      I've done some initial work to update the help tooltip to style it similarly to other popups and alerts in Moodle.

        Issue Links

          Activity

          Hide
          Andrew Nicols added a comment -

          After looking at the available options for improving the performance here, it seems that most improvements would just be a bit of a hack. As a result, I've rewritten the help popup system to use M.core.dialogue instead of Y.overlay, to improve efficiency, and to reduce page size.

          I haven't removed the old code as it's still used by old_help_icon and in my opinion, they should be removed together in 2.5.

          Because of the use of M.core.dialogue, we also gain stock ARIA support for 2.4. We also benefit from being able move the popup around; and the ability to use the escape key to close the popup. I've also added some caching of help strings already called. Other functionality should remain.

          This probably needs some thoughts on styling but I'll leave that for another issue and for someone who has better clue on the CSS front.

          Note, that the 'More help' link relies on the doctonewwindow rewrite in MDL-35836 so the setting won't be respected until that issue is integrated.

          Also, this is a master only change at present. If the powers that be deem that it should be backported to 2.3, this should be relatively trivial. If not, then we still need to find a solution to the inefficient use of one('#foo').on('click'...) that this encourages.

          Show
          Andrew Nicols added a comment - After looking at the available options for improving the performance here, it seems that most improvements would just be a bit of a hack. As a result, I've rewritten the help popup system to use M.core.dialogue instead of Y.overlay, to improve efficiency, and to reduce page size. I haven't removed the old code as it's still used by old_help_icon and in my opinion, they should be removed together in 2.5. Because of the use of M.core.dialogue, we also gain stock ARIA support for 2.4. We also benefit from being able move the popup around; and the ability to use the escape key to close the popup. I've also added some caching of help strings already called. Other functionality should remain. This probably needs some thoughts on styling but I'll leave that for another issue and for someone who has better clue on the CSS front. Note, that the 'More help' link relies on the doctonewwindow rewrite in MDL-35836 so the setting won't be respected until that issue is integrated. Also, this is a master only change at present. If the powers that be deem that it should be backported to 2.3, this should be relatively trivial. If not, then we still need to find a solution to the inefficient use of one('#foo').on('click'...) that this encourages.
          Hide
          Andrew Nicols added a comment -

          Adding Barbara as a watcher here so she can be aware of any theme changes this may warrant.

          Show
          Andrew Nicols added a comment - Adding Barbara as a watcher here so she can be aware of any theme changes this may warrant.
          Hide
          Andrew Nicols added a comment -

          Added an screenshot to demonstrate the new styling.

          Show
          Andrew Nicols added a comment - Added an screenshot to demonstrate the new styling.
          Hide
          Andrew Nicols added a comment -

          This can be applied without 35836, but I wouldn't recommend it.

          Show
          Andrew Nicols added a comment - This can be applied without 35836, but I wouldn't recommend it.
          Hide
          Tim Hunt added a comment -

          I expressed some amateur options about how this should look: https://moodle.org/local/chatlogs/index.php?conversationid=11237#c387854. Basically I think it should look less like a dialogue and more like a tool-tip. However, I agree with Andrew that we should get an expert like Barbara to do the graphic design.

          Show
          Tim Hunt added a comment - I expressed some amateur options about how this should look: https://moodle.org/local/chatlogs/index.php?conversationid=11237#c387854 . Basically I think it should look less like a dialogue and more like a tool-tip. However, I agree with Andrew that we should get an expert like Barbara to do the graphic design.
          Hide
          Paul Nicholls added a comment -

          Although it's a bit of a change, I actually quite like the change to a dialogue - it just seems to fit a bit better. In my mind, tooltips are for hover events; since these are triggered by clicks rather than hovers, it makes sense (to me) that they ought to be dialogues rather than tooltips.

          That said, I'm sure there are ways in which it could be made to look better (such as tightening up the line spacing in the help text - at the moment, the gaps between lines are huge!), so input from a designer certainly wouldn't be a bad thing.

          Show
          Paul Nicholls added a comment - Although it's a bit of a change, I actually quite like the change to a dialogue - it just seems to fit a bit better. In my mind, tooltips are for hover events; since these are triggered by clicks rather than hovers, it makes sense (to me) that they ought to be dialogues rather than tooltips. That said, I'm sure there are ways in which it could be made to look better (such as tightening up the line spacing in the help text - at the moment, the gaps between lines are huge!), so input from a designer certainly wouldn't be a bad thing.
          Hide
          Andrew Nicols added a comment -

          Heh. The reason it's a large line height is because that's the style Barbara chose for the activity chooser help text. Personally I quite like it - I disliked it at first on the activity chooser but the moment I tried with something less I realised I preferred It with a double line height. Anyhoo, bet left to the designer!

          Show
          Andrew Nicols added a comment - Heh. The reason it's a large line height is because that's the style Barbara chose for the activity chooser help text. Personally I quite like it - I disliked it at first on the activity chooser but the moment I tried with something less I realised I preferred It with a double line height. Anyhoo, bet left to the designer!
          Hide
          Paul Nicholls added a comment -

          Ah. While I'm happy with a larger-than-normal line height, I think the current line height is a bit excessive (in the activity chooser, too) - the gap is actually large enough that it starts to impede readability. I guess I can always override it in our theme, though

          Show
          Paul Nicholls added a comment - Ah. While I'm happy with a larger-than-normal line height, I think the current line height is a bit excessive (in the activity chooser, too) - the gap is actually large enough that it starts to impede readability. I guess I can always override it in our theme, though
          Hide
          Frédéric Massart added a comment -

          Hi Andrew,

          thanks for working on this. Here are a few of my comments, most of them probably just need some feedback and not code change. Also, please forgive me if I misunderstood something, I am currently working on improving my JS skills .

          [Y] Syntax
          [Y] Output
          [Y] Whitespace
          [Y] Language
          [-] Databases
          [Y] Testing
          [Y] Security
          [N] Documentation
          [Y] Git
          [Y] Sanity check

          help.php

          • Line 85: It looks like $completedoclink is not used any more, shouldn't it be added to $output->text? (Is this waiting for MDL-35836?)

          I find it a bit confusing having $OUTPUT and $output as two different variables, at first I thought there was an issue at line 96. Wouldn't it improve readability by using more distinct names?

          javascript-static.js

          We probably won't remove the function before 2.6 (according to http://docs.moodle.org/dev/Deprecation). Also, that might be helpful to add when we actually started deprecating it.

          outputrenderers.php

          • Line 374: Could you justify this require for every page Moodle will load? Would there be a way to load the module only when a popup help is used? (IE: in render_help_icon())

          popuphelp.js

          • Line 78: The comment does not reflect the action
          • Line 86: Would appending '&ajax=1' work in all cases?
          • Line 134: Why setting the class and then adding it? It this common?

          General

          • I noticed that comments are missing some punctuation at the end. (http://docs.moodle.org/dev/Coding_style#Inline_comments)
          • It looks a bit strange having the popup filled with some text explaining that we are loading something, but not having time to read it. Although I can't think of any better solution, exception by just having "loading...", but that's just me.
          • As we just finished an accessibility sprint and it is still fresh in my mind, I wonder if it would be possible to place the popuphelp panel right after the icon the user has clicked. I guess this would help a user which uses the keyboard to browse.
          • Again about accessibility, would the role presentation act as a trigger along with 'aria-haspopup' on the link?
          • The git commit message could use the component name, which I have no idea what it should be here lol. Libraries? (http://docs.moodle.org/dev/Peer_reviewing_checklist#Git)

          Thanks again!

          Fred

          Show
          Frédéric Massart added a comment - Hi Andrew, thanks for working on this. Here are a few of my comments, most of them probably just need some feedback and not code change. Also, please forgive me if I misunderstood something, I am currently working on improving my JS skills . [Y] Syntax [Y] Output [Y] Whitespace [Y] Language [-] Databases [Y] Testing [Y] Security [N] Documentation [Y] Git [Y] Sanity check help.php Line 85: It looks like $completedoclink is not used any more, shouldn't it be added to $output->text? (Is this waiting for MDL-35836 ?) I find it a bit confusing having $OUTPUT and $output as two different variables, at first I thought there was an issue at line 96. Wouldn't it improve readability by using more distinct names? javascript-static.js We probably won't remove the function before 2.6 (according to http://docs.moodle.org/dev/Deprecation ). Also, that might be helpful to add when we actually started deprecating it. outputrenderers.php Line 374: Could you justify this require for every page Moodle will load? Would there be a way to load the module only when a popup help is used? (IE: in render_help_icon()) popuphelp.js Line 78: The comment does not reflect the action Line 86: Would appending '&ajax=1' work in all cases? Line 134: Why setting the class and then adding it? It this common? General I noticed that comments are missing some punctuation at the end. ( http://docs.moodle.org/dev/Coding_style#Inline_comments ) It looks a bit strange having the popup filled with some text explaining that we are loading something, but not having time to read it. Although I can't think of any better solution, exception by just having "loading...", but that's just me. As we just finished an accessibility sprint and it is still fresh in my mind, I wonder if it would be possible to place the popuphelp panel right after the icon the user has clicked. I guess this would help a user which uses the keyboard to browse. Again about accessibility, would the role presentation act as a trigger along with 'aria-haspopup' on the link? The git commit message could use the component name, which I have no idea what it should be here lol. Libraries? ( http://docs.moodle.org/dev/Peer_reviewing_checklist#Git ) Thanks again! Fred
          Hide
          Andrew Nicols added a comment -
          help.php
          • Good catch - I'll ensure that's added to the output
          • I had intended to rename $output to $data, but I seem to have missed doing so. I've done so now
          javascript-static.js
          • Thanks - I've updated this to point to 2.6
          outputrenderers.php
          • We may not need it for every page, but there are pages where we may not need it initially, but JS will add something later. Because we're using a Y.delegate this can happen. That said, it wouldn't be too tricky to adjust this to only load when a help icon is shown on page created from HTML
          popuphelp.js
          • Line 78: I'll adjust this
          • Line 86: Yes - there are always existing parameters. This is the same as the old functionality.
          • Line 134: Thanks for spotting that - removed the extraneous addClass call
          General
          • Will fix the comments
          • We could change this to just 'Loading...' or similar. I'll have an experiment
          • Can you expand upon this? 'Right after the icon'? At present it will be put immedetiately to the right of the icon, but within the constraints of the window
          • I'm not sure on the aria-haspopup. I'm afraid I don't know enough about ARIA myself. The popup is a moodle-core-notification dialogue and these have been expanded to add ARIA support, so I believe that there should be some. If you mean an ARIA field on the morehelp hyperlink, we can add this.
          • I guess the component is AJAX?

          Andrew

          Show
          Andrew Nicols added a comment - help.php Good catch - I'll ensure that's added to the output I had intended to rename $output to $data, but I seem to have missed doing so. I've done so now javascript-static.js Thanks - I've updated this to point to 2.6 outputrenderers.php We may not need it for every page, but there are pages where we may not need it initially, but JS will add something later. Because we're using a Y.delegate this can happen. That said, it wouldn't be too tricky to adjust this to only load when a help icon is shown on page created from HTML popuphelp.js Line 78: I'll adjust this Line 86: Yes - there are always existing parameters. This is the same as the old functionality. Line 134: Thanks for spotting that - removed the extraneous addClass call General Will fix the comments We could change this to just 'Loading...' or similar. I'll have an experiment Can you expand upon this? 'Right after the icon'? At present it will be put immedetiately to the right of the icon, but within the constraints of the window I'm not sure on the aria-haspopup. I'm afraid I don't know enough about ARIA myself. The popup is a moodle-core-notification dialogue and these have been expanded to add ARIA support, so I believe that there should be some. If you mean an ARIA field on the morehelp hyperlink, we can add this. I guess the component is AJAX? Andrew
          Hide
          Frédéric Massart added a comment -

          Hey Andrew,

          Can you expand upon this? 'Right after the icon'? At present it will be put immedetiately to the right of the icon, but within the constraints of the window
          I meant within the DOM itself. Previous reports mentioned that the helppopup was added at the end of the body tag which is not directly positioned next to the element triggering the popup. I don't know if that is possible, just thought that mentioning it would be useful.

          I'm not sure on the aria-haspopup. I'm afraid I don't know enough about ARIA myself. The popup is a moodle-core-notification dialogue and these have been expanded to add ARIA support, so I believe that there should be some. If you mean an ARIA field on the morehelp hyperlink, we can add this.
          I'm afraid I don't know much about that either, but I noticed that the 'role' attribute of the previous help popup was set to 'alert' where as the new one is set to 'presentation'. Maybe the correct way to work on this is to set another aria attribute on the link triggering the popup itself. I can't help much here sorry. If this blocks you, maybe raising another issue to investigate would be useful.

          I guess the component is AJAX?
          I am not sure, integrators will have the best guess on that. I don't think this is blocking anyway.

          Thanks for your fixes and clarifications. Push for integration whenever you're ready. Cheers!

          Show
          Frédéric Massart added a comment - Hey Andrew, Can you expand upon this? 'Right after the icon'? At present it will be put immedetiately to the right of the icon, but within the constraints of the window I meant within the DOM itself. Previous reports mentioned that the helppopup was added at the end of the body tag which is not directly positioned next to the element triggering the popup. I don't know if that is possible, just thought that mentioning it would be useful. I'm not sure on the aria-haspopup. I'm afraid I don't know enough about ARIA myself. The popup is a moodle-core-notification dialogue and these have been expanded to add ARIA support, so I believe that there should be some. If you mean an ARIA field on the morehelp hyperlink, we can add this. I'm afraid I don't know much about that either, but I noticed that the 'role' attribute of the previous help popup was set to 'alert' where as the new one is set to 'presentation'. Maybe the correct way to work on this is to set another aria attribute on the link triggering the popup itself. I can't help much here sorry. If this blocks you, maybe raising another issue to investigate would be useful. I guess the component is AJAX? I am not sure, integrators will have the best guess on that. I don't think this is blocking anyway. Thanks for your fixes and clarifications. Push for integration whenever you're ready. Cheers!
          Hide
          Andrew Nicols added a comment -

          I've updates this taking most of your comments on board:

          • For the moment I've not moved the help popup within the DOM because this has an effect on theme and styling of it. If we switch to using reset-context then this may be more appropriate but I'm not 100% sure if this will help or not
          • I've added an aria role of definition - this seems more appropriate than dialog
          • I've chosen not to only load this on those pages with help. A majority of pages already do have at least one popup help tooltip included at the bottom of the page. In addition, it's likely that changes we'll make in the future to ajaxify moodle are likely could include help anyway.
          Show
          Andrew Nicols added a comment - I've updates this taking most of your comments on board: For the moment I've not moved the help popup within the DOM because this has an effect on theme and styling of it. If we switch to using reset-context then this may be more appropriate but I'm not 100% sure if this will help or not I've added an aria role of definition - this seems more appropriate than dialog I've chosen not to only load this on those pages with help. A majority of pages already do have at least one popup help tooltip included at the bottom of the page. In addition, it's likely that changes we'll make in the future to ajaxify moodle are likely could include help anyway.
          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
          Andrew Nicols added a comment -

          Rebased and ready to go.

          Show
          Andrew Nicols added a comment - Rebased and ready to go.
          Hide
          Martin Dougiamas added a comment -

          Just had a look at this ... it looks nice and I'm glad there's apparently a performance improvement but a big problem is that we have a regression in behaviour from 2.3 from a keyboard control point of view (not all the fault of this patch).

          If you look at 2.3 you'll see that the help icon can be launched by keyboard easily, then the X is focussed, so that it can be closed easily. You can also easily navigate to links in the help content and launch them. The X should have alt text too.

          All of this needs to still work in 2.4. It would be good if they could be fixed in this patch, and if so I would give it my +1.

          Show
          Martin Dougiamas added a comment - Just had a look at this ... it looks nice and I'm glad there's apparently a performance improvement but a big problem is that we have a regression in behaviour from 2.3 from a keyboard control point of view (not all the fault of this patch). If you look at 2.3 you'll see that the help icon can be launched by keyboard easily, then the X is focussed, so that it can be closed easily. You can also easily navigate to links in the help content and launch them. The X should have alt text too. All of this needs to still work in 2.4. It would be good if they could be fixed in this patch, and if so I would give it my +1.
          Hide
          Dan Poltawski added a comment -

          I'm reopening this for the time being. Andrew: if this looks feasible to achieve with the new approach, then we can submit again.

          If restoring the existing keyboard behaviour is too much work then I think we need to open a new bug for the keyboard regressions in current popup.

          Show
          Dan Poltawski added a comment - I'm reopening this for the time being. Andrew: if this looks feasible to achieve with the new approach, then we can submit again. If restoring the existing keyboard behaviour is too much work then I think we need to open a new bug for the keyboard regressions in current popup.
          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
          Andrew Nicols added a comment -

          I discovered this the other day when rebasing against weekly master and my brain went Doh!

          Initially I thought about applying this straight to the new help popup, but after thinking about it, I think that we should apply this to the moodle-core-notification dialogue class that this extends. This would then benefit all popups which use those classes in moodle (enrol selector, warnings, notifications, errors, activity chooser, glossary auto-linker, community block, backup cancel/confirm).

          It should still be possible to tab to open the new help popup but as you say, it won't focus on there and the X is not a link so cannot be clicked to close the popup. It's also not easy to tab to any links included within the popup (e.g. More help) and this also needs to be addressed.

          I think that it should be possible to convert the X (currently a div) to an anchor such that it can have a tab index, or to use the YUI3 FocusManager to handle focus correctly throughout the dialogue.

          We've also added some other ways to close the help popup in particular:

          • the escape key
          • clicking elsewhere on the page

          I'll try and take a look at this further today.

          Show
          Andrew Nicols added a comment - I discovered this the other day when rebasing against weekly master and my brain went Doh! Initially I thought about applying this straight to the new help popup, but after thinking about it, I think that we should apply this to the moodle-core-notification dialogue class that this extends. This would then benefit all popups which use those classes in moodle (enrol selector, warnings, notifications, errors, activity chooser, glossary auto-linker, community block, backup cancel/confirm). It should still be possible to tab to open the new help popup but as you say, it won't focus on there and the X is not a link so cannot be clicked to close the popup. It's also not easy to tab to any links included within the popup (e.g. More help) and this also needs to be addressed. I think that it should be possible to convert the X (currently a div) to an anchor such that it can have a tab index, or to use the YUI3 FocusManager to handle focus correctly throughout the dialogue. We've also added some other ways to close the help popup in particular: the escape key clicking elsewhere on the page I'll try and take a look at this further today.
          Hide
          Andrew Nicols added a comment -

          I've updated the master branch to the change in MDL-36327 which adds close button keyboard nav support.

          Show
          Andrew Nicols added a comment - I've updated the master branch to the change in MDL-36327 which adds close button keyboard nav support.
          Hide
          Andrew Nicols added a comment -

          Apparently this is already being addressed by an issue in the PR queue.

          Show
          Andrew Nicols added a comment - Apparently this is already being addressed by an issue in the PR queue.
          Hide
          Andrew Nicols added a comment -

          I've tried to take into account the various suggestions that have been made regarding this:

          • keyboard navigation should be restored
          • the X is now focused initially

          This should work in a backwards-compatible fashion.

          Show
          Andrew Nicols added a comment - I've tried to take into account the various suggestions that have been made regarding this: keyboard navigation should be restored the X is now focused initially This should work in a backwards-compatible fashion.
          Hide
          Frédéric Massart added a comment - - edited

          Thanks Andrew, I'll be happy to see that landing! It brings consistency over Moodle look and feel. \o/ Please find some comments below.

          • When the text to be displayed is really long (or the height of browser window is too small), the text can't be read to the end. Perhaps a scrollbar such as in the Activity Chooser would do.
          • I am pinging Barbara to gather her feedback on the line-height and general look and feel of the popup.
          • lib/outputrenderers.php: Could you comment on the use of helpicon on the <a> tag? I'm not too keen on this, as it's not an icon but a link, and because the name is really generic (and there are already enough confusion with the class icon lol). If it's needed as a selector, I'd suggest using the parent helplink: .helplink > a
          • javascript-static.js: The code would be removed in 2.7 as we are deprecating it in 2.5.
          • upgrade.txt: Could you add a little more information on how to upgrade the code? Does it need a lot of rewriting or just changing the module name would do?

          lib/yui/popuphelp/popuphelp.js

          • Line 31: I noticed we defined constants for selectors, I like that idea as they're all defined somewhere, even if only used one. Could you use a constant for that selector as well? Also, I'm not sure to understand why we're using tooltip at all, even if you leave tooltip in the <a> tag, perhaps the selectors could ignore it. If dangerous ignore me.
          • Line 130: According to the accessibility documentation, it is preferable to avoid having similar title and alt on the same tag. I'd suggest you to move the title information on the link instead of the <img>.
          • Line 145: Silently catching the exception could make it harder to track down issues, would that be a problem to render them? Or at least display them somehow? Even if we rely on the debug status.

          Cheers,
          Fred

          (Attaching helppopup and helppopup2 as preview of the actual look and feel)

          Show
          Frédéric Massart added a comment - - edited Thanks Andrew, I'll be happy to see that landing! It brings consistency over Moodle look and feel. \o/ Please find some comments below. When the text to be displayed is really long (or the height of browser window is too small), the text can't be read to the end. Perhaps a scrollbar such as in the Activity Chooser would do. I am pinging Barbara to gather her feedback on the line-height and general look and feel of the popup. lib/outputrenderers.php: Could you comment on the use of helpicon on the <a> tag? I'm not too keen on this, as it's not an icon but a link, and because the name is really generic (and there are already enough confusion with the class icon lol). If it's needed as a selector, I'd suggest using the parent helplink : .helplink > a javascript-static.js: The code would be removed in 2.7 as we are deprecating it in 2.5. upgrade.txt: Could you add a little more information on how to upgrade the code? Does it need a lot of rewriting or just changing the module name would do? lib/yui/popuphelp/popuphelp.js Line 31: I noticed we defined constants for selectors, I like that idea as they're all defined somewhere, even if only used one. Could you use a constant for that selector as well? Also, I'm not sure to understand why we're using tooltip at all, even if you leave tooltip in the <a> tag, perhaps the selectors could ignore it. If dangerous ignore me. Line 130: According to the accessibility documentation, it is preferable to avoid having similar title and alt on the same tag. I'd suggest you to move the title information on the link instead of the <img>. Line 145: Silently catching the exception could make it harder to track down issues, would that be a problem to render them? Or at least display them somehow? Even if we rely on the debug status. Cheers, Fred (Attaching helppopup and helppopup2 as preview of the actual look and feel)
          Hide
          Frédéric Massart added a comment - - edited

          As per my chat with Barbara, here are the conclusions to make this consistent with the rest:

          • The size of the content of the body should be 12px;
          • The line-height of the content of the body should be 2em (as it is now);
          • The icon for "More help" should lose the iconhelp class in benefit of icon and icon-pre (hence #panelhelp .moodle-dialogue-ft a img is not required any more);
          • "More help" should be sized 12px as well;
          • theme/base/style/core.css:1094 padding is set twice;
          • theme/base/style/core.css:1105 is this rule really helpful? #panelhelp .moodle-dialogue-ft a. It does not seem to change when disabled;
          • theme/base/style/core.css:1099 if you're forcing the alignment to the right, you should invert it on RTL (see .dir-rtl rules).

          In the future we should enforce those with generic CSS rules on moodle-dialogue but we can probably ignore that for now to help this issue being integrated.

          Show
          Frédéric Massart added a comment - - edited As per my chat with Barbara, here are the conclusions to make this consistent with the rest: The size of the content of the body should be 12px; The line-height of the content of the body should be 2em (as it is now); The icon for "More help" should lose the iconhelp class in benefit of icon and icon-pre (hence #panelhelp .moodle-dialogue-ft a img is not required any more); "More help" should be sized 12px as well; theme/base/style/core.css:1094 padding is set twice; theme/base/style/core.css:1105 is this rule really helpful? #panelhelp .moodle-dialogue-ft a . It does not seem to change when disabled; theme/base/style/core.css:1099 if you're forcing the alignment to the right, you should invert it on RTL (see .dir-rtl rules). In the future we should enforce those with generic CSS rules on moodle-dialogue but we can probably ignore that for now to help this issue being integrated.
          Hide
          Andrew Nicols added a comment -

          I'm still working on and tweaking this. I'm trying to make this into a re-usable component such that it an be used for things like:

          • glossary definition display; and
          • the forum vote popup.

          I've also opened a separate issue to generalise the look and feel of all moodle dialogues to reduce the amount of CSS duplication at play.

          With regards your comments:

          • set the overflow to add the scroll bars. At present I've set a fixed max-height value. The resizing system built into the chooser dialogue needs to be split out and turned into a widget component before we can do this more dynamically. I'm also looking to add the ability to resize the dialogue but I think that this should be done as a separate change as it requires further thought.
          • dropped helpicon as you suggest. Using .helplink > a instead.
          • updated the deprecation notice
          • will update the upgrade.txt. It's not a huge difference on the whole
          • added the constants for both CSS, and seleectors (they are different things);
          • not made a change to the title of the link. I'd like to confirm that this is the right thing to do (I think it is). Specifically, do we remove the title or the alt (confused by your wording, sorry)
          • have adjusted exception handling to show an error

          Re the styling, I've taken on board everything Barbara has suggested and I'm trying to roll it into MDL-37645.

          Once I've got the final few bits of the style issue completed and integrated, I just need to:

          • remove that alt or title
          • set a max-height on the tooltip (suggestions appreciated thanks)
          Show
          Andrew Nicols added a comment - I'm still working on and tweaking this. I'm trying to make this into a re-usable component such that it an be used for things like: glossary definition display; and the forum vote popup. I've also opened a separate issue to generalise the look and feel of all moodle dialogues to reduce the amount of CSS duplication at play. With regards your comments: set the overflow to add the scroll bars. At present I've set a fixed max-height value. The resizing system built into the chooser dialogue needs to be split out and turned into a widget component before we can do this more dynamically. I'm also looking to add the ability to resize the dialogue but I think that this should be done as a separate change as it requires further thought. dropped helpicon as you suggest. Using .helplink > a instead. updated the deprecation notice will update the upgrade.txt. It's not a huge difference on the whole added the constants for both CSS, and seleectors (they are different things); not made a change to the title of the link. I'd like to confirm that this is the right thing to do (I think it is). Specifically, do we remove the title or the alt (confused by your wording, sorry) have adjusted exception handling to show an error Re the styling, I've taken on board everything Barbara has suggested and I'm trying to roll it into MDL-37645 . Once I've got the final few bits of the style issue completed and integrated, I just need to: remove that alt or title set a max-height on the tooltip (suggestions appreciated thanks)
          Hide
          Andrew Nicols added a comment -

          The only thing I haven't done is to remove the alt/title text. I'm in the process of discussing a similar issue in MDL-37574 and I think that it should perhaps be a separate change in a new issue which is backported to stable branches.

          As I mentioned before, this is a complete rewrite to offer reusable components (M.core.tooltip).

          Show
          Andrew Nicols added a comment - The only thing I haven't done is to remove the alt/title text. I'm in the process of discussing a similar issue in MDL-37574 and I think that it should perhaps be a separate change in a new issue which is backported to stable branches. As I mentioned before, this is a complete rewrite to offer reusable components (M.core.tooltip).
          Hide
          Andrew Nicols added a comment -

          Another example of one of the new help popups.

          Show
          Andrew Nicols added a comment - Another example of one of the new help popups.
          Hide
          Andrew Nicols added a comment -

          I've just force pushed with some minor alterations to address an accessibility issue (duplicating the 'More help' text in the title, alt, and link text).

          Show
          Andrew Nicols added a comment - I've just force pushed with some minor alterations to address an accessibility issue (duplicating the 'More help' text in the title, alt, and link text).
          Hide
          Frédéric Massart added a comment - - edited

          Hi Andrew,

          I think this has been through quite a lot of iterations now, so after a quick look, here are few minor comments:

          • Should the CSS rule .tooltiptext be a bit less generic? As in .moodle-dialogue .tooltiptext? Just wondering.
          • You have some 'function (e)' which whould be function(e).
          • Do we have a coding style for the objects {foo: 'bar', bar: 'foo'}

            ?

            • Should the colon follow the variable name without whitespace (except when aligning the colon on multiple lines)?
            • Should we add quotes to the variable name? Some places have quotes, some don't.
          • You could make use of html_writer in help.php. I know you're not introducing this, but as you're editing those lines...

          Nothing really problematic here, feel free to push for integration whenever ready. Great work !
          Fred

          Show
          Frédéric Massart added a comment - - edited Hi Andrew, I think this has been through quite a lot of iterations now, so after a quick look, here are few minor comments: Should the CSS rule .tooltiptext be a bit less generic? As in .moodle-dialogue .tooltiptext? Just wondering. You have some 'function (e)' which whould be function(e). Do we have a coding style for the objects {foo: 'bar', bar: 'foo'} ? Should the colon follow the variable name without whitespace (except when aligning the colon on multiple lines)? Should we add quotes to the variable name? Some places have quotes, some don't. You could make use of html_writer in help.php. I know you're not introducing this, but as you're editing those lines... Nothing really problematic here, feel free to push for integration whenever ready. Great work ! Fred
          Hide
          Andrew Nicols added a comment -

          Thanks Fred,

          I've changed the CSS as you suggest - should have done so in the first place really.
          I've changed function (e) to function(e). I don't know whether this can be checked with jshint somehow.
          We don't have a coding style, but I've been trying to follow the YUI coding style where possible. They use:

          {
              foo: 'bar',
              baz: 'bum'
          }
          

          and I'm inclined to stick with that.

          I've changed to use html_writer in help.php. I think that it's to make these kinds of improvements if we're in the right neck of the woods.

          Andrew

          Show
          Andrew Nicols added a comment - Thanks Fred, I've changed the CSS as you suggest - should have done so in the first place really. I've changed function (e) to function(e). I don't know whether this can be checked with jshint somehow. We don't have a coding style, but I've been trying to follow the YUI coding style where possible. They use: { foo: 'bar', baz: 'bum' } and I'm inclined to stick with that. I've changed to use html_writer in help.php. I think that it's to make these kinds of improvements if we're in the right neck of the woods. Andrew
          Hide
          Andrew Nicols added a comment -

          Monsieur le integrator: I've based this commit on MDL-37645 and I'd advise integrating that issue first.

          Show
          Andrew Nicols added a comment - Monsieur le integrator: I've based this commit on MDL-37645 and I'd advise integrating that issue first.
          Hide
          Tim Hunt added a comment -

          I codechecker checks js files as well as PHP files, however, there is a blanket exclusion on lib/yui, since the core YUI libraries don't follow our coding style. I created MDLSITE-2112 for Eloy.

          Show
          Tim Hunt added a comment - I codechecker checks js files as well as PHP files, however, there is a blanket exclusion on lib/yui, since the core YUI libraries don't follow our coding style. I created MDLSITE-2112 for Eloy.
          Hide
          Dan Poltawski added a comment -

          Hi Andrew,

          I'm reopening this sorry, but in testing the 'help popup' from the network, I got a js syntax error. See "Screen Shot 2013-02-04 at 10.54.27.png"

          1. Login as admin
          2. Create a forum in a course
          3. Set the rating to count of ratings
          4. Set the scale to something (e.g. seperated and connected ways of knowing)
          5. Login as a student
          6. Post in the forum
          7. Login as admin
          8. Note the 'Count of ratings' rating popup and click the help button
          9. Get 'Please wait while we get the help you requested'.. then Syntax error popup with 'Unable to retrieve the requested content. The following error was returned: Unexpected token <"

          Not that the help popup does open in a new window.

          Also, just a tiny thing for my 2 cents. Maybe it is because I got that error, but I thought that the 'Please wait' message was too obvious. (I mean in terms of perception, sometimes if you say 'please wait' it reminds you that you are waiting).

          Show
          Dan Poltawski added a comment - Hi Andrew, I'm reopening this sorry, but in testing the 'help popup' from the network, I got a js syntax error. See "Screen Shot 2013-02-04 at 10.54.27.png" Login as admin Create a forum in a course Set the rating to count of ratings Set the scale to something (e.g. seperated and connected ways of knowing) Login as a student Post in the forum Login as admin Note the 'Count of ratings' rating popup and click the help button Get 'Please wait while we get the help you requested'.. then Syntax error popup with 'Unable to retrieve the requested content. The following error was returned: Unexpected token <" Not that the help popup does open in a new window. Also, just a tiny thing for my 2 cents. Maybe it is because I got that error, but I thought that the 'Please wait' message was too obvious. (I mean in terms of perception, sometimes if you say 'please wait' it reminds you that you are waiting).
          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
          Andrew Nicols added a comment -

          Thanks Dan,

          Well, would appreciate some views on how best to fix this.

          This happens because the 'helplink' class is (over?)used by a variety of things, including the Scales. There are a couple of options to fixing this:

          Option 1 - change the class + CSS selector

          The easy solution is to change the class on the <span> that we're targetting - perhaps to helptooltip or helplinktooltip. This is a very minor change and solves the problem for all instances. However, this does mean that we're then changing the selector which has been in use for a very long time and that this may irritate our theme designers. As an example, we'd end up with:

          {span}
          <span class="helptooltip">
          <a href="http://tangerine.local/moodle/help.php?component=moodle&identifier=search&lang=en" title="Help with Search" aria-haspopup="true">
          <img src="http://tangerine.local/moodle/theme/image.php?theme=standard&component=core&image=help" alt="Help with Search" class="iconhelp" />
          </a>
          </span>{span}

          Option 2 - Add an additional class and target both

          An equally easy solution is to add an additional class to the same span. This should maintain backwards compatibility for theme designers and means that we are using something fairly specific (which is a good thing really). If doing this, we should avoid using the overly generic 'tooltip' - perhaps helptooltip.

          We could either target just the new selector:
          .helptooltip > a

          Or target both in combination:
          .helplink.helptooltip > a

          I think it would be best to target both together as this would encourage developers to use .helplink to get the relevant style.

          {span}
          <span class="helplink helptooltip">
          <a href="http://tangerine.local/moodle/help.php?component=moodle&identifier=search&lang=en" title="Help with Search" aria-haspopup="true">
          <img src="http://tangerine.local/moodle/theme/image.php?theme=standard&component=core&image=help" alt="Help with Search" class="iconhelp" />
          </a>
          </span>{span}

          Option 3 - Rewrite the world

          The final solution would be to update all of the locations that the existing 'helplink' class is used in Moodle.
          This is achievable for core, but will probably really upset lots of third-party developers who may have information which doesn't fit well in the style of popup.

          In writing the above, I think option 2b is the best option. Frédéric Massart had some views on the classes we use when I first wrote this - do you have any views on how best to tackle this?

          Incidentally, the Scales popup was raised in MDL-36387 because it is currently slightly broken in other ways and my proposal was to switch to using the new tooltip system.

          WRT the 'Please wait' message, do you have any suggestions? I've changed it at present to 'Loading...' but I'm not sure that this is much better. I think it's wise to have some form of message because there may be occasions where a help text is slow to load. Helen Foster, do you have any specific views on the language used? It's presently:
          "Please wait while we load the help you requested..."

          Thanks in advance all,

          Andrew

          Show
          Andrew Nicols added a comment - Thanks Dan, Well, would appreciate some views on how best to fix this. This happens because the 'helplink' class is (over?)used by a variety of things, including the Scales. There are a couple of options to fixing this: Option 1 - change the class + CSS selector The easy solution is to change the class on the <span> that we're targetting - perhaps to helptooltip or helplinktooltip. This is a very minor change and solves the problem for all instances. However, this does mean that we're then changing the selector which has been in use for a very long time and that this may irritate our theme designers. As an example, we'd end up with: {span} <span class="helptooltip"> <a href="http://tangerine.local/moodle/help.php?component=moodle&identifier=search&lang=en" title="Help with Search" aria-haspopup="true"> <img src="http://tangerine.local/moodle/theme/image.php?theme=standard&component=core&image=help" alt="Help with Search" class="iconhelp" /> </a> </span>{span} Option 2 - Add an additional class and target both An equally easy solution is to add an additional class to the same span. This should maintain backwards compatibility for theme designers and means that we are using something fairly specific (which is a good thing really). If doing this, we should avoid using the overly generic 'tooltip' - perhaps helptooltip. We could either target just the new selector: .helptooltip > a Or target both in combination: .helplink.helptooltip > a I think it would be best to target both together as this would encourage developers to use .helplink to get the relevant style. {span} <span class="helplink helptooltip"> <a href="http://tangerine.local/moodle/help.php?component=moodle&identifier=search&lang=en" title="Help with Search" aria-haspopup="true"> <img src="http://tangerine.local/moodle/theme/image.php?theme=standard&component=core&image=help" alt="Help with Search" class="iconhelp" /> </a> </span>{span} Option 3 - Rewrite the world The final solution would be to update all of the locations that the existing 'helplink' class is used in Moodle. This is achievable for core, but will probably really upset lots of third-party developers who may have information which doesn't fit well in the style of popup. In writing the above, I think option 2b is the best option. Frédéric Massart had some views on the classes we use when I first wrote this - do you have any views on how best to tackle this? Incidentally, the Scales popup was raised in MDL-36387 because it is currently slightly broken in other ways and my proposal was to switch to using the new tooltip system. WRT the 'Please wait' message, do you have any suggestions? I've changed it at present to 'Loading...' but I'm not sure that this is much better. I think it's wise to have some form of message because there may be occasions where a help text is slow to load. Helen Foster , do you have any specific views on the language used? It's presently: "Please wait while we load the help you requested..." Thanks in advance all, Andrew
          Hide
          Frédéric Massart added a comment -

          I'd give my +1 to Option 2 as Option 3 is difficult to achieve.

          What I'd suggest is to use a new selector such as span.helppopup > a (without references to helplink) which matches the module name (as the selector is set in helppopup.js, I don't see any reason to use tooltip). And whilst keeping helplink, I'd add some information in theme/upgrade.txt to inform that ideally we should not use that class anymore to style the help popup link and that it could be removed in the future. Also you'd update the current themes to use the right class where need be.

          Show
          Frédéric Massart added a comment - I'd give my +1 to Option 2 as Option 3 is difficult to achieve. What I'd suggest is to use a new selector such as span.helppopup > a (without references to helplink ) which matches the module name (as the selector is set in helppopup.js , I don't see any reason to use tooltip ). And whilst keeping helplink , I'd add some information in theme/upgrade.txt to inform that ideally we should not use that class anymore to style the help popup link and that it could be removed in the future. Also you'd update the current themes to use the right class where need be.
          Hide
          Helen Foster added a comment -

          Regarding the 'Please wait' message, I think if the loading icon (a spinner) is displayed, then we don't need any text. (Trying to keep things as simple as possible.)

          Show
          Helen Foster added a comment - Regarding the 'Please wait' message, I think if the loading icon (a spinner) is displayed, then we don't need any text. (Trying to keep things as simple as possible.)
          Hide
          Andrew Nicols added a comment -

          I've made the above changes and I think that should solve these issues.
          Putting back up for integration.

          Show
          Andrew Nicols added a comment - I've made the above changes and I think that should solve these issues. Putting back up for integration.
          Hide
          Damyon Wiese 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.

          Cheers!

          Show
          Damyon Wiese 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. Cheers!
          Hide
          Andrew Nicols added a comment -

          Rebased.

          Show
          Andrew Nicols added a comment - Rebased.
          Hide
          Dan Poltawski added a comment -

          Integrated to master, thanks!

          Show
          Dan Poltawski added a comment - Integrated to master, thanks!
          Hide
          Damyon Wiese added a comment -

          Not looked closely to see if this issue is the cause - but I just noticed something odd on the Upload Users page. Clicking help opens the help popup - but the arrow for the drag and drop file upload appears on top of the help window. Will add screenshot.

          Show
          Damyon Wiese added a comment - Not looked closely to see if this issue is the cause - but I just noticed something odd on the Upload Users page. Clicking help opens the help popup - but the arrow for the drag and drop file upload appears on top of the help window. Will add screenshot.
          Hide
          Damyon Wiese added a comment -

          Oddity in upload users page.

          User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.11 (KHTML, like Gecko) Chrome/23.0.1271.97 Safari/537.11

          Show
          Damyon Wiese added a comment - Oddity in upload users page. User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.11 (KHTML, like Gecko) Chrome/23.0.1271.97 Safari/537.11
          Hide
          Frédéric Massart added a comment -

          Aaaah, playing with z-index.

          Show
          Frédéric Massart added a comment - Aaaah, playing with z-index.
          Hide
          Andrew Nicols added a comment -

          Looks like the culprit is a z-index of 3000 on .filepicker-filelist .filepicker-container, .filemanager.fm-noitems .fm-empty-container

          This is set in theme/base/style/filemanager.css and came from MDL-33218

          No idea why the the filepicker container needs such a high z-index...

          Show
          Andrew Nicols added a comment - Looks like the culprit is a z-index of 3000 on .filepicker-filelist .filepicker-container, .filemanager.fm-noitems .fm-empty-container This is set in theme/base/style/filemanager.css and came from MDL-33218 No idea why the the filepicker container needs such a high z-index...
          Hide
          Andrew Davis added a comment -

          Works as described. Passing.

          Show
          Andrew Davis added a comment - Works as described. Passing.
          Hide
          Damyon Wiese added a comment -

          Congratulations this fix has been added to Moodle!

          You may want to dedicate this issue to someone special on this Valentines day.

          Thanks!

          Show
          Damyon Wiese added a comment - Congratulations this fix has been added to Moodle! You may want to dedicate this issue to someone special on this Valentines day. Thanks!

            People

            • Votes:
              12 Vote for this issue
              Watchers:
              16 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: