Add ability to recognize links to the same page

[nvdaes]

Link to issue number:

Fixes #141

Summary of the issue:

NVDA cannot recognize when link destination points to the same document, and reporting this information is desired by many users.

Description of user facing changes

NVDA can report if a link destination points to the same document.

Description of development approach

• A new INTERNAL_LINK state has been added to controlTypes.State.
• NVDA object has a new linkType property, used to report internal (same page) links.
• If the object is an internal link, the INTERNAL_LINKstate is added in _get_states, in IAccessible/IA2web and in UIA/Chromium.
• In the gecko_ia2 virtual buffer backend, accValue is exposed for links.
• In virtualBuffers.gecko_ia2.Gecko_ia2_TextInfo._normalizeControlField, the accValue is used to add the linkType state.
• A configuration option has been added to enable or disable this feature. This can be changed through gui or with an unassigned gesture.
• BrowseModeInterceptor object has a new documentUrl property and a new getLinkTypeInDocument method. This method uses a helper function of a new urlUtils submodule to check the provided URL.

Testing strategy:

Tested manually:

• Checked that same page links can be reported in focus and browse mode. Tests have been performed in Firefox, Edge and Chrome.
• Checked that changing the configuration produces expected results, and that it can be modified assigning a gesture or using the document formatting settings panel.

Known issues with pull request:

None

Code Review Checklist:

• [x] Documentation:
  • Change log entry
  • User Documentation
  • Developer / Technical Documentation
  • Context sensitive help for GUI changes
• [ ] Testing:
  • Unit tests
  • System (end to end) tests
  • Manual testing
• [x] UX of all users considered:
  • Speech
  • Braille
  • Low Vision
  • Different web browsers
  • Localization in other languages / culture than English
• [x] API is compatible with existing add-ons.
• [x] Security precautions taken.

Summary by CodeRabbit

Summary by CodeRabbit

• New Features

  • Enhanced accessibility handling for same-page links.
  • Introduced the isInternalLink property for better identification of internal links.
  • Added a feature flag for configurable link type reporting.
  • New utility functions for validating same-page links and determining link types.
  • Improved settings dialog for selecting report link types and managing their states.

• Bug Fixes

  • Enhanced logical flow for enabling/disabling report link type settings based on user selections.

• Documentation

  • Updated user guide to include information on the new link type reporting options.

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: License check.
• PASS: Unit tests.
• PASS: Lint check.
• FAIL: System tests (tags: installer NVDA). See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/s0tq77hudd0nukpr/artifacts/output/nvda_snapshot_pr16994-33345,faef7e20.exe
• CI timing (mins): INIT 0.0, INSTALL_START 1.1, INSTALL_END 1.1, BUILD_START 0.0, BUILD_END 13.3, TESTSETUP_START 0.0, TESTSETUP_END 0.4, TEST_START 0.0, TEST_END 18.3, FINISH_END 0.2

See test results for failed build of commit faef7e208e

[XLTechie]

@nvdaes may I suggest "isSamePage", and in other cases, "same page" instead of "internal"?

I think (not 100%) that other screen readers use the phrasing "in-page link" or "same page link" for this.

[nvdaes]

@XLTechie , I've added _("some page") for the display string of the new created INTERNAL_LINK state. Let me know if this is not enough, though this is blocked waiting for product decision, according to a label from @seanbudd.

[seanbudd]

Hi @nvdaes we're just waiting to see how the PR approach develops before it is marked as ready for review and given the conceptApproved label

[LeonarddeR]

This approach is missing some things @jcsteh mentioned in https://github.com/nvaccess/nvda/issues/141#issuecomment-2284080383, notably changes to the virtual buffer code. Furthermore, UIA tree interceptors have a documentConstantIdentifier that is not an URL.

[nvdaes]

@LeonarddeR , as mentioned by me, I don't have experience with C++, so I was waiting for further guidance. If you want to complete this work, for me is OK. I will learn a lot anyway.

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: License check.
• PASS: Unit tests.
• PASS: Lint check.
• FAIL: System tests (tags: installer NVDA). See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/bq9hwu0wsmvq2ad2/artifacts/output/nvda_snapshot_pr16994-33352,c78e95c8.exe
• CI timing (mins): INIT 0.0, INSTALL_START 1.4, INSTALL_END 1.0, BUILD_START 0.0, BUILD_END 13.9, TESTSETUP_START 0.0, TESTSETUP_END 0.4, TEST_START 0.0, TEST_END 18.5, FINISH_END 0.2

See test results for failed build of commit c78e95c866

[nvdaes]

@LeonarddeR has done a great work here, and I think we can include him as the first author of this in the changelog entry, and thanks also Jamie for his guidance. I haven't worked with C++ nor with virtualBuffers, so I'm sure that my work is wrong. Anyway, at least some "same page links" are recognized now. Please provide more feedback. I don't know how to get info from virtualBuffers. Thanks.

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: License check.
• PASS: Unit tests.
• PASS: Lint check.
• FAIL: System tests (tags: installer NVDA). See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/xb5enk74bbc7tfsh/artifacts/output/nvda_snapshot_pr16994-33354,af3213bb.exe
• CI timing (mins): INIT 0.0, INSTALL_START 1.3, INSTALL_END 1.0, BUILD_START 0.0, BUILD_END 13.4, TESTSETUP_START 0.0, TESTSETUP_END 0.4, TEST_START 0.0, TEST_END 18.0, FINISH_END 0.2

See test results for failed build of commit af3213bbf6

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: License check.
• PASS: Unit tests.
• PASS: Lint check.
• FAIL: System tests (tags: installer NVDA). See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/8h74q54tc3qq9h8n/artifacts/output/nvda_snapshot_pr16994-33358,ae331b77.exe
• CI timing (mins): INIT 0.0, INSTALL_START 1.2, INSTALL_END 1.1, BUILD_START 0.0, BUILD_END 13.3, TESTSETUP_START 0.0, TESTSETUP_END 0.4, TEST_START 0.0, TEST_END 18.2, FINISH_END 0.1

See test results for failed build of commit ae331b776e

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: License check.
• PASS: Unit tests.
• PASS: Lint check.
• FAIL: System tests (tags: installer NVDA). See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/j9sp72t50y9m4jwb/artifacts/output/nvda_snapshot_pr16994-33360,72332730.exe
• CI timing (mins): INIT 0.0, INSTALL_START 1.3, INSTALL_END 1.0, BUILD_START 0.0, BUILD_END 12.5, TESTSETUP_START 0.0, TESTSETUP_END 0.4, TEST_START 0.0, TEST_END 17.9, FINISH_END 0.2

See test results for failed build of commit 72332730ff

[nvdaes]

@LeonarddeR , I think that the cpp file hchanges are not following the style of this project, since space is used after if. Seeems that this is not been formatted automatically. I'll change this is reviewers request to do it. Thanks a lot.

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: License check.
• PASS: Unit tests.
• PASS: Lint check.
• FAIL: System tests (tags: installer NVDA). See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/m8ggiq06w0dn5409/artifacts/output/nvda_snapshot_pr16994-33363,4516d2d2.exe
• CI timing (mins): INIT 0.0, INSTALL_START 1.3, INSTALL_END 0.9, BUILD_START 0.0, BUILD_END 12.7, TESTSETUP_START 0.0, TESTSETUP_END 0.4, TEST_START 0.0, TEST_END 18.5, FINISH_END 0.1

See test results for failed build of commit 4516d2d20c

[nvdaes]

Robot . chromeTests . Only report description in focus mode is failing, but I think that this is not related to this PR, though I'm not 100% sure. Previously, other tests failed and this is expected since links used for testing were identified as same page links. If finally, as some users prefer, we enable this feature by default, we need to specify that this is disabled in the configuration file used by tests.

[nvdaes]

@coderabbitai review

[coderabbitai[bot]]

Walkthrough

This set of changes enhances NVDA's accessibility features by clearly distinguishing between external links and internal links, particularly same-page links. Key updates include new methods and properties for identifying internal links, a configuration option for reporting link types, and improvements to the user interface for managing these settings. These modifications ensure that NVDA accurately announces same-page links, significantly improving navigational clarity for users.

Changes

Files	Change Summary
nvdaHelper/vbufBackends/gecko_ia2/gecko_ia2.cpp	Enhanced fillVBuf to store IAccessible values for same-page links, improving accessibility handling.
source/NVDAObjects/IAccessible/ia2Web.py	Added isInternalLink method to identify internal links and updated _get_states to include the INTERNAL_LINK state.
source/config/configSpec.py	Introduced reportLinkType configuration option to control the reporting of link types as a feature flag.
source/controlTypes/state.py	Added INTERNAL_LINK constant for representing same-page links in control states.
source/globalCommands.py	New function script_cyclesReportLinkType to cycle through report link types in document formatting settings.
source/gui/settingsDialogs.py	Implemented a new combo box for selecting report link types, linked to checkbox state, and updated onSave to save the selected type.
source/utils/urlUtils.py	Introduced utility functions getLinkType and isSamePageUrl to determine link types and verify if a link points to the same page.
tests/system/nvdaSettingsFiles/standard-dontShowWelcomeDialog.ini	Added new configuration section for reportLinkType to manage link reporting preferences.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant GUI
    participant Settings
    participant Commands
    participant Utils

    User->>GUI: Opens settings
    GUI->>Settings: Load current link settings
    Settings-->>GUI: Display combo box for report link type
    User->>Settings: Change link type
    Settings->>Commands: Call script_cyclesReportLinkType
    Commands->>Settings: Update link type state
    Settings->>GUI: Refresh display

Assessment against linked issues

Objective	Addressed	Explanation
Differentiate between external and same-page links (#[141])	✅
Enhance user awareness of same-page links (#[141])	✅

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[seanbudd]

It seems like there's ongoing discussion on #141 on whether or not this should be enabled by default. I think it should be the same as other similar config values, boolean(default=true)

[nvdaes]

@coderabbitai, I won't address your comments about code related to the configuration, including the combo box, since I wrote code based on the unicode normalization combo box and the script_braille_cycleShowSelection global commands, included in this project.

[coderabbitai[bot]]

[!TIP] For best results, initiate chat on the files or code changes.

@nvdaes, understood. If you've based your implementation on existing patterns within the project, it makes sense to maintain consistency. Let me know if there's anything else you'd like me to review or assist with.

[nvdaes]

@seanbudd and participants in the discussion about the default value, this is now enabled by default. If alpha users complain about this, it can be changed later. Also, thanks to participants of the spanish community on a mailing list and privately.

[LeonarddeR]

@nvdaes wrote:

@LeonarddeR , I think that the cpp file hchanges are not following the style of this project, since space is used after if. Seeems that this is not been formatted automatically.

The C++ code style is a mess. Unwritten consensus is now that we stick mostly to the style I used here (i.e. add a space after if, add spaces around operators like ==, etc.) I think in the event limiter, the Visual Studio code style was used, though, so I'm really not sure. I opened https://github.com/nvaccess/nvda/issues/17009 to discuss this further.

[AppVeyorBot]

• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/oe5qgmyx1hhhqc9l/artifacts/output/
• CI timing (mins): INIT 0.0, INSTALL_START 1.4, INSTALL_END 1.0, BUILD_START 0.0, FINISH_END 10.9

See test results for failed build of commit a6c59d2772

[nvdaes]

@LeonarddeR , unfortunately, changes proposed in virtualBuffers\gecko_ia2 doesn't work. Let's use NVDAObjectAtStart again.

[nvdaes]

I've tested this after fixing syntax errors and importing urlUtils, but this doesn't work and same page links aren't detected now.

[nvdaes]

@LeonarddeR , I'll test carefully changes proposed by AI about using urllib. Also, I'm curious about what will happen if we don't change the VirtualBuffer backend, just using NVDAObjectAtStart and the isInternalLink property. I'll test this, though the changes in backend are valuable anyway.

[nvdaes]

Just as a note, seems that using the isInternalLink property of the TextInfo.NVDAObjectAtStart object, it's not needed to change virtual buffer backends for this PR. Anyway I think we can keep this. Maybe needed in the future.

[LeonarddeR]

Just as a note, seems that using the isInternalLink property of the TextInfo.NVDAObjectAtStart object, it's not needed to change virtual buffer backends for this PR.

While it technically works, it requires an extra out of process IAccessible2 call to fetch the value on every calculation of the fields in the textInfo. I would discourage it.

[LeonarddeR]

I filed https://github.com/nvdaes/nvda/pull/5 Note that if we want quick nav to same page links, we will need to port the urlUtils function to C++. That's not ideal, so lets leave that out of scope.

[nvdaes]

Thanks @LeonarddeR . You are right: I didn't perform an extra scons source. I've merged your pull request. We have to add the utlUtils to changes for developers. I'll see if more tests are needed. I was building the unit tests but you were faster than me 😀

[nvdaes]

I'm marking this as ready for review. I'll on hollidays from next Monday and I'll come back on September. TomorrowI can address suggestions, and if this is marked as a draft I'll take care when I come back.