-
Notifications
You must be signed in to change notification settings - Fork 46
8268 | Mobile header accessibility #513
8268 | Mobile header accessibility #513
Conversation
@@ -140,6 +140,11 @@ | |||
// Mobile menu | |||
.mobile-nav & { | |||
border-top: 1px solid hsla(0, 0%, 100%, 0.2); | |||
visibility: hidden; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something is not quite right with the links in the menu drop-down on the home page, where the focus is skipping straight past the drop down. I don't think it is this rule that is the issue - I haven't tracked it down from a quick look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you got a video?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you go:
Screen.Recording.2023-08-01.at.18.50.55.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flagging that this is still an issue although this is on staging now. I'll mention in stand-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's to do with the order of the DOM elements on the home page template, as with other pages on staging the focus moves in a logical order - https://torchbox.staging.torchbox.com/digital-products/
On the homepage template I moved patterns/atoms/menu-button/menu-button.html
down to here as otherwise the menu button was out of tab order - you can test this on https://torchbox.com/.
I think the fix will be to move patterns/molecules/navigation/mobile-nav.html
to directly after menu-button.html
on the homepage template I linked to above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had more of a look into this and think i've resolved it here - 08590fd
It looks like we were overriding the {% block header %}
tag on home_page.html
as we don't include the primary nav on it but moving elements out of that header messes up the tabbing order.
So i've now matched the contents of {% block header %}
on home_page.html
to that of base_page.html
but add a conditional on the primary nav as to wether to include it or not.
This in turn also fixes inconsistencies in logo placement when comparing the homepage to any other page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rebased the branch shortly after which is probably why.
It's here now (140b626) but prob easier to checkout the branch and pull : )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that would do it - this is working nicely now - well done. I will update staging.
Everything looks good apart from an issue with tabbing the menu links in the dropdown on the home page. It would be nice if possible to also fix the 'more' button on the home page which currently can't be expanded via the keyboard. |
d9fc072
to
b0653a9
Compare
Should be fixed in #509. I think it can be opened via keyboard but there's no focus state so you can't tell it's the active element. |
Great thanks - you're right it's just the focus state that was the issue |
…ng clicked on smaller viewports
08590fd
to
140b626
Compare
Link to Ticket
Description of Changes Made
aria-expanded
attr on the mobile menu toggleHow to Test
MR Checklist