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

Don't braille or speak duplicate name / content / description #12888

Merged
merged 7 commits into from
Oct 11, 2021

Conversation

feerrenrut
Copy link
Contributor

@feerrenrut feerrenrut commented Sep 30, 2021

Link to issue number:

Related to #12750

Summary of the issue:

When a link has a title attribute that matches the content, braille (and speech) is duplicated.

An example:

<a href="#" title="apple" style="display:block">apple</a>

Two links URL encoded:

data:text/html,%3Ca href%3D"%23" title%3D"apple" style%3D"display%3Ablock"%3Eapple%3C%2Fa%3E%3Ca href%3D"%23" title%3D"banana" aria-label%3D"banana" style%3D"display%3Ablock"%3Econtents%3C%2Fa%3E

Description of how this pull request fixes the issue:

Tests have been added for speech, to show current behavior (duplicated name/content/description)
The duplicate speech/braille is fixed:

  • In focus mode, ensuring that description is not reported if it matches name
  • In browse mode, when filling the virtual buffer identifying when description matches the node contents and adding an attribute.

Testing strategy:

  • System tests added for speech:
    • browse mode navigation with tab
    • browse mode navigation with down arrow
    • focus mode navigation with tab

Manual testing using the braille viewer with the same sample.

  • Further manual testing from more experienced braille users would be appreciated.

Known issues with pull request:

None

Change log entries:

Bug fixes

- Fixed duplicate braille and speech when 'description' matches 'content' or 'name'. #12888

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

These tests pass, showing the current behavior
@lukaszgo1

This comment has been minimized.

@ABuffEr

This comment has been minimized.

@feerrenrut

This comment has been minimized.

Adjust system tests to match new behavior
@feerrenrut
Copy link
Contributor Author

The Firefox crash should now be resolved, please test the [latest PR build to confirm].(https://ci.appveyor.com/api/buildjobs/n8wrf4lcrm3hqn4v/artifacts/output%2Fnvda_snapshot_pr12888-23854%2C2ea82fef.exe)

@lukaszgo1
Copy link
Contributor

I can confirm that Firefox crash is fixed on my end. Also I would like to thank @feerrenrut for this fix - not announcing description when it equals the name has been requested for years and it makes various programs (even outside of the web browser) so more pleasant to use.
Unfortunately this PR does not improve #12750 in any way since the issue described there is not caused by title being equal to the link name. Could you please open the sample from the issue in Firefox and navigate with braille viewer? You would immediately notice that title attribute is different than the link name, it should not be shown in braille at all and what is even worse content of the title attribute is shown before the content of the link.
The markup for this particular case is:
<a href="/wiki/Indo-European_languages" title="Indo-European languages">Indo-European</a>

@ABuffEr
Copy link
Contributor

ABuffEr commented Oct 1, 2021

Thanks @lukaszgo1 for explanation, I confirm all on my part.
If it can be useful, I additionally noticed that moving focus on a link with the issue, then changing and go back to tab, Braille shows the correct text until I move by line again.

@feerrenrut
Copy link
Contributor Author

Oh thanks @lukaszgo1, I seem to have mixed up my test cases while working on this.

There are a few aspects to this:

  • Description is becoming much more useful due to Aria Annotations.
  • We aim to exclude description with it's source is the title attribute, this isn't yet available in Firefox. We suspect this accounts for the majority of noisy cases.
  • Historically the option "Object presentation: Report Object Descriptions" (default: true) has been limited to focus mode. 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 specific behavior".

I will look at reverting the change in behavior for "Report Object Descriptions". I'll also see if I can add further information to the settings panel / user guide to explain the significance of "object".

Although this PR does not fix the specifically reported but, it does seem to be a general improvement. I suggest that we merge this on it's own (after updating the description / test cases). I'll continue the work in a new PR.

@feerrenrut feerrenrut changed the title fix Excessive Braille Don't braille or speak duplicate name / content / description Oct 5, 2021
@feerrenrut feerrenrut marked this pull request as ready for review October 5, 2021 09:22
@feerrenrut feerrenrut requested a review from a team as a code owner October 5, 2021 09:22
tests/system/robot/chromeTests.py Outdated Show resolved Hide resolved
nvdaHelper/vbufBackends/gecko_ia2/gecko_ia2.cpp Outdated Show resolved Hide resolved
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.

I would much prefer it if the commit to "tidy up the tests" was separated into a separate pr. This changes other tests than aria-description and creates a 300+ line diff.
But other than that everything before looks good.

@feerrenrut
Copy link
Contributor Author

I would much prefer it if the commit to "tidy up the tests" was separated into a separate pr. This changes other tests than aria-description and creates a 300+ line diff.

Hmm, on github it doesn't show many changes, but the summary does say 300+ lines changed. I'll investigate. Happy to move it to a separate PR.

@feerrenrut
Copy link
Contributor Author

I have no idea where the 300 is coming from. My local diff shows 26 lines with a + and 18 with a -

@feerrenrut feerrenrut merged commit cfeb3d2 into master Oct 11, 2021
@feerrenrut feerrenrut deleted the fixExcessiveBraille-12750 branch October 11, 2021 06:44
@isidorn
Copy link
Sponsor

isidorn commented Oct 11, 2021

Thanks so much for making this happen 👏
I really appreciate this!
I just verified with latest NVDA alpha that this indeed fixes the double read behaviour in VS Code!

@feerrenrut
Copy link
Contributor Author

Fixes #7841

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants