feat(mouse-gesture): Add wheel gesture support #2119
Conversation
…mouse-gesture config and controller - Add UI changes in Preferences.tsx
- Insert "wheelGesturesEnabled" entry under mouseGesture in locale JSON files
Summary of ChangesHello @mystster, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new feature that enables mouse wheel gestures for improved navigation within the application. It integrates the necessary configuration, event handling logic, and user interface elements to support this functionality. Initially, the wheel gestures are specifically designed for efficient tab switching, with the potential for further customization based on user feedback. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for wheel gestures, which is a great enhancement for mouse navigation. The implementation is mostly solid, but I've identified a few areas for improvement regarding code maintainability, type consistency, and internationalization. Specifically, there's some duplicated code that could be refactored, a type inconsistency between the feature's configuration and the settings page, and several missing or partial translations for the new UI text. Addressing these points will improve the overall quality and robustness of the feature.
| checked={config.wheelGesturesEnabled ?? true} | ||
| onChange={() => updateConfig({ wheelGesturesEnabled: !(config.wheelGesturesEnabled ?? true) })} |
There was a problem hiding this comment.
The use of the nullish coalescing operator (?? true) suggests that config.wheelGesturesEnabled might be optional. However, this property is defined as required in browser-features/chrome/common/mouse-gesture/config.ts. This inconsistency indicates that the MouseGestureConfig type in browser-features/pages-settings/src/types/pref.ts is out of sync with the main configuration. To ensure type safety and consistency, the type definition in pref.ts should be updated to include wheelGesturesEnabled as a required boolean property. After that, the ?? true will no longer be necessary.
| checked={config.wheelGesturesEnabled ?? true} | |
| onChange={() => updateConfig({ wheelGesturesEnabled: !(config.wheelGesturesEnabled ?? true) })} | |
| checked={config.wheelGesturesEnabled} | |
| onChange={() => updateConfig({ wheelGesturesEnabled: !config.wheelGesturesEnabled })} |
| let action: string | null = null; | ||
| if (event.deltaY < 0) { | ||
| action = "gecko-show-previous-tab"; | ||
| } else if (event.deltaY > 0) { | ||
| action = "gecko-show-next-tab"; | ||
| } |
There was a problem hiding this comment.
The action strings gecko-show-previous-tab and gecko-show-next-tab are hardcoded. It's a good practice to define them as constants to improve readability and make them easier to manage, especially if they might be used elsewhere or become configurable in the future.
const WHEEL_UP_ACTION = "gecko-show-previous-tab";
const WHEEL_DOWN_ACTION = "gecko-show-next-tab";
let action: string | null = null;
if (event.deltaY < 0) {
action = WHEEL_UP_ACTION;
} else if (event.deltaY > 0) {
action = WHEEL_DOWN_ACTION;
}| if (this.preventionTimeoutId) { | ||
| clearTimeout(this.preventionTimeoutId); | ||
| } | ||
| this.preventionTimeoutId = setTimeout(() => { | ||
| this.isContextMenuPrevented = false; | ||
| this.preventionTimeoutId = null; | ||
| }, getConfig().contextMenu.preventionTimeout); |
There was a problem hiding this comment.
This block of code for managing the context menu prevention timeout is duplicated in other parts of this class (e.g., in handleMouseUp). To improve code maintainability and reduce redundancy, consider extracting this logic into a private helper method. This will make the code cleaner and less error-prone when future changes are needed.
| "actionDescription": "Description of the gesture", | ||
| "rockerGesturesEnabled": "Enable rocker gestures", | ||
| "rockerGesturesDescription": "Right-click and move left to go back, right-click and move right to go forward. (Experimental)", | ||
| "wheelGesturesEnabled": "Enable wheel gestures", |
There was a problem hiding this comment.
The value for wheelGesturesEnabled is not translated into Arabic. Please provide the correct translation. This seems to be the case for several other languages as well.
| "actionDescription": "Hareketin açıklaması", | ||
| "rockerGesturesEnabled": "Rocker hareketlerini etkinleştirin", | ||
| "rockerGesturesDescription": "Geri gitmek için sağ tıklayın ve sola hareket edin, ileri gitmek için sağ tıklayın ve sağa hareket edin. (Deneysel)", | ||
| "wheelGesturesEnabled": "Wheel hareketlerini etkinleştirin", |
There was a problem hiding this comment.
The translation for wheelGesturesEnabled seems to be partial. The word 'Wheel' is in English. A better translation might be 'Tekerlek hareketlerini etkinleştirin'.
1 participant
This branch introduces support for wheel (scroll) gestures to improve mouse-based navigation.
High-level changes:
Currently, the wheel gesture is limited to tab switching only. Please let us know if it is a requirement to allow users to customize the wheel gesture action for this PR to be approved.