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

          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