Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-33198

book chapter titles should be headings

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.1, 2.3, 2.4
    • Fix Version/s: 2.3.3
    • Component/s: Accessibility, Book
    • Labels:
      None
    • Testing Instructions:
      Hide
      1. Goto a book resource
      2. Create chapters as below:-
        1 Normal
        1.1 Normal
        2 Hidden
        3 Hidden
        3.1 Hidden
        4 Normal
        4.1 Hidden
      3. Click on each chapter and verify the following:-
      4. Make sure each chapter name is a h2 tag
      5. Make sure each sub-chapter name is a h3 tag
      6. Make sure each normal chapter/sub-chapter name has class as below
        1 - "book_chapter_title"
        1.1 - both titles- "book_chapter_title"
        2 - "book_chapter_title dimmed_text"
        3 - "book_chapter_title dimmed_text"
        3.1 - both titles - "book_chapter_title dimmed_text"
        4 - "book_chapter_title"
        4.1 - both titles - "book_chapter_title dimmed_text"
      7. Print book (book admin>print book)
      8. Make sure the titles are as described in step 6. (Hidden chapters wont be present, just check for rest)
      9. Print individual chapters(book admin>print this chapter) (needs to be done for all chapters)
      10. Make sure the titles are as described in step 6. (Hidden chapters wont be present, just check for rest)
      Show
      Goto a book resource Create chapters as below:- 1 Normal 1.1 Normal 2 Hidden 3 Hidden 3.1 Hidden 4 Normal 4.1 Hidden Click on each chapter and verify the following:- Make sure each chapter name is a h2 tag Make sure each sub-chapter name is a h3 tag Make sure each normal chapter/sub-chapter name has class as below 1 - "book_chapter_title" 1.1 - both titles- "book_chapter_title" 2 - "book_chapter_title dimmed_text" 3 - "book_chapter_title dimmed_text" 3.1 - both titles - "book_chapter_title dimmed_text" 4 - "book_chapter_title" 4.1 - both titles - "book_chapter_title dimmed_text" Print book (book admin>print book) Make sure the titles are as described in step 6. (Hidden chapters wont be present, just check for rest) Print individual chapters(book admin>print this chapter) (needs to be done for all chapters) Make sure the titles are as described in step 6. (Hidden chapters wont be present, just check for rest)
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-33198-master

      Description

      Book chapter titles should be headings and not plain text

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              mchurch Mike Churchward added a comment -

              Can you be more specific? What tags should surround the chapter titles?

              Show
              mchurch Mike Churchward added a comment - Can you be more specific? What tags should surround the chapter titles?
              Hide
              ankit_frenz Ankit Agarwal added a comment -

              I am not sure if the changes in the book_tool_print is needed or not.
              Requesting a review to get more feedback.

              Thanks

              Show
              ankit_frenz Ankit Agarwal added a comment - I am not sure if the changes in the book_tool_print is needed or not. Requesting a review to get more feedback. Thanks
              Hide
              gdkraus Greg Kraus added a comment -

              At a minimum, I'd make the chapter title an <h2> instead of a <p>.

              Current: <p class="book_chapter_title ">1 Introduction</p>

              Proposed: <h2 class="book_chapter_title ">1 Introduction</h2>

              At one level, this text here is being used visually as a heading. Since it is being used visually in this way the underlying semantics should reflect that too.

              At another level, this also gets to the larger issue of providing a heading at the beginning of the main content. Moodle pages already provide off-screen skip to main content links, which is good. To make it more usable a heading should also be at the beginning of the main content. I argue that the heading at the beginning of the main content should actually be an <h1>, even if there is already another <h1> on the page. <h1> helps denote the most important aspect about a page, and the beginning of the content that is unique to that page is arguably the most important item on that page. There is a great deal of debate about whether you should have only one or two <h1> elements, or if it's OK to have an <h2> placed before an <h1>. In my opinion, at the end of the day, if you have placed an <h1> or <h2> at the beginning of the main content, I'm happy.

              Show
              gdkraus Greg Kraus added a comment - At a minimum, I'd make the chapter title an <h2> instead of a <p>. Current: <p class="book_chapter_title ">1 Introduction</p> Proposed: <h2 class="book_chapter_title ">1 Introduction</h2> At one level, this text here is being used visually as a heading. Since it is being used visually in this way the underlying semantics should reflect that too. At another level, this also gets to the larger issue of providing a heading at the beginning of the main content. Moodle pages already provide off-screen skip to main content links, which is good. To make it more usable a heading should also be at the beginning of the main content. I argue that the heading at the beginning of the main content should actually be an <h1>, even if there is already another <h1> on the page. <h1> helps denote the most important aspect about a page, and the beginning of the content that is unique to that page is arguably the most important item on that page. There is a great deal of debate about whether you should have only one or two <h1> elements, or if it's OK to have an <h2> placed before an <h1>. In my opinion, at the end of the day, if you have placed an <h1> or <h2> at the beginning of the main content, I'm happy.
              Hide
              ankit_frenz Ankit Agarwal added a comment -

              Hi Greg,
              Thanks for your feedback. I have added <h2> tags to chapter titles and <h3> to sub-chapter titles.

              Thanks

              Show
              ankit_frenz Ankit Agarwal added a comment - Hi Greg, Thanks for your feedback. I have added <h2> tags to chapter titles and <h3> to sub-chapter titles. Thanks
              Hide
              ankit_frenz Ankit Agarwal added a comment -

              Book/tool/index.php should use html writer. and needs other cleanup
              I will create a issue for that

              Show
              ankit_frenz Ankit Agarwal added a comment - Book/tool/index.php should use html writer. and needs other cleanup I will create a issue for that
              Hide
              rwijaya Rossiani Wijaya added a comment -

              This looks great Ankit.

              +1 for integration.

              Show
              rwijaya Rossiani Wijaya added a comment - This looks great Ankit. +1 for integration.
              Hide
              ankit_frenz Ankit Agarwal added a comment -

              Thanks for the review Rosie.
              updated stables.
              Created and linked issue for cleanup.
              Sending for integration.
              Thanks

              Show
              ankit_frenz Ankit Agarwal added a comment - Thanks for the review Rosie. updated stables. Created and linked issue for cleanup. Sending for integration. Thanks
              Hide
              stronk7 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
              stronk7 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
              ankit_frenz Ankit Agarwal added a comment -

              rebased
              Thanks

              Show
              ankit_frenz Ankit Agarwal added a comment - rebased Thanks
              Hide
              poltawski Dan Poltawski added a comment -

              moodle:master and ankitagarwal:MDL-33198-master are identical.

              Show
              poltawski Dan Poltawski added a comment - moodle:master and ankitagarwal: MDL-33198 -master are identical.
              Hide
              ankit_frenz Ankit Agarwal added a comment -

              Grr, not sure how that happened.
              Anyway updated.
              Thanks

              Show
              ankit_frenz Ankit Agarwal added a comment - Grr, not sure how that happened. Anyway updated. Thanks
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Sorry, I'm reopening this because I cannot see those headings being applied to view.php and I would expect the same behavior than the one @ tool/print.

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Sorry, I'm reopening this because I cannot see those headings being applied to view.php and I would expect the same behavior than the one @ tool/print.
              Hide
              ankit_frenz Ankit Agarwal added a comment -

              Oh crap, it is missing the second commit. Something went crazily wrong with this branch
              Anyways updated both branches.

              Sorry for the trouble.

              Thanks

              Show
              ankit_frenz Ankit Agarwal added a comment - Oh crap, it is missing the second commit. Something went crazily wrong with this branch Anyways updated both branches. Sorry for the trouble. Thanks
              Hide
              gdkraus Greg Kraus added a comment -

              The patch looks good to me.

              Thanks!

              Show
              gdkraus Greg Kraus added a comment - The patch looks good to me. Thanks!
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Integrated (23 and master), thanks!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (23 and master), thanks!
              Hide
              ankit_frenz Ankit Agarwal added a comment -

              Thanks Greg for your feedback. Really appreciate it.

              Show
              ankit_frenz Ankit Agarwal added a comment - Thanks Greg for your feedback. Really appreciate it.
              Hide
              andyjdavis Andrew Davis added a comment -

              Ive raised and linked an issue I discovered while testing this.

              Show
              andyjdavis Andrew Davis added a comment - Ive raised and linked an issue I discovered while testing this.
              Hide
              markn Mark Nelson added a comment - - edited

              The CSS classes and tags all look good. However, if you are viewing a hidden chapter and click 'Print this chapter' it is actually rendered (w/o the CSS class dimmed_text). Not sure if this is expected behaviour or if the ability to print hidden chapters should be disabled altogether, if it should be disabled it warrants a separate tracker issue, so passing this regardless.

              Show
              markn Mark Nelson added a comment - - edited The CSS classes and tags all look good. However, if you are viewing a hidden chapter and click 'Print this chapter' it is actually rendered (w/o the CSS class dimmed_text). Not sure if this is expected behaviour or if the ability to print hidden chapters should be disabled altogether, if it should be disabled it warrants a separate tracker issue, so passing this regardless.
              Hide
              poltawski Dan Poltawski added a comment -

              Congratulations, you've done it!

              Thanks, this change is now in the latest weekly release!

              Join the crowds of people tomorrow from 8am and download this Moodle release from your local apple store!

              Show
              poltawski Dan Poltawski added a comment - Congratulations, you've done it! Thanks, this change is now in the latest weekly release! Join the crowds of people tomorrow from 8am and download this Moodle release from your local apple store!

                People

                • Votes:
                  0 Vote for this issue
                  Watchers:
                  2 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    12/Nov/12