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

          Attachments

            Issue Links

              Activity

              Hide
              jrh18 Jason Hardin added a comment -

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

              Show
              jrh18 Jason Hardin added a comment - MDL-28235 doesn't mention the Heading 1 issue but does mention resolving accessibility issues with help.
              Hide
              phalacee 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
              phalacee 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
              jrh18 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
              jrh18 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
              phalacee Jason Fowler added a comment -

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

              Show
              phalacee Jason Fowler added a comment - okay, understood. I'll convert it to a level 2 heading instead.
              Hide
              fred 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
              fred 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
              phalacee Jason Fowler added a comment -

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

              Show
              phalacee Jason Fowler added a comment - Changes made as per Fred's review. Pushing for integration now.
              Hide
              poltawski 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
              poltawski 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
              phalacee Jason Fowler added a comment -

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

              Show
              phalacee Jason Fowler added a comment - Thanks, and good thinking, although the themes in moodle shouldn't be affected, third party themes may be.
              Hide
              rajeshtaneja 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
              rajeshtaneja 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
              phalacee 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
              phalacee 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
              poltawski Dan Poltawski added a comment -

              Thanks, back to testing.

              Show
              poltawski Dan Poltawski added a comment - Thanks, back to testing.
              Hide
              rajeshtaneja 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
              rajeshtaneja 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
              phalacee Jason Fowler added a comment -

              Thanks for spotting that Raj, patch has been pushed again

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

              Pulled in the fix.

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

              Thanks Jason,

              Works grt.

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

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

              Show
              dobedobedoh 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 Damyon Wiese added a comment -

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

              Regards, Damyon

              Show
              damyon 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:
                    Fix Release Date:
                    14/May/13