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

Change these allowed attributes to required as per ARIA 1.1 spec #969

Closed
iamrafan opened this issue Jun 25, 2018 · 13 comments
Closed

Change these allowed attributes to required as per ARIA 1.1 spec #969

iamrafan opened this issue Jun 25, 2018 · 13 comments
Labels

Comments

@iamrafan
Copy link
Contributor

The attributes mentioned in the table below are required by ARIA, but allowed by Axe, because they have a default value. But looking at the description of the default values seems that the default value would not be applicable on the respective roles mentioned and hence they are required to be explicitly defined.

Role Attribute required by ARIA Attribute's default value description
Checkbox Menuitemcheckbox Menuitemradio Radio Aria-checked The element does not support being checked.
Combobox Aria-expanded The element, or another grouping element it controls, is neither expandable nor collapsible; all its child elements are shown or there are no child elements.
Scrollbar Aria-orientation The element's orientation is unknown/ambiguous.
Treeitem Aria-selected The element is not selectable.
axe-core version: 3.0.3
@WilcoFiers
Copy link
Contributor

@dylanb would you say this is a breaking change, or can we do this is a minor?

@dylanb
Copy link
Contributor

dylanb commented Jul 2, 2018

Using semver's major.minor.patch meaning, I would do this as a minor release change because it does change the results of the rules due to a change in interpretation, not a bug fix

@WilcoFiers
Copy link
Contributor

@dylanb The problem is that this is a change we can't undo with axe.configure(). This is kind of a question about how we want that to work. What I've been striving for is that we get to a point where custom rulesets are only tied to major versions of Axe-core. So, I would write a ruleset for 3.0.3 and can assume it's going to get me consistent results for all future 3.x versions. It can turn up fewer violations due to bug fixes, but it shouldn't add new ones.

That's the reason we introduced the disableOtherRules option to axe.configure() - to allow you to future proof your custom rule, not just for patch releases but for minors as well. The idea was that that would make it much easier to use custom rules, and much easier to always have the latest bug fixes from Axe-core, without having to go and rebuild and distribute a new custom rules file every month.

A change like this - a change to a commons, essentially undoes this and makes the disableOtherRules functionality unreliable.

One way we could go about solving this would be to move more work to check options. If we start including check options into custom rules - we can change options in minor releases, and just have custom rules override those. It seems like that could be done without breaking the reliability of axe.configure(), but we'd have to upgrade how custom rules get generated first.

@iamrafan
Copy link
Contributor Author

iamrafan commented Jul 3, 2018

As discussed, here are the AT test results

aria-checked on role checkbox

Link to test page: https://codepen.io/iamrafan/pen/XPbRag?editors=1000

  Browser/AT Aria-checked="true" Aria-checked="false" No aria-checked attribute
1 Edge/Narrator Checked "label" checkbox Unchecked "label" checkbox Unchecked "label" checkbox
2 Edge/JAWS Checkbox checked Checkbox not checked Checkbox not checked
3 Edge/NVDA Checkbox checked Checkbox not checked Checkbox not checked
4 Chrome/NVDA Checkbox checked Checkbox not checked Checkbox not checked
5 Chrome/JAWS Checkbox checked Checkbox not checked Checkbox not checked
6 IE/JAWS Checkbox checked Checkbox not checked Checkbox not checked
7 IE/NVDA Checkbox checked Checkbox not checked Checkbox not checked
8 Safari/voiceover Checked checkbox Unchecked checkbox Unchecked checkbox
9 Firefox/NVDA Checkbox checked Checkbox not checked Checkbox not checked

@dylanb
Copy link
Contributor

dylanb commented Jul 11, 2018

@WilcoFiers The assumption should be that custom rules work for all patch releases of a version. The reason for this is that we have decided in the past that new rules can show up in a minor release. This means that custom rule sets that do not disable that rule, will end up with that rule.

Major releases are reserved for breaking API changes or significant additions of functionality.

@WilcoFiers
Copy link
Contributor

@dylanb that's what disableOtherRules was designed to fix. If you set that, which we now do for all custom rulesets - undeclared rules (which are all the new ones) won't run. As of 3.0 we could choose to support custom rules for major versions as long as we're careful not to introduce breaking things in commons. That is something you asked us to do in the past.

@WilcoFiers
Copy link
Contributor

@iamrafan Following up on this... going from your testing, it sounds like leaving off aria-checked for checkbox works just fine. It's a good bet that it'll work for the others too. That makes me hesitant to require it. It both works, and there's a very reasonable use case for it:

const elmHtml = `<div ... ${checked ? 'aria-checked="true"' : ''}`

Given that this works, despite what ARIA says, I would say that flagging this as an error is a false positive. So I don't think we should change this. I think we should do some AT testing for aria-expanded on combobox, see if it too defaults to false. If not, we add it, but if it does, same situation. I think that shouldn't be failed because it can be a false positive.

@iamrafan
Copy link
Contributor Author

iamrafan commented Aug 13, 2018

@WilcoFiers Sure, here are the test results for aria-expanded on role combobox

Link to test page: https://codepen.io/iamrafan/pen/EejvjY?editors=1000

  Browser/AT aria-expanded="true" aria-expanded="false" No aria-expanded attribute Does aria-expanded default to false?
1 Edge/Narrator Editable combobox expanded Editable combobox collapsed Editable combobox No
2 Edge/JAWS Combobox Combobox Combobox N/A
3 Edge/NVDA combobox expanded combobox collapsed combobox No
4 Chrome/NVDA "label" combobox expanded "label" combobox collapsed "label" combobox No
5 Chrome/JAWS "label" combobox "label" combobox "label" combobox N/A
6 IE/JAWS "label" combobox "label" combobox "label" combobox N/A
7 IE/NVDA "label" combobox expanded "label" combobox collapsed "label" combobox No
8 Safari/voiceover "label" combobox "label" combobox "label" combobox N/A
9 Firefox/NVDA combobox expanded combobox collapsed combobox collapsed Yes

aria-expanded does not default to false in a few cases and it is not supported at all in few other cases.

@iamrafan
Copy link
Contributor Author

iamrafan commented Aug 13, 2018

AT test results for aria-orientation on role scrollbar:

Link to test page: https://codepen.io/iamrafan/pen/WgvEQy?editors=1000

  Browser/AT aria-orientation= "horizontal" aria-orientation= "vertical" aria-orientation="" No 'aria-orientation' attribute Does aria-orientation default to some value?
1 Edge/Narrator Not announced Not announced Not announced Not announced N/A
2 Edge/JAWS Not announced Not announced Not announced Not announced N/A
3 Edge/NVDA Not announced Not announced Not announced Not announced N/A
4 Chrome/NVDA Not announced Not announced Not announced Not announced N/A
5 Chrome/JAWS Horizontal Not announced Not announced Not announced Default behavior same as vertical
6 IE/JAWS Horizontal Not announced Not announced Not announced Default behavior same as vertical
7 IE/NVDA Not announced Not announced Not announced Not announced N/A
8 Safari/Voiceover Horizontal Vertical Vertical Vertical Defaults to vertical
9 Firefox/NVDA Not announced Not announced Not announced Not announced N/A

Looks like aria-orientation defaults to vertical only in Safar/VO

@iamrafan
Copy link
Contributor Author

iamrafan commented Aug 13, 2018

AT tests for aria-checked on role radio

Link to test page: https://codepen.io/iamrafan/pen/EejvKx?editors=1000

  1. <div role="radio" tabindex="1" aria-checked="?????">label</div>
  Browser/AT aria-checked=  "true" aria-checked=  "false" No 'aria-checked' attribute Does aria-checked default to false?
1 Edge/Narrator Not announced Not announced Not announced N/A
2 Edge/JAWS Checked Not checked Not checked Yes
3 Edge/NVDA Checked Not checked Not checked Yes
4 Chrome/NVDA Checked Not checked Not checked Yes
5 Chrome/JAWS Checked Not checked Not checked Yes
6 IE/JAWS Checked Not checked Not checked Yes
7 IE/NVDA Not checked Not checked Not checked N/A
8 Safari/voiceover Selected Not announced Not announced Default behavior same as false
9 Firefox/NVDA Checked Not checked Not checked Yes
  1. <input type="radio" aria-checked="???" aria-label="label">label
  Browser/AT aria-checked=  "true" aria-checked=  "false" No 'aria-checked' attribute Does aria-checked default to false?
1 Edge/Narrator Selected Non-selected Non-selected Yes
2 Edge/JAWS Checked Not checked Not checked Yes
3 Edge/NVDA Checked Not checked Not checked Yes
4 Chrome/NVDA Checked Not checked Not checked Yes
5 Chrome/JAWS Checked Not checked Not checked Yes
6 IE/JAWS Not checked Not checked Not checked N/A
7 IE/NVDA Not checked Not checked Not checked N/A
8 Safari/voiceover Not announced Not announced Not announced N/A
9 Firefox/NVDA Not checked Not checked Not checked N/A

@iamrafan
Copy link
Contributor Author

iamrafan commented Aug 14, 2018

@WilcoFiers Looking at the AT tests, do you agree with my conclusion that

  • we need to change aria-expanded on combobox to required
  • let aria-checked on radio be a allowed attribute as it either defaults to false or is not supported at all

I think there is no need to make any changes for

  1. scrollbar
    - aria-orientation may be moved to a supported attribute: Scrollbar aria-orientation required property? w3c/aria#780

  2. treeitem , menuitemcheckbox, menuitemradio
    - It is unclear if the respective attributes on these roles should be explicitly defined on them or if they get inherited from parent roles.
    - Filed this issue with ARIA Clarify if aria-selected & aria-checked are required on roles 'treeitem' & ('menuitemcheckbox', 'menuitemradio') respectively w3c/aria#798

@dylanb
Copy link
Contributor

dylanb commented Aug 22, 2018

Can we make some test pages that allow us to validate/test when these change?

@iamrafan what pages do you use to test these and can you contribute those to this repo?

@iamrafan
Copy link
Contributor Author

@dylanb I create basic samples using only the minimum required ARIA to test the particular scenario. I have added links to test pages in the comments above. I am happy to contribute the test pages to the repo as well, please provide me a placeholder where I can add these tests

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

No branches or pull requests

4 participants