Details

    • Testing Instructions:
      Hide
      1. As an admin, navigate to a course settings
      2. Make sure the help buttons next to some labels open in a div in the same page (does not follow the link)
      3. Make sure the help box can be closed by clicking outside the box
      4. Make sure the help box can be closed by clicking the 'Close' button
      5. Make sure there are no JS errors in the console.

      Bonus points

      • Test other forms with help buttons
      Show
      As an admin, navigate to a course settings Make sure the help buttons next to some labels open in a div in the same page (does not follow the link) Make sure the help box can be closed by clicking outside the box Make sure the help box can be closed by clicking the 'Close' button Make sure there are no JS errors in the console. Bonus points Test other forms with help buttons
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-28235-master
    • Rank:
      16855

      Description

      When a tool tip pop ups open the screen reader says 'link graphic theme slash image', which refers to the close buttons on the tool tips, but the screen reader is reading the file name as there is no alt text. We need to add alt text to each of the close buttons to read 'close help pop up'.

      Also, the screen reader reads the text of a help popup ok, but the user then has to navigate backwards to close the pop up. Please add a button after the text, as an extra means of closing the tool tip. Text for the button should be 'close', and the alt text for the button should read 'close help pop up'.

        Issue Links

          Activity

          Hide
          Frédéric Massart added a comment -

          Here are my patches. I have added the close button by default in the help boxes, and some alt text on the 'close' icon.

          I first thought that it would be better not to display the close button to non-accessibility required users, but finally that does not seem disturbing at all.

          Show
          Frédéric Massart added a comment - Here are my patches. I have added the close button by default in the help boxes, and some alt text on the 'close' icon. I first thought that it would be better not to display the close button to non-accessibility required users, but finally that does not seem disturbing at all.
          Hide
          Dan Poltawski added a comment -

          Hi Fred,

          Generally looks ok, but you are not aware of $PAGE->requires->strings_for_js()

          (Which I think populates M.str IIRC)

          Show
          Dan Poltawski added a comment - Hi Fred, Generally looks ok, but you are not aware of $PAGE->requires->strings_for_js() (Which I think populates M.str IIRC)
          Hide
          Frédéric Massart added a comment -

          Thanks Dan. I didn't know about that method, I have amended my commit and am pushing for integration. Cheers!

          Show
          Frédéric Massart added a comment - Thanks Dan. I didn't know about that method, I have amended my commit and am pushing for integration. Cheers!
          Hide
          Dan Poltawski added a comment -

          Tsh. I didn't properly review this yet, I pointed out the js strings intially. Reopening this whilst i've not reviewed this issue properly.

          Show
          Dan Poltawski added a comment - Tsh. I didn't properly review this yet, I pointed out the js strings intially. Reopening this whilst i've not reviewed this issue properly.
          Hide
          Frédéric Massart added a comment -

          Changing state to 'Waiting for peer review'

          Show
          Frédéric Massart added a comment - Changing state to 'Waiting for peer review'
          Hide
          Dan Poltawski added a comment -

          +1, the escaping is an interesting issue, which Fred discussed here: http://moodle.org/local/chatlogs/index.php?conversationid=11042#c380741

          Show
          Dan Poltawski added a comment - +1, the escaping is an interesting issue, which Fred discussed here: http://moodle.org/local/chatlogs/index.php?conversationid=11042#c380741
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Greg Kraus added a comment -

          Is it possible to insert the pop-up help box into the DOM at the location where the help link is or does it have to be inserted at the end of the DOM?

          Show
          Greg Kraus added a comment - Is it possible to insert the pop-up help box into the DOM at the location where the help link is or does it have to be inserted at the end of the DOM?
          Hide
          Frédéric Massart added a comment -

          Nothing is impossible, but I had a look at it and it requires to rewrite part of the help box system. First of all, the styles within the popup have to be strongly defined as all the parent nodes define others (I experienced the issue), that's not a big deal though. The other problem is that the help class creates a DOM element once and only and reuse it from where it is. Changing this behaviour would require to rewrite part of the implementation and would only benefit 2.4 onwards users. Is this something that is highly recommended knowing that MDL-30847 introduces ARIA attributes on those popups?

          Show
          Frédéric Massart added a comment - Nothing is impossible, but I had a look at it and it requires to rewrite part of the help box system. First of all, the styles within the popup have to be strongly defined as all the parent nodes define others (I experienced the issue), that's not a big deal though. The other problem is that the help class creates a DOM element once and only and reuse it from where it is. Changing this behaviour would require to rewrite part of the implementation and would only benefit 2.4 onwards users. Is this something that is highly recommended knowing that MDL-30847 introduces ARIA attributes on those popups?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

          This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

          This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

          Apologises for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologises for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Sam Hemelryk added a comment -

          Hi Fred,

          I'm reopening this sorry.

          I've just been discussing it with Eloy to check his opinion and have agreed that having two close buttons like this is not a great solution.
          Certainly its more usable for screen readers and the likes but it just doesn't look right to those who can see the two buttons and adds undue complexity to that simple interface.
          I think a better solution would be to keep just one button and make sure it is more accessible for those with screen readers, trying for the best of both worlds really.
          In discussing with Eloy we considered keeping just the top button but moving its HTML to the bottom and the repositioning it back up top right in CSS, and alternatively keeping the bottom button only.
          We both thought keeping the top right button but moving its HTML and repositioning it back there would be the way to go.

          Also just noting I don't think the new string is needed. Surely close would be enough. The user of the screen reader triggered the help box and is aware of what is being read to them hopefully, as the normal user also triggered it manually and should be aware of where they are.

          Feel free to discuss etc.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Fred, I'm reopening this sorry. I've just been discussing it with Eloy to check his opinion and have agreed that having two close buttons like this is not a great solution. Certainly its more usable for screen readers and the likes but it just doesn't look right to those who can see the two buttons and adds undue complexity to that simple interface. I think a better solution would be to keep just one button and make sure it is more accessible for those with screen readers, trying for the best of both worlds really. In discussing with Eloy we considered keeping just the top button but moving its HTML to the bottom and the repositioning it back up top right in CSS, and alternatively keeping the bottom button only. We both thought keeping the top right button but moving its HTML and repositioning it back there would be the way to go. Also just noting I don't think the new string is needed. Surely close would be enough. The user of the screen reader triggered the help box and is aware of what is being read to them hopefully, as the normal user also triggered it manually and should be aware of where they are. Feel free to discuss etc. Many thanks Sam
          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
          Dan Poltawski added a comment -

          Sorry, I missed the dual buttons in my review, but just to agree with the comments about that, I don't think two buttons is good solution despite what my +1 said.

          Also, worth considering that (in reality, rather than the ideal world withe everything is instantly translated) I think that introducing a new string limits the 'accessibility' for non-english speakers, as it'll probably result in a non-translated string.

          Show
          Dan Poltawski added a comment - Sorry, I missed the dual buttons in my review, but just to agree with the comments about that, I don't think two buttons is good solution despite what my +1 said. Also, worth considering that (in reality, rather than the ideal world withe everything is instantly translated) I think that introducing a new string limits the 'accessibility' for non-english speakers, as it'll probably result in a non-translated string.
          Hide
          Frédéric Massart added a comment -

          Hi guys,

          I am pushing this for second review.

          About the HTML repositioning of the close icon, I thought that it wasn't worth doing some funky CSS to have the icon semantically after the text but visible before. Especially knowing (from comments on other issues) that this kind of small icons are not always easy to be targeted by impaired users.

          What I have done:

          • I kept the close button on the bottom.
          • I removed the new string to use a already translated one, although I had to use an AMOS script to copy it.
          • I have added a separate commit to remove the close icon completely if necessary.

          This makes it consistent with the popup displayed by the glossary entry filter.

          Let me know what you think.

          Cheers!

          Show
          Frédéric Massart added a comment - Hi guys, I am pushing this for second review. About the HTML repositioning of the close icon, I thought that it wasn't worth doing some funky CSS to have the icon semantically after the text but visible before. Especially knowing (from comments on other issues) that this kind of small icons are not always easy to be targeted by impaired users. What I have done: I kept the close button on the bottom. I removed the new string to use a already translated one, although I had to use an AMOS script to copy it. I have added a separate commit to remove the close icon completely if necessary. This makes it consistent with the popup displayed by the glossary entry filter. Let me know what you think. Cheers!
          Hide
          Frédéric Massart added a comment -

          Removing Dan from peer reviewer as he's away this week.

          Show
          Frédéric Massart added a comment - Removing Dan from peer reviewer as he's away this week.
          Hide
          Mark Nelson added a comment - - edited

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

          Hi Fred, after confirming with you that Y.on("key", this.close, closebtn , "down:13", this); was no longer needed for the new button, as down:13 refers to the 'enter' key being pushed, which by default will execute the button, I am passing. Good job.

          Show
          Mark Nelson added a comment - - edited [Y] Syntax [-] Output [Y] Whitespace [Y] Language [-] Databases [Y] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check Hi Fred, after confirming with you that Y.on("key", this.close, closebtn , "down:13", this); was no longer needed for the new button, as down:13 refers to the 'enter' key being pushed, which by default will execute the button, I am passing. Good job.
          Hide
          Frédéric Massart added a comment -

          Thanks Mark, pushing for integration!

          Show
          Frédéric Massart added a comment - Thanks Mark, pushing for integration!
          Hide
          Frédéric Massart added a comment -

          Linking to MDL-35819 which would create a new help popup system.

          Show
          Frédéric Massart added a comment - Linking to MDL-35819 which would create a new help popup system.
          Hide
          Sam Hemelryk added a comment -

          Thanks Fred, this has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks Fred, this has been integrated now.
          Hide
          Michael de Raadt added a comment -

          I've added a ui_change label to remind me to note this issue in the Release notes. All sites that upgrade will see an error message about the string used on the help dialogue until they upgrade (or purge caches separately, I think). We might see some errors being reported about this.

          Show
          Michael de Raadt added a comment - I've added a ui_change label to remind me to note this issue in the Release notes. All sites that upgrade will see an error message about the string used on the help dialogue until they upgrade (or purge caches separately, I think). We might see some errors being reported about this.
          Hide
          Mark Nelson added a comment -

          Hi Fred, functionality is great. However I get a JS error in the console -

          TypeError: Y.one("#closehelpbox") is null http://localhost/im/lib/javascript-static.js Line 1538

          Show
          Mark Nelson added a comment - Hi Fred, functionality is great. However I get a JS error in the console - TypeError: Y.one("#closehelpbox") is null http://localhost/im/lib/javascript-static.js Line 1538
          Hide
          Frédéric Massart added a comment -

          Thanks Mark, forgot to remove that reference! I have added a commit on top of each branch to fix it. Thanks!

          Show
          Frédéric Massart added a comment - Thanks Mark, forgot to remove that reference! I have added a commit on top of each branch to fix it. Thanks!
          Hide
          Dan Poltawski added a comment -

          Pulled in reseting

          Show
          Dan Poltawski added a comment - Pulled in reseting
          Hide
          Mark Nelson added a comment -

          Looks good, passing.

          Show
          Mark Nelson added a comment - Looks good, passing.
          Hide
          Michael de Raadt added a comment -

          Test result: Success!

          Tested on 2.2, 2.3 and master.

          I tested the Course settings page and a few other forms.

          Show
          Michael de Raadt added a comment - Test result: Success! Tested on 2.2, 2.3 and master. I tested the Course settings page and a few other forms.
          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:
              1 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: