Skip to content

Support multiple details roles#14426

Closed
feerrenrut wants to merge 17 commits into
masterfrom
supportMultipleDetailsRoles
Closed

Support multiple details roles#14426
feerrenrut wants to merge 17 commits into
masterfrom
supportMultipleDetailsRoles

Conversation

@feerrenrut

@feerrenrut feerrenrut commented Dec 7, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Link to issue number:

None

Summary of the issue:

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.
  • Instead, NVDA was only reporting the role of the first annotation target.

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.

Testing strategy:

Manual testing with: https://codepen.io/reefturner/pen/poKLQyj

Known issues with pull request:

None

Change log entries:

New features

- The presence of multiple annotation types are now reported.

For Developers

- Deprecated `nvdaObject.hasDetails`, `nvdaObject.detailsSummary`, `nvdaObject.detailsRole`
   - These are replaced with new Annotations types.

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.

@feerrenrut feerrenrut added the ARIA label Dec 7, 2022
@feerrenrut
Refactor Annotations
9c48764
@feerrenrut
Jump to last annotation summary reported.
4067c71
@feerrenrut
Return from annotation
6e66d70
@feerrenrut
Move to object refactor
4128542
@feerrenrut
Report summary for multi-targets
ba0da60
@feerrenrut
Missing role is not an error
bc91b07
@feerrenrut
Handle IAccessible is both origin and target.
15fc9ad
@feerrenrut
Collect multiple relation elements
dc47fa0
@feerrenrut
Convert detailsRole to detailsRoles
05e2d69
@feerrenrut
braille hasDetails for non-landmark
00122f4 
Fixes #13815

This can be squashed into commit: "Convert detailsRole to detailsRoles"
when rebasing on master.
@feerrenrut
Handle missing details role
1ed0325 
Use "unknown" for a virtualBuffer node that is missing a role.
This should be reported as "has details" / "details" in
speech / braille respectively.
@feerrenrut
Use warnings.warn for deprecations.
87c3c06 
Shows in log as follows:
DEBUGWARNING - Python warning (16:47:09.529) - MainThread (1772):
source\globalCommands.py:2307: DeprecationWarning: NVDAObject.detailsSummary is deprecated. Use NVDAObject.annotations instead.
  if objAtStart.detailsSummary:
@feerrenrut
fix aria_details_role tests
476f62a 
Braille output has changed
@feerrenrut
fix aria_details_role test
8567b1f 
details is now output as has details
Braille device only has 120 chars, so output is cut off.
Update cell count in 'def _onNvdaStartupComplete' see 'self.setBrailleCellCount(120)'
@feerrenrut feerrenrut force-pushed the supportMultipleDetailsRoles branch from 36cd788 to 8567b1f Compare December 8, 2022 09:11
@feerrenrut feerrenrut force-pushed the navToAnnotation-browseMode branch from 3fe1efd to ba0da60 Compare December 8, 2022 09:11
@feerrenrut feerrenrut changed the base branch from navToAnnotation-browseMode to master December 8, 2022 09:13
@AppVeyorBot

This comment was marked as outdated.

Sign in to view
@AppVeyorBot

This comment was marked as outdated.

Sign in to view
@feerrenrut feerrenrut marked this pull request as ready for review December 12, 2022 05:10
@feerrenrut feerrenrut requested a review from a team as a code owner December 12, 2022 05:10
@seanbudd seanbudd self-requested a review December 18, 2022 23:59
@seanbudd seanbudd self-assigned this Dec 19, 2022
@seanbudd seanbudd marked this pull request as draft December 22, 2022 01:28
@seanbudd seanbudd mentioned this pull request Dec 28, 2022
Merged
7 tasks
@seanbudd seanbudd mentioned this pull request Jan 4, 2023
Merged
8 tasks
@seanbudd

seanbudd commented Jan 4, 2023

Copy link
Copy Markdown
Member

Closing this in favour of #14507

@seanbudd seanbudd closed this Jan 4, 2023
seanbudd added a commit that referenced this pull request Jan 5, 2023
@seanbudd
Cycle through reporting annotation target summaries with nvda+d (#14480)
104d3af 
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.
seanbudd added a commit that referenced this pull request Jan 5, 2023
@seanbudd
Report the presence of multiple annotations (#14507)
a34da14 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelDCurran michaelDCurran Awaiting requested review from michaelDCurran michaelDCurran is a code owner automatically assigned from nvaccess/developers

@seanbudd seanbudd Awaiting requested review from seanbudd

Assignees

@seanbudd seanbudd

Labels

ARIA

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@feerrenrut @AppVeyorBot @seanbudd