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.

Sign up for GitHub

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

Jump to bottom

Report Aria Details In Focus Mode #13106

Merged
merged 31 commits into from
Jan 10, 2022
Merged

Report Aria Details In Focus Mode #13106

merged 31 commits into from
Jan 10, 2022

Conversation

feerrenrut
Copy link
Contributor

@feerrenrut feerrenrut commented Nov 30, 2021 •
edited
Loading

Link to issue number:

Follow on from #12439

Summary of the issue:

Reporting "has details" and summarizing the details was not supported in focus mode or content editable.

Description of how this pull request fixes the issue:

Relies on

Changes in this PR:

  • Adds system tests for focus mode / content editable
  • Exposes an in-process approach to summarize an IAccessible ( getTextFromIAccessible moved from ia2LiveRegions.cpp to textFromIAccessible.h/cpp and exposed as nvdaInProcUtils_getTextFromIAccessible via IA2Support.cpp)
  • Make it easier to fetch the details relation targets from python code.
  • Updates reporting of has details and details summary to work in both browse and focus mode.

Testing strategy:

  • System tests
  • Alpha users

Known issues with pull request:

Currently only a single relation can be summarized.
Support for multiple details relations will be added later, there are various UX concerns that will need to be resolved.

  • Braille expectation should be added to the system tests

Change log entries:

New features

- Reporting of "has details" and the associated command to summarize the details relation have been updated to work in focus mode.

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

@feerrenrut feerrenrut changed the base branch from master to splitRelationsConstants November 30, 2021 03:59
Base automatically changed from splitRelationsConstants to master November 30, 2021 06:42
lukaszgo1
lukaszgo1 reviewed Nov 30, 2021
View reviewed changes
nvdaHelper/interfaces/nvdaInProcUtils/nvdaInProcUtils.acf Outdated Show resolved Hide resolved
nvdaHelper/remote/IA2Support.cpp Outdated Show resolved Hide resolved
source/globalCommands.py Outdated Show resolved Hide resolved
@feerrenrut feerrenrut changed the base branch from master to fixEnumStrMixup December 1, 2021 09:49
Base automatically changed from fixEnumStrMixup to master December 2, 2021 02:44
@feerrenrut
Aria details test use content editable
d014bdf 
So that focus mode can also be tested.

When there is a contenteditable div, chrome/nvda sets the cursor to
within the contenteditiable.

Use F6 to toggle through to the document.
@feerrenrut
Use has details property rather than state
eb3579a 
Introduce NVDAObject hasDetails / detailsSummary
Speech and braille use NVDAObject.hasDetails and hasDetails via controlField

Why is this preferrable over the state based approach?
@feerrenrut
Expose nvdaInProcUtils_getTextFromIAccessible
33e3be9
@feerrenrut
Move textFromIAccessible
756abb1 
Make textFromIAccessible a separate compilation unit
@feerrenrut
fix reused variable name
c92a805 
This does not seem intentional, and could cause bugs
@feerrenrut
Add fetching and summarizing details relation.
509c4e4 
_getIA2RelationFirstTarget is a faster way to get relation targets
for IAccessible_2_2 objects.
@feerrenrut feerrenrut marked this pull request as ready for review December 2, 2021 14:11
@feerrenrut feerrenrut requested a review from a team as a code owner December 2, 2021 14:11
@AppVeyorBot
Copy link

See test results for failed build of commit b035766262

@feerrenrut
Copy link
Contributor Author

@michaelDCurran I'd like your feedback on the UX, particularly with braille. But also I'd like to hear your thoughts on the suggestions from @lukaszgo1

@seanbudd
Copy link
Member

seanbudd commented Dec 3, 2021

I'd also like an additional reviewer for the C++, I don't feel comfortable reviewing it alone. I'll leave an approval when I am done reviewing everything (and happy), I've had a look at the rest of the PR already.

This was referenced Dec 3, 2021
Merged
Merged
michaelDCurran
michaelDCurran requested changes Dec 3, 2021
View reviewed changes
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.

What about testing the situation where the aria-details is on a span around a link?

<p>Hello <span aria-details="xx" role="mark">this is a <a href="https://www.google.com/">test</a></span></p>

Will show details summary work when the caret is within the link? I'm not sure it will as NVDAObjectAtStart will just be the link, but details will be on its parent.

nvdaHelper/interfaces/nvdaInProcUtils/nvdaInProcUtils.idl Outdated Show resolved Hide resolved
nvdaHelper/readme.md Outdated Show resolved Hide resolved
source/appModuleHandler.py Outdated Show resolved Hide resolved
tests/system/libraries/ChromeLib.py Outdated Show resolved Hide resolved
michaelDCurran
michaelDCurran requested changes Dec 14, 2021
View reviewed changes
nvdaHelper/remote/textFromIAccessible.h Outdated Show resolved Hide resolved
source/globalCommands.py Outdated Show resolved Hide resolved
@AppVeyorBot

This comment has been minimized.

Sign in to view
@feerrenrut
Copy link
Contributor Author

I have addressed all the feedback and pushed 4 new commits. It turns out we can't find an example where getting the caret position will fail with Chromium / Firefox, theoretically the fallback to rely on the focus object is not necessary, however, I have opted to retain it since there is no negative impact, and the behavior from browsers is somewhat surprising here. I have attempted to add comments to clarify.

lukaszgo1
lukaszgo1 reviewed Jan 6, 2022
View reviewed changes
source/globalCommands.py Show resolved Hide resolved
source/globalCommands.py Outdated Show resolved Hide resolved
source/globalCommands.py Outdated Show resolved Hide resolved
source/globalCommands.py Outdated Show resolved Hide resolved
source/globalCommands.py Outdated Show resolved Hide resolved
source/globalCommands.py Outdated Show resolved Hide resolved
source/globalCommands.py Outdated Show resolved Hide resolved
@AppVeyorBot

This comment has been minimized.

Sign in to view
@feerrenrut feerrenrut requested a review from a team as a code owner January 10, 2022 07:07
@feerrenrut feerrenrut merged commit aa351c5 into master Jan 10, 2022
@feerrenrut feerrenrut deleted the reportAriaDetailsInFocusMode branch January 10, 2022 10:44
@nvaccessAuto nvaccessAuto added this to the 2022.1 milestone Jan 10, 2022
feerrenrut added a commit that referenced this pull request Jan 17, 2022
@feerrenrut
Add helper: getAccessibleChildren (PR #13126)
c6a6df8 
Make all usages of AccessibleChildren conform to a consistent approach.
Management of resources is automatic.
Related to #13106
This was referenced Feb 25, 2022
Merged
Merged
feerrenrut added a commit that referenced this pull request Mar 2, 2022
@feerrenrut
Use NVDA+d for report details summary (PR #13391)
c267db9 
Background:
Improved support for reporting details summary: #13106

Summary:
Web authors would like to start using aria-details, however:
- It is not enabled by default.
- There is no default gesture to fetch the details.

Information about longdesc:
- The longdesc attribute is only valid on an img element.
- longdesc does not seem to be exposed in Chrome / edge, it continues to work in Firefox.
- longdesc is deprecated, mdn advises not using it and transitioning away from current uses.
- longdesc has an incompatible UX with aria details. The longdesc attribute contains a URL,
  the browser exposes an action, when triggered the URL is opened in a new tab. In contrast,
  Aria details is expected to be read without leaving the page.
- Anyone who still relies on longdesc support can assign an alternative gesture.

Description of Change:
- NVDA+d now runs aria-details summary script
- Report 'has details' by default
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanbudd seanbudd seanbudd left review comments

@lukaszgo1 lukaszgo1 lukaszgo1 left review comments

@michaelDCurran michaelDCurran michaelDCurran approved these changes

@Qchristensen Qchristensen Awaiting requested review from Qchristensen Qchristensen was automatically assigned from nvaccess/userdocs

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2022.1
Development

Successfully merging this pull request may close these issues.
6 participants
@feerrenrut @AppVeyorBot @seanbudd @lukaszgo1 @michaelDCurran @nvaccessAuto