Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: lesson side navigation a11y issues #4423

Open
2 of 7 tasks
thatblindgeye opened this issue Mar 2, 2024 · 3 comments
Open
2 of 7 tasks

Bug: lesson side navigation a11y issues #4423

thatblindgeye opened this issue Mar 2, 2024 · 3 comments
Labels
Status: Needs Review This issue/PR needs an initial or additional review Type: Bug Involves something that isn't working as intended

Comments

@thatblindgeye
Copy link
Contributor

thatblindgeye commented Mar 2, 2024

Checks

  • I have thoroughly read and understand The Odin Project Contributing Guide
  • The title of this issue follows the Bug: brief description of bug format, e.g. Bug: Lesson complete button does not update on click
  • Would you like to work on this issue?

Bug description

Various a11y issues that should be resolved for the lesson side navigation (this can be considered an epic consisting of several issues; each of these issues should have a separate PR):

  • Side navigation not visible at smaller viewports - Basically the lesson side navigation is not visible when zoom is higher or viewport is smaller, which is an issue that needs to be resolved. Ideally the navigation should be collapsible/expandable regardless of viewport/zoom, rather than just completely hiding it at a certain point. This may involve updating the h4 heading

  • Reconsider wrapper element for the lesson nav from an aside to a nav. While an aside may make sense in terms of positioning, it really is more of a navigation. Doing this we could also remove the h4 tag as it's no longer really a section of content; we should keep the text, though, and this text should be used as the labeling text for the nav/ul via aria-labelledby

  • Apply aria-current="location" to the li element that contains the link which is currently "active". May also want to consider applying a more noticeable style to the current jump link rather than just a slight background/text color change, like removing the left border from all li's except the active one (screenshot below)
    TOP lesson side navigation with updated styling, where only the active jump link item has a left border applied

  • Apply a role="list" to the nav ul element. It's redundant, but Safari removes the semantics for lists that remove the list styling. This would apply to other areas of the site that uses lists in this way as well.

How to reproduce

Visit any lesson (like our working with text lesson) and compare the above comments to the side navigation. Some may not be reproducible without a screen reader (aria-current for example).

Expected behavior

  1. The side nav is able to be viewed regardless of zoom/viewport width
  2. The side navigation is announced semantically as a navigation element as it contains links for the lesson itself (rather than external links)
  3. The side nav and/or ul elements are labeled by the "Lesson content" text
  4. The currently active jump link is announced as the current link via aria-current, and has improved active styling
  5. The ul element retains its semantics via role="list" for Safari, which removes the semantics when certain styles are applied to the ul

What browsers are you seeing the problem on?

No response

What OS are you using?

No response

Discord Name

No response

Additional Comments

cc @KevinMulhern in case some of these issues need some design input, may be worth getting together async to chat about them

May also be coupled to #4407 and/or #3498

@thatblindgeye thatblindgeye added Type: Bug Involves something that isn't working as intended Status: Needs Review This issue/PR needs an initial or additional review labels Mar 2, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog / Ideas in Main Site Mar 2, 2024
@KevinMulhern
Copy link
Member

Thanks for the great write up @thatblindgeye 💪

I've noticed it switches to the mobile layout when zoomed to that level - the lesson content centers and the side nav is hidden. There likely won't be enough space beside the lesson content to show the side nav on that viewport. Repositioning above the lesson content may be an option though?

But, it might also be ok to keep hiding it on smaller view ports. There was a discussion a while back where some users indicated they thought the side nav was a "nice to have". We haven't got any feedback about users missing it on mobile which validates that to a degree.

Is it important to always have the lesson table of contents on all viewports for a11y purposes? if it is, its definitely worth the effort.

@thatblindgeye
Copy link
Contributor Author

So my rationale is that if it's something we're providing at X viewport/zoom level, it's something we should provide at all viewport/zoom levels. I'd agree that a repositioning would be the best approach (though we'd also need to be sure to make it sticky so that it's always within view to interact with). Repositioning may actually be best in general, i.e. not just at X viewport/zoom level, which can be better explained when I get into keyboard navigation below.

I think the side nav is a little more than just "nice to have", more so in lessons that have more sections to parse. It's a quicker way to jump to a specific section than just scrolling or using the browser search to get to that section, as well as get an idea of the structure/contents of the page (kinda similar to how AT may be able to view all the headings on a page to garner similar info).

In terms of a11y, there's two examples to view it from. For someone like me, I typically need any site I visit to be zoomed at at least 125% so that the text is large enough to read (not just TOP, just basically any site I visit). Currently it basically becomes choosing between readable text or navigation within a lesson. Another scenario is mobile viewing, where it can be even more tedious to scroll through a lesson that has a lot of content just to get to a specific part.

Another PoV would actually be keyboard navigation (which I may need to make an edit to my original post now that I think about it). If I'm navigating via keyboard, there's potentially a lot of content I need to tab through in order to get to a specific section. The lesson navigation alleviates that a lot, though we also need to move the lesson navigation to be before the lesson content within the DOM (currently you'd have to tab through the entire lesson content just to get to the lesson nav)

I'd argue that there being no feedback about a lack of side nav on mobile isn't necessarily validation. It could be, but it could also as likely be that people don't know that a side nav even exists depending on their setup (I think I may have been shocked that there even was one 😆 ) , or they don't know where/how to bring it up.

@KevinMulhern
Copy link
Member

KevinMulhern commented Mar 2, 2024

Everything except the lesson content is a nice to have in my book 😆

Putting it before the content on smaller viewports seems to be the most practical way to go about it, and a fairly popular UI pattern.

Web.dev
Screenshot 2024-03-02 at 23 39 23

Rails Guides
Screenshot 2024-03-02 at 23 41 27

Educative do something a bit more interesting, they incorporate it into the lesson content itself, its kind of like what we have with our "what you'll learn in this lesson" list:
Screenshot 2024-03-02 at 23 53 41

I'd push back on sticky being a hard requirement for this. The only way I think we'd be able to pull that off is with a select box - it'll likely be too distracting on smaller viewports. But, I'd be open to it if we can find a nice way of doing it. Do you know of any sites that have sticky TOC navs we could look at?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Needs Review This issue/PR needs an initial or additional review Type: Bug Involves something that isn't working as intended
Projects
Status: 📋 Backlog / Ideas
Development

No branches or pull requests

2 participants