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

Add ability to recognize links to the same page #16994

Merged
merged 76 commits into from
Sep 6, 2024
Merged

Conversation

nvdaes
Copy link
Collaborator

@nvdaes nvdaes commented Aug 12, 2024

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

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
Copy link

See test results for failed build of commit faef7e208e

@XLTechie
Copy link
Collaborator

XLTechie commented Aug 12, 2024 via email

@seanbudd 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
@nvdaes
Copy link
Collaborator Author

nvdaes commented Aug 13, 2024

@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
Copy link
Member

seanbudd commented Aug 13, 2024

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
Copy link
Collaborator

LeonarddeR commented Aug 13, 2024

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
Copy link
Collaborator Author

nvdaes commented Aug 13, 2024

@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
Copy link

See test results for failed build of commit c78e95c866

@nvdaes
Copy link
Collaborator Author

nvdaes commented Aug 13, 2024

@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
Copy link

See test results for failed build of commit af3213bbf6

@AppVeyorBot
Copy link

See test results for failed build of commit ae331b776e

@AppVeyorBot
Copy link

See test results for failed build of commit 72332730ff

@nvdaes
Copy link
Collaborator Author

nvdaes commented Aug 14, 2024

@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
Copy link

See test results for failed build of commit 4516d2d20c

@nvdaes
Copy link
Collaborator Author

nvdaes commented Aug 14, 2024

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
Copy link
Collaborator Author

nvdaes commented Aug 14, 2024

@coderabbitai review

Copy link
Contributor

coderabbitai bot commented Sep 4, 2024

Tip

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

@nvdaes, I've reviewed the latest changes, and I can confirm that the missing translator comments have been successfully added to the script_toggleReportLinkType function. This should resolve the previous issue related to translation comments. Everything looks good from my side! If there's anything else you'd like to review or discuss, feel free to reach out. Great job on the updates!

Analysis chain
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Fetch the latest diffs for the most recent commits in the PR to review the changes made.
git log -p -1

Length of output: 835

@nvdaes
Copy link
Collaborator Author

nvdaes commented Sep 4, 2024

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

Copy link
Contributor

coderabbitai bot commented Sep 4, 2024

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
Copy link
Collaborator Author

nvdaes commented Sep 4, 2024

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

Copy link
Contributor

coderabbitai bot commented Sep 4, 2024

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:

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

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

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

  4. 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
Copy link
Collaborator Author

nvdaes commented Sep 4, 2024

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?

Copy link
Contributor

coderabbitai bot commented Sep 4, 2024

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
Copy link
Collaborator Author

nvdaes commented Sep 4, 2024

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

Copy link
Contributor

coderabbitai bot commented Sep 4, 2024

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
Copy link
Collaborator

XLTechie commented Sep 4, 2024 via email

@AppVeyorBot
Copy link

See test results for failed build of commit 3bc70532e5

@seanbudd
Copy link
Member

seanbudd commented Sep 5, 2024

@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
Copy link
Collaborator Author

nvdaes commented Sep 5, 2024

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.

tests/unit/test_util/test_urlUtils.py Outdated Show resolved Hide resolved
Copy link
Member

@Qchristensen Qchristensen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@seanbudd
Copy link
Member

seanbudd commented Sep 6, 2024

@coderabbitai resolve

@seanbudd seanbudd merged commit 21fbfa7 into nvaccess:master Sep 6, 2024
1 of 3 checks passed
SaschaCowley pushed a commit that referenced this pull request Sep 18, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked/needs-product-decision A product decision needs to be made. Decisions about NVDA UX or supported use-cases. merge-early Merge Early in a developer cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NVDA doesn't know a difference between Links and Same Page Links
6 participants