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
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
76 commits
Select commit Hold shift + click to select a range
4e1f7cd
Add ability to recognize links to the same page
nvdaes Aug 12, 2024
60dd50a
Add IAccessible::value to vbuf
LeonarddeR Aug 13, 2024
9d13634
Merge pull request #4 from LeonarddeR/accValue
nvdaes Aug 13, 2024
445be6c
Add internal link state for some links in virtualBuffers
nvdaes Aug 13, 2024
82d34be
Cleanup code
nvdaes Aug 14, 2024
e0ac8a8
Merge branch 'master' into links
nvdaes Aug 14, 2024
253e576
Make _valueToSamePage function private
nvdaes Aug 14, 2024
eb01b6f
Add configuration options and unassigned command to cycle between rep…
nvdaes Aug 14, 2024
5cf585c
Don't check if object is internal link if the option is disabled
nvdaes Aug 14, 2024
ba36a24
dd configuration to format document settings panel
nvdaes Aug 14, 2024
4c4d8e3
Add urlUtils module
nvdaes Aug 14, 2024
a965d2d
Updated user guide nentioning this option, without a specific section
nvdaes Aug 14, 2024
fae812b
Update documentation for changes
nvdaes Aug 14, 2024
cedfc69
Enable reporting of link type by default, addressing reviewsuggestion
nvdaes Aug 15, 2024
362e583
Apply suggestions from code review
nvdaes Aug 15, 2024
549a40c
Revert some changes
nvdaes Aug 15, 2024
5e35313
Add braille label for INTERNAL_LINK state, addressing review suggestion
nvdaes Aug 15, 2024
13bf11d
Use urlparse for function to check same page links, suggested by reviewr
nvdaes Aug 15, 2024
bf290bb
Merge branch 'master' into links
nvdaes Aug 15, 2024
b032b2e
Several changes as proposed in review, working
LeonarddeR Aug 15, 2024
c156c71
Merge pull request #5 from LeonarddeR/linksFollowUp
nvdaes Aug 15, 2024
456facb
Pre-commit auto-fix
pre-commit-ci[bot] Aug 15, 2024
b483be6
Update changes for developers
nvdaes Aug 15, 2024
a18de14
Don't consider same page if fragment contains a path like in Gmail la…
nvdaes Aug 15, 2024
5efea77
Add support for Edge
LeonarddeR Aug 16, 2024
f84d23e
Add LINKED state check
LeonarddeR Aug 16, 2024
ffad846
Merge pull request #6 from LeonarddeR/linksEdge
nvdaes Aug 16, 2024
37c9bf4
Apply suggestions from code review
nvdaes Aug 16, 2024
d4b9ea2
Mention developer in changelog
nvdaes Aug 16, 2024
69bd7e7
Update changes for developers
nvdaes Aug 16, 2024
e207ca5
Applysuggestions from coderabbitai
nvdaes Aug 16, 2024
07ddb09
Ensure that fragments of internal links are alnum
nvdaes Aug 16, 2024
984a59e
Accept more characters in fragments
nvdaes Aug 16, 2024
90bbd12
Use link type property, instead of isInternalLink
nvdaes Aug 17, 2024
4b21bb3
Fix documentation for function
nvdaes Aug 17, 2024
d602cbe
Apply suggestions from coderabbitai
nvdaes Aug 17, 2024
4fbe4cb
Add STETE_LINK_TYPE
nvdaes Aug 17, 2024
cd347d2
Merge branch 'master' into links
nvdaes Aug 31, 2024
df22719
Apply suggestions from code review
nvdaes Sep 2, 2024
a0af6ee
Address review
nvdaes Sep 2, 2024
d1c6286
Fix system tests
nvdaes Sep 2, 2024
af32760
Apply suggestions from code review
nvdaes Sep 3, 2024
65d82db
Pre-commit auto-fix
pre-commit-ci[bot] Sep 3, 2024
161b2fa
Fux urlUtils
nvdaes Sep 3, 2024
a2b1a88
Add type hint for gesture
nvdaes Sep 3, 2024
f0cbcbe
Improve unit tests
nvdaes Sep 3, 2024
9e767d2
Improve urlUtils
nvdaes Sep 3, 2024
48f1b61
Fix urlUtils
nvdaes Sep 3, 2024
db0c8cd
Fix variable
nvdaes Sep 3, 2024
fab26cf
Add toggleBooleanValue function
nvdaes Sep 3, 2024
b91c8d2
Pre-commit auto-fix
pre-commit-ci[bot] Sep 3, 2024
0bcc928
Fix changes for developers
nvdaes Sep 3, 2024
4849e6b
Merge branch 'links' of https://github.com/nvdaes/nvda into links
nvdaes Sep 3, 2024
bbce160
Update changes for developers
nvdaes Sep 3, 2024
8fb71ca
Remove % from invalidChars
nvdaes Sep 3, 2024
d6d59f8
Fix script and function to toggle boolean values
nvdaes Sep 3, 2024
1fc8382
Pre-commit auto-fix
pre-commit-ci[bot] Sep 3, 2024
4e331d5
Apply suggestions from code review
nvdaes Sep 4, 2024
e526e25
Merge remote-tracking branch 'nvaccess/master' into links
nvdaes Sep 4, 2024
0cb9b6b
Remove accidentally included info
nvdaes Sep 4, 2024
19bcae9
Apply the same page link concept, not anchor approach
nvdaes Sep 4, 2024
ce7396b
Pre-commit auto-fix
pre-commit-ci[bot] Sep 4, 2024
1dab305
Fix script
nvdaes Sep 4, 2024
64cbf92
Uppercase URL word
nvdaes Sep 4, 2024
da60228
Pre-commit auto-fix
pre-commit-ci[bot] Sep 4, 2024
8eddd33
Fix grammar in vaariable names
nvdaes Sep 4, 2024
1529e2c
Merge branch 'links' of https://github.com/nvdaes/nvda into links
nvdaes Sep 4, 2024
9dff709
Add missing translator comments
nvdaes Sep 4, 2024
30381ed
Add missing import
nvdaes Sep 4, 2024
8620913
Fix variable name
nvdaes Sep 4, 2024
957d541
Fix variable name
nvdaes Sep 4, 2024
96d8462
Improve function name
nvdaes Sep 4, 2024
aa83277
Don't toggle report link type if report link is disabled
nvdaes Sep 4, 2024
b8eb917
Update tests/unit/test_util/test_urlUtils.py
seanbudd Sep 5, 2024
2ca89c0
Merge branch 'master' into links
seanbudd Sep 5, 2024
18b0034
Merge branch 'master' into links
seanbudd Sep 6, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions nvdaHelper/vbufBackends/gecko_ia2/gecko_ia2.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1000,10 +1000,16 @@ VBufStorage_fieldNode_t* GeckoVBufBackend_t::fillVBuf(
}

BSTR value=NULL;
if(pacc->get_accValue(varChild,&value)==S_OK) {
if(value&&SysStringLen(value)==0) {
SysFreeString(value);
value=NULL;
if (pacc->get_accValue(varChild, &value) == S_OK) {
if (value) {
if (role == ROLE_SYSTEM_LINK) {
// For links, store the IAccessible value to handle same page link detection.
parentNode->addAttribute(L"IAccessible::value", value);
}
if (SysStringLen(value)==0) {
nvdaes marked this conversation as resolved.
Show resolved Hide resolved
SysFreeString(value);
value=NULL;
nvdaes marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

Expand Down
6 changes: 6 additions & 0 deletions source/NVDAObjects/IAccessible/ia2Web.py
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,12 @@ def _get_states(self):
if popupState:
states.discard(controlTypes.State.HASPOPUP)
states.add(popupState)
if (
self.role == controlTypes.Role.LINK
and controlTypes.State.LINKED in states
and self.isInternalLink
):
states.add(controlTypes.State.INTERNAL_LINK)
return states

def _get_landmark(self):
Expand Down
9 changes: 9 additions & 0 deletions source/NVDAObjects/UIA/chromium.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,20 @@ def _getControlFieldForUIAObject(self, obj, isEmbedded=False, startOfNode=False,
class ChromiumUIA(web.UIAWeb):
_TextInfo = ChromiumUIATextInfo

def _get_states(self):
seanbudd marked this conversation as resolved.
Show resolved Hide resolved
states = super().states
if self.role == controlTypes.Role.LINK and self.isInternalLink:
states.add(controlTypes.State.INTERNAL_LINK)
return states


class ChromiumUIATreeInterceptor(web.UIAWebTreeInterceptor):
def _get_documentConstantIdentifier(self):
return self.rootNVDAObject.parent._getUIACacheablePropertyValue(UIAHandler.UIA_AutomationIdPropertyId)

def _get_documentUrl(self):
seanbudd marked this conversation as resolved.
Show resolved Hide resolved
return self.rootNVDAObject.value


class ChromiumUIADocument(ChromiumUIA):
treeInterceptorClass = ChromiumUIATreeInterceptor
Expand Down
14 changes: 14 additions & 0 deletions source/NVDAObjects/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -1627,3 +1627,17 @@ def _get_isBelowLockScreen(self) -> bool:
if not isLockScreenModeActive():
return False
return _isObjectBelowLockScreen(self)

isInternalLink: bool
"""Typing information for auto property _get_isInternalLink
"""

def _get_isInternalLink(self) -> bool:
if self.role != controlTypes.Role.LINK:
return False
from browseMode import BrowseModeDocumentTreeInterceptor

ti = getattr(self, "treeInterceptor", None)
if not isinstance(ti, BrowseModeDocumentTreeInterceptor):
return False
return ti.isInternalLinkInDocument(self.value)
2 changes: 2 additions & 0 deletions source/braille.py
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,8 @@
controlTypes.State.HASCOMMENT: _("cmnt"),
# Translators: Displayed in braille when a control is switched on
controlTypes.State.ON: "⣏⣿⣹",
# Translators: Displayed in braille when a link destination pointsto the same page
nvdaes marked this conversation as resolved.
Show resolved Hide resolved
controlTypes.State.INTERNAL_LINK: _("smp"),
}
negativeStateLabels = {
# Translators: Displayed in braille when an object is not selected.
Expand Down
9 changes: 9 additions & 0 deletions source/browseMode.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
import gui.contextHelp
from abc import ABCMeta, abstractmethod
import globalVars
from utils import urlUtils
from typing import Optional


Expand Down Expand Up @@ -304,6 +305,14 @@ class BrowseModeTreeInterceptor(treeInterceptorHandler.TreeInterceptor):
scriptCategory = inputCore.SCRCAT_BROWSEMODE
_disableAutoPassThrough = False
APPLICATION_ROLES = (controlTypes.Role.APPLICATION, controlTypes.Role.DIALOG)
documentUrl: str | None = None
"""The URL of the current browse mode document.
C{None} when there is no URL or it is unknown.
"""

def isInternalLinkInDocument(self, url: str) -> bool:
"""Returns whether the given url is a same page link within the current browse mode document."""
return urlUtils.isSamePageUrl(url, self.documentUrl)

def _get_currentNVDAObject(self):
raise NotImplementedError
Expand Down
1 change: 1 addition & 0 deletions source/config/configSpec.py
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,7 @@
# 0: Off, 1: style, 2: color and style
reportCellBorders = integer(0, 2, default=0)
reportLinks = boolean(default=true)
reportLinkType = featureFlag(optionsEnum="BoolFlag", behaviorOfDefault="enabled")
nvdaes marked this conversation as resolved.
Show resolved Hide resolved
reportGraphics = boolean(default=True)
reportComments = boolean(default=true)
reportBookmarks = boolean(default=true)
Expand Down
3 changes: 3 additions & 0 deletions source/controlTypes/processAndLabelStates.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ def _processPositiveStates(
positiveStates.discard(State.EDITABLE)
if role != Role.LINK:
positiveStates.discard(State.VISITED)
positiveStates.discard(State.INTERNAL_LINK)
positiveStates.discard(State.SELECTABLE)
positiveStates.discard(State.FOCUSABLE)
positiveStates.discard(State.CHECKABLE)
Expand All @@ -47,6 +48,8 @@ def _processPositiveStates(
# or reporting clickable just isn't useful,
# or the user has explicitly requested no reporting clickable
positiveStates.discard(State.CLICKABLE)
if not config.conf["documentFormatting"]["reportLinkType"]:
positiveStates.discard(State.INTERNAL_LINK)
if reason == OutputReason.QUERY:
return positiveStates
positiveStates.discard(State.DEFUNCT)
Expand Down
4 changes: 4 additions & 0 deletions source/controlTypes/state.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ def negativeDisplayString(self) -> str:
HASPOPUP_GRID = setBit(48)
HASPOPUP_LIST = setBit(49)
HASPOPUP_TREE = setBit(50)
INTERNAL_LINK = setBit(51)


STATES_SORTED = frozenset([State.SORTED, State.SORTED_ASCENDING, State.SORTED_DESCENDING])
Expand Down Expand Up @@ -204,6 +205,9 @@ def negativeDisplayString(self) -> str:
State.HASPOPUP_LIST: _("opens list"),
# Translators: Presented when a control has a pop-up tree.
State.HASPOPUP_TREE: _("opens tree"),
# Translators: Presented when a link destination points to the page containing the link.
# For example, links of a table of contents of a document with different sections.
State.INTERNAL_LINK: _("same page"),
nvdaes marked this conversation as resolved.
Show resolved Hide resolved
}


Expand Down
25 changes: 25 additions & 0 deletions source/globalCommands.py
Original file line number Diff line number Diff line change
Expand Up @@ -902,6 +902,31 @@ def script_toggleReportLinks(self, gesture):
config.conf["documentFormatting"]["reportLinks"] = True
ui.message(state)

@script(
# Translators: Input help mode message for cycle through report link type command.
description=_("Cycles through the report link type states"),
category=SCRCAT_DOCUMENTFORMATTING,
)
def script_cyclesReportLinkType(self, gesture: inputCore.InputGesture) -> None:
"""Set next state of report link type and reports it with ui.message."""
featureFlag: FeatureFlag = config.conf["documentFormatting"]["reportLinkType"]
boolFlag: BoolFlag = featureFlag.enumClassType
values = [x.value for x in boolFlag]
currentValue = featureFlag.value.value
nextValueIndex = (currentValue % len(values)) + 1
nextName: str = boolFlag(nextValueIndex).name
config.conf["documentFormatting"]["reportLinkType"] = nextName
featureFlag = config.conf["documentFormatting"]["reportLinkType"]
if featureFlag.isDefault():
# Translators: Used to announce reporting link type state
# (default behavior).
msg = _("Report link type default (%s)") % featureFlag.behaviorOfDefault.displayString
else:
# Translators: Announces which report link type state is used
# (disabled or enabled).
msg = _("Report link type %s") % BoolFlag[nextName].displayString
ui.message(msg)
seanbudd marked this conversation as resolved.
Show resolved Hide resolved

coderabbitai[bot] marked this conversation as resolved.
Show resolved Hide resolved
@script(
# Translators: Input help mode message for toggle report graphics command.
description=_("Toggles on and off the reporting of graphics"),
Expand Down
18 changes: 18 additions & 0 deletions source/gui/settingsDialogs.py
Original file line number Diff line number Diff line change
Expand Up @@ -2799,8 +2799,22 @@ def makeSettings(self, settingsSizer):
# Translators: This is the label for a checkbox in the
# document formatting settings panel.
self.linksCheckBox = elementsGroup.addItem(wx.CheckBox(elementsGroupBox, label=_("Lin&ks")))
self.linksCheckBox.Bind(wx.EVT_CHECKBOX, self.onLinksChange)
self.linksCheckBox.SetValue(config.conf["documentFormatting"]["reportLinks"])

self.reportLinkTypeCombo: nvdaControls.FeatureFlagCombo = elementsGroup.addLabeledControl(
labelText=_(
# Translators: This is the label for a combo box in the
# document formatting settings panel.
"Report link type",
),
wxCtrlClass=nvdaControls.FeatureFlagCombo,
keyPath=["documentFormatting", "reportLinkType"],
conf=config.conf,
)
if not self.linksCheckBox.GetValue():
self.reportLinkTypeCombo.Disable()

# Translators: This is the label for a checkbox in the
# document formatting settings panel.
self.graphicsCheckBox = elementsGroup.addItem(wx.CheckBox(elementsGroupBox, label=_("&Graphics")))
Expand Down Expand Up @@ -2867,6 +2881,9 @@ def makeSettings(self, settingsSizer):
def _onLineIndentationChange(self, evt: wx.CommandEvent) -> None:
self.ignoreBlankLinesRLICheckbox.Enable(evt.GetSelection() != 0)

def onLinksChange(self, evt: wx.CommandEvent):
nvdaes marked this conversation as resolved.
Show resolved Hide resolved
self.reportLinkTypeCombo.Enable(evt.IsChecked())

def onSave(self):
config.conf["documentFormatting"]["detectFormatAfterCursor"] = (
self.detectFormatAfterCursorCheckBox.IsChecked()
Expand Down Expand Up @@ -2901,6 +2918,7 @@ def onSave(self):
config.conf["documentFormatting"]["reportTableCellCoords"] = self.tableCellCoordsCheckBox.IsChecked()
config.conf["documentFormatting"]["reportCellBorders"] = self.borderComboBox.GetSelection()
config.conf["documentFormatting"]["reportLinks"] = self.linksCheckBox.IsChecked()
self.reportLinkTypeCombo.saveCurrentValueToConf()
config.conf["documentFormatting"]["reportGraphics"] = self.graphicsCheckBox.IsChecked()
config.conf["documentFormatting"]["reportHeadings"] = self.headingsCheckBox.IsChecked()
config.conf["documentFormatting"]["reportLists"] = self.listsCheckBox.IsChecked()
Expand Down
33 changes: 33 additions & 0 deletions source/utils/urlUtils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# A part of NonVisual Desktop Access (NVDA)
# This file is covered by the GNU General Public License.
# See the file COPYING for more details.
# Copyright (C) 2024 NV Access Limited, Noelia Ruiz Martínez, Leonard de Ruijter

from urllib.parse import ParseResult, urlparse


def isSamePageUrl(urlOnPage: str, rootUrl: str) -> bool:
nvdaes marked this conversation as resolved.
Show resolved Hide resolved
"""Returns whether a given URL belongs to the same page as another URL.

:param urlOnPage: The URL that should be on the same page as `rootUrl`
:param rootUrl: The root URL of the page
:return: Whether `urlOnPage` belongs to the same page as `rootUrl`
"""
if not urlOnPage or not rootUrl:
return False

# Parse the URLs
urlOnPageParsed: ParseResult = urlparse(urlOnPage)
rootUrlParsed: ParseResult = urlparse(rootUrl)

if (
urlOnPageParsed.scheme == rootUrlParsed.scheme
and urlOnPageParsed.netloc == rootUrlParsed.netloc
and urlOnPageParsed.path == rootUrlParsed.path
and urlOnPageParsed.query == rootUrlParsed.query
and urlOnPageParsed.fragment
and "/" not in urlOnPageParsed.fragment
):
return True

return False
17 changes: 13 additions & 4 deletions source/virtualBuffers/gecko_ia2.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
# A part of NonVisual Desktop Access (NVDA)
# This file is covered by the GNU General Public License.
# See the file COPYING for more details.
# Copyright (C) 2008-2023 NV Access Limited, Babbage B.V., Mozilla Corporation, Accessolutions, Julien Cochuyt
# Copyright (C) 2008-2024 NV Access Limited, Babbage B.V., Mozilla Corporation, Accessolutions,
# Julien Cochuyt, Noelia Ruiz Martínez, Leonard de Ruijter

from dataclasses import dataclass
from typing import (
Expand Down Expand Up @@ -167,9 +168,14 @@ def _normalizeControlField(self, attrs): # noqa: C901
attrs["roleTextBraille"] = roleTextBraille
if attrs.get("IAccessible2::attribute_dropeffect", "none") != "none":
states.add(controlTypes.State.DROPTARGET)
if role == controlTypes.Role.LINK and controlTypes.State.LINKED not in states:
# This is a named link destination, not a link which can be activated. The user doesn't care about these.
role = controlTypes.Role.TEXTFRAME
if role == controlTypes.Role.LINK:
if controlTypes.State.LINKED not in states:
# This is a named link destination, not a link which can be activated. The user doesn't care about these.
role = controlTypes.Role.TEXTFRAME
elif (value := attrs.get("IAccessible::value")) is not None and self.obj.isInternalLinkInDocument(
value,
):
states.add(controlTypes.State.INTERNAL_LINK)
level = attrs.get("IAccessible2::attribute_level", "")
xmlRoles = attrs.get("IAccessible2::attribute_xml-roles", "").split(" ")
landmark = next((xr for xr in xmlRoles if xr in aria.landmarkRoles), None)
Expand Down Expand Up @@ -322,6 +328,9 @@ def _get_isAlive(self):
isDefunct = True
return not isDefunct

def _get_documentUrl(self) -> str:
return self.documentConstantIdentifier

def getNVDAObjectFromIdentifier(
self,
docHandle: int,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,5 @@ schemaVersion = 2
highlightFocus = True
highlightNavigator = True
highlightBrowseMode = True
[documentFormatting]
reportLinkType = DISABLED
45 changes: 45 additions & 0 deletions tests/unit/test_util/test_urlUtils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
# A part of NonVisual Desktop Access (NVDA)
# This file is covered by the GNU General Public License.
# See the file COPYING for more details.
# Copyright (C) 2024 NV Access Limited, Noelia Ruiz Martínez, Leonard de Ruijter

"""Unit tests for the urlUtils submodule."""

import unittest
from utils.urlUtils import isSamePageUrl


class TestIsSamePageUrl(unittest.TestCase):
def test_samePage_basic(self):
self.assertTrue(isSamePageUrl("http://example.com/page#section", "http://example.com/page"))

def test_samePage_bothHaveFragments(self):
self.assertTrue(isSamePageUrl("http://example.com/page#section", "http://example.com/page#main"))

def test_differentPage(self):
self.assertFalse(isSamePageUrl("http://example.com/otherpage#section", "http://example.com/page"))

def test_noFragment(self):
"""URLs without fragments are not considered the same page."""
seanbudd marked this conversation as resolved.
Show resolved Hide resolved
self.assertFalse(isSamePageUrl("http://example.com/page", "http://example.com/page"))
Copy link
Member

Choose a reason for hiding this comment

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

why not? this seems like unexpected behaviour?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that this test is valid, since the target and root url are equal and then the page maybe refreshed, but the link target is not an anchor for the same page.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps the function should be renamed then isLinkToSamePageAnchor or similar?

Copy link
Collaborator

@XLTechie XLTechie Sep 3, 2024

Choose a reason for hiding this comment

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

I really don't follow the logic on this one.
If I am on the page:

https://example.com/contact

And there is a standard menu on that page with a link called "Contact Us", linking to:

https://example.com/contact

I would expect that to say "Contact Us Link in-page link", or whatever the label for a same page link is.
This seems like the very definition of a same page link.

But this test is expected to fail, meaning that what I would anticipate is wrong.
Can you explain this more?

Copy link
Member

Choose a reason for hiding this comment

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

I agree.
Either the concept is just for anchors, and we communicate it that way, or the concept is for same page links, which includes links to the exact same page.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The concept is for same page links


def test_differentDomain(self):
self.assertFalse(isSamePageUrl("http://other.com/page#section", "http://example.com/page"))

def test_empty_urlOnPage(self):
self.assertFalse(isSamePageUrl("", "http://example.com/page"))

def test_empty_pageUrl(self):
self.assertFalse(isSamePageUrl("http://example.com/page#section", ""))

def test_differentScheme(self):
self.assertFalse(isSamePageUrl("http://example.com/page#section", "https://example.com/page"))
Copy link
Member

Choose a reason for hiding this comment

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

this should maybe be True? shouldn't these both return to the same page? someone may have opened a website directly to http, but the internal links redirect to https

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree with @seanbudd about this question. Let's ask to @coderabbitai

Copy link
Contributor

Choose a reason for hiding this comment

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

@nvdaes, the current implementation of the isSamePageUrl function compares URLs without their fragments, but it includes the scheme (e.g., http vs. https) in the comparison. This means that URLs with different schemes are not considered the same page, which is why the test_differentScheme test case currently asserts False.

If the desired behavior is to treat http and https URLs as the same page, the function would need to be modified to ignore the scheme when comparing URLs. This change would align the function's behavior with the suggestion to return True for URLs with different schemes.

Would you like to proceed with modifying the function to ignore the scheme, or should we keep the current behavior?

Analysis chain

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for the implementation of the `isSamePageUrl` function to understand how it handles different schemes.
ast-grep --lang python --pattern 'def isSamePageUrl($_, $_) -> bool' --context 10

Length of output: 428


Script:

#!/bin/bash
# Use ripgrep to search for the implementation of the `isSamePageUrl` function in the codebase.
rg 'def isSamePageUrl' -A 10

Length of output: 715


Script:

#!/bin/bash
# Use ripgrep to retrieve the full implementation of the `isSamePageUrl` function by extending the context lines.
rg 'def isSamePageUrl' -A 20

Length of output: 1483

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I want to modify this, but just when URLs start with http, not ftp, mailto etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!


def test_differentQuery(self):
self.assertFalse(
isSamePageUrl("http://example.com/page?q=3#section", "http://example.com/page?q4#section"),
)

def test_fragmentHasPath(self):
"""URLs whose fragments contain paths are not considered the same page."""
self.assertFalse(isSamePageUrl("http://example.com/page#fragment/path", "http://example.com/page"))
5 changes: 5 additions & 0 deletions user_docs/en/changes.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ The available options are:
* The timeout to perform a multiple keypress is now configurable; this may be especially useful for people with dexterity impairment. (#11929, @CyrilleB79)
* When performing a braille cursor routing action, NVDA can now automatically speak the character at the cursor. (#8072, @LeonarddeR)
* This option is disabled by default. You can enable "Speak character when routing cursor in text" in NVDA's braille settings.
* NVDA can report when a link destination points to the current page (#141, @LeonarddeR, @nvdaes)

### Changes

Expand Down Expand Up @@ -58,6 +59,10 @@ Please refer to [the developer guide](https://www.nvaccess.org/files/nvda/docume
* It is now possible to redirect objects retrieved from on-screen coordinates, by using the `NVDAObject.objectFromPointRedirect` method. (#16788, @Emil-18)
* Running SCons with the parameter `--all-cores` will automatically pick the maximum number of available CPU cores. (#16943, #16868, @LeonarddeR)
* Developer info now includes information on app architecture (such as AMD64) for the navigator object. (#16488, @josephsl)
* In the gecko_ia2 virtual buffer backend, the accValue is exposed for links (#141, @LeonarddeR).
nvdaes marked this conversation as resolved.
Show resolved Hide resolved
* Anew utils/urlUtils/isSamePageUrl function has been added to check links (#141, @LeonarddeR)
* A new INTERNAL_LINK state has been added to controlTypes.states.State (#141, @nvdaes)
* A new isInternalLink property has been added for NVDAObjects.IAccessible.ia2Web.Ia2Web objects (#141, @nvdaes)
nvdaes marked this conversation as resolved.
Show resolved Hide resolved

#### Deprecations

Expand Down
1 change: 1 addition & 0 deletions user_docs/en/userGuide.md
Original file line number Diff line number Diff line change
Expand Up @@ -2933,6 +2933,7 @@ You can configure reporting of:
* Elements
* Headings
* Links
* Link type (destination to same page)
* Graphics
* Lists
* Block quotes
Expand Down