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

Add annotation roles and group pos for comment #63

Merged
merged 4 commits into from
Feb 25, 2020
Merged

Conversation

aleventhal
Copy link
Contributor

@aleventhal aleventhal commented Jan 14, 2020

Closes issue #62


Preview | Diff

Copy link

@kbabbitt kbabbitt left a comment

Choose a reason for hiding this comment

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

The UIA ControlType and LocalizedControlType mappings make sense to me.

I'm wondering if we should also specify Control Pattern: Annotation. Looking at the documentation on MSDN, it seems pretty fleshed out in core UIA and pretty well suited to these new roles. I haven't experimented with Narrator to see how well it supports annotations though.
https://docs.microsoft.com/en-us/windows/win32/winauto/uiauto-implementingannotation
https://docs.microsoft.com/en-us/windows/win32/winauto/uiauto-annotation-type-identifiers

@boggydigital WDYT?

@aleventhal
Copy link
Contributor Author

@kbabbitt @boggydigital, can you help map out how to use IAnnotationProvider? We should add that info to this doc -- there doesn't seem to be a clean mapping. For example, would IAnnotationProvider be supported on the element with aria-details, or on the target of the aria-details?

As far as knowing what type id to use, the semantics for that can come from different places in ARIA.

In the following cases, the aria-details target role tells us what the annotation type is:
AnnotationType_Comment (role=comment)
AnnotationType_Endnote (role=doc-endnote)
AnnotationType_Footnote (role=doc-footnote)

However, in these cases, the source element role tells us what the annotation type is:
AnnotationType_Highlighted (role=mark)
AnnotationType_Mathematics (role=math)
AnnotationType_InsertionChange (role=insertion)
AnnotationType_DeletionChange (role=deletion)
Note that an insertion/deletion pair can be parented by role=suggestion, but I don't see anything in IAnnotationProvider for that.

And in these cases, aria-invalid on the source element tells us what the annotation type is:
AnnotationType_SpellingError (aria-invalid=spelling)
AnnotationType_GrammarError (aria-invalid=grammar)
AnnotationType_DataValidationError (aria-invalid=true)

Otherwise, AnnotationType_Unknown could be used.

Annotation types that I don't see an ARIA analog for:
AnnotationType_AdvancedProofingIssue
AnnotationType_Author
AnnotationType_CircularReferenceError
AnnotationType_ConflictingChange
AnnotationType_ExternalChange
AnnotationType_Footer
AnnotationType_FormatChange
AnnotationType_FormulaError
AnnotationType_Header
AnnotationType_MoveChange
AnnotationType_TrackChanges
AnnotationType_UnsyncedChange (or is this role=suggestion?)

@kbabbitt
Copy link

@aleventhal My read of the documentation is that IAnnotationProvider is implemented on the UI Automation element that represents the annotation. Then, the IAnnotationProvider.Target property points back to the UI Automation element being annotated. So for HTML elements connected by an aria-details relationship, IAnnotationProvider would be implemented on the referred-to element.

For the aria-invalid cases you mentioned - note that aria-invalid is already mapped to the IsDataValidForForm property in UI Automation. But if the author provided aria-details alongside aria-invalid, we could consider implementing IAnnotationProvider on the referred-to element in that case as well.

We can leave this for a future PR if you'd prefer, but I wanted to share my thoughts just the same.

@aleventhal
Copy link
Contributor Author

Or maybe aria-errormessage in the aria-invalid case?

Yes, let's leave the IAnnotationProvider stuff for a future PR. Maybe MS should write that PR, sound good?

@kbabbitt
Copy link

SGTM

@aleventhal
Copy link
Contributor Author

@kbabbitt @joanmarie @boggydigital Can I get reviews now that this is in the spec?

@aleventhal
Copy link
Contributor Author

@joanmarie what's next for this one?

Copy link
Contributor

@joanmarie joanmarie left a comment

Choose a reason for hiding this comment

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

LGTM, but please fix the missing <br />

@joanmarie joanmarie merged commit 2fee3cf into master Feb 25, 2020
@cookiecrook cookiecrook removed their request for review April 2, 2020 17:34
@pkra pkra mentioned this pull request Jan 26, 2022
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.

3 participants