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

Report case sensitivity issues #2695

Closed
WilcoFiers opened this issue Dec 16, 2020 · 22 comments · Fixed by #4358 · 4 remaining pull requests
Closed

Report case sensitivity issues #2695

WilcoFiers opened this issue Dec 16, 2020 · 22 comments · Fixed by #4358 · 4 remaining pull requests
Assignees
Labels
good first issue For first-time contributors rules Issue or false result from an axe-core rule

Comments

@WilcoFiers
Copy link
Contributor

Saw a question come up today about:

<ul role="list">
  <li role="listItem">...</li>
</ul>

The problem is that capital letter "I" in "listItem". There are two possible issues there:

  1. I'm not so sure this should be case sensitive, we should test our ARIA rules to figure out where case sensitivity matters and where it doesn't. If it followed the specs, it should be case insensitive in HTML and case sensitive in XHTML.

  2. In such cases, it would be good for axe-core to explicitly call out that the item was failed because of case sensitivity.

  3. It would be nice to report this particular case as needs review instead of a fail, because the implicit role here is the correct role. This is a false positive.

@WilcoFiers WilcoFiers added fix Bug fixes rules Issue or false result from an axe-core rule labels Dec 16, 2020
@WilcoFiers
Copy link
Contributor Author

May be enough to typecast the result of getRole(). We'll need to test if this works reliably.

@jnurthen
Copy link

I'm not so sure this should be case sensitive, we should test our ARIA rules to figure out where case sensitivity matters and where it doesn't. If it followed the specs, it should be case insensitive in HTML and case sensitive in XHTML.

This is defined in https://w3c.github.io/html-aria/#case-sensitivity - it MUST be lower case only

@dylanb
Copy link
Contributor

dylanb commented Mar 24, 2021

@WilcoFiers close?

@WilcoFiers
Copy link
Contributor Author

This is an ongoing discussion, see w3c/html-aria#280, w3c/html-aria#287 and w3c/aria#1264.

I ran the following through a few AT:

<button role="heading">lower-case</button>
<button role="HEADING">upper-case</button>

The latest versions of all the AT/browser combinations axe-core supports treats the role as case insensitive. That includes Safari+VO, JAWS+IE, NVDA+Firefox, NVDA+Chrome. I found it wasn't working in my older version of Firefox, but after upgrading to the latest one, there was no issue.

In conclusion, axe-core should normalise to lower case when testing anything to do with the role attribute. When this issue was opened, I still think this was an issue, but now that Firefox has changed I think we can reasonably say that using upper case in roles doesn't matter.

@dylanb
Copy link
Contributor

dylanb commented Jun 7, 2021

what about Dragon?

@jnurthen
Copy link

jnurthen commented Jun 7, 2021

@dylanb looking at the extension code it should be ok
I see the line role = role ? role.toLowerCase() : "";

@dylanb
Copy link
Contributor

dylanb commented Jun 8, 2021

@WilcoFiers , if Dragon does in fact support all cases, then I think we should put this into the category of warning

@WilcoFiers
Copy link
Contributor Author

We don't really have a "warning" category. If something works, I'm not sure why would need to tell someone about it. Seems like that'd only create more noise.

@straker
Copy link
Contributor

straker commented Feb 7, 2022

Just confirmed with @mfairchild365 that dragon is case sensitive. The following does not work in Dragon

<div role="Button">Capital B</div>
<div role="BUTTON">All caps</div>
  • Dragon does this via document.querySelectorAll (case sensitive attribute values by default) but apparently this would work if Nuance decided to finally update their code… document.querySelectorAll("[role=button i]")
  • Dragon will discover the element if it has an onclick attribute and then parse out that it is a button no matter what case was used (this threw me through a loop at first because I tested by adding onclick to all the buttons and it somehow worked)

@jnurthen
Copy link

jnurthen commented Feb 7, 2022

Just confirmed with @mfairchild365 that dragon is case sensitive. The following does not work in Dragon

<div role="Button">Capital B</div>
<div role="BUTTON">All caps</div>
  • Dragon does this via document.querySelectorAll (case sensitive attribute values by default) but apparently this would work if Nuance decided to finally update their code… document.querySelectorAll("[role=button i]")
  • Dragon will discover the element if it has an onclick attribute and then parse out that it is a button no matter what case was used (this threw me through a loop at first because I tested by adding onclick to all the buttons and it somehow worked)

Looking at the code this looks correct. I was only looking at the clickable code branch not the non-clickable code branch previously.

@WilcoFiers
Copy link
Contributor Author

WilcoFiers commented Feb 9, 2022

Alright, in that case I think a new check in the aria-roles rule would be useful here. Have the rest be case insensitive, since that's how it's supposed to work, but call out use of uppercase as not being widely supported. Thanks for checking Dragon on this Steve.

@WilcoFiers WilcoFiers added good first issue For first-time contributors and removed hackathon labels Feb 9, 2022
dan-tripp added a commit to dan-tripp/axe-core that referenced this issue Feb 26, 2022
dan-tripp added a commit to dan-tripp/axe-core that referenced this issue Apr 15, 2022
@dan-tripp
Copy link
Contributor

I've been working on this and I could use some guidance.

One hurdle is to make each check's CSS selector case-insensitive. I couldn't find a case-insensitive CSS selector option. So I used parser.registerAttrEqualityMods() to make a generic regex CSS selector option, then I used the regex "i" flag to get case-insensitivity.

For example, this lets me change page-no-duplicate-banner.json from this:
"selector": "header:not([role]), [role=banner]",
to this:
"selector": "header:not([role]), [role/='/banner/i']",

It works. But is it the cleanest way?

@straker
Copy link
Contributor

straker commented May 19, 2022

Sorry, forgot to respond to this. I'll try taking a look at this in the next few days and see if I can help you with guidance.

@straker
Copy link
Contributor

straker commented Jun 1, 2022

@dan-tripp Alright. Talked with @WilcoFiers and we decided that we just want to deal with informing users of the case sensitivity problem and deal with axe-core being case insensitive in a separate issue.

So what we'll want to do is update the invalidrole check to call toLowerCase on the role before it passes it to isValidRole (that will stop aria-roles from failing on the roles). We'll then want to add a new check to the aria-roles rule that specifically looks for roles that are not all lower case and flag them as Needs Review (i.e. returns undefined) and informs the user that roles that are not all lower case are not widely supported.

@dan-tripp
Copy link
Contributor

Right on. I think that I can do all this. Hard to say when, but I will try.

@dan-tripp
Copy link
Contributor

Should I create a new issue for the case-insensitive part?

@straker
Copy link
Contributor

straker commented Jun 2, 2022

Nah, we'll do it when we're ready

@dan-tripp
Copy link
Contributor

Okay.

@dan-tripp
Copy link
Contributor

I'd like to note something for completeness:

Unfortunately I am very unlikely to ever finish the work that I started on this issue.

Reason being: I work at Siteimprove now. If I contributed to axe-core while working at Siteimprove, it would look a little weird.

@straker straker added this to the Axe-core 4.8 milestone Apr 17, 2023
@WilcoFiers WilcoFiers removed this from the Axe-core 4.8 milestone Jun 29, 2023
@WilcoFiers WilcoFiers added this to the Axe-core 4.9 milestone Jun 29, 2023
@lsprr
Copy link
Contributor

lsprr commented Mar 5, 2024

@dan-tripp Alright. Talked with @WilcoFiers and we decided that we just want to deal with informing users of the case sensitivity problem and deal with axe-core being case insensitive in a separate issue.

So what we'll want to do is update the invalidrole check to call toLowerCase on the role before it passes it to isValidRole (that will stop aria-roles from failing on the roles). We'll then want to add a new check to the aria-roles rule that specifically looks for roles that are not all lower case and flag them as Needs Review (i.e. returns undefined) and informs the user that roles that are not all lower case are not widely supported.

Hello @straker, may I work on this issue? If not, please let me know. Thanks.

@straker
Copy link
Contributor

straker commented Mar 5, 2024

@lsprr yep! Please let us know if you have any questions. For this particular ticket we just want to add the toLowerCase part to the invalidrole check.

@padmavemulapati
Copy link

Verified with the latest dev branch code base for axe-core,
When role is upper case valid role like <div role='COMMAND' id='aria-rolesfail1'>fail</div> - earlier it is failing and now it is passing
Also for uppercase nonsensical role like '

Contents
` - this is failing even now.

Screen.Recording.2024-05-03.at.4.27.46.PM.mov

Environment:

Label Value
Product axe-core
Version 4.9.0(Dev Branch Code base
OS-Details _MAC- Intel Core i7 - 11.6.8 _
BrowserDetails Chrome Version 124.0.6367.119 (Official Build) (64-bit)

WilcoFiers added a commit that referenced this issue May 6, 2024
###
[4.9.1](v4.9.0...v4.9.1)
(2024-05-06)

### Bug Fixes

- Prevent errors when loading axe in a page with prototype.js
- **aria-allowed-attr:** allow meter role allowed aria-\* attributes on
meter element
([#4435](#4435))
([7ac6392](7ac6392))
- **aria-allowed-role:** add gridcell, separator, slider and treeitem to
allowed roles of button element
([#4398](#4398))
([4788bf8](4788bf8))
- **aria-roles:** correct abstract roles (types) for
aria-roles([#4421](#4421))
- **aria-valid-attr-value:** aria-controls & aria-haspopup incomplete
([#4418](#4418))
- fix building axe-core translation files with region locales
([#4396](#4396))
([5c318f3](5c318f3)),
closes [#4388](#4388)
- **invalidrole:** allow upper and mixed case role names
([#4358](#4358))
([105016c](105016c)),
closes [#2695](#2695)
- **isVisibleOnScreen:** account for position: absolute elements inside
overflow container
([#4405](#4405))
([2940f6e](2940f6e)),
closes [#4016](#4016)
- **label-content-name-mismatch:** better dismiss and wysiwyg symbolic
text characters
([#4402](#4402))
- **region:** Decorative images ignored by region rule
([#4412](#4412))
- **target-size:** ignore descendant elements in shadow dom
([#4410](#4410))
([6091367](6091367))
- **target-size:** pass for element that has nearby elements that are
obscured ([#4422](#4422))
([3a90bb7](3a90bb7)),
closes [#4387](#4387)


This PR was opened by a robot 🤖 🎉 (And updated by @WilcoFiers
)
@WilcoFiers WilcoFiers removed this from the Axe-core 4.11 milestone Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment