|
Description
Wainui Witika-Park
2021-02-03 21:14:34 UTC
Created attachment 116296 [details] [review] Bug 27610: Changed <h1> around logo to <div> To test: 1) Look at OPAC and the logo 2) Apply patch 3) Check Koha logo looks the same as before applying the patch 4) Ensure page source shows logo inside <div> and not <h1> Sponsored-by: Catalyst IT Should we do the same thing in the staff client? (In reply to Lucas Gass from comment #2) > Should we do the same thing in the staff client? Yes definitely, I will make a bug report for it now. (In reply to Lucas Gass from comment #2) > Should we do the same thing in the staff client? (In reply to wainuiwitikapark from comment #3) > (In reply to Lucas Gass from comment #2) > > Should we do the same thing in the staff client? > > Yes definitely, I will make a bug report for it now. Please see https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27631 for the Staff Client version of this bug Created attachment 117017 [details] [review] Bug 27610: Changed pages of OPAC to have a <h1> Changed each of the pages in the OPAC to have one <h1> tag showing that describes the page, rather than the <h1> describing the logo. The hierarchy of heading tags may be broken in many pages, but this will be dealt with in a follow up patch or bug. To test: 1) Go to the OPAC 2) Apply patch 3) Go to each of the pages and check that they have an obvious and descriptive heading 4) Ensure that the heading in the page is <h1> Sponsored by: Catalyst IT Looking good! One area that is a bit odd looking now is OPAC purchase suggestions. Now there is an h1 and and h2 with very similar text. Can this be fixed? https://snipboard.io/SCFALO.jpg (In reply to Lucas Gass from comment #6) > Looking good! > > One area that is a bit odd looking now is OPAC purchase suggestions. Now > there is an h1 and and h2 with very similar text. Can this be fixed? > > https://snipboard.io/SCFALO.jpg Hi Lucas! Thanks for your comment! Yes that is definitely very redundant. However, the aim of this bug was to just ensure that there is one <h1> heading on each page of the OPAC. Then the Bug 27740 ensures the structure and hierarchy of the headings under <h1> are correct. Neither of those are specifically dealing with redundancy of headings and/or whether or not headings under <h1> are actually headings (i.e. there are currently some errors and alerts that are <h> tags that shouldn't actually be headings). Therefore, I think if you want to make a follow up patch about this particular issue, that depends on this Bug 27610 and Bug 27740, that would probably be the best way to go. Great spotting though and appreciate the feedback! (In reply to wainuiwitikapark from comment #7) > (In reply to Lucas Gass from comment #6) > > Looking good! > > > > One area that is a bit odd looking now is OPAC purchase suggestions. Now > > there is an h1 and and h2 with very similar text. Can this be fixed? > > > > https://snipboard.io/SCFALO.jpg > > Hi Lucas! Thanks for your comment! Yes that is definitely very redundant. > However, the aim of this bug was to just ensure that there is one <h1> > heading on each page of the OPAC. Then the Bug 27740 ensures the structure > and hierarchy of the headings under <h1> are correct. Neither of those are > specifically dealing with redundancy of headings and/or whether or not > headings under <h1> are actually headings (i.e. there are currently some > errors and alerts that are <h> tags that shouldn't actually be headings). > Therefore, I think if you want to make a follow up patch about this > particular issue, that depends on this Bug 27610 and Bug 27740, that would > probably be the best way to go. Great spotting though and appreciate the > feedback! Hey again, on second thought, I'll just make a new bug myself that deals with things like redundancy of page titles and then I will also make another bug about things that shouldn't be headings. Watch this space for the new bugs :) I'll make them soon. Created attachment 117769 [details] [review] Bug 27610: (follow-up) Hide <h1> on opac-main.tt This patch visually hides the <h1> on opac-main.tt so that libraries' customisations of their home pages are not disturbed by the introduction of a descriptive <h1>, but the heading is still available to be read out by screenreaders in the summary of headings on the page. Test plan: 1) Apply dependencies and all patches on this bug report 2) Observe a new heading level 1 'Home' on the OPAC main page 2) Apply this patch 3) Observe the heading is hidden visually, but in the source code you can see it is in the correct hierarchy and it is more descriptive 'Koha home' Sponsored-by: Catalyst IT Created attachment 117772 [details] [review] Bug 27610: Changed <h1> around logo to <div> To test: 1) Look at OPAC and the logo 2) Apply patch 3) Check Koha logo looks the same as before applying the patch 4) Ensure page source shows logo inside <div> and not <h1> Sponsored-by: Catalyst IT Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Created attachment 117773 [details] [review] Bug 27610: Changed <h1> around logo to <div> To test: 1) Look at OPAC and the logo 2) Apply patch 3) Check Koha logo looks the same as before applying the patch 4) Ensure page source shows logo inside <div> and not <h1> Sponsored-by: Catalyst IT Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Created attachment 117774 [details] [review] Bug 27610: Changed pages of OPAC to have a <h1> Changed each of the pages in the OPAC to have one <h1> tag showing that describes the page, rather than the <h1> describing the logo. The hierarchy of heading tags may be broken in many pages, but this will be dealt with in a follow up patch or bug. To test: 1) Go to the OPAC 2) Apply patch 3) Go to each of the pages and check that they have an obvious and descriptive heading 4) Ensure that the heading in the page is <h1> Sponsored by: Catalyst IT Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Created attachment 117775 [details] [review] Bug 27610: (follow-up) Hide <h1> on opac-main.tt This patch visually hides the <h1> on opac-main.tt so that libraries' customisations of their home pages are not disturbed by the introduction of a descriptive <h1>, but the heading is still available to be read out by screenreaders in the summary of headings on the page. Test plan: 1) Apply dependencies and all patches on this bug report 2) Observe a new heading level 1 'Home' on the OPAC main page 2) Apply this patch 3) Observe the heading is hidden visually, but in the source code you can see it is in the correct hierarchy and it is more descriptive 'Koha home' Sponsored-by: Catalyst IT Hello everyone, I've signed off this patch set also to indicate that I've tested it and semantically everything is looking good. 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 Bug 27876. 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 block Bug 27740 off or provide some feedback! (In reply to Lucas Gass from comment #6) > Looking good! > > One area that is a bit odd looking now is OPAC purchase suggestions. Now > there is an h1 and and h2 with very similar text. Can this be fixed? > > https://snipboard.io/SCFALO.jpg Hi Lucas, I have now made Bug 27876 and Bug 27877 to deal with redundancies. This is great work, it's awesome to see people working on accessibility improvements! I think we would want the <h1> on the OPAC main page to be visible to screenreaders and I think they ignore content with "display:none" (please correct me if I'm wrong!) I recently learnt that Bootstrap has a built-in screenreader only class: ".sr-only". Can we use that instead? https://getbootstrap.com/docs/4.0/utilities/screenreaders/ (In reply to Henry Bolshaw from comment #16) > This is great work, it's awesome to see people working on accessibility > improvements! > > I think we would want the <h1> on the OPAC main page to be visible to > screenreaders and I think they ignore content with "display:none" (please > correct me if I'm wrong!) > > I recently learnt that Bootstrap has a built-in screenreader only class: > ".sr-only". Can we use that instead? > > https://getbootstrap.com/docs/4.0/utilities/screenreaders/ Hi Henry, thanks so much for your feedback! Owen also pointed this out in the comments on Bug 27740 so we will definitely work on amending that! I'm also wondering about the <h1>Home</h1> that is being added to opac-main.tt. Sites with existing designs are going to want to hide this or change the language. Since there is a custom content block there ( OPACMainUSerBlock ) would it be unreasonable to expect librarians to add their own h1 here? At the very least this should include a id/class so it can be easy to target for hiding or editing the text via jQuery. (In reply to Lucas Gass from comment #18) > I'm also wondering about the <h1>Home</h1> that is being added to > opac-main.tt. Sites with existing designs are going to want to hide this or > change the language. > > Since there is a custom content block there ( OPACMainUSerBlock ) would it > be unreasonable to expect librarians to add their own h1 here? > > At the very least this should include a id/class so it can be easy to target > for hiding or editing the text via jQuery. Maybe content in OpacMainUserBlock if set could replace the default h1? > Maybe content in OpacMainUserBlock if set could replace the default h1?
+1
I don't think we should be relying on users to set an <h1>, and it's important as this is the home page that a top level heading indicating this is here. As noted in comments above, we can use the class .sr-only to hide the heading instead of display:none. I agree that it should be hidden, which is why we wrote a patch to do so. Created attachment 118084 [details] [review] Bug 27610: (follow-up) Hide <h1> on opac-main.tt This patch visually hides the <h1> on opac-main.tt so that libraries' customisations of their home pages are not disturbed by the introduction of a descriptive <h1>, but the heading is still available to be read out by screenreaders in the summary of headings on the page. Test plan: 1) Apply dependencies and all patches on this bug report 3) Ensure the <h1> "Koha home" heading on the OPAC main/home page is hidden visually, but in the source code you can see it Sponsored-by: Catalyst IT Sorry, Patch 118084 - Bug 27610: (follow-up) Hide <h1> on opac-main.tt - doesn't apply. It's just a one line change, changing the class over to the one that's been suggested. If you can fix the merge conflict manually that'd be great, otherwise we'll be on that shortly! Created attachment 118445 [details] [review] Bug 27610: Changed <h1> around logo to <div> To test: 1) Look at OPAC and the logo 2) Apply patch 3) Check Koha logo looks the same as before applying the patch 4) Ensure page source shows logo inside <div> and not <h1> Sponsored-by: Catalyst IT Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Created attachment 118446 [details] [review] Bug 27610: Changed pages of OPAC to have a <h1> Changed each of the pages in the OPAC to have one <h1> tag showing that describes the page, rather than the <h1> describing the logo. The hierarchy of heading tags may be broken in many pages, but this will be dealt with in a follow up patch or bug. To test: 1) Go to the OPAC 2) Apply patch 3) Go to each of the pages and check that they have an obvious and descriptive heading 4) Ensure that the heading in the page is <h1> Sponsored by: Catalyst IT Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Created attachment 118447 [details] [review] Bug 27610: (follow-up) Hide <h1> on opac-main.tt This patch visually hides the <h1> on opac-main.tt so that libraries' customisations of their home pages are not disturbed by the introduction of a descriptive <h1>, but the heading is still available to be read out by screenreaders in the summary of headings on the page. Test plan: 1) Apply dependencies and all patches on this bug report 3) Ensure the <h1> "Koha home" heading on the OPAC main/home page is hidden visually, but in the source code you can see it Sponsored-by: Catalyst IT Have solved the merge conflict in the follow-up, ready for sign off again! Created attachment 118449 [details] [review] Bug 27610: Changed <h1> around logo to <div> To test: 1) Look at OPAC and the logo 2) Apply patch 3) Check Koha logo looks the same as before applying the patch 4) Ensure page source shows logo inside <div> and not <h1> Sponsored-by: Catalyst IT Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Signed-off-by: Henry Bolshaw <bolshawh@parliament.uk> Created attachment 118450 [details] [review] Bug 27610: Changed pages of OPAC to have a <h1> Changed each of the pages in the OPAC to have one <h1> tag showing that describes the page, rather than the <h1> describing the logo. The hierarchy of heading tags may be broken in many pages, but this will be dealt with in a follow up patch or bug. To test: 1) Go to the OPAC 2) Apply patch 3) Go to each of the pages and check that they have an obvious and descriptive heading 4) Ensure that the heading in the page is <h1> Sponsored by: Catalyst IT Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Signed-off-by: Henry Bolshaw <bolshawh@parliament.uk> Created attachment 118451 [details] [review] Bug 27610: (follow-up) Hide <h1> on opac-main.tt This patch visually hides the <h1> on opac-main.tt so that libraries' customisations of their home pages are not disturbed by the introduction of a descriptive <h1>, but the heading is still available to be read out by screenreaders in the summary of headings on the page. Test plan: 1) Apply dependencies and all patches on this bug report 3) Ensure the <h1> "Koha home" heading on the OPAC main/home page is hidden visually, but in the source code you can see it Sponsored-by: Catalyst IT Signed-off-by: Henry Bolshaw <bolshawh@parliament.uk> "If you can fix the merge conflict manually that'd be great, otherwise we'll be on that shortly!" Sorry Hadley, I wasn't confident enough to do that. (Git bz terrifies me - frankly, I'm amazed I managed to sign off the patches.) Thanks for the quick fix! I agree that we shouldn't rely on users to set an <h1>. Hehe that's not a problem! Thanks for signing off the patches! :) Argh, Hayley, I just noticed I misspelt your name in my previous comment - sorry! (In reply to Henry Bolshaw from comment #34) > Argh, Hayley, I just noticed I misspelt your name in my previous comment - > sorry! Not a worry! Happens all the time! :) Created attachment 118773 [details] [review] Bug 27610: Changed <h1> around logo to <div> To test: 1) Look at OPAC and the logo 2) Apply patch 3) Check Koha logo looks the same as before applying the patch 4) Ensure page source shows logo inside <div> and not <h1> Sponsored-by: Catalyst IT Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Signed-off-by: Henry Bolshaw <bolshawh@parliament.uk> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 118774 [details] [review] Bug 27610: Changed pages of OPAC to have a <h1> Changed each of the pages in the OPAC to have one <h1> tag showing that describes the page, rather than the <h1> describing the logo. The hierarchy of heading tags may be broken in many pages, but this will be dealt with in a follow up patch or bug. To test: 1) Go to the OPAC 2) Apply patch 3) Go to each of the pages and check that they have an obvious and descriptive heading 4) Ensure that the heading in the page is <h1> Sponsored by: Catalyst IT Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Signed-off-by: Henry Bolshaw <bolshawh@parliament.uk> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 118775 [details] [review] Bug 27610: (follow-up) Hide <h1> on opac-main.tt This patch visually hides the <h1> on opac-main.tt so that libraries' customisations of their home pages are not disturbed by the introduction of a descriptive <h1>, but the heading is still available to be read out by screenreaders in the summary of headings on the page. Test plan: 1) Apply dependencies and all patches on this bug report 3) Ensure the <h1> "Koha home" heading on the OPAC main/home page is hidden visually, but in the source code you can see it Sponsored-by: Catalyst IT Signed-off-by: Henry Bolshaw <bolshawh@parliament.uk> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> So, yes.. this does leave lots of pages with bad hierarchies as it said it would. Having been down this road for the last release, that slightly scares me.. but I do completely agree with the direction it's taking us.. I really didn't like that we had H1's as the title logo everywhere when I was going through the OPAC ensuring we at least had a half reasonable hierarchy of headings. However, given Wainui obvious continuing efforts and evidence that she's on the case.. I am happy to PQA on this bug and help shepherd through the followup bugs that resolve the issue's this bug causes. Passing QA Great work! (In reply to Martin Renvoize from comment #39) > So, yes.. this does leave lots of pages with bad hierarchies as it said it > would. Having been down this road for the last release, that slightly scares > me.. but I do completely agree with the direction it's taking us.. I really > didn't like that we had H1's as the title logo everywhere when I was going > through the OPAC ensuring we at least had a half reasonable hierarchy of > headings. > > However, given Wainui obvious continuing efforts and evidence that she's on > the case.. I am happy to PQA on this bug and help shepherd through the > followup bugs that resolve the issue's this bug causes. > > Passing QA > > Great work! Yay thanks Martin! I will continue working on the rest of the headings to make sure these issues are resolved quickly! Pushed to master for 21.05, thanks to everybody involved! Enhancement not pushed to 20.11.x <h1> didn't print to https://gitlab.com/koha-community/koha-release-notes/-/blob/master/release_notes_20_12_00.md, so changed the bug title removing the <> |
