Enhancement 511: Skip Navigation Button

bingeboy commented 7 months ago

This PR:

Resolves #511

Screenshots (if applicable):

screen-recorder-tue-nov-28-2023-13-43-35.webm

Additional Context (optional):

Follow best practice from webaim

Issues needing discussion/feedback (optional):

1. Where is a good location for the button to reveal? 2. Add higher contrast styles.

leekahung commented 7 months ago

Hm...think we could have the button appear like where you had it on the top. Think we just need to make it more noticeable with a contrasting background.

Also think you could do a similar approach to what was merged in PR #516 with the useRef and scrollIntoView for scrolling. That way we could avoid hashes in our URL.

bingeboy commented 7 months ago

I could be wrong but I think semantically we want a <a/> tag. I can add a event.preventDefault(); to the handler and the scrolling effect will work.

For styling I can reverse the colors and add a white background with the NavBar background color for the text link. Open to any suggestions.

xscottxbrownx commented 7 months ago

I could be wrong but I think semantically we want a <a/> tag. I can add a event.preventDefault(); to the handler and the scrolling effect will work.

For styling I can reverse the colors and add a white background with the NavBar background color for the text link. Open to any suggestions.

The MUI Link component ends up creating an <a> tag in the HTML. I'd have to check about the React Router Link component, I assume it does as well.

For MUI Link, check the Footer's RenderCopyrightAndLinksSection.jsx to see example of it compiling down to <a>.

For example of the useRef, check my icon button on home page to scroll down. I'm pretty confident we could do the same with a Link.

xscottxbrownx commented 7 months ago

Another thought:

With the current state of this button, I'm not seeing the benefit to the user.

I expected the tabbing order to also be affected, but after clicking this "skip to main", if you tab again it starts back up at the logo in navbar. I would expect it to be part of the main content instead. Is this something we could control??

AND

should the breadcrumbs be inside the <main> or no? Should it be the next tabbable item? Just an honest question.

bingeboy commented 7 months ago

@xscottxbrownx good point. Checkout how mozilla added the "skip to main" functionality. I'll have to research a little on how to get the tab index to work like this. This seems like a good implementation. They also seem to skip directly into the content so I'd recommend bypassing the breadcrumb.

I'll also update the <a/> with a <Link />.

leekahung commented 7 months ago

I'll also update the <a/> with a <Link />.

Oh, about <Link>, think we should use React Router's version of it and update any potential styling with style. That way we could get a better handle on routing. Or you could wrap React Router's <Link> with MUI's <Link> but with a different name just for that file to get the MUI styling included.

xscottxbrownx commented 7 months ago

I'll also update the <a/> with a <Link />.

Oh, about <Link>, think we should use React Router's version of it and update any potential styling with style. That way we could get a better handle on routing. Or you could wrap React Router's <Link> with MUI's <Link> but with a different name just for that file to get the MUI styling included.

Yeah we usually do something like:

import { Link as RouterLink} from ‘react-router-dom’;
import Link from ‘@mui/material/Link;

<Link component={RouterLink}>

so we get the MUI styling, but react router functionality of interrupting the network call.

xscottxbrownx commented 7 months ago

@xscottxbrownx good point. Checkout how mozilla added the "skip to main" functionality. I'll have to research a little on how to get the tab index to work like this. This seems like a good implementation. They also seem to skip directly into the content so I'd recommend bypassing the breadcrumb.

I'll also update the <a/> with a <Link />.

Interesting that it does it there too. The tabbing. Just makes no sense to me.

So you use keyboard only and want to tab straight down to the main content (or scroll to main content via the "skip to main content button"), then why would next tab take you right back up to the top. That makes zero sense to me.

See the "Manual Checks" section here: https://fae.disability.illinois.edu/rulesets/BYPASS_1/#:~:text=The%20id%20attribute%20is%20preferred,it%20from%20the%20tab%20order.

According to that, my thoughts are the way it's intended to be implemented. But honestly I have no idea what that link is - just a quick google search of topic.

bingeboy commented 7 months ago

@xscottxbrownx I think the Mozilla site mirrors the focusing. The homepage might be an exception. Pretty sure we are both on the same page. I haven't looked into skipping the breadcrumb. Based on my research some sites skip "sub nav" , some don't, some have a dropdown with options. I'll just need a consistent node with an ID to focus.

Example skipping to the content with focus: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object

@leekahung do you think I can continue to use a hash with the location hook from react-router-dom? IMHO using a <a/> instead of a <button /> seems more semantic and allows for history api. The only link that seems to not work is the "login" for the podserver which I haven't researched into why.

The link @milofultz referenced recommeneded:

<body>
<a href="#maincontent">Skip to main content</a>
...
<main id="maincontent">
<h1>Heading</h1>
<p>This is the first paragraph</p>

This is working on my local:


      <Link
        component={RouterLink}
        to={{ pathname: location.pathname, hash: '#main-content' }}
        tabIndex={0}
        onClick={handleLinkClick}
        ... >
          Skip to main content
         </Link>

leekahung commented 7 months ago

@leekahung do you think I can continue to use a hash with the location hook from react-router-dom? IMHO using a <a/> instead of a <button /> seems more semantic and allows for history api. The only link that seems to not work is the "login" for the podserver which I haven't researched into why.

Hm...looking at this more closely and reading over the WebAIM site for "Skip Navigation", we are not exactly navigating between pages, but rather across the same page.

The main purpose was to help mouse-less users and screen readers skip over elements that could be of little or no benefit to them going over the site, so I think a button would work just as well as an internal anchor tag (see clip, first with clicking, the second time with just using the enter key without the mouse).

https://github.com/codeforpdx/PASS/assets/14917816/09d915dc-b91b-415e-b3ad-6c9258001280

The benefit here of course is to allow the user to navigate within the same route without altering the existing URL. And since they're not really leaving the page, guess we don't need to worry about navigation history.

If we wish to make this more semantic, we could fallback on using aria-label.

https://github.com/codeforpdx/PASS/assets/14917816/15c4dc22-3787-4e7f-95b1-c2fce34f5389

One thing I've notice though is how the tabs behaves with "Skip Navigation":

https://github.com/codeforpdx/PASS/assets/14917816/4c82524d-b00a-4a1a-8de9-007d6dd597c2

Since the purpose for a "Skip Navigation" is to help ease access for potential mouse-less users and screen readers, we would probably want the tab right after the skip to be those in main and beyond instead of jumping back. The tab to nav should return last. So we might need to think of a way to make the tabIndex dynamic or jump ahead if "Skip Navigation" is triggered.

milofultz commented 7 months ago

Hey all,

Re: tab order, we need to move our focus to the element in question. The easiest way to do that is to use the default in-browser method of supplying an ID and letting the browser navigate there. Using a standard a element without removing the default behavior is the way to go, as it handles all of what we're trying to reverse engineer automatically. On the WebAIM site, tab to the skip navigation button, press "Enter", and it will work as we're discussing.

While this does change the URL, but if the goal is to ensure it gets added to the history, it looks like there is a solution: https://github.com/rafgraph/react-router-hash-link

Re: using a or button, a tags are explicitly for navigation and are the most semantically clear to the user that this goes somewhere. A button is usually some kind of functionality aside from navigation, so I'd probably opt for an a tag in this instance.

so we get the MUI styling, but react router functionality of interrupting the network call.

A nit: because this is targeting an element within the page, there is no network call. ~~React router~~ The network should not be involved because we're not having to load up another page through clicking this link.

milofultz commented 7 months ago

Also, for navigating using the keyboard, you want to use Enter, not Space on links. See https://www.w3.org/WAI/ARIA/apg/patterns/link/examples/link/#kbd_label

leekahung commented 7 months ago

Re: tab order, we need to move our focus to the element in question. The easiest way to do that is to use the default in-browser method of supplying an ID and letting the browser navigate there. Using a standard a element without removing the default behavior is the way to go, as it handles all of what we're trying to reverse engineer automatically. On the WebAIM site, tab to the skip navigation button, press "Enter", and it will work as we're discussing. While this does change the URL, but if the goal is to ensure it gets added to the history, it looks like there is a solution: https://github.com/rafgraph/react-router-hash-link

Considering we're simply creating a method to "skip navigation" to the main content (the main focus for this PR), I don't think we need to include that to the browser history.

Re: using a or button, a tags are explicitly for navigation and are the most semantically clear to the user that this goes somewhere. A button is usually some kind of functionality aside from navigation, so I'd probably opt for an a tag in this instance.

This feels more like user interaction rather than navigation with the user interacting with the site to "scroll" within a page (without a mouse or they're using a screen reader) rather than "navigate" from one page to another, which was why I think button seems to be the more appropriate choice rather than a. We could with the button element, we can do what we can with the a tag, with the "Enter" key jumping straight to the main content without involving the browser history, so I'm in favor of using a button tag instead, at least with this case. I would favor the a tag otherwise though if we are jumping between specific routes.

But I could be wrong about the definitions though. (Been trying to dig around with little luck) 😆 Would be good to discuss about it. If it ends up being anchor tags being the right choice, I think we could as @milofultz suggested, follow through with that.

milofultz commented 7 months ago

Considering we're simply creating a method to "skip navigation" to the main content (the main focus for this PR), I don't think we need to include that to the browser history.

That makes sense. I am curious the behavior of trying to use the back button after clicking that and if it would be any different than a non-SPA page. If only from my own experience, when a website has different behavior of navigating history, it can result in super frustrating experiences (losing form data, refreshing unnecessarily on low-bandwidth connections when expecting to go to a # link, etc.).

This feels more like user interaction rather than navigation with the user interacting with the site to "scroll" within a page (without a mouse or they're using a screen reader) rather than "navigate" from one page to another, which was why I think button seems to be the more appropriate choice rather than a. We could with the button element, we can do what we can with the a tag, with the "Enter" key jumping straight to the main content without involving the browser history, so I'm in favor of using a button tag instead, at least with this case. I would favor the a tag otherwise though if we are jumping between specific routes.

But I could be wrong about the definitions though. (Been trying to dig around with little luck) 😆 Would be good to discuss about it. If it ends up being anchor tags being the right choice, I think we could as @milofultz suggested, follow through with that.

I'm going to respectfully disagree on using button over a. We have literal decades of precedent in intra- and inter-page navigation using a elements and functionality that does something (submitting forms, triggering Javascript functions, etc.) being triggered with button elements. That kind of affordance, if only cultural, is a huge boon for good design and UX that we don't ever have to worry about ever again. All browsers and assistive technologies will change with these affordances and entrenched ideas in mind and fighting that current will be annoying at best and detrimental at worst (long term, think lawsuits).

If the only thing we do is look at every page talking about implementing skip links, that will show us that the vast majority, if not totality, of implementations use links. Note first that they are called skip links by w3 themselves, which is the committee that writes the guidelines this issue is looking to adhere to. Second, if we take the first few results on DDG of "skip links", all implementations are links:

And probably the most important thing: if users of assistive technology expect a skip link to be activated with "Enter" and then we have a button which by convention is activated with "Space", they will be likely very confused. If only that every skip link they have probably ever experienced was a link, but also that a button is expected to do something, not navigate.

I appreciate you reading this, I just fear the unnecessary reinvention of the wheel with this and think we could follow the existing guidelines and make a slam dunk for our users here.

leekahung commented 7 months ago

And probably the most important thing: if users of assistive technology expect a skip link to be activated with "Enter" and then we have a button which only activates with "Space", they will be likely very confused. If only that every skip link they have probably ever experienced was a link, but also that a button is expected to do something, not navigate.

I appreciate you reading this, I just fear the unnecessary reinvention of the wheel with this and think we could follow the existing guidelines and make a slam dunk for our users here.

Thanks Milo for the clarification! Yeah, re-inventing the wheel is probably something we don't really need in our case. I guess navigation was the term I was most confused about. I've always expected that navigation only refers to inter-page interactions. I've never thought intra-page interaction like with the internal anchor tags as navigation (definitely clears up my confusion over internal anchor tags to begin with if you could just use a button to scroll to a destination in the same page). Again, I didn't thought "navigating" would be the same as "scrolling" until now.

Edit: Oh! Got it working with space too. I see the support for buttons and space key for button (https://www.w3.org/WAI/ARIA/apg/patterns/button/examples/button/) and the enter support for link (https://www.w3.org/WAI/ARIA/apg/patterns/link/)

I'm guessing the discussions with keeping the "Enter" key with buttons when they want to keep buttons and links interactions separate was a can of worms topic for the w3.

milofultz commented 7 months ago

I'm guessing the discussions with keeping the "Enter" key with buttons when they want to keep buttons and links interactions separate was a can of worms topic for the w3.

Yeah, I'm very curious about the history! I'm sure there's some "let's put this on temporarily" stuff along with some IE "do it our way" stuff, too haha.

bingeboy commented 7 months ago

Improved test and resolved merge conflict.

bingeboy commented 6 months ago

I believe I have addressed all the provided feedback.

bingeboy commented 6 months ago

Squashing

codeforpdx / PASS