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

Accessibility updates for base.html #1023

Closed
lwrubel opened this issue Apr 21, 2020 · 19 comments
Closed

Accessibility updates for base.html #1023

lwrubel opened this issue Apr 21, 2020 · 19 comments
Assignees
Milestone

Comments

@lwrubel
Copy link
Collaborator

lwrubel commented Apr 21, 2020

  1. Change sfm-container div to a main tag.
  2. Change alerts to have a role of "status" not "alert" when yellow or green.
  3. Add skip navigation: hidden but focusable skip link at the top of the DOM that skips over the nav and breadcrumbs to the h1.

Lower priority, but review as an option:

  1. User profile dropdown in nav bar should be instead of with role=button

For further accessibility details see #1004

@lwrubel lwrubel added this to the 2.3.0 milestone Apr 21, 2020
@kerchner
Copy link
Member

Recommend working on this after #1009 and #1003 have been completed, to avoid complicated merge

@kerchner kerchner self-assigned this Apr 23, 2020
@kerchner
Copy link
Member

kerchner commented Apr 27, 2020

@lwrubel What do you mean by "a main tag" in item 1 above?

@lwrubel
Copy link
Collaborator Author

lwrubel commented Apr 27, 2020

From Max's comment in the accessibility review doc:
"I would make this a <main> instead of a <div>. If you can't change the element, then the next best approach would be to add an attribute of role="main"."

@maxstarkenburg
Copy link

I'm not sure what doing <main><div class="container sfm-container"></div></main> would necessarily buy you over <main class="container sfm-container"></main> unless there was other code specifically seeking out a <div> with those classes (e.g. CSS expecting div.sfm-container .someclass), or perhaps something about your setup makes it much easier to wrap elements than to change their tag?

In any case, it should be redundant have role="main" on a <main>. There might be some historical lack of support (e.g. browsers like IE11, which I think might also require specially setting main { display: block; } so that it doesn't display inline), but I've seen quite a bit of arguing that it's better these days to leave such aria landmark roles off if using the corresponding HTML5 equivalent (though haven't necessarily heard reasons related to actual damage caused to disabled users (just things like "it encourages misguided practice to other developers" or "lets browser/AT makers think they shouldn't have to use standards")).

@kerchner
Copy link
Member

@lwrubel Can you point me to the source of item 3? I don't see it in either of the links referenced in #1004 - the Google Doc or the Monday board.

@lwrubel
Copy link
Collaborator Author

lwrubel commented Apr 27, 2020

@kerchner See @maxstarkenburg's comment on bypass blocks: "I would still recommend for these pages a hidden but focusable skip link at the top of the DOM that skips over the nav and breadcrumbs to the h1."

@maxstarkenburg
Copy link

FWIW, just peeking into the bootstrap CSS, it looks like they have a .sr-only-focusable class seemingly available for the "hidden but focusable" case (I think it looks like you'd need to add two classes, e.g. <p class="sr-only sr-only-focusable"><a href="#someID">Skip to main content</a></p>)

@kerchner
Copy link
Member

Note that changing the user profile/logout dropdown also changes its styling to button styling and coloring. This is probably for the best as far as accessibility/usability, but just wanted to inform of the visual effect of this change.
image

kerchner added a commit that referenced this issue Apr 27, 2020
…th value of status or alert; changes user profile link to button; code tidying. Ref #1023.
kerchner added a commit that referenced this issue Apr 27, 2020
…th value of status or alert; changes user profile link to button; code tidying. Ref #1023.
@maxstarkenburg
Copy link

It looks like the blue color change is due to the btn-primary class on it, which you could remove if desired to revert to a look similar to the previous (for a11y, a button doesn't necessarily have to look like a traditional button).

@kerchner
Copy link
Member

kerchner commented Apr 27, 2020

@maxstarkenburg I wonder if you can recommend a solution here. I see on the library site:

  1. when the cookie consent appears, the tab order starts in the cookie consent message
  2. when the cookie consent does not appear, the tab order starts with the skip link.

The way I've currently implemented this -- see https://github.com/gwu-libraries/sfm-ui/blob/t1023-accessibility-base-html/sfm/ui/templates/base.html#L83-L108 -- the cookie consent code is always present in the DOM, just not visible. If I place the skip link first, the tab order will always start there, which is not good when we want the user to start in the cookie consent (when the consent is visible). But if I place the cookie consent first, the user will have to tab through the invisible cookie consent even when it's invisible and they have already consented, to get to the skip link.

From your experience, what might be a simple way to solve this?

@kerchner
Copy link
Member

@maxstarkenburg re: button color - Thank you, I didn't realize that the button color was unnecessary for accessibility. I've reverted the color to the current look, although it is now a <button>.

@maxstarkenburg
Copy link

Re: button styling, there are definitely some a11y considerations related to color (i.e. not relying solely on it), but this particular button's location on the page + the caret help indicate it's probably something different than just plain text.

Though here are a few instances of the kind of "relying on color" issue I noticed today (don't recall if these had underlines the other day or just wasn't paying enough attention, haha):

sfm-text-color-01-x2
The right-hand side shows what a color-blind person could see. Without underlining (or some other way of distinguishing it) they might have a hard time telling "last harvest" is a link.

sfm-text-color-02-x2
In this case, I think "Details" is saved by the caret (i.e. it's ok not to be underlined), and "Deleted" is saved by its placement in a tab structure, but "ElliottSchoolGW" and the number "18683120" aren't distinguishable as links as opposed to plain text (and the underling on hover differentiating things doesn't count).

sfm-text-color-04-x2
In this case I'd give the top links a pass based on their position (i.e. highest-level list of words on a page = probably navigational menu items) and the fact that they all behave the same (i.e. they're links) but for the breadcrumbs, I'd say it's pushing things (since on other sites the last item in a breadcrumb row isn't always a non-link).

Sorry didn't mean to create new work or anything, haha, just thought these were some good examples.

@maxstarkenburg
Copy link

maxstarkenburg commented Apr 27, 2020

Re: the cookie notice: In a case like this, I'd recommend trying to somehow get the cookie notice to have display: none, but it looks like the implementation is changing from opacity: 1 to opacity: 0 based on a class of show being removed? But since CSS transitions don't really play well with the display property, adding something .cookiealert:not(.show) { display: none; } would likely hide things immediately and lose your fade effect. If you wanna do something with CSS only, I think you might be able to get away w/ something like: Ignore this ugliness:

.cookiealert:not(.show) {
  animation: myHide 1s;
  animation-fill-mode: forwards;
}
@keyframes myHide {
  0% {
    visibility: visible;
  }
  99% {
    visibility: visible;
  }
  100% {
    visibility: hidden;
  }
}

but that feels quite hacky and even if it worked I think would still have the link being focusable for the first 1 second of each page load. Lemme think if I can't think of something more robust that doesn't involve just editing cookie_consent.js (though if you're open to that, that'd make things easier).

@maxstarkenburg
Copy link

maxstarkenburg commented Apr 27, 2020

Ignore above code, this feels less hacky and having just the links/buttons changing to "hidden" isn't really too noticeable given the rest of the fading and moving happening:
Ignore this one too:

.cookiealert:not(.show) a, .cookiealert:not(.show) button {
    visibility: hidden;
}

Edit: I think I was overthinking things related to animating visibility, but adding this would seem to change things to hidden once the animation via .show removal is done, but also have hidden be the default value:

.cookiealert {
  visibility: hidden;
}
.cookiealert.show {
  visibility: show
}

@kerchner
Copy link
Member

kerchner commented Apr 28, 2020

@maxstarkenburg the last suggestion worked well. Tab order is correct in both states, and there is no adverse effect on fade in / fade out.

@maxstarkenburg
Copy link

Great. I also decided to go ahead and make a pull request in the original creator's repo so that hopefully other folks might get such a change (also noticed their mystery use of aria-label="Close" which was overriding the button label, so I suggested that change as well).

@kerchner
Copy link
Member

Nice! Should I also make a modification to the code where I used aria-label="Close"?

@maxstarkenburg
Copy link

Yeah, that would be good. In my PR'd commits, I just removed it from the markup.

@kerchner
Copy link
Member

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants