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

Cycle through reporting annotation target summaries with nvda+d #14480

Merged
merged 1 commit into from
Jan 5, 2023

Conversation

seanbudd
Copy link
Member

@seanbudd seanbudd commented Dec 28, 2022

Link to issue number:

Supersedes #14389 and #14426.
Fixes #14360

Summary of the issue:

Before this PR, NVDA was only aware of the first annotation target.
NVDA announces the presence of an annotation target.
nvda+d is used to announce the summary of the first annotation target.

This PR introduces base code to support multiple annotation targets, and enables cycling though reporting each summary.
Similarly, #14507 introduces reporting the presence of multiple annotation targets.

Description of user facing changes

nvda+d now cycles through reporting each annotation target for an annotated item. For example reading the summary of each child comment.

Description of development approach

Creates abstractions for the relationships between annotation origins and targets.
The global command to report a summary of an annotation target has been updated.

Testing strategy:

Known issues with pull request:

Max 10 targets are fetched, this should be dynamic, however there is a Chrome bug preventing this, requiring investigation.
https://crbug.com/1399184

Change log entries:

New features

- `nvda+d` now cycles through reporting the summary of each annotation target for origins with multiple annotation targets. For example reading the summary of each child comment.

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • Security precautions taken.

@seanbudd seanbudd requested a review from a team as a code owner December 28, 2022 05:04
@seanbudd seanbudd requested review from feerrenrut and michaelDCurran and removed request for feerrenrut December 28, 2022 05:04
@AppVeyorBot
Copy link

See test results for failed build of commit 6244b4a766

Copy link
Member

@michaelDCurran michaelDCurran left a comment

Choose a reason for hiding this comment

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

Generally this looks good. And I've also tested myself and found no issues so far.
However, I would probably recommend the second pr be completed for review before this one is merged, just in case the other one shows up issues missed when consuming this one.
Then both can be merged around the same time.

source/globalCommands.py Show resolved Hide resolved
@seanbudd seanbudd changed the title Reporting the details summary now cycles through the available details targets Cycle through annotation targets with nvda+d Jan 4, 2023
@seanbudd seanbudd changed the title Cycle through annotation targets with nvda+d Cycle through reporting annotation target summaries with nvda+d Jan 4, 2023
@seanbudd seanbudd merged commit 104d3af into master Jan 5, 2023
@seanbudd seanbudd deleted the annotationsBase branch January 5, 2023 01:20
@nvaccessAuto nvaccessAuto added this to the 2023.1 milestone Jan 5, 2023
seanbudd added a commit that referenced this pull request Jan 5, 2023
Stacked against #14480
Supersedes #14426

Summary of the issue:
Currently, NVDA only announces the first annotation target, and only announces the first annotation summary.

#14480 introduces the ability to cycle through multiple annotations targets.
This PR introduces announcing the presence of multiple annotation targets.

When multiple annotation targets are present with different roles, NVDA should announce them.
Example:

A footnote, a comment, and a no-role annotation are present
NVDA should announce "has footnote, has comment, has details" to make it easier for the user to find the annotation they want to interact with.
Description of user facing changes
When the annotation feature is enabled, NVDA will now report all the unique annotation target roles.

Description of development approach
From the virtual buffer, exposed comma separated values for the roles of annotation targets via the "detailsRoles" property.
Converted all aria-details functions to plural form (handling a collection rather than a single optional value).
Removed usages of, and deprecated nvdaObject.hasDetails, nvdaObject.detailsSummary, nvdaObject.detailsRole
These are replaced with new Annotations types
seanbudd added a commit that referenced this pull request Jan 13, 2023
…14526)

Follow up of #14507, #14480

Summary of the issue:
Firefox incorrectly throws an error when browsing annotated content with an unsupported role.
Instead an unknown role should be reported: i.e. "has details".

The typing for annotation data structures were incorrect.

Description of user facing changes
When browsing content in Firefox with an annotation with unsupported role, "has details" is reported instead of throwing an error.

Description of development approach
Update typing to be more accurate for annotation data structures, log and return None in the Firefox case to match Chromium behaviour.
Use Tuples rather than generators for annotations.targets and annotations.roles.
Drops the superfluous/unused annotations.summaries
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 this pull request may close these issues.

Don't fall back to IA2 nRelations and friends if relationTargetsOfType succeeds but returns no targets
4 participants