Add touch based web element navigation in browse mode#19414
Add touch based web element navigation in browse mode#19414kefaslungu wants to merge 14 commits intonvaccess:masterfrom
Conversation
Signed-off-by: kefaslungu <jameskefaslungu@gmail.com>
Signed-off-by: kefaslungu <jameskefaslungu@gmail.com>
Signed-off-by: kefaslungu <jameskefaslungu@gmail.com>
|
@kefaslungu - do you intend to finish this? |
SaschaCowley
left a comment
There was a problem hiding this comment.
Welcome @kefaslungu! Thanks for opening your first PR for NVDA!
There are a few things in here that have me concerned, but overall I really like the direction you're going.
source/globalCommands.py
Outdated
| "default", | ||
| "link", | ||
| "button", | ||
| "form field", | ||
| "heading", | ||
| "frame", | ||
| "table", | ||
| "list", | ||
| "graphic", | ||
| "landmark", |
There was a problem hiding this comment.
These all need to be made translateable, since they're used in the UI. For example
# Translators: An element type in browse mode.
"link",
source/globalCommands.py
Outdated
| browseModeCommands = ( | ||
| ( | ||
| browseMode.BrowseModeTreeInterceptor.script_nextLink, | ||
| browseMode.BrowseModeTreeInterceptor.script_previousLink, | ||
| ), | ||
| ( | ||
| browseMode.BrowseModeTreeInterceptor.script_nextButton, | ||
| browseMode.BrowseModeTreeInterceptor.script_previousButton, | ||
| ), | ||
| ( | ||
| browseMode.BrowseModeTreeInterceptor.script_nextFormField, | ||
| browseMode.BrowseModeTreeInterceptor.script_previousFormField, | ||
| ), | ||
| ( | ||
| browseMode.BrowseModeTreeInterceptor.script_nextHeading, | ||
| browseMode.BrowseModeTreeInterceptor.script_previousHeading, | ||
| ), | ||
| ( | ||
| browseMode.BrowseModeTreeInterceptor.script_nextFrame, | ||
| browseMode.BrowseModeTreeInterceptor.script_previousFrame, | ||
| ), | ||
| ( | ||
| browseMode.BrowseModeTreeInterceptor.script_nextTable, | ||
| browseMode.BrowseModeTreeInterceptor.script_previousTable, | ||
| ), | ||
| ( | ||
| browseMode.BrowseModeTreeInterceptor.script_nextList, | ||
| browseMode.BrowseModeTreeInterceptor.script_previousList, | ||
| ), | ||
| ( | ||
| browseMode.BrowseModeTreeInterceptor.script_nextGraphic, | ||
| browseMode.BrowseModeTreeInterceptor.script_previousGraphic, | ||
| ), | ||
| ( | ||
| browseMode.BrowseModeTreeInterceptor.script_nextLandmark, | ||
| browseMode.BrowseModeTreeInterceptor.script_previousLandmark, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
These can be constructed dynamically.
Lines 605 to 607 in 8dbe0fd
source/globalCommands.py
Outdated
| @scriptHandler.script( | ||
| description="Select next web navigation element", | ||
| gesture="ts(Web):flickDown", | ||
| ) | ||
| def script_nextWebElement(self, gesture): | ||
| ti = api.getNavigatorObject().treeInterceptor | ||
| if not isinstance(ti, browseMode.BrowseModeTreeInterceptor): | ||
| return | ||
|
|
||
| self.webBrowseMode = (self.webBrowseMode + 1) % len(self.webElements) | ||
| ui.message(self.webElements[self.webBrowseMode]) | ||
|
|
||
| @scriptHandler.script( | ||
| description="Select previous web navigation element", | ||
| gesture="ts(Web):flickUp", | ||
| ) | ||
| def script_prevWebElement(self, gesture): | ||
| ti = api.getNavigatorObject().treeInterceptor | ||
| if not isinstance(ti, browseMode.BrowseModeTreeInterceptor): | ||
| return | ||
|
|
||
| self.webBrowseMode = (self.webBrowseMode - 1) % len(self.webElements) | ||
| ui.message(self.webElements[self.webBrowseMode]) | ||
|
|
||
| @scriptHandler.script(gesture="ts(Web):flickRight") | ||
| def script_nextSelectedElement(self, gesture): | ||
| ti = api.getNavigatorObject().treeInterceptor | ||
| if not isinstance(ti, browseMode.BrowseModeTreeInterceptor): | ||
| return | ||
|
|
||
| if self.webBrowseMode == 0: | ||
| self.script_navigatorObject_nextInFlow(gesture) | ||
| else: | ||
| nextScript, _ = self.browseModeCommands[self.webBrowseMode - 1] | ||
| nextScript(ti, gesture) | ||
|
|
||
| @scriptHandler.script(gesture="ts(Web):flickLeft") | ||
| def script_prevSelectedElement(self, gesture): | ||
| ti = api.getNavigatorObject().treeInterceptor | ||
| if not isinstance(ti, browseMode.BrowseModeTreeInterceptor): | ||
| return | ||
|
|
||
| if self.webBrowseMode == 0: | ||
| self.script_navigatorObject_previousInFlow(gesture) | ||
| else: | ||
| _, prevScript = self.browseModeCommands[self.webBrowseMode - 1] | ||
| prevScript(ti, gesture) |
There was a problem hiding this comment.
You need to make the descriptions for all of these scripts translatable. Also add the browse mode category to the scripts.
source/touchHandler.py
Outdated
| if browseMode: | ||
| # Entering browse mode | ||
| if webModeName not in availableTouchModes: | ||
| availableTouchModes.append(webModeName) |
There was a problem hiding this comment.
You also need to add this, and a corresponding translatable name, to touchModeLabels . This doesn't need to be done dynamically.
There was a problem hiding this comment.
has this comment from sascha been addressed? https://github.com/nvaccess/nvda/pull/19414/files#r2772194150
|
Hi, I am the original creator of the parts of the code reviewed in this pull request discussion. Kefas has adopted it to NVDA Core expectations and structure. The comment on implementation in the global commands module can be explained by the fact that the code and the idea behind it comes from an add-on (Enhanced Touch Gestures). The add-on used a global plugin class to house list of navigable elements, browse mode script calls, and the gesture to cycle between web elements. Our (Kefas and I) plan is to adopt Enhanced Touch Gestures add-on based on the pull request code structure and make changes based on review comments, with the end goal of removing web mode from the add-on in favor of NVDA Core version. Thanks for reviewing Kefas' pull request. |
… element registration and settings Signed-off-by: kefaslungu <jameskefaslungu@gmail.com>
|
Hi all, I've made some changes as follows:
|
SaschaCowley
left a comment
There was a problem hiding this comment.
Heading in the right direction, but still needs some work. Good progress though :)
Signed-off-by: kefaslungu <jameskefaslungu@gmail.com>
…mode # Conflicts: # source/gui/settingsDialogs.py
|
Hi @SaschaCowley, Thanks for the detailed review! I've addressed all the feedback as follows:
All that is left now is documentation, but I'll do that as soon as the code is approved. Cheers! |
There was a problem hiding this comment.
Pull request overview
This PR implements touch-based navigation for web elements in browse mode, addressing issue #3424 from 2013. The feature introduces a new "web" touch mode that allows touchscreen users to navigate structural elements (links, headings, form fields, etc.) using touch gestures, providing an experience similar to keyboard-based browse mode navigation.
Changes:
- Adds a "web" touch mode that activates automatically when entering browse mode
- Implements four touch gestures for cycling through and navigating between element types (flick up/down to select element type, left/right to navigate)
- Provides GUI settings to configure which element types are available for touch navigation
- Includes comprehensive user documentation explaining the new feature
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| user_docs/en/userGuide.md | Adds documentation for the new web touch mode, including a table of gestures and usage explanations |
| user_docs/en/changes.md | Adds changelog entry describing the new feature |
| source/touchHandler.py | Implements browse mode state change handler to add/remove web mode and switch to it automatically |
| source/gui/settingsDialogs.py | Adds UI controls for selecting which web elements are available in touch navigation |
| source/config/configSpec.py | Adds configuration option for web touch navigation elements with sensible defaults |
| source/browseMode.py | Implements core touch navigation scripts, element cycling logic, and registry system for available elements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: kefaslungu <jameskefaslungu@gmail.com>
|
Hi @seanbudd, All requested changes has been made, please take a look. |
source/browseMode.py
Outdated
| @param availableInWebTouch: If True, register this element type for web touch navigation cycling. | ||
| Set to False to exclude an element type entirely from web touch navigation. | ||
| @param touchLabel: A short, translated, plural label for this element type used in web touch navigation | ||
| cycling (e.g. C{_("links")}). Required when C{availableInWebTouch} is True. |
There was a problem hiding this comment.
Please see the coding standards for more information.
| @param availableInWebTouch: If True, register this element type for web touch navigation cycling. | |
| Set to False to exclude an element type entirely from web touch navigation. | |
| @param touchLabel: A short, translated, plural label for this element type used in web touch navigation | |
| cycling (e.g. C{_("links")}). Required when C{availableInWebTouch} is True. | |
| :param availableInWebTouch: If True, register this element type for web touch navigation cycling. | |
| Set to False to exclude an element type entirely from web touch navigation. | |
| :param touchLabel: A short, translated, plural label for this element type used in web touch navigation | |
| cycling (e.g. ``_("links")``). Required when ``availableInWebTouch`` is True. |
source/browseMode.py
Outdated
|
|
||
| #: The itemType currently selected for web touch navigation. None means "default" (all content). | ||
| #: Stored as an instance attribute so each document remembers its own preference. | ||
| _webBrowseCurrentType: Optional[str] = None |
source/touchHandler.py
Outdated
| if browseMode: | ||
| # Entering browse mode | ||
| if webModeName not in availableTouchModes: | ||
| availableTouchModes.append(webModeName) |
There was a problem hiding this comment.
has this comment from sascha been addressed? https://github.com/nvaccess/nvda/pull/19414/files#r2772194150
source/browseMode.py
Outdated
| "Selects the previous element type for web touch navigation", | ||
| ), | ||
| category=inputCore.SCRCAT_BROWSEMODE, | ||
| gesture="ts(Web):flickUp", |
There was a problem hiding this comment.
| gesture="ts(Web):flickUp", | |
| gesture="ts(web):flickUp", |
Signed-off-by: kefaslungu <jameskefaslungu@gmail.com>
|
Hi @seanbudd, All suggestions have been addressed. |
Link to issue number:
Closes #3424
Summary of the issue:
As noted in the above issue, touch users do not have a dedicated way to perform browse mode style navigation of web content using touch gestures.
Description of user facing changes:
A new Web touch navigation mode has been introduced. When active, touch gestures allow users to navigate common web elements such as links, buttons, headings, form fields, landmarks, and other structural elements in browse mode documents.
This enables touch based navigation that mirrors existing browse mode navigation commands.
When browse mode is exited, the Web touch mode is automatically removed and touch navigation returns to its previous behavior.
Description of developer facing changes:
New touch gesture scripts have been added to invoke existing browse mode navigation commands. These scripts reuse the existing BrowseModeTreeInterceptor logic rather than introducing new navigation implementations.
Supporting logic has been added to track the active browse mode context and route touch gestures to the appropriate browse mode commands when available.
Description of development approach:
This change was implemented by subscribing to browse mode state change notifications and using them as the authoritative signal for enabling and disabling web specific touch behavior. The active browse mode tree interceptor is cached on activation and cleared on deactivation.
Testing strategy:
Manual testing was performed using touch navigation in multiple browse mode documents across supported web browsers. Testing verified:
Known issues with pull request:
None
Code Review Checklist: