Skip to content

Report aria-description always#12500

Merged
feerrenrut merged 10 commits intomasterfrom
addAriaDescription
Aug 5, 2021
Merged

Report aria-description always#12500
feerrenrut merged 10 commits intomasterfrom
addAriaDescription

Conversation

@feerrenrut
Copy link
Copy Markdown
Contributor

@feerrenrut feerrenrut commented Jun 2, 2021 •
edited
Loading

Reviewers - There are several commits to refactor / fix issues encountered. It will be easier to review the changes one commit at a time. I have already grouped and squashed related changes. Commits with messages starting with "Fixup / squash" will be squashed into the commit they reference

Link to issue number:

None

Summary of the issue:

The HTML attribute aria-description has been introduced as one aspect of supporting a broader concept of annotations on the web. It maps to accDescription. In contrast to other sources for accDescription such as title, our expectation is annotations should be reported whenever present.

Description of how this pull request fixes the issue:

Experimental support has been added for reporting aria-description.

Testing strategy:

System tests for Chrome checking speech output in browse mode, focus mode, and say-all.

Manual testing with Braille, mouse, and object navigation.

Known issues with pull request:

  • Firefox and Edge do not yet support aria-description as expected.
  • Reporting due to mouse seems broken in Chrome?
  • Garbage handler warnings: Fixed with Fix Enum Cyclic Ref #12617

Change log entries:

New features:
aria-description will now be reported by default.

Code Review Checklist:

  • Pull Request description is up to date.
  • Unit tests.
  • System (end to end) tests.
  • Manual testing.
  • User Documentation.
  • Change log entry.
  • Context sensitive help for GUI changes.
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers

@feerrenrut feerrenrut added ARIA app/chrome app/firefox app/edge/anaheim MS browser, chromium based, replaces Spartan in 2019 by Anaheim. NVDA access via IA2. labels Jun 2, 2021
@AppVeyorBot

This comment has been minimized.

Sign in to view
@feerrenrut feerrenrut force-pushed the addAriaDescription branch from d0cab79 to c2aaa4a Compare June 3, 2021 10:14
@AppVeyorBot

This comment has been minimized.

Sign in to view
@feerrenrut feerrenrut force-pushed the addAriaDescription branch from c2aaa4a to f5084b7 Compare June 3, 2021 10:31
@AppVeyorBot

This comment has been minimized.

Sign in to view
@feerrenrut
Copy link
Copy Markdown
Contributor Author

feerrenrut commented Jun 3, 2021

I suspect that the version of chrome on appveyor does not yet support the "descriptionFrom" attribute.

@CyrilleB79
Copy link
Copy Markdown
Contributor

CyrilleB79 commented Jun 3, 2021

Why is the option named "Report aria-description always" and not just "Report aria-description"?

@AppVeyorBot

This comment has been minimized.

Sign in to view
@feerrenrut
Copy link
Copy Markdown
Contributor Author

feerrenrut commented Jun 17, 2021

Why is the option named "Report aria-description always" and not just "Report aria-description"?

In the past we reported accDescription under certain (but not all) circumstances. aria-description is one source for accDescription. We'll continue to report accDescription under the same circumstances as before, unless the source is aria-description in which case we'll always report it. Turning it off does not mean aria-description will never be reported.

As an advanced setting the naming is developer centric. Happy to hear suggestions.

@AppVeyorBot

This comment has been minimized.

Sign in to view
seanbudd
seanbudd requested changes Jun 17, 2021
View reviewed changes
@AppVeyorBot

This comment has been minimized.

Sign in to view
@feerrenrut feerrenrut force-pushed the addAriaDescription branch from d965d62 to d23f22e Compare June 17, 2021 13:01
@AppVeyorBot

This comment has been minimized.

Sign in to view
@CyrilleB79
Copy link
Copy Markdown
Contributor

CyrilleB79 commented Jun 17, 2021

Why is the option named "Report aria-description always" and not just "Report aria-description"?

In the past we reported accDescription under certain (but not all) circumstances. aria-description is one source for accDescription. We'll continue to report accDescription under the same circumstances as before, unless the source is aria-description in which case we'll always report it. Turning it off does not mean aria-description will never be reported.

As an advanced setting the naming is developer centric. Happy to hear suggestions.

I am a developper and tester but I do not know the whole codebase of NVDA. More specifically, I did not know that only some descriptions were reported, not all (provided the corresponding option in 'Object presentation' is checked).

Regarding a suggestion to rephrase this option, it seems hard to have a one-line formulation. I would suggest to add more details in the user guide.
There could be a dedicated paragraph with more details for this option in the user guide rather than a paragraph for the whole ARIA group of options. E.g.:

  • "Report aria-description always": enables reporting of aria-descriptions.
    If this option is checked, aria-description is always reported
    If this option is unchecked, legacy pre-2021.2 behaviour occurs. (please describe more precisely the legacy behaviour)
    Note: aria-description only applies to the web, and is only a subset of the sources for "description".

@feerrenrut feerrenrut force-pushed the addAriaDescription branch from d23f22e to 95f17f8 Compare June 17, 2021 14:34
@feerrenrut
Copy link
Copy Markdown
Contributor Author

feerrenrut commented Jun 17, 2021

I can add more details to the userguide. But do note these features and options are unlikely to stay the same. This option exists so that NV Access developers working on this can iteratively progress the related features. It also allows us to give specific instructions to browser developers to enable this for testing.

@AppVeyorBot

This comment has been minimized.

Sign in to view
@feerrenrut feerrenrut force-pushed the addAriaDescription branch from 95f17f8 to aaee223 Compare June 17, 2021 15:10
CyrilleB79
CyrilleB79 reviewed Jun 17, 2021
View reviewed changes
@feerrenrut feerrenrut force-pushed the addAriaDescription branch from aaee223 to ae697b9 Compare June 18, 2021 08:44
@AppVeyorBot

This comment has been minimized.

Sign in to view
lukaszgo1
lukaszgo1 reviewed Jul 2, 2021
View reviewed changes
@feerrenrut feerrenrut mentioned this pull request Jul 5, 2021
Merged
8 tasks
@feerrenrut
Copy link
Copy Markdown
Contributor Author

feerrenrut commented Jul 5, 2021

Blocked by #12617

@feerrenrut feerrenrut mentioned this pull request Jul 7, 2021
Closed
@feerrenrut feerrenrut requested a review from a team as a code owner July 27, 2021 06:44
@feerrenrut
Rename UIA._getControlFieldForObject to _getControlFieldForUIAObject
27382ac 
Typing in NVDA is not yet ubiquitous, a lot of development relies on
text search / grep. This name was easy to confuse with
CompoundTextInfo._getControlFieldForObject
@feerrenrut
Update type information for NVDAObject
d133367
@feerrenrut
Add typing for virtual buffer / text info / fields normalaziation
7ae2867 
String type can also be returned from XMLFormatting.XMLTextParser.parse
Update typing to include optional string.
Since data could be an optional string, but it is apparently not ommited
when None, log a warning. This will help us to track if it ever is None.

If it is not ever None, explicitly exclude None and tighten up the
typing.
@feerrenrut
Use constants for Unicode specials
e9ba4fe 
The following are used regularly in the code, descriptions
from https://en.wikipedia.org/wiki/Specials_(Unicode_block)

 OBJECT REPLACEMENT CHARACTER, placeholder in the text
for another unspecified object, for example in a compound document.

� REPLACEMENT CHARACTER, used to replace an unknown, unrecognized,
or unrepresentable character.
@feerrenrut
Remove ignoreEditableText param
6262596 
ControlStack previously had None inserted, now it can be empty.
@feerrenrut
Fix names, simplify logic in getTextWithFields
24e3f39 
field -> childObject
field -> textWithEmbeddedObjectsItem
@feerrenrut
Announce aria-description always
470083f 
Disabling setting 'presentation/object description' should not prevent
aria-description from being reported.

When present, aria-description should be reported in browse mode,
focus mode, during say-all, and in object navigation.

Aria-description should also be presented via both Braille and speech.
@feerrenrut
Add tests for reporting aria-description
34f6a91
@feerrenrut
Update settings GUI and supoprting docs
4aa379f
@feerrenrut
Copy link
Copy Markdown
Contributor Author

feerrenrut commented Aug 3, 2021

I have rebased this to condense all the "fixup" commits, and merge in the latest from master. When approved this can be merged as a squash merge. Keeping the commits separated is useful for reviewing this PR, but not entirely required once merged to master. There is no moved code, only modifications.

michaelDCurran
michaelDCurran approved these changes Aug 4, 2021
View reviewed changes
Copy link
Copy Markdown
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.

I skipped the first 4 commits and started reviewing at "Remove ignoreEditableText param...".
The rest looks good to me.

feerrenrut
feerrenrut commented Aug 5, 2021
View reviewed changes
@feerrenrut feerrenrut merged commit fab35ec into master Aug 5, 2021
@feerrenrut feerrenrut deleted the addAriaDescription branch August 5, 2021 11:08
@nvaccessAuto nvaccessAuto added this to the 2021.3 milestone Aug 5, 2021
@michaelDCurran michaelDCurran mentioned this pull request Aug 19, 2021
Closed
@feerrenrut feerrenrut mentioned this pull request Aug 20, 2021
Closed
@michaelDCurran michaelDCurran mentioned this pull request Aug 23, 2021
Merged
@CyrilleB79 CyrilleB79 mentioned this pull request Aug 25, 2021
Merged
8 tasks
michaelDCurran added a commit that referenced this pull request Sep 24, 2021
@michaelDCurran
Revert "Remove ignoreEditableText param" (#12766)
5b821c6 
This reverts commit 6262596.
In issue #12746 it was reported that moving by paragraph in Google Docs in Google Chrome would sometimes result in NVDA saying "blank" for certain paragraphs.
Similarly although not noted in an issue yet, Some lines in Thunderbird message composition windows would also say blank.
These regressions were found to be caused by the merging of pr #12500
Further investigation found that the specific commit in the pr that caused this was 6262596
I'm not entirely sure on why this commit causes the regression, however my understanding is that this commit was only to improve readability / understanding of logic along the way and that the actual feature introduced by the pr had no dependency on this commit as such.
Thus, this pr reverts that single commit, and therefore fixes #12746.
Perhaps the logic of this commit can be better investigated at a later stage, but for now it is better to return to what we know works.
This was referenced Oct 4, 2021
Merged
Merged
feerrenrut added a commit that referenced this pull request Oct 13, 2021
@feerrenrut
Dont report description in browse mode with reportObjectDescriptions …
c6aa771 
…(PR #12917)

Historically the option "Object presentation: Report Object Descriptions" (default: true) has been limited to focus mode and object navigation.
In Report aria-description always #12500 this was changed to report descriptions always.
Historically, the word "object" in the settings category, and the name of this option was supposed to imply "focus mode / object nav specific behavior".

This change introduces a way to test braille (not dots, raw text) output.
Reverts changes from Report aria-description always #12500 that aimed to make the reportObjectDescriptions behavior consistent between browse and focus modes.
Updates the user docs to specify that options in the Object Presentation category don't apply to browse mode.
feerrenrut added a commit that referenced this pull request Oct 18, 2021
@feerrenrut
Fix reading order / description spoken during quick nav (PR #12933)
f2bb38c 
Fixes #12751
Broken in #12500

When using quick nav ("h" in browse mode) to navigate to a heading nested in an article with a description, the description was announced before the heading. For quicknav and for focus changes, the most inner element should always be announced first, working outwards to provide additional context.
@feerrenrut feerrenrut mentioned this pull request Oct 20, 2021
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seanbudd seanbudd seanbudd requested changes

@michaelDCurran michaelDCurran michaelDCurran approved these changes

@Qchristensen Qchristensen Awaiting requested review from Qchristensen

+2 more reviewers

@lukaszgo1 lukaszgo1 lukaszgo1 left review comments

@CyrilleB79 CyrilleB79 CyrilleB79 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

app/chrome app/edge/anaheim MS browser, chromium based, replaces Spartan in 2019 by Anaheim. NVDA access via IA2. app/firefox ARIA

Projects

None yet

Milestone

2021.3

Development

Successfully merging this pull request may close these issues.

7 participants

@feerrenrut @AppVeyorBot @CyrilleB79 @seanbudd @lukaszgo1 @michaelDCurran @nvaccessAuto