|
Description
Wainui Witika-Park
2021-02-21 21:00:29 UTC
Created attachment 117629 [details] [review] Bug 27740: Corrected tags & hierarchy of headings Bug 27610 corrected the <h1> tags in each of the OPAC files. This patch corrects the hierarchy of the rest of the headings on each of the pages. To test: 1) Go to the OPAC 2) Apply Bug 27610 patch 3) Apply patch (Bug 27740) 4) Go to each of the pages and check that the headings under <h1> are in the correct order and hierarchy Sponsored-by: Catalyst IT Created attachment 117771 [details] [review] Bug 27740: Corrected tags & hierarchy of headings Bug 27610 corrected the <h1> tags in each of the OPAC files. This patch corrects the hierarchy of the rest of the headings on each of the pages. To test: 1) Go to the OPAC 2) Apply Bug 27610 patch 3) Apply patch (Bug 27740) 4) Go to each of the pages and check that the headings under <h1> are in the correct order and hierarchy Sponsored-by: Catalyst IT Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Hello everyone, I've signed off this patch to indicate that it's been checked through and tested and everything's looking good semantically. As the goal for this bug is to correct the existing hierarchy of headings, we can deal with any redundancy (or locations where headings are used they perhaps shouldn't be) on a separate bug report. As Wainui and I are both with Catalyst IT it'd be great if someone could take a look at this and sign it and its dependency Bug 27610 off or provide some feedback! > As the goal for this bug is to correct the existing hierarchy of headings, > we can deal with any redundancy (or locations where headings are used they > perhaps shouldn't be) on a separate bug report. Bug 27876 has been created for this purpose. Owen, Martin, could you help this one (and bug 27610) to avoid unnecessary rebases? Some comments:
1. We don't need new CSS to hide the heading on the main page. We can add the Bootstrap class ".sr-only"
2. I don't think it's necessary to add headings inside <caption> and <legend>, e.g. opac-memberentry.tt:
- <legend id="mainaddress_legend">Main address</legend>
+ <legend id="mainaddress_legend"><h2>Main address</h2></legend>
Semantically speaking I think the legend is correct, and that the heading isn't necessary. Is there a particular accessibility tool behavior you're trying to address?
3. It is invalid to put a heading tag inside a <label> (e.g. opac-advsearch.tt). I have the same question as above about whether this change addresses some specific need.
4. I disagree that a heading shouldn't be used inside a message like this (opac-results.tt):
<div class="dialog alert">
- <h4>Error:</h4>
+ <p><strong>Error:</strong></p>
4. This patch adds an invalid file: .opac-readingrecord.tt.swo
(In reply to Owen Leonard from comment #6) > 4. This patch adds an invalid file: .opac-readingrecord.tt.swo Check added to qa-test-tools Issue #40 - Prevent .swo file to be added to the codebase (In reply to Owen Leonard from comment #6) > Some comments: > > 1. We don't need new CSS to hide the heading on the main page. We can add > the Bootstrap class ".sr-only" > > 2. I don't think it's necessary to add headings inside <caption> and > <legend>, e.g. opac-memberentry.tt: > > - <legend id="mainaddress_legend">Main address</legend> > + <legend id="mainaddress_legend"><h2>Main address</h2></legend> > > Semantically speaking I think the legend is correct, and that the heading > isn't necessary. Is there a particular accessibility tool behavior you're > trying to address? > > 3. It is invalid to put a heading tag inside a <label> (e.g. > opac-advsearch.tt). I have the same question as above about whether this > change addresses some specific need. > > 4. I disagree that a heading shouldn't be used inside a message like this > (opac-results.tt): > > <div class="dialog alert"> > - <h4>Error:</h4> > + <p><strong>Error:</strong></p> > > 4. This patch adds an invalid file: .opac-readingrecord.tt.swo Thanks so much for testing, will have a look into those things now. (In reply to Owen Leonard from comment #6) > Some comments: > > 1. We don't need new CSS to hide the heading on the main page. We can add > the Bootstrap class ".sr-only" > Okay cool, didn't know about that class. Thanks, will amend that. > 2. I don't think it's necessary to add headings inside <caption> and > <legend>, e.g. opac-memberentry.tt: > > - <legend id="mainaddress_legend">Main address</legend> > + <legend id="mainaddress_legend"><h2>Main address</h2></legend> > > Semantically speaking I think the legend is correct, and that the heading > isn't necessary. Is there a particular accessibility tool behavior you're > trying to address? > > 3. It is invalid to put a heading tag inside a <label> (e.g. > opac-advsearch.tt). I have the same question as above about whether this > change addresses some specific need. The reason we are putting heading tags in things such as legends and labels is that they appear to visually be heading a particular block on a page. We out the <h> tags inside the legends and labels as we were not sure what the best way to include them was, and we didn't want to remove the legend and label tags. With screen readers, it is easiest for a person to scan through the page by headings, to get an idea about the certain areas of the page. We don't believe it is possible to scan through the page using legends or labels, and even if it is possible, we think that this is an inconvenience to screen reader users as they would have to be aware of the legends and labels to know to do this. If you are aware of a better way to include these headings, please let us know. One option we thought of was to possibly remove the headings from the labels and legends and put them outside and hide them. > > 4. I disagree that a heading shouldn't be used inside a message like this > (opac-results.tt): > > <div class="dialog alert"> > - <h4>Error:</h4> > + <p><strong>Error:</strong></p> We will make a follow-up patch to move some of these alerts back to headings, but move them up levels so they are in the correct hierarchy. Some of these however, are not meant to be headings as they have no information under them, so these do need to be dealt with. > > 4. This patch adds an invalid file: .opac-readingrecord.tt.swo Oh yes thanks, will remove this. Thanks again for all your feedback! Hi Owen, I have contacted our Accessibility team at Catalyst and have received the following message: ------------------------------------------------------------------------- "First, I would make sure you're using the proper HTML. According to MDN, <h#> is a permitted child of <legend> and <caption>, but not of <label>. Instead, the <h#> would be the parent of <label>. Second, consider the context of the headings. According to W3C, "Headings communicate the organization of the content on the page" so yes I do think that headings can help screen reader users to have an overview of the website (https://www.w3.org/WAI/tutorials/page-structure/headings/). However, headings should be used to identify sections of the page instead of providing name or specify title of an element. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/label "Placing heading elements within a <label> interferes with many kinds of assistive technology, because headings are commonly used as a navigation aid. If the label's text needs to be adjusted visually, use CSS classes applied to the <label> element instead." You can look at this link to see examples of "Making labels and legends headings" https://design-system.service.gov.uk/get-started/labels-legends-headings/" ------------------------------------------------------------------------- Therefore, I am going to have another look at everything and keep headings where they are specific sections of the page, as this helps screen-readers navigate a webpage. However, if the element of the page doesn't seem to be a stand alone section, I will amend this to put it back to a legend, caption or label. Created attachment 118837 [details] [review] Bug 27740: Corrected tags & hierarchy of headings Bug 27610 corrected the <h1> tags in each of the OPAC files. This patch corrects the hierarchy of the rest of the headings on each of the pages. To test: 1) Go to the OPAC 2) Apply Bug 27610 patch 3) Apply patch (Bug 27740) 4) Go to each of the pages and check that the headings under <h1> are in the correct order and hierarchy Sponsored-by: Catalyst IT Created attachment 118858 [details] [review] Bug 27740: Corrected tags & hierarchy of headings Bug 27610 corrected the <h1> tags in each of the OPAC files. This patch corrects the hierarchy of the rest of the headings on each of the pages. To test: 1) Go to the OPAC 2) Apply Bug 27610 patch 3) Apply patch (Bug 27740) 4) Go to each of the pages and check that the headings under <h1> are in the correct order and hierarchy Sponsored-by: Catalyst IT Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 119633 [details] [review] Bug 27740: Corrected tags & hierarchy of headings Bug 27610 corrected the <h1> tags in each of the OPAC files. This patch corrects the hierarchy of the rest of the headings on each of the pages. To test: 1) Go to the OPAC 2) Apply Bug 27610 patch 3) Apply patch (Bug 27740) 4) Go to each of the pages and check that the headings under <h1> are in the correct order and hierarchy Sponsored-by: Catalyst IT Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 119634 [details] [review] Bug 27740: (QA follow-up) Fix 'Payment method' The 'Payment method' heading now needs to be moved to the second level to ensure consistent header heirarchy. Test plan 1/ Add some debts to a patrons account 2/ Enable online payments (Paypal is a good example) 3/ Login to the OPAC and navigate to the 'Charges' page 4/ Note the header hierarchy has no gaps... (h1 -> h2, not h1 -> h3) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 119635 [details] [review] Bug 27740: (QA follow-up) Fix 'Your purchase suggestions' On the purchase suggestions page you could end up with two "Your purchase suggestions" headings, one as an h1 and one as an h2.. Test plan 1/ Add at least one purchage suggestion to a patron 2/ Navigate to 'Purchase suggestions' 3/ Note that just one 'Your purchase suggestsions' heading appears with a top level '1'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> OK, So, I added a couple of follow-ups that would be worthwhile checking. I also notice that on the opac-main page, once logged in you can be presented with a malformed header hierarchy still.. 1. The 'user_summary' block, where is says "Welcome, user" in the top of the box on the right side of the page.. that's an 'h3'.. I'm not sure it should be a header element at all.. it's a 'box title'.. thoughts? 2. If the quote of the day feature is enabled, we display a 'Quote of the day' heading above all over content in the 'main' content area of the page.. that is also an 'h3' which doesn't feel right... maybe this should be converted to more of a clear 'content box, with a title' again, instead of looking like part of the main content of the page with a standard header? Given the above I think I need some feedback before being able to PQA Created attachment 119687 [details] [review] Bug 27740: Corrected tags & hierarchy of headings Bug 27610 corrected the <h1> tags in each of the OPAC files. This patch corrects the hierarchy of the rest of the headings on each of the pages. To test: 1) Go to the OPAC 2) Apply Bug 27610 patch 3) Apply patch (Bug 27740) 4) Go to each of the pages and check that the headings under <h1> are in the correct order and hierarchy Sponsored-by: Catalyst IT Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 119688 [details] [review] Bug 27740: (QA follow-up) Fix 'Payment method' The 'Payment method' heading now needs to be moved to the second level to ensure consistent header heirarchy. Test plan 1/ Add some debts to a patrons account 2/ Enable online payments (Paypal is a good example) 3/ Login to the OPAC and navigate to the 'Charges' page 4/ Note the header hierarchy has no gaps... (h1 -> h2, not h1 -> h3) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> Created attachment 119689 [details] [review] Bug 27740: (QA follow-up) Fix 'Your purchase suggestions' On the purchase suggestions page you could end up with two "Your purchase suggestions" headings, one as an h1 and one as an h2.. Test plan 1/ Add at least one purchage suggestion to a patron 2/ Navigate to 'Purchase suggestions' 3/ Note that just one 'Your purchase suggestsions' heading appears with a top level '1'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> (In reply to Martin Renvoize from comment #16) > OK, So, I added a couple of follow-ups that would be worthwhile checking. > > I also notice that on the opac-main page, once logged in you can be > presented with a malformed header hierarchy still.. > > 1. The 'user_summary' block, where is says "Welcome, user" in the top of the > box on the right side of the page.. that's an 'h3'.. I'm not sure it should > be a header element at all.. it's a 'box title'.. thoughts? > 2. If the quote of the day feature is enabled, we display a 'Quote of the > day' heading above all over content in the 'main' content area of the page.. > that is also an 'h3' which doesn't feel right... maybe this should be > converted to more of a clear 'content box, with a title' again, instead of > looking like part of the main content of the page with a standard header? > > Given the above I think I need some feedback before being able to PQA Hi Martin! Thanks so much, I tested and signed off your 2 follow-up patches. Yes I agree that the "Welcome, user" shouldn't actually even be a heading. I can make a follow up patch to amend this now. I also agree the Quote of the day is incorrect so I'll make a follow up patch to amend that too. Thanks again! Created attachment 119690 [details] [review] Bug 27740: (follow-up) changed welcome user to <p> In the 'user_summary block', the "Welcome, user" was an <h3>. This has been changed to a <p> as it isn't really a heading. The styling can be changed to make it appear as it was before, or to make it appear more significant on the page. To test: 1) Go to the OPAC 2) Go to the home page 3) Go to the 'user_summary' block 4) Check that the part where is says "Welcome, user" is now a <p> Sponsored-by: Catalyst IT Created attachment 119691 [details] [review] Bug 27740: (follow-up) quote of the day to <h2> If the quote of the day feature is enabled, a "Quote of the day" heading is displayed. It was an <h3> which was not proper hierarchy, so this has been changed to an <h2>. To test: 1) Go to staff client 2) Enable quote of the day preference 3) Go to the OPAC 4) Observe "Quote of the day" heading 5) Ensure it is an <h2> Sponsored-by: Catalyst IT Hi Martin, please I added 2 follow-up patches to fix the above concerns. However, please feel free to change them if you had other/better ideas in mind :) This is really great work, Wainui! On the "user_summary" block, I agree that "Welcome, user" isn't really appropriate text for a heading but I think we do need a heading in that area to help users navigate to that section of the page. The user_summary block comes at the bottom of the page structure and I think it would be much harder to navigate to without a heading and the fact that we style it to be quite visually prominent suggests it's an important part of the page. If we remove the heading from "Welcome, User" I'd prefer an additional heading sitting above it that says something like "My Account Summary" or "My Loans". That way you can use the headings option on a screen reader to quickly navigate to that part of the page. But I don't want to stop momentum on this bug if others disagree. Created attachment 119815 [details] [review] Bug 27740: Added User summary heading Added a User summary <h2> To test: 1) Go to OPAC 2) Go to home page 3) Check that there is a <h2> saying "User summary" Sponsored-by: Catalyst IT Created attachment 119816 [details] [review] Bug 27740: (follow-up) Added User summary heading Added a User summary <h2> To test: 1) Go to OPAC 2) Go to home page 3) Check that there is a <h2> saying "User summary" Sponsored-by: Catalyst IT (In reply to Henry Bolshaw from comment #24) > This is really great work, Wainui! > > On the "user_summary" block, I agree that "Welcome, user" isn't really > appropriate text for a heading but I think we do need a heading in that area > to help users navigate to that section of the page. The user_summary block > comes at the bottom of the page structure and I think it would be much > harder to navigate to without a heading and the fact that we style it to be > quite visually prominent suggests it's an important part of the page. > > If we remove the heading from "Welcome, User" I'd prefer an additional > heading sitting above it that says something like "My Account Summary" or > "My Loans". That way you can use the headings option on a screen reader to > quickly navigate to that part of the page. > > But I don't want to stop momentum on this bug if others disagree. Hi Henry, thanks so much for your input! I totally agree with you so have added an additional follow-up patch. Let me know what you think :) Thanks! Created attachment 119953 [details] [review] Bug 27740: (follow-up) changed welcome user to <p> In the 'user_summary block', the "Welcome, user" was an <h3>. This has been changed to a <p> as it isn't really a heading. The styling can be changed to make it appear as it was before, or to make it appear more significant on the page. To test: 1) Go to the OPAC 2) Go to the home page 3) Go to the 'user_summary' block 4) Check that the part where is says "Welcome, user" is now a <p> Sponsored-by: Catalyst IT Signed-off-by: Henry Bolshaw <bolshawh@parliament.uk> Created attachment 119954 [details] [review] Bug 27740: (follow-up) quote of the day to <h2> If the quote of the day feature is enabled, a "Quote of the day" heading is displayed. It was an <h3> which was not proper hierarchy, so this has been changed to an <h2>. To test: 1) Go to staff client 2) Enable quote of the day preference 3) Go to the OPAC 4) Observe "Quote of the day" heading 5) Ensure it is an <h2> Sponsored-by: Catalyst IT Signed-off-by: Henry Bolshaw <bolshawh@parliament.uk> Created attachment 119955 [details] [review] Bug 27740: (follow-up) Added User summary heading Added a User summary <h2> To test: 1) Go to OPAC 2) Go to home page 3) Check that there is a <h2> saying "User summary" Sponsored-by: Catalyst IT Signed-off-by: Henry Bolshaw <bolshawh@parliament.uk> It would be great to have this one for 21.05, keep going :) Created attachment 119958 [details] [review] Bug 27740: (follow-up) changed "User summary" css Changed user summary CSS to style H2 not H3 heading To test: 1) Go to OPAC homepage 2) Login as a user with at least one loan 3) See that the "User summary" heading is not styled 4) Apply patch 5) Run yarn build --view OPAC 6) Confirm the heading is now styled with grey background Wainui, I've signed off your patches and added a follow up to tweak the opac.scss so that the new H2 heading is styled the same way as the old 'Welcome, user' heading. The 'welcome, user' text still looks slightly adrift but I'm not sure how to make it look better. The current heading styling on the OPAC has an insufficient contrast ratio for accessibility purposes. I've created bug 28188 to collect all of these instances. Created attachment 119981 [details] [review] Bug 27740: (follow-up) changed "User summary" css Changed user summary CSS to style H2 not H3 heading To test: 1) Go to OPAC homepage 2) Login as a user with at least one loan 3) See that the "User summary" heading is not styled 4) Apply patch 5) Run yarn build --view OPAC 6) Confirm the heading is now styled with grey background Signed-off-by: Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> Created attachment 120437 [details] [review] Bug 27740: Corrected tags & hierarchy of headings Bug 27610 corrected the <h1> tags in each of the OPAC files. This patch corrects the hierarchy of the rest of the headings on each of the pages. To test: 1) Go to the OPAC 2) Apply Bug 27610 patch 3) Apply patch (Bug 27740) 4) Go to each of the pages and check that the headings under <h1> are in the correct order and hierarchy Sponsored-by: Catalyst IT Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 120438 [details] [review] Bug 27740: (QA follow-up) Fix 'Payment method' The 'Payment method' heading now needs to be moved to the second level to ensure consistent header heirarchy. Test plan 1/ Add some debts to a patrons account 2/ Enable online payments (Paypal is a good example) 3/ Login to the OPAC and navigate to the 'Charges' page 4/ Note the header hierarchy has no gaps... (h1 -> h2, not h1 -> h3) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 120439 [details] [review] Bug 27740: (QA follow-up) Fix 'Your purchase suggestions' On the purchase suggestions page you could end up with two "Your purchase suggestions" headings, one as an h1 and one as an h2.. Test plan 1/ Add at least one purchage suggestion to a patron 2/ Navigate to 'Purchase suggestions' 3/ Note that just one 'Your purchase suggestsions' heading appears with a top level '1'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 120440 [details] [review] Bug 27740: (follow-up) changed welcome user to <p> In the 'user_summary block', the "Welcome, user" was an <h3>. This has been changed to a <p> as it isn't really a heading. The styling can be changed to make it appear as it was before, or to make it appear more significant on the page. To test: 1) Go to the OPAC 2) Go to the home page 3) Go to the 'user_summary' block 4) Check that the part where is says "Welcome, user" is now a <p> Sponsored-by: Catalyst IT Signed-off-by: Henry Bolshaw <bolshawh@parliament.uk> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 120441 [details] [review] Bug 27740: (follow-up) quote of the day to <h2> If the quote of the day feature is enabled, a "Quote of the day" heading is displayed. It was an <h3> which was not proper hierarchy, so this has been changed to an <h2>. To test: 1) Go to staff client 2) Enable quote of the day preference 3) Go to the OPAC 4) Observe "Quote of the day" heading 5) Ensure it is an <h2> Sponsored-by: Catalyst IT Signed-off-by: Henry Bolshaw <bolshawh@parliament.uk> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 120442 [details] [review] Bug 27740: (follow-up) Added User summary heading Added a User summary <h2> To test: 1) Go to OPAC 2) Go to home page 3) Check that there is a <h2> saying "User summary" Sponsored-by: Catalyst IT Signed-off-by: Henry Bolshaw <bolshawh@parliament.uk> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 120443 [details] [review] Bug 27740: (follow-up) changed "User summary" css Changed user summary CSS to style H2 not H3 heading To test: 1) Go to OPAC homepage 2) Login as a user with at least one loan 3) See that the "User summary" heading is not styled 4) Apply patch 5) Run yarn build --view OPAC 6) Confirm the heading is now styled with grey background Signed-off-by: Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Great work team! I can't spot any further issues, but with Koha as "optional" as it is with various preferences and options changing what's displayed on the page I wouldn't be at all surprised if between us we've missed an occurrence somewhere with a specific setup. However, I think this is a great foundation and should be pushed as is.. we should just keep an eye out for bugs going forward. So.. with that said.. PQA! Pushed to master for 21.05, thanks to everybody involved! Enhancement not pushed to 20.11.x |