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

handle aria-invalid="spelling,grammar" #11787

Merged
merged 10 commits into from Nov 26, 2020
Merged

Conversation

michaelDCurran
Copy link
Member

Link to issue number:

None.

Summary of the issue:

A web page author can indicate to ATs that text is a spelling error, grammar error, or both, via the aria-invalid attribute.
NVDA currently supports "spelling" and "grammar" but does not handle both at the same time.
Web applications such as Google Docs would like to specify that text is both a spelling and grammar error, but right now cannot, as if they expose "spelling, grammar" NVDA will not announce anything at all.

Description of how this pull request fixes the issue:

When NVDA collects the "invalid" IAccessible2 text attribute, it splits the value on "," and removes whitespace, and then looks for one or more of "spelling" or "grammar" separately.

Testing performed:

Added a system test which tests three paragraphs in Chrome, containing a spelling error, a grammar error, and both spelling and grammar error.

Known issues with pull request:

Although this has been done generically for all Gecko IA2 compatible browsers in NvDA, this does not work in Firefox as Firefox does not currently expose multiple values for aria-invalid via IAccessible2, rather it just exposes "true" in this case. This should be fixed in Firefox.

Change log entry:

Bug fixes:

  • Text marked both as being a spelling and grammar error at the same time in Google Chrome will be appropriately announced as both a spelling and grammar error by NVDA.

@AppVeyorBot
Copy link

See test results for failed build of commit 2f4fdc30a8

@lijianwei2019
Copy link

lijianwei2019 commented Oct 27, 2020

@jcsteh

@jcsteh
Copy link
Contributor

jcsteh commented Oct 27, 2020

Note that this violates the ARIA spec as it currently stands. The spec only allows a single token for aria-invalid, not a token list. If we were going to support this in Firefox, I'd want to see this accepted in at least the editor's draft of ARIA.

Furthermore, ARIA properties accepting multiple values should use a token list, which is defined as "Space-separated tokens". Thus, comma + space is a further spec violation.

@feerrenrut
Copy link
Contributor

Thanks for the heads up Jamie, we'll discuss this further.

@michaelDCurran
Copy link
Member Author

what is your view @aleventhal , as you were driving this request I believe? Are there plans in ARIA to allow for a set of values? I'm not too keen on supporting this in NVDA if there are no documented plans to get this in to ARIA, especially also if there is no real-world web app doing this yet...
Though having said all of this, if we see this pr just as supporting the idea of a set of values for the 'invalid' object attribute in IAccessible2, which Chrome does in deed expose, then perhaps things are okay.

@jcsteh
Copy link
Contributor

jcsteh commented Oct 27, 2020 via email

@feerrenrut
Copy link
Contributor

Although we care about the ARIA spec for this and advocate for any issues to be resolved, I believe we are consuming the IA2 spec correctly and think we should push forward on this. If the spec is updated, it will be a minor change to our sample. Perhaps a note should be added to the sample to highlight that it is still under discussion.

@michaelDCurran
Copy link
Member Author

@feerrenrut I have fixed conflicts and added a note about it not yet being standard ARIA and how it may need to change.

@AppVeyorBot
Copy link

See test results for failed build of commit 8a72bde3e0

feerrenrut
feerrenrut previously approved these changes Nov 26, 2020
Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michaelDCurran michaelDCurran merged commit 4704df6 into master Nov 26, 2020
@michaelDCurran michaelDCurran deleted the invalidSpellingAndGrammar branch November 26, 2020 06:24
@nvaccessAuto nvaccessAuto added this to the 2020.4 milestone Nov 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants