Report the presence of multiple annotations #14507

seanbudd · 2023-01-04T02:27:17Z

Link to issue number:

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

Testing strategy:

Manual testing on Chrome and Firefox with: https://codepen.io/reefturner/pen/poKLQyj
System tests have been added for the presence and reporting of multiple annotations

Known issues with pull request:

It appears that an internally documented chrome bug has not been reported to Chromium/Google yet.

Change log entries:

New features

- The presence of multiple annotations are now reported.
For example, when text has a comment and a footnote associated with it.

For Developers

- ``NVDAObject.hasDetails``, ``NVDAObject.detailsSummary``, ``NVDAObject.detailsRole`` has been deprecated, instead use ``NVDAObject.annotations``.

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.

AppVeyorBot · 2023-01-04T03:15:01Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/572ugky6f1v255bl/artifacts/output/nvda_snapshot_pr14507-27406,5f860557.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.8,
INSTALL_END 0.8,
BUILD_START 0.0,
BUILD_END 23.2,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 22.3,
FINISH_END 0.1

See test results for failed build of commit 5f8605573e

seanbudd · 2023-01-04T03:21:51Z

nvdaHelper/vbufBackends/gecko_ia2/gecko_ia2.cpp

+	const bool isChrome = this->toolkitName.compare(L"Chrome") == 0;
+	if (isChrome) {
+		// A bug in Chrome causes a buffer overrun if numRelations is less than the total number of targets the node has.
+		// TODO: has this been reported?


is it worth testing, confirming and reporting this bug to Chromium? I don't think we have yet.

It was reported to Chromium in https://bugs.chromium.org/p/chromium/issues/detail?id=1399184
However, the bug only states that Chromium ignores numRelations and gives back all available. It does not mention any buffer overrun. If there is in deed an overrun, it would be within Chromium, as chromium is the one that allocates the buffer.
Either way though, the general issue has been reported and Google will no doubt look into it.
Our work around is sufficient here.

michaelDCurran · 2023-01-04T05:31:20Z

This pr looks all good, except for the changes to braille.py. This is very large, and I am struggling to follow what was necessary to change verses a whole bunch of refactoring. Would it be at all possible to drop the refactoring from this pr and only include the ncessary changes to braille.py? If this is not possible and or if there is good reason and @seanbudd knows this history, then I'm put in a bit more effort to try and get through that particular file diff.

seanbudd · 2023-01-04T23:51:42Z

@michaelDCurran I've slightly amended braille.py. It should now be much easier to review when ignoring whitespace changes.
If it is still cumbersome, let me know, and I will move the refactor out of this PR.

…scence

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.

…scence

AppVeyorBot · 2023-01-05T02:45:37Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/ocd493xk7cf6mawn/artifacts/output/nvda_snapshot_pr14507-27425,9282cdf6.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.8,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 23.6,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 22.5,
FINISH_END 0.1

See test results for failed build of commit 9282cdf66c

…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

seanbudd added 2 commits December 28, 2022 16:06

Create base structure for multiple aria details targets

d62f8ac

Report the presence of multiple annotation targets

af8f9a3

seanbudd mentioned this pull request Jan 4, 2023

Support multiple details roles #14426

Closed

8 tasks

seanbudd changed the base branch from master to annotationsBase January 4, 2023 02:28

seanbudd mentioned this pull request Jan 4, 2023

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

Merged

7 tasks

fix lint

6e0a265

seanbudd commented Jan 4, 2023

View reviewed changes

seanbudd marked this pull request as ready for review January 4, 2023 03:22

seanbudd requested a review from a team as a code owner January 4, 2023 03:22

seanbudd requested review from feerrenrut and michaelDCurran and removed request for a team and feerrenrut January 4, 2023 03:22

seanbudd added 2 commits January 5, 2023 10:43

minimize diff

a3e13b9

simplify getPropertiesBraille

ca96dd1

michaelDCurran approved these changes Jan 5, 2023

View reviewed changes

seanbudd added 3 commits January 5, 2023 12:15

Add system tests

d0c80f2

Merge remote-tracking branch 'origin/master' into annotationsMultiPre…

7557e2d

…scence

update changes

d322b33

Base automatically changed from annotationsBase to master January 5, 2023 01:20

seanbudd added 2 commits January 5, 2023 12:21

update changes

73916b7

Merge remote-tracking branch 'origin/master' into annotationsMultiPre…

39f945a

…scence

michaelDCurran approved these changes Jan 5, 2023

View reviewed changes

fix lint

a9430c2

seanbudd merged commit a34da14 into master Jan 5, 2023

seanbudd deleted the annotationsMultiPrescence branch January 5, 2023 02:50

nvaccessAuto added this to the 2023.1 milestone Jan 5, 2023

seanbudd mentioned this pull request Jan 10, 2023

Handle Firefox annotation with unknown role, simplify datastructures #14526

Merged

7 tasks

seanbudd self-assigned this Jan 10, 2023

CyrilleB79 mentioned this pull request Mar 2, 2023

Fixed a string that was not translatable. #14691

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Report the presence of multiple annotations #14507

Report the presence of multiple annotations #14507

Uh oh!

seanbudd commented Jan 4, 2023 •

edited

Loading

Uh oh!

AppVeyorBot commented Jan 4, 2023

Uh oh!

seanbudd Jan 4, 2023

Uh oh!

michaelDCurran Jan 4, 2023

Uh oh!

michaelDCurran commented Jan 4, 2023

Uh oh!

seanbudd commented Jan 4, 2023 •

edited

Loading

Uh oh!

AppVeyorBot commented Jan 5, 2023

Uh oh!

Uh oh!

Uh oh!

Report the presence of multiple annotations #14507

Report the presence of multiple annotations #14507

Uh oh!

Conversation

seanbudd commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of user facing changes

Description of development approach

Testing strategy:

Known issues with pull request:

Change log entries:

Code Review Checklist:

Uh oh!

AppVeyorBot commented Jan 4, 2023

Uh oh!

seanbudd Jan 4, 2023

Choose a reason for hiding this comment

Uh oh!

michaelDCurran Jan 4, 2023

Choose a reason for hiding this comment

Uh oh!

michaelDCurran commented Jan 4, 2023

Uh oh!

seanbudd commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AppVeyorBot commented Jan 5, 2023

Uh oh!

Uh oh!

seanbudd commented Jan 4, 2023 •

edited

Loading

seanbudd commented Jan 4, 2023 •

edited

Loading