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

      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.

        Gliffy Diagrams

          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: