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

aria-valid-attr-value rule too strict on aria-controls / aria-owns #4202

Open
WilcoFiers opened this issue Oct 18, 2023 · 2 comments · May be fixed by #4204
Open

aria-valid-attr-value rule too strict on aria-controls / aria-owns #4202

WilcoFiers opened this issue Oct 18, 2023 · 2 comments · May be fixed by #4204
Labels
Milestone

Comments

@WilcoFiers
Copy link
Contributor

The following shouldn't fail this rule:

<button aria-controls="my-modal">Open the modal</button>

This is an example from the new ARIA required ID references exist ACT rule that axe got wrong. It seems we put some logic around not requiring aria-controls not to fail when aria-expanded="false" is used. That came from combobox, but I think that when aria-controls (and aria-owns) isn't required axe-core should report it as needs review when the ref is missing, not as fail.

@WilcoFiers WilcoFiers added this to the Axe-core 4.9 milestone Oct 18, 2023
@WilcoFiers
Copy link
Contributor Author

WilcoFiers commented Oct 20, 2023

Looking at this in a little more detail. I think this is the way I'd like to have axe behave:

<!-- Pass, as aria IDREFs is not required here. Required things will be tested by other rules -->
<div aria-controls="missing">hi</div>
<!-- Incomplete, aria IDREFs is not required here, but this can be a real problem -->
<div aria-owns="missing">hi</div>
<div aria-describedby="missing">hi</div>
<div aria-labelledby="missing">hi</div>

<div role="tablist">
  <!-- Pass, no control expected -->
  <button aria-controls="missing" role="tab" aria-selected="false">hi</button>
  <!-- Pass, aria-selected default = false -->
  <button aria-controls="missing" role="tab">hi</button>
  <!-- Incomplete, because aria-controls is not required here -->
  <button aria-controls="missing" role="tab" aria-selected="true">hi</button>
  <!-- Invalid value defaults to "true" (weird that it doesn't default to the default) -->
  <button aria-controls="missing" role="tab" aria-selected="selected">hi</button>
</div>

<!-- Pass, no control expected -->
<input aria-controls="missing" role="combobox" aria-expanded="false" />
<!-- Pass. Axe will raise that aria-expanded is required -->
<input aria-controls="missing" role="combobox" />
<!-- Fail, control required -->
<input aria-controls="missing" role="combobox" aria-expanded="true" />

<!-- Pass, no control expected -->
<input aria-owns="missing" role="combobox" aria-expanded="false" />
<!-- Pass. Axe will raise that aria-expanded is required -->
<input aria-owns="missing" role="combobox" />
<!-- Fail, control required -->
<input aria-owns="missing" role="combobox" aria-expanded="true" />

I'm leaving aria-errormessage out of this, as it has its own check.

--- UPDATED ---
Decided to go with Dan's solution from below. Might need to put this in a separate rule.

@dbjorge
Copy link
Contributor

dbjorge commented Oct 20, 2023

I agree with all of the "pass" and "fail" results, and I agree with incomplete for the case of the selected tab with a missing aria-controls value.

I begrudgingly agree with "incomplete" for cases 2-4, but I'm not so sure about incomplete for the first case. I think there's a meaningful difference between aria-controls and the rest: the aria-controls property definition explicitly allows for identifying elements whose "presence" is controlled by the current element, which isn't a notion that appears in the aria-owns, aria-describedby, aria-labelledby, "ID reference list", or "ID reference" definitions - they all ultimately refer to being a "reference to the ID of another element in the same document".

That difference makes me lean towards replacing your first 4 examples with:

<!--
  Pass:
  - aria-controls explicitly allows for elements that may not be present
  - ...and unlike the "selected tab" case, there is no role-based guidance that suggests it should most likely be otherwise
-->
<div aria-controls="missing">hi</div>

<!--
  Incomplete:
  - arguably, "ID reference" is required to be an ID of another element in the same document, so this should fail
  - ...but the history from issues #1524/#2621/#1151 convinces me that incomplete is ok.
  - Maybe we should double check AT impact and consider making these fail a separate best-practice rule
-->
<div aria-owns="missing">hi</div>
<div aria-describedby="missing">hi</div>
<div aria-labelledby="missing">hi</div>

<!-- Pass (I'm pretty sure you intended these to be implied already, just clarifying) -->
<div aria-controls="present">hi</div>
<div aria-owns="present">hi</div>
<div aria-describedby="present">hi</div>
<div aria-labelledby="present">hi</div>

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

Successfully merging a pull request may close this issue.

2 participants