remove redundant ARIA roles from elements with implicit role

[metamoni]

What does this do?

Fixes #2244 by removing the role attribute from all HTML elements that have an implicit role.

Why?

Because using the role attribute on HTML elements with implicit roles is bad practice.

[cabe56]

@metamoni thanks for bringing this up, made me more aware of accessibility bad practices. I found more references like the ones you edited in this PR in the codebase, do you think we should get rid of those too?

From my limited understanding [1] [2], it seems the only appropriate use of ARIA in Administrate is search. Do you think that's the case or are there other instances that adhere to best practices in the codebase?

[metamoni]

@cabe56 I've removed all the roles I was sure about.

I wasn't sure what this code did:

<% if accessible_action?(resource, :show) %>
  <%= %(tabindex=0 role=link data-url=#{polymorphic_path([namespace, resource])}) %>

If this creates a link_to or an a element, then it's definitely safe to remove the role="link", because the link_to helper outputs an a element, which has that role already. Judging by the fact that there's a tabindex=0 on this, my guess was that this is a non-interactive element (a span or div or something similar), which needs to be forced to be clickable (that's what tabindex=0 does in this context). If this is the case, the role="link" is needed here to communicate that this element is intended to be used as a link. It would be helpful to see this in the right context. Happy to remove it if you think it's needed, let me know what you think.

No harm in keeping the one in the Search form (<form class="search" role="search">), as form doesn't have an implicit role and specifying the search role gives assistive tech users more context.

Other than that, I think I removed all the references in this PR, but let me know if I've missed something 🤔

[cabe56]

I wasn't sure what this code did:

I had to read it several times to figure out it was adding attributes to the tr in the line above . Confirmed in my instance of Administrate that the role=link applies this style meant to signal the whole row is clickable.

These rows usually have a elements inside table cells that are also clickable and lead to other resources :S

We could create another issue about removing this link role + styling as it takes more work and this PR is already a good addition to the codebase.

Other than that, I think I removed all the references in this PR, but let me know if I've missed something 🤔

My bad, you are right. Went through all the references and you left just these related to the link that are going to take a bit more work to get rid of.

[metamoni]

We could create another issue about removing this link role + styling as it takes more work and this PR is already a good addition to the codebase.

That sounds like a good idea. It might require some rethinking/redesign. It sounds like you're just looking to create links inside a table. As long as you make sure the links are either simple a elements or link_to elements, the role should not be needed. Either way, it sounds like there's no need for the rows themselves to be focusable/clickable, so you'll then be able to remove the tabindex=0 on them too.

Happy to take a look at the issue/PR when it's ready, so feel free to tag me. If you let me know what the steps are to get to the right page, I can pull the branch and test the behaviour with a keyboard and screen reader.

[pablobm]

Thank you so much for this!

Re: that tabindex=0 role=link, it was introduced in this PR: #1557. The idea was to make the whole row clickable when possible, leaving links in the individual cell contents are a fallback. The reason is that the table row is a block element (or whatever table rows are) as opposed to the content of each cell being an inline element, more difficult to click on. Thoughts?

[cabe56]

@pablobm thanks for the context.

IMO this PR is good to go as @metamoni did not remove the link role which seems to be the only usage that has unintended consequences in other Administrate features (making rows look clickable as you mention).

We can deal with that specific link role in a separate PR as it requires more thought (as @metamoni mentions here #2245 (comment)).

With this PR merged and a new PR for link we can close #2244.

[cabe56]

Was about to write an update to the issue regarding my suggestion about another PR, but it seems that I overcomplicated this whole thing: could we just change the CSS selector from role=link to something else?

For example, if we just replace role=link with a reference to .js-table-row we get the same styling without using a role.

[metamoni]

@pablobm Hmm thanks for the context, it's really useful to understand why this change was introduced. I get the intention behind it, but I don't think that behavior of tabbing through rows and cells that way is an established pattern. This article by Leonie Watson explains how real assistive tech users would navigate a table. Unfortunately, I think making non-interactive elements focusable would actually make for a confusing experience in this case.

It's best to stick to semantic HTML and leave links as interactive elements, as assistive tech gives users various ways to navigate the page, navigate tables, find links, etc.

[brunoprietog]

Hi, as a blind user here, I confirm that putting a role=link in a tr breaks the table navigation commands for screen readers.

[pablobm]

Thank you all for your input! I agree that this PR is good to go. Merging.

Re: the row links, it does seem that you are all right and the current implementation is counterproductive. @metamoni - Would you be able to create an issue as you propose? Then we can agree on a suitable alternative.