Add ability to recognize links to the same page by nvdaes · Pull Request #16994 · nvaccess/nvda

nvdaes · 2024-08-12T21:22:08Z

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:

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
UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers
- Localization in other languages / culture than English
API is compatible with existing add-ons.
Security precautions taken.

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.

AppVeyorBot · 2024-08-12T21:57:47Z

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 · 2024-08-12T22:28:23Z

@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 · 2024-08-13T03:19:41Z

@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 · 2024-08-13T04:32:42Z

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 · 2024-08-13T07:14:04Z

This approach is missing some things @jcsteh mentioned in #141 (comment), notably changes to the virtual buffer code.
Furthermore, UIA tree interceptors have a documentConstantIdentifier that is not an URL.

nvdaes · 2024-08-13T07:18:33Z

@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.

Add IAccessible::value to vbuf

AppVeyorBot · 2024-08-13T17:52:52Z

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 · 2024-08-13T20:56:32Z

@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 · 2024-08-13T21:27:57Z

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 · 2024-08-14T05:08:28Z

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 · 2024-08-14T06:10:31Z

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

…ort link type states

nvdaes · 2024-08-14T18:14:25Z

@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 · 2024-08-14T18:40:08Z

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 · 2024-08-14T19:26:03Z

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 · 2024-08-14T21:27:05Z

@coderabbitai review

nvdaes · 2024-09-04T09:27:22Z

@coderabbitai, unit test are failing now. Can you comment on this?

coderabbitai · 2024-09-04T09:29:43Z

Tip

For best results, initiate chat on the files or code changes.

@nvdaes, the unit test failures are due to a NameError indicating that the isSamePageURL function is not defined in the test_urlUtils.py file. This suggests that the function might not have been imported or defined correctly in the test file.

To resolve this issue, please ensure that the isSamePageURL function is correctly imported from the module where it is defined. If the function is defined in a different file, you can import it at the beginning of the test_urlUtils.py file like this:

from source.utils.urlUtils import isSamePageURL

Once the import is correctly set up, the tests should be able to recognize the isSamePageURL function, and the errors should be resolved. Let me know if you need further assistance!

Analysis chain

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Fetch the details of the failing unit tests from the most recent build logs.
# The logs will help identify which tests are failing and why.

# Extract the last commit hash to ensure we're analyzing the latest build.
last_commit=$(git log -1 --format="%H")

# Fetch the unit test results from the latest build associated with the last commit.
curl -s "https://ci.appveyor.com/api/buildjobs/mlgnc1u3l7m8fw86/log" | grep -A 20 "FAILED TESTS"

Length of output: 177

Script:

#!/bin/bash
# Fetch the details of the failing unit tests from the most recent build logs.
# The logs will help identify which tests are failing and why.

# Extract the last commit hash to ensure we're analyzing the latest build.
last_commit=$(git log -1 --format="%H")

# Fetch the unit test results from the latest build associated with the last commit.
curl -s "https://ci.appveyor.com/api/buildjobs/mlgnc1u3l7m8fw86/log" | grep -E -A 50 "FAIL|ERROR|Traceback"

Length of output: 32759

nvdaes · 2024-09-04T09:45:44Z

@coderabbitai, can you check if function and variable names should be improved for English or to follow recommended conventions?

coderabbitai · 2024-09-04T09:49:56Z

Tip

For best results, initiate chat on the files or code changes.

@nvdaes, I've reviewed the variable names extracted from the urlUtils.py file, and I have a few suggestions to improve clarity and adherence to Python naming conventions:

targetURLOnPageWithoutFragments: This name is descriptive but uses camelCase, which is not the recommended convention in Python. I suggest renaming it to target_url_on_page_without_fragments to follow the snake_case convention.
rootURLWithoutFragments: Similar to the previous variable, this name uses camelCase. I recommend renaming it to root_url_without_fragments for consistency with Python naming conventions.
ParseResult: This name is capitalized, which suggests it might be a class or a type rather than a variable. If it's a variable, it should follow the snake_case convention. If it's a class or type, the name is fine as is.
str: This name shadows the built-in str type in Python, which can lead to potential issues. I suggest renaming it to something more descriptive, such as url_str, to avoid conflicts.

The other variable names, such as validSchemes, scheme, and fragment, are clear and follow the recommended conventions.

Let me know if you need any further assistance or if you'd like to discuss these suggestions!

Analysis chain

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Extract function and variable names from the urlUtils.py file for review.

# Extract function names
echo "Function Names:"
ast-grep --lang python --pattern 'def $_($_):' -- source/utils/urlUtils.py

# Extract variable names
echo "Variable Names:"
rg --python --pcre2 --no-heading --no-line-number --only-matching '\b\w+\b(?=\s*=)' source/utils/urlUtils.py

Length of output: 680

Script:

#!/bin/bash
# Extract function and variable names from the urlUtils.py file for review.

# Extract function names using ast-grep
echo "Function Names:"
ast-grep --lang python --pattern 'def $_($_):' -- source/utils/urlUtils.py

# Extract variable names using rg without the invalid --python flag
echo "Variable Names:"
rg --no-heading --no-line-number --only-matching '\b\w+\b(?=\s*=)' source/utils/urlUtils.py

Length of output: 478

Script:

#!/bin/bash
# Extract function and variable names from the urlUtils.py file for review.

# Extract function names using ast-grep
echo "Function Names:"
ast-grep --lang python --pattern 'def $_($_):' -- source/utils/urlUtils.py

# Extract variable names using rg with the --pcre2 flag to support look-around assertions
echo "Variable Names:"
rg --pcre2 --no-heading --no-line-number --only-matching '\b\w+\b(?=\s*=)' source/utils/urlUtils.py

Length of output: 421

nvdaes · 2024-09-04T09:53:11Z

h@coderabbitai, NVDA project doesn't use the snakeCase style.
Can you check if names in test_urlUtils.py should be improved to follow Python conventions or English grammar?

coderabbitai · 2024-09-04T09:55:13Z

Tip

For best results, initiate chat on the files or code changes.

@nvdaes, the test_urlUtils.py file contains only one variable name, q, which is not very descriptive. I recommend renaming it to something more meaningful to improve clarity. If you need further assistance or have any other questions, feel free to reach out!

Analysis chain

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Extract function and variable names from the test_urlUtils.py file for review.

# Extract function names using ast-grep
echo "Function Names:"
ast-grep --lang python --pattern 'def $_($_):' -- test_urlUtils.py

# Extract variable names using rg with the --pcre2 flag to support look-around assertions
echo "Variable Names:"
rg --pcre2 --no-heading --no-line-number --only-matching '\b\w+\b(?=\s*=)' test_urlUtils.py

Length of output: 365

Script:

#!/bin/bash
# Search for files with names that include "urlUtils" or similar.
fd -i urlUtils

Length of output: 79

Script:

#!/bin/bash
# Extract function and variable names from the correct test_urlUtils.py file for review.

# Extract function names using ast-grep
echo "Function Names:"
ast-grep --lang python --pattern 'def $_($_):' -- tests/unit/test_util/test_urlUtils.py

# Extract variable names using rg with the --pcre2 flag to support look-around assertions
echo "Variable Names:"
rg --pcre2 --no-heading --no-line-number --only-matching '\b\w+\b(?=\s*=)' tests/unit/test_util/test_urlUtils.py

Length of output: 288

nvdaes · 2024-09-04T09:56:32Z

@coderabbitai, can you check if function names in test_urlUtils should be improved?

coderabbitai · 2024-09-04T09:59:44Z

Tip

For best results, initiate chat on the files or code changes.

@nvdaes, I've reviewed the function names in test_urlUtils.py, and they are well-structured, descriptive, and follow the camelCase convention, which aligns with the NVDA project's style. There is no need for any changes to these function names. Let me know if there's anything else you'd like to review or discuss!

Analysis chain

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Extract function names from the test_urlUtils.py file for review.
ast-grep --lang python --pattern 'def $_($_):' -- tests/unit/test_util/test_urlUtils.py

Length of output: 89

Script:

#!/bin/bash
# Search for function definitions in the test_urlUtils.py file using rg.
rg --no-heading --line-number --only-matching 'def \w+' tests/unit/test_util/test_urlUtils.py

Length of output: 450

XLTechie · 2024-09-04T10:18:40Z

@seanbudd, can we tell the rabbit in the config file, that we use cammelCase, and not to suggest snake_case?

AppVeyorBot · 2024-09-04T11:38:22Z

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/8re15mo5wk0v00nn/artifacts/output/nvda_snapshot_pr16994-33821,3bc70532.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.3,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 12.6,
TESTSETUP_START 0.0,
TESTSETUP_END 0.4,
TEST_START 0.0,
TEST_END 18.8,
FINISH_END 0.1

See test results for failed build of commit 3bc70532e5

seanbudd · 2024-09-05T02:42:43Z

@XLTechie sure that can be proposed as a PR to the config file.
I would say though to @nvdaes - I think you are pushing the limits of how CodeRabbit should be used. It's not designed in the same way as GitHub CoPilot or CodiumAI as an integrated generative code dev tool. It's primarily designed for code reviews. I would encourage using CoPilot or similar to get integrated AI into your developer environment, rather than trying to get general code suggestions from CodeRabbit.

nvdaes · 2024-09-05T04:00:52Z

Sean wrote:

I think you are pushing the limits of how CodeRabbit should be used.

OK, thanks for the clarification. I think that this is ready for review.

Qchristensen

Good work, and well done on closing one of our older issues!

seanbudd · 2024-09-06T01:08:41Z

@coderabbitai resolve

Fixes issue #17127 Fixup of PR #16994 Summary of the issue: Same page links are reported, but not on local files. Description of user facing changes: Same page links will be also reported on local files like HTML documents. Description of development approach: In the `samePageURL` function, "file" has been added to `validSchemes`. Testing strategy: - Tested manually with NVDA's user guide. - Performed unit tests.

Add ability to recognize links to the same page

4e1f7cd

seanbudd added the blocked/needs-product-decision A product decision needs to be made. Decisions about NVDA UX or supported use-cases. label Aug 13, 2024

LeonarddeR and others added 2 commits August 13, 2024 18:53

Add IAccessible::value to vbuf

60dd50a

Merge pull request #4 from LeonarddeR/accValue

9d13634

Add IAccessible::value to vbuf

Add internal link state for some links in virtualBuffers

445be6c

Cleanup code

82d34be

nvdaes added 2 commits August 14, 2024 07:21

Merge branch 'master' into links

e0ac8a8

Make _valueToSamePage function private

253e576

LeonarddeR reviewed Aug 14, 2024

View reviewed changes

Comment thread source/NVDAObjects/IAccessible/ia2Web.py Outdated

Add configuration options and unassigned command to cycle between rep…

eb01b6f

…ort link type states

Don't check if object is internal link if the option is disabled

5cf585c

nvdaes added 4 commits August 14, 2024 22:16

dd configuration to format document settings panel

ba36a24

Add urlUtils module

4c4d8e3

Updated user guide nentioning this option, without a specific section

a965d2d

Update documentation for changes

fae812b

nvdaes added 3 commits September 4, 2024 11:33

Add missing import

30381ed

Fix variable name

8620913

Fix variable name

957d541

Improve function name

96d8462

Don't toggle report link type if report link is disabled

aa83277

seanbudd approved these changes Sep 5, 2024

View reviewed changes

Comment thread tests/unit/test_util/test_urlUtils.py Outdated

seanbudd and others added 2 commits September 6, 2024 09:54

Update tests/unit/test_util/test_urlUtils.py

b8eb917

Merge branch 'master' into links

2ca89c0

Qchristensen approved these changes Sep 6, 2024

View reviewed changes

Merge branch 'master' into links

18b0034

seanbudd merged commit 21fbfa7 into nvaccess:master Sep 6, 2024

nvdaes mentioned this pull request Sep 6, 2024

Improve the report link feature #17127

Closed

codeofdusk mentioned this pull request Sep 6, 2024

Disable reporting of link types by default #17128

Closed

5 tasks

nvdaes mentioned this pull request Sep 11, 2024

Report same page links on local files #17161

Merged

5 tasks

nvdaes mentioned this pull request Dec 12, 2024

Add "Only in edit controls" mode for typing echo #17505

Merged

5 tasks

nishimotz mentioned this pull request Apr 14, 2025

2025.1 本家の翻訳 nvdajp/nvdajp#498

Closed

Uh oh!

Conversation

nvdaes commented Aug 12, 2024 • edited by coderabbitai Bot 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:

Code Review Checklist:

Summary by CodeRabbit

Summary by CodeRabbit

Uh oh!

AppVeyorBot commented Aug 12, 2024

Uh oh!

XLTechie commented Aug 12, 2024 via email

Uh oh!

nvdaes commented Aug 13, 2024

Uh oh!

seanbudd commented Aug 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LeonarddeR commented Aug 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nvdaes commented Aug 13, 2024

Uh oh!

AppVeyorBot commented Aug 13, 2024

Uh oh!

nvdaes commented Aug 13, 2024

Uh oh!

AppVeyorBot commented Aug 13, 2024

Uh oh!

AppVeyorBot commented Aug 14, 2024

Uh oh!

AppVeyorBot commented Aug 14, 2024

Uh oh!

Uh oh!

nvdaes commented Aug 14, 2024

Uh oh!

AppVeyorBot commented Aug 14, 2024

Uh oh!

nvdaes commented Aug 14, 2024

Uh oh!

nvdaes commented Aug 14, 2024

Uh oh!

nvdaes commented Sep 4, 2024

Uh oh!

coderabbitai Bot commented Sep 4, 2024

Uh oh!

nvdaes commented Sep 4, 2024

Uh oh!

coderabbitai Bot commented Sep 4, 2024

Uh oh!

nvdaes commented Sep 4, 2024

Uh oh!

coderabbitai Bot commented Sep 4, 2024

Uh oh!

nvdaes commented Sep 4, 2024

Uh oh!

coderabbitai Bot commented Sep 4, 2024

Uh oh!

XLTechie commented Sep 4, 2024 via email

Uh oh!

AppVeyorBot commented Sep 4, 2024

Uh oh!

seanbudd commented Sep 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nvdaes commented Sep 5, 2024

Uh oh!

Uh oh!

Qchristensen left a comment

Choose a reason for hiding this comment

Uh oh!

seanbudd commented Sep 6, 2024

Uh oh!

Reviewers

Assignees

nvdaes commented Aug 12, 2024 •

edited by coderabbitai Bot

Loading

seanbudd commented Aug 13, 2024 •

edited

Loading

LeonarddeR commented Aug 13, 2024 •

edited

Loading

seanbudd commented Sep 5, 2024 •

edited

Loading