Details

    • Testing Instructions:
      Hide
      1. Go to login page
      2. Click the help icon
      3. Inspect the bold title at the top of the help popup in firebug or similar dev tool
      4. Ensure the title is a h2 element
      5. Check in multiple themes to ensure the appearance isn't broken
      6. Turn off ajax (or disable javascript in your browser)
      7. Refresh the login page, and click the icon again
      8. Inspect the bold title at the top of the help popup in firebug or similar dev tool
      9. Ensure the title is a h1 element
      Show
      Go to login page Click the help icon Inspect the bold title at the top of the help popup in firebug or similar dev tool Ensure the title is a h2 element Check in multiple themes to ensure the appearance isn't broken Turn off ajax (or disable javascript in your browser) Refresh the login page, and click the icon again Inspect the bold title at the top of the help popup in firebug or similar dev tool Ensure the title is a h1 element
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-35823-master
    • Rank:
      44584

      Description

      Issue
      Heading structure - Help text presents a h1 when the help modal window is brought up. That level heading isn't structuring main content and should either be made into a h2, or not made into a heading level at all. This occurs in most help text, but may not be all.

      Standard Level
      WCAG 2 A 1.3.1 http://www.w3.org/WAI/WCAG20/quickref/#qr-content-structure-separation-programmatic

      Impact
      Serious

      Example Link
      http://demo.moodle.net/course/view.php?id=625 see help icon for search forum and search will be an heading 1 with class helpheading.

        Issue Links

          Activity

          Hide
          Jason Hardin added a comment -

          MDL-28235 doesn't mention the Heading 1 issue but does mention resolving accessibility issues with help.

          Show
          Jason Hardin added a comment - MDL-28235 doesn't mention the Heading 1 issue but does mention resolving accessibility issues with help.
          Hide
          Jason Fowler added a comment -

          Just looking at this again, after looking at it in the scrum. Why shouldn't it be a level 1 heading? It is the header for a new, ajax'd modal DOM. Within the context of the popup, it is right to be a level 1 heading. At least, that's how I understand it. If you could provide further clarification, that would be great.

          Show
          Jason Fowler added a comment - Just looking at this again, after looking at it in the scrum. Why shouldn't it be a level 1 heading? It is the header for a new, ajax'd modal DOM. Within the context of the popup, it is right to be a level 1 heading. At least, that's how I understand it. If you could provide further clarification, that would be great.
          Hide
          Jason Hardin added a comment -

          It shouldn't be level 1 because there is already one level 1 on the page. Because Moodle no longer pops up help text is a separate window it is now in a modal that causes there to be 2 H1s on the page. There should only be 1 h1 on a page for accessibility. Just because it is in a modal dom does not mean it is not on the same page as the themes H1 to a screen reader.

          Show
          Jason Hardin added a comment - It shouldn't be level 1 because there is already one level 1 on the page. Because Moodle no longer pops up help text is a separate window it is now in a modal that causes there to be 2 H1s on the page. There should only be 1 h1 on a page for accessibility. Just because it is in a modal dom does not mean it is not on the same page as the themes H1 to a screen reader.
          Hide
          Jason Fowler added a comment -

          okay, understood. I'll convert it to a level 2 heading instead.

          Show
          Jason Fowler added a comment - okay, understood. I'll convert it to a level 2 heading instead.
          Hide
          Frédéric Massart added a comment -

          Hi Jason,

          your patch looks good, and I think it does make sense to use <h2> instead of <h1> in there. The only little that I have spotted is that in non-JS mode, the heading should still be an <h1> and it becomes an <h2>. If you adapt the logic in help.php, could you comment on it so that we'd understand why the heading is different in Ajax mode?

          Cheers,
          Fred

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

          Show
          Frédéric Massart added a comment - Hi Jason, your patch looks good, and I think it does make sense to use <h2> instead of <h1> in there. The only little that I have spotted is that in non-JS mode, the heading should still be an <h1> and it becomes an <h2>. If you adapt the logic in help.php, could you comment on it so that we'd understand why the heading is different in Ajax mode? Cheers, Fred [Y] Syntax [Y] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing [-] Security [-] Documentation [Y] Git [?] Sanity check
          Hide
          Jason Fowler added a comment -

          Changes made as per Fred's review.
          Pushing for integration now.

          Show
          Jason Fowler added a comment - Changes made as per Fred's review. Pushing for integration now.
          Hide
          Dan Poltawski added a comment -

          Hi Jason,

          Thanks - i've integrated this now. I added one commit to fix some trailing whitespace.

          NOTE: I've not backported this as this change in html could impact on themes.

          Show
          Dan Poltawski added a comment - Hi Jason, Thanks - i've integrated this now. I added one commit to fix some trailing whitespace. NOTE: I've not backported this as this change in html could impact on themes.
          Hide
          Jason Fowler added a comment -

          Thanks, and good thinking, although the themes in moodle shouldn't be affected, third party themes may be.

          Show
          Jason Fowler added a comment - Thanks, and good thinking, although the themes in moodle shouldn't be affected, third party themes may be.
          Hide
          Rajesh Taneja added a comment -

          Sorry Jason,

          It doesn't look fine in arialist theme. Although it is h2, color is grey and not bold.

          Attaching screen shot.

          Show
          Rajesh Taneja added a comment - Sorry Jason, It doesn't look fine in arialist theme. Although it is h2, color is grey and not bold. Attaching screen shot.
          Hide
          Jason Fowler added a comment -

          Fixes for the themes and an update to theme/upgrade.txt have been pushed. If the integrators could please re-integrate the patch and re-push for testing, that would be greatly appreciated, thanks.

          Show
          Jason Fowler added a comment - Fixes for the themes and an update to theme/upgrade.txt have been pushed. If the integrators could please re-integrate the patch and re-push for testing, that would be greatly appreciated, thanks.
          Hide
          Dan Poltawski added a comment -

          Thanks, back to testing.

          Show
          Dan Poltawski added a comment - Thanks, back to testing.
          Hide
          Rajesh Taneja added a comment -

          Thanks Jason and Dan,

          It seems the header is not selected for accessibility.
          Will wait for Jason to fix this and then test again.

          Show
          Rajesh Taneja added a comment - Thanks Jason and Dan, It seems the header is not selected for accessibility. Will wait for Jason to fix this and then test again.
          Hide
          Jason Fowler added a comment -

          Thanks for spotting that Raj, patch has been pushed again

          Show
          Jason Fowler added a comment - Thanks for spotting that Raj, patch has been pushed again
          Hide
          Dan Poltawski added a comment -

          Pulled in the fix.

          Show
          Dan Poltawski added a comment - Pulled in the fix.
          Hide
          Rajesh Taneja added a comment -

          Thanks Jason,

          Works grt.

          Show
          Rajesh Taneja added a comment - Thanks Jason, Works grt.
          Hide
          Andrew Nicols added a comment -

          Just so you know, this whole popup will be replaced by MDL-35819 which is up for integration.

          Show
          Andrew Nicols added a comment - Just so you know, this whole popup will be replaced by MDL-35819 which is up for integration.
          Hide
          Damyon Wiese added a comment -

          Congratulations! This issue has been resolved. Thanks for helping to make Moodle better for everyone!

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Congratulations! This issue has been resolved. Thanks for helping to make Moodle better for everyone! Regards, Damyon

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: