accessibility changes - Githubissues

mreinstein commented 5 years ago

My employer has recently hired very talented consultants to evaluate how accessible our software is. They've also evaluated tippy since it's part of that product. Mostly the review came back pretty clean, but they flagged a few issues related to some aria-* property declarations.

Would this feedback be something you'd be interested in? I'm happy to forward the tippy specific stuff if you'd find that useful.

atomiks commented 5 years ago

That would be nice yes 🙂

mreinstein commented 5 years ago

In my particular use case, I have a group of buttons, and clicking each one opens a tippy next to it's associated button.

Some of those tippy elements are not interactive (they just have static text in them or an image) and some of them are interactive (they embed a carousel/item slider, or a video, etc.)

For the non-interactive tippy + button pair, the markup looks like this:

<button aria-label="travel friendly"
        aria-expanded="true">
        <!-- icon button's svg element -->
        <svg aria-hidden="true" focusable="false" viewBox="0 0 470 470"> ...</svg>
</button>

<!-- the element managed by tippy -->
<div class="acmeinc-popper" 
        id="acmeinc-3"
        role="tooltip"
        tabindex="-1"
        x-placement="right"
        style="...">
        ...
</div>

the role="tooltip" should be set on non-interactive tippy elements
aria-expanded should be set to true or false based on if the tippy is open or not

For interactive tippys, the markup looks like this:

<button aria-label="works on dc or ac power"
        aria-expanded="true"
        aria-controls="acmeinc-2">
        <!-- icon button's svg element -->
        <svg aria-hidden="true" focusable="false" viewBox="0 0 470 470"> ...</svg>
</button>

<!-- the element managed by tippy -->
<div class="acmeinc-popper" 
        id="acmeinc-2"
        role="tooltip"
        tabindex="-1"
        x-placement="right"
        style="...">
        ...
</div>

no role attribute is needed on a tippy on interactive elements
aria-expanded should be set to true or false based on if the tippy is open or not
aria-controls references the id of it's tippy, so screen readers can associate the button to it's tippy content.

atomiks commented 5 years ago

Can that safely be applied to any reference element type (not just button)?

As of now, I have been recommending the following (which requires manual work): https://atomiks.github.io/tippyjs/accessibility/#creating-accessible-interactive-tooltips

Making this automatic and default would be great, but I want to ensure it has no unintented consequences. Also, the user still needs to think about how focus works with their interactive tippys which is not possible for the library to automatically deal with properly.

mreinstein commented 5 years ago

Can that safely be applied to any reference element type (not just button)?

I believe we can uniformly apply aria-expanded to the reference element. Right now the accessibility sample code on tippy's doc website (https://atomiks.github.io/tippyjs/accessibility/) shows wiring up this state manually via onMount and onHide. This could be internalized to tippy's guts, but it doesn't have to be. The example accessibility code works correctly as-is.

I think believe this is the set of functionality that should change (pseudocode):

referenceElement.setAttribute('aria-controls', tippyElement.id)

if (options.interactive) {
   tippyElement.removeAttribute('role')
} else {
   tippyElement.setAttribute('role', 'tooltip')
}

/*
optional: toggle aria-expanded attribute on referenceElement based on
the visibility of it's associated tippyElement
*/

mreinstein commented 5 years ago

There was one additional bit of feedback, regarding making sure that when the focus leaves the tippy popover, the tippy automatically closes. I don't know if there's a way to do this programmatically. In my code I basically find the last element in the tippy content, attach an onblur event listener, and close the tippy.

That might be brittle to try to code into tippy itself. Perhaps it just gets added to the accessibility documentation section on the tippy website so we're at least setting people up for success?

atomiks commented 5 years ago

referenceElement.setAttribute('aria-controls', tippyElement.id)

Is this article still relevant? Seems like we should avoid adding it automatically.

https://www.heydonworks.com/article/aria-controls-is-poop

Also for the aria-describedby attribute, the user can set null to prevent it from being added altogether, because I recommend to disable it if interactive. Should we disable that attribute as well if it's interactive forcefully? Maybe there are some scenarios where they'd want the tippy content to be announced when expanded straight away, despite focus not being inside the tippy element. But having it enabled by default seems to cause unexpected accessibility problems.

There was one additional bit of feedback, regarding making sure that when the focus leaves the tippy popover, the tippy automatically closes

I also wondered about that, and I think it's not actually necessary. GitHub doesn't automatically close the popover either (you can try it out on the reactions thing). I think the reasoning is like this:

The keyboard user should decide when the popover should be closed by doing it themselves, just like a mouse user would "clean up" the popover by clicking elsewhere for example. So it shouldn't actually hide automatically when focus leaves the element.

mreinstein commented 5 years ago

Is this article still relevant?

I can't speak to that article, but here is the feedback verbatim from our accessibility experts:

Use the aria-controls attribute to programmatically define the relationship between
the button and the controlled popover. This allows assistive technologies to move
focus to the associated panel. For example, JAWS (most widely used screen reader)
announces, “Use JAWS key + ALT + M to move to controlled element”

I think it's not actually necessary. GitHub doesn't automatically close the popover either

I can't speak to how accessible github's popover experience is, but according to the accessibility experts we spoke to here is their guidance, verbatim:

When users navigate out of popovers, focus goes to content behind the panel
and there is no visible indication of focus. 
If users navigate out of the popover, close the panel so that the user’s
focus is not masked by the panel.

It sounds like the problem is the element that has focus can often be visually obscured by the open tippy element, and that has implications for people that have low vision conditions.

atomiks commented 5 years ago

I can't speak to that article, but here is the feedback verbatim from our accessibility experts:

I think they're only considering JAWS screanreaders judging by that excerpt, as the article mentioned it's only supported (and in limited fashion) by that. I think it isn't too difficult to add your own attribute here, so I'll leave this attribute out.

It sounds like the problem is the element that has focus can often be visually obscured by the open tippy element, and that has implications for people that have low vision conditions.

Makes sense, although, it makes equal sense that a keyboard user who's blind (not just low vision) once tabbed out of the popover without explicitly closing it, would expect backtracking in tab order to return to one of the popover elements would not find it to be closed. That's just my reasoning. If you want, you can send a PR detailing a solution for this if necessary, but I don't think it should be automatic as it can be done in userland.

As far as I can tell from this issue when it comes to including code in the core library,

Need to set aria: null by default if interactive: true, so that the popover's contents are not announced upon expanding the tippy (without moving focus to within it)
Need to set aria-expanded: true onMount if interactive: true, so the screenreader knows there's a popup that has been shown
The other ones (aria-controls, close on blur) can be done by the developer themselves and are not critical to basic accessibility function imo, since there are limitations and counterpoints to them.

mreinstein commented 5 years ago

they're only considering JAWS screanreaders

Yes, the only known screen reader that supports aria-controls right now is Jaws.

I would argue though there is no downside to including this attribute. 50% of the existing screen readers out there will be able to use it today and get a better experience. The other screen readers will simply ignore this attribute.

It very likely aria-controls will get added to other screen readers in the near future (NVDA is the 2nd most popular, and it has an open issue to add it, which has some interest https://github.com/nvaccess/nvda/issues/8983)

The problem with doing this in user land is the user needs to resort to some detection of what the id of it's associated tippy element is. (it's not exposed anywhere.) It would be a lot easier if tippy simply set this on it's reference element, since there's no downside.

All of the other issues would be nice to have but not critical.

atomiks commented 5 years ago

The problem with doing this in user land is the user needs to resort to some detection of what the id of it's associated tippy element is. (it's not exposed anywhere.)

The instance has an id property though?

mreinstein commented 5 years ago

The instance has an id property though?

That only gives the id of the popper, not the id attribute of it's associated dom element

atomiks commented 5 years ago

You mean tippy-${id}? That is instance.popper.id (or instance.popperChildren.tooltip.id in v5), or am I missing something?

mreinstein commented 5 years ago

am I missing something?

I'm getting a bit of a defensive vibe here.

I asked you if you wanted feedback on the parts of the accessibility audit that tippy has failed at. If you don't wish to apply them that's certainly up to you, but I can tell you that these extra bits in userland will need to be applied to pass wcag 2.0 guidelines for website accessibility every time a tippy is employed.

atomiks commented 5 years ago

That wasn't my intention, I don't understand which id you're referring to. Tippy exposes the popper element's id on the element itself instance.popper.id in v4. That allows you to set an aria-controls attribute if needed on the reference element.

But regqrdless, I fail to see how aria-controls solves anything given it has limitations. The article states that it can make things worse in many cases, I'd rather leave it up to the developer than cause additional problems.

So while many screenreaders will ignore it, the article states that it causes problems with JAWS itself:

It’s poorly supported, does very little, and does what it does when it does badly.

The aria-controls attribute is a prime example of something we’d love to ‘just work’. Trouble is, there’s no clear way how it should work. JAWS makes a perfunctory attempt at implementation, but it’s incomplete and I suspect it creates much more confusion than it provides clarity for real users.

aria-expanded, I agree with. And the aria-describedby thing for interactive tooltips. But the aria-controls thing, I don't see how given the problems it causes as stated by the article. If 50% of screenreader users don't support it, and the remaining 50% just causes additional problems, is it really necessary?

We can keep the issues open because those need to be solved still..

atomiks commented 5 years ago

Just to reinforce my points about aria-controls: https://github.com/w3c/aria/issues/995

If you encounter an element on a web page with a defined ARIA controls relationship, JAWS will no longer say “use JAWSKEY+ALT+M to move to controlled element” by default. In most cases, the target of the controls relationship is adjacent to the element or does not provide any useful information.

It is pretty verbose It doesn't allow you to jump back from the controlled element to the controlling element It does not deal properly with an element controlling multiple other elements, which is allowed according to the spec.

re: mention in ARIA WG call today, JAWS does have this set to ignore by default now.

I think at the least in the short term, in the aria-practices document, we can leave off aria-controls when the content that is being controlled immediately follows the controller.

So to summarize, just have the popover directly after the reference element in the source order and use aria-expanded. There's no need for aria-controls because it doesn't solve anything useful.

I'm sure aria-controls might be "fixed" in the future to provide a way to connect the popover with the reference when they don't exist directly next to each other in tabbing order. But the implementation isn't there yet and not supported properly. Therefore I wouldn't recommend using it currently, and this library has no use for it.

I hope that clears up why I don't want to add this by default (and why you should probably not ever add it yourself). These guys are from w3c and know more than anyone else I'd say.

atomiks commented 5 years ago

I've pushed a fix for the role problem. As for aria-expanded, given how I've been suggesting manual fixes for v4, I believe that I'll have to leave it for v5 in case it might cause conflicts or problems with the user's code, as it might be a breaking change somehow.

atomiks commented 5 years ago

I've pushed a fix for the role problem.

@mreinstein after checking Google Docs interface it looks like their menu popovers (e.g. font picker) can use role="menu" with the list items having role="menuitem". This means an interactive tippy shouldn't disable the role automatically. If need be, you can set role: null and it will remove the role. So it's not possible to forcefully disable the role attribute here unfortunately.

mreinstein commented 5 years ago

I pointed our accessibility experts at this thread, to get a 3rd opinion. From someone that is an authority on actual a11y issues. Here is what they said:

Heydon's article on aria-controls has some valid points, but the conclusion in the article is 
overstated and dismissive. It is frustrating that aria-controls is only fully supported in JAWS, but 
that's a large user-base that will be familiar with the interaction pattern. Correctly using the aria-
controls attribute has no negative consequences at all, and greatly aids some assistive technology
 users. It is part of the standard design for a disclosure widget, defined in the WAI-ARIA authoring
 practices: https://www.w3.org/TR/wai-aria-practices-1.1/#disclosure

Regarding the point about closing the panels if a user navigates out of the panel being problematic
 for screen reader users navigating back and expecting to navigate into the panel: If focus
 management is handled correctly, this is not an issue. Navigating backwards would take the user
 to the button that triggered the disclosure panel, and they would hear that it has been closed, and
 can open it again. However, focus going to elements behind the panel is a serious issue, and a
 failure of WCAG 2.1 2.4.7 focus visible: https://www.w3.org/TR/WCAG/#focus-visible

atomiks commented 5 years ago

Seems like valid points.

Correctly using the aria-controls attribute has no negative consequences at all

Optionally, the element with role button has a value specified for aria-controls that refers to the element that contains all the content that is shown or hidden.

It seems like it can be optional, and should only be used correctly. Which means it would only work for a certain type of UI element.

Due to that, I don't think it should be added by default because it depends on the type of UI element that you are creating. A tippy is a generic popover which can be a menu, tooltip, popover, dropdown, or something else. If I'm reading it correctly, not every interactive tippy needs the attribute.

To add it in v4, you can just do:

const instance = tippy(reference);
reference.setAttribute('aria-controls', instance.popper.id);

It's only another line you need to add to get that behavior. It seems like something the developer needs to manually think about and take care with, just like focus management (which tippy can't reliably do for you)

atomiks commented 5 years ago

As for the blur thing, that seems perfectly valid to me.

idk about adding it in core though, I remember v3 had something similar and sometimes it caused issues (shouldPopperHideOnBlur). It seems like another thing developer should add in manually like aria-controls. Not all the time is there an element underneath the popover that shows a focus ring that's obstructed and depends on the UI (e.g. GitHub doesn't need it).

My philosophy is just to keep core as lean as possible to reduce surface area for bugs. If the dev can add it in, and it's not absolutely critical in all scenarios, I'm going to leave it out.

The aria-expanded thing seems critical to me, so I will add it in.

mreinstein commented 5 years ago

so one thing I discovered though this accessibility evaluation: When setting up aria-controls it needs to be on a stable element. (it needs to be in the dom ahead of time so the screen reader can associate them.

Here's the accessible markup structure:

<button aria-controls="acmeinc-storypoints-callout-popper-1"> ... </button>
<div id="acmeinc-storypoints-callout-popper-1">
    <!-- tippy injects it's DOM here -->
    <div class="tippy-popper"> ... </div>
</div>

And then the setup code:

const storypointId = 'acmeinc-storypoints-callout-popper-1';

const button = document.createElement('button');
button.setAttribute('aria-controls', storypointId);

const storypointDom = document.createElement('div');
storypointDom.id = storypointId;

const tippyConfig = {
   appendTo: storypointDom,
   // ...
}

Something to consider, maybe worth updating the accessbility docs to account for this.

atomiks commented 5 years ago

@mreinstein I think you need to use the lifecycle functions for that

Btw can you ask the accessibility people about this? I asked on Twitter:

With ARIA attributes, when should changes occur when the element has a transition animation?

e.g. should aria-expanded be set to true once the popover's transition finishes, or as soon as it starts? #a11y

If the duration is long, setting it to true at the end causes a long delay for those users before they know it's expanded. They can interact with the popover before the transition ends as well

But Bootstrap seems to wait until the end of the transition (and that's how I currently do it)

^ same with aria-describedby

mreinstein commented 5 years ago

Btw can you ask the accessibility people about this?

This advice was relayed directly from the accessibility people. I've already adopted the necessary changes in my fork. I'm just listing this as a courtesy here.

mreinstein commented 5 years ago

oh, sorry I misunderstood your statement. Yes I can ask them.

atomiks / tippyjs

accessibility changes #546