Moodle
  1. Moodle
  2. MDL-33198

book chapter titles should be headings

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      33921

      Description

      Book chapter titles should be headings and not plain text

        Issue Links

          Activity

          Hide
          Mike Churchward added a comment -

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

          Show
          Mike Churchward added a comment - Can you be more specific? What tags should surround the chapter titles?
          Hide
          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 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
          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
          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 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 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 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 Agarwal added a comment - Book/tool/index.php should use html writer. and needs other cleanup I will create a issue for that
          Hide
          Rossiani Wijaya added a comment -

          This looks great Ankit.

          +1 for integration.

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

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

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

          rebased
          Thanks

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

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

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

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

          Show
          Ankit Agarwal added a comment - Grr, not sure how that happened. Anyway updated. Thanks
          Hide
          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
          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 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 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
          Greg Kraus added a comment -

          The patch looks good to me.

          Thanks!

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

          Integrated (23 and master), thanks!

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

          Thanks Greg for your feedback. Really appreciate it.

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

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

          Show
          Andrew Davis added a comment - Ive raised and linked an issue I discovered while testing this.
          Hide
          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
          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
          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
          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: