fix(nested-interactive/aria-text): allow "tabindex=-1" on elements with no role

[dan-tripp]

Closes issue: #2934

[dan-tripp]

"ci/circleci: test_win — Your tests failed on CircleCI" - I can't tell if that test failed because of my changes, or if it was going to fail anyway. code owners - let me know if I should do anything.

[bastilimbach]

@dan-tripp Thanks for fixing this!
I came across a different case where tabindex="-1" should be ignored: presentation-role-conflict

Fails:

<button>
  Test
  <svg class="icon" tabindex="-1" role="presentation">...</svg>
</button>

Works:

<button>
  Test
  <svg class="icon" role="presentation">...</svg>
</button>

Codepen:
https://codepen.io/bastilimbach/full/KKqEVGG

Is this something you are willing to include in this PR? I can also create a separate issue.

[dan-tripp]

@bastilimbach I'd be happy to, and that change seems sensible to me. However, I'm new here, so I think that I had better leave it to the core team to decide what the desired behaviour ought to be. If it's not too much trouble, could you create a new issue and tag me? Then I'll try to make a separate PR for your new issue, if it gets some approval from the core team.

[straker]

@bastilimbach so with presentation-role-conflict that result from axe-core is correct. The algorithm takes into account any value of tabindex, so testing a similar example all browsers and screen readers ignore the role=presentation on the element.

<ul>
  <li tabindex="-1" role="presentation">World</li>
</ul>

Chrome - Treats the li element as a listitem role
Firefox - Treats the li element as a listitem role
Safari / VO - Announces "group" (instead of no role)

The reason is that because of the tabindex the element can be focused programmatically which means that screen readers must announce a role for the element (it can't be presentational).

[straker]

@dan-tripp Thanks for taking on this pr. I've been trying to figure out if there were any problems with allowing tabindex=-1 on the descendants of role=text. What I've decided is that we should hold off on doing this.

The reason for that is that this change directly affects the nested-interactive rule, allowing tabindex=-1 on descendants of interactive elements to pass the rule. It was pointed out that doing this would pose problems for screen readers and so we decided to create new messages to point that out. So before we do this we first need to resolve that issue.

If this change only affected the aria-text rule I would be fine with it, but since it also affects the nested-interactive rule we should hold off.

[dan-tripp]

@straker Many thanks for your comments. I'll look into that new issue (#3163).

[straker]

@dan-tripp I believe this pr is replaced by #3194, since we don't want tabindex=-1 to pass but now warn of the problem.

[straker]

Alright. Discussed with @WilcoFiers after thoroughly testing things. Testing both role="text and interactive roles (.e.g role="button"), we've determined that a tabindex can be allowed under certain conditions. Basically if the element with the tabindex is not a widget role, then tabindex on the element is fine. Otherwise we should still flag it as we do now.

So something like this:

// no-focusable-content-evaluate.js
import { getRole, getRoleType } from '../../commons/aria';

function getFocusableDescendants(vNode) {
  if (!vNode.children) {
    if (vNode.props.nodeType === 1) {
      throw new Error('Cannot determine children');
    }

    return [];
  }

  const retVal = [];
  vNode.children.forEach(child => {
    const role = getRole(child);

    if(getRoleType(role) === 'widget' && isFocusable(child)) {
      retVal.push(child);
    } else {
      retVal.push(...getFocusableDescendants(child));
    }
  });
  return retVal;
}

[dan-tripp]

My latest commits on this PR are ready for review. Let me know what you think. And thank you for your patience.

[WilcoFiers]

Close, couple more minor things

[dan-tripp]

Okay, I think this PR is ready for review again. Thank you for all your suggested changes, @WilcoFiers - I think that I'm getting the hang of this.

[WilcoFiers]

Reviewed for security

[WilcoFiers]

@dan-tripp Thank you so much for this contribution! Great work, really appreciate it.

[dan-tripp]

Great! Glad I can help.