|
Description
Martin Renvoize (ashimema)
2020-04-15 09:38:33 UTC
Created attachment 105720 [details] [review] Bug 25155: Fixing semantic headings in opac-auth.tt Test plan: 1) Follow test plan from comment #1 2) Apply the patch 3) Reload the page, and note that is still displays fine 4) Check that the main heading is now an h1, and that the other headings on the page make sense Sorry.. this is another case of a somewhat misdescribed bug.. I actually think the text in the original report this came from is also incorrect. Anyway.. the patchset does resolve a related bug which I have not yet filed... the only missing piece here is that the modal title is still an H3.. for which I'll add a followup. Created attachment 105722 [details] [review] Bug 25155: Fixing semantic headings in opac-auth.tt Test plan: 1) Follow test plan from comment #1 2) Apply the patch 3) Reload the page, and note that is still displays fine 4) Check that the main heading is now an h1, and that the other headings on the page make sense Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 105723 [details] [review] Bug 25155: Fixing semantic headings login modal The login modal headings were semantically incorrect prior to this patch. Test plan: 1) Click the 'Log in to your account' option to expose the modal 2) Use a headings inspector and note we jump from H1 (Page Title) -> H3 (Modal Title) 3) Apply the patch and reload the page. 4) Click the 'Log in to your account' option to expose the modal 5) The modal should still appear well formed 6) Use a headings inspector and note we jump from H1 (Page Title) -> H2 (Modal Title) -> H3 (Shibboleth title if enabled) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 105724 [details] [review] Bug 25155: (follow-up) Increase headings levels by 1 In the previous patch we reduced the headings levels by 2, but this was one level too many as we already have an H1 in the navbar and so all other heading sets should start at level 2 to semantically follow the title heading. I've signed off the first patch, but added two followups I'm afraid so we still need an overall signoff. I'm using the headingsMap extension for chrome to highlight heading orders and catch errors. According to the report, a page should only ever contain one H1 level heading, and as we use h1 in the navbar for the logo that means all sets of headings anywhere else on the page should start at level 2.. hence the final followup. Thanks again for working on this Hayley :) Created attachment 105755 [details] [review] Bug 25155: Fixing semantic headings in opac-auth.tt Test plan: 1) Follow test plan from comment #1 2) Apply the patch 3) Reload the page, and note that is still displays fine 4) Check that the main heading is now an h1, and that the other headings on the page make sense Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Created attachment 105756 [details] [review] Bug 25155: Fixing semantic headings login modal The login modal headings were semantically incorrect prior to this patch. Test plan: 1) Click the 'Log in to your account' option to expose the modal 2) Use a headings inspector and note we jump from H1 (Page Title) -> H3 (Modal Title) 3) Apply the patch and reload the page. 4) Click the 'Log in to your account' option to expose the modal 5) The modal should still appear well formed 6) Use a headings inspector and note we jump from H1 (Page Title) -> H2 (Modal Title) -> H3 (Shibboleth title if enabled) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Created attachment 105757 [details] [review] Bug 25155: (follow-up) Increase headings levels by 1 In the previous patch we reduced the headings levels by 2, but this was one level too many as we already have an H1 in the navbar and so all other heading sets should start at level 2 to semantically follow the title heading. Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Hi Martin, What are your thoughts on that navbar h1? To me semantically it doesn't feel like it makes much sense, it'll be considered empty or it'll say "logo" I think by screen readers which doesn't really inform users of the content of the page at all (if it's a screen reader people are using then the page title (tab) will be read so they'll know they're on Koha). I'd like to propose we remove that as h1, leave it as a link. Anyone else have input here? (In reply to Hayley Mapley from comment #10) > I'd like to propose we remove that as h1, leave it as a link. > Anyone else have input here? I've gone back and forth on this issue for a long time and I think I'm coming down on your side of it now. When using VoiceOver in macOS the page context is stated clearly, as you say, by the page title. In my test the logo is read correctly--the hidden text is used instead of the image, but it's not read any differently than the other menu links. I think it would be better to remove the H1 tag around the logo. Agreed, though not as part of this bug ;) Having started working through this I quickly realised I wanted to remove the H1 around the logo block.. however, I decided as that would entail touching all other templates to decrement the headers that I would instead work through doing bugfixes first and then do a followup on a final bug touching everything. Additionally, it raised a few questions.. best practice is to only include one H1 level heading per page.. as such the Logo H1 does make a lot of sense, even if highlighting the wrong content. If we move the H1 to the actual maincontent of each page, it raises questions regarding some of our other uses of headings markup... for example what header level should modals end up as (their title's make sense as an H1, but then you would end up with multiple H1's on the page as the 'background' H1's would still count). What heading level would the left hand side navigation blocks end up as? Created attachment 106618 [details] [review] Bug 25155: (follow-up) Increase headings levels by 1 In the previous patch we reduced the headings levels by 2, but this was one level too many as we already have an H1 in the navbar and so all other heading sets should start at level 2 to semantically follow the title heading. Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Pushed to master for 20.11, thanks to everybody involved! backported to 20.05.x for 20.05.03 backported to 19.11.x for 19.11.09 Backported to 19.05.x branch for 19.05.15 |