Gesturefy v4 (Port to manifest v3) #715
Conversation
Firefox will provide global user controls for site access per add-on. See: - https://bugzilla.mozilla.org/show_bug.cgi?id=1497075 - https://bugzilla.mozilla.org/show_bug.cgi?id=1711787 Removing this option from Gesturefy reduces the amount of code that needs to be maintained. It's also more performant to exclude the entire content script from running than doing content script side checks. Last but not least exclusions have always been a power feature that never really fit into Gesturefy from a design/UI standpoint. Also resolves #542
The reason behind this change is that content scripts no longer get access to extension files unless they are declared as a web_accessible_resource. I'm against doing so for privacy reasons. At least in theory websites could use web_accessible_resource for fingerprinting. Some additional information can be found here: https://bugzilla.mozilla.org/show_bug.cgi?id=1578405
Previously all unmatching gestures were hidden including the "new gesture button". This could lead to confusion that gestures are missing/disappeared and no new gestures can be added. The new concept only unhighlights unmatching gestures which still provides an overview over all gestures, indicates that some sort of filtering is active and allows adding new gestures. The only downside is that users have to scan the screen (on smaller screens even need to scroll) in order to find the results.
Historical moment. I hope you're all proud of me, that these actions finally leave the "about" page.
Also add the ability to backup/restore the config to/from the cloud
It now works with urls instead of tabs and globs instead of regex The latter ensures backwards compatibility and allows a potential change to the dynamic content script API in the future
So far there is now way to do this via browser functionality, or dynamic content script registration. See also w3c/webextensions#653
- update related models - remove MultiPurposeCommand
WalkthroughThis PR migrates the extension to Manifest V3, replaces PNG icons with a single SVG, and reorganizes core internals into services and model classes. ConfigManager now accepts an options object and uses DefaultConfig; new services ExclusionService and HostPermissionService (event APIs) were added. The command system was refactored into Command, CommandStack and many command classes; the legacy commands module and several legacy UI components were removed and replaced by new web components (command-picker, command-card, gesture-card, pattern-preview, orderable-collection, collapsible-item, locale-text/locale-option, command-stacker, pattern-recorder). Options, popup, theming, and data/backup flows were reworked to use the new services and components. Sequence Diagram(s)sequenceDiagram
participant User
participant Popup
participant HostPermissionService
participant ExclusionService
participant Config
participant BrowserAPI as Browser
User->>Popup: open popup
Popup->>Config: await Config.loaded
Popup->>ExclusionService: await Exclusions.loaded
Popup->>HostPermissionService: check global permission
HostPermissionService->>BrowserAPI: browser.permissions.contains()
BrowserAPI-->>HostPermissionService: permission status
HostPermissionService->>BrowserAPI: getActiveTab()
BrowserAPI-->>HostPermissionService: active tab
Popup->>ExclusionService: isEnabledFor(activeTab.url)
ExclusionService-->>Popup: enabled/disabled
Popup->>Popup: render UI (permission & exclusion state)
alt user toggles domain activation
User->>Popup: click toggle
Popup->>ExclusionService: enableFor/disableFor(activeTab.url)
ExclusionService->>BrowserAPI: storage.local.set/update
ExclusionService-->>Popup: emit "change"
Popup->>Popup: update UI
end
alt user requests global permission
User->>Popup: click request
Popup->>HostPermissionService: requestGlobalPermission()
HostPermissionService->>BrowserAPI: browser.permissions.request()
BrowserAPI-->>HostPermissionService: grant/deny
HostPermissionService-->>Popup: result
Popup->>Popup: refresh UI
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/bundle/content.bundle.js (1)
546-547:hasEventListenerin ConfigManager also missing return statement.Same issue as BaseEventListener - the result of
.has()is not returned.hasEventListener (event, callback) { if (!this._events.hasOwnProperty(event)) { throw "The first argument is not a valid event."; } if (typeof callback !== "function") { throw "The second argument must be a function."; } - this._events[event].has(callback); + return this._events[event].has(callback); }
🟠 Major comments (12)
src/views/shared/components/locale-text.mjs-11-30 (1)
11-30: Fallback text mechanism won't work as documented.The docstring states that light DOM children serve as fallback text, but the shadow DOM has no
<slot>element. Whenbrowser.i18n.getMessage()returns an empty string, the light DOM children won't be visible because they're not projected into the shadow DOM.Either add a slot for fallback content or adjust the documentation:
constructor() { super(); this.attachShadow({ mode: 'open' }); + // Add slot for fallback content + this.shadowRoot.innerHTML = '<slot></slot>'; } // ... attributeChangedCallback(name, oldValue, newValue) { if (name === 'key') { const string = browser.i18n.getMessage(newValue); if (string) { - this.shadowRoot.textContent = string; + // Replace slot with localized text + this.shadowRoot.innerHTML = string; } + else { + // Restore slot for fallback content + this.shadowRoot.innerHTML = '<slot></slot>'; + } } }Alternatively, if fallback isn't needed, update the docstring to remove that claim.
Committable suggestion skipped: line range outside the PR's diff.
src/views/options/components/command-card/layout.css-1-6 (1)
1-6: Globaloutline: nonereset removes focus indicators.Removing outlines globally can harm keyboard accessibility. Users relying on keyboard navigation lose visual feedback for focused elements.
Consider removing
outline: nonefrom the global reset and applying custom focus styles where needed:* { margin: 0; padding: 0; - outline: none; border: none; }Then add explicit focus styles for interactive elements:
:focus-visible { outline: 2px solid var(--highlighted-color); outline-offset: 2px; }src/views/options/components/command-card/layout.css-232-243 (1)
232-243: Replacetext-overflow: ellipsis ellipsis;withtext-overflow: ellipsis;The two-value
text-overflowsyntax is only supported in Firefox and is not implemented in Chrome, Edge, Safari, Opera, or mobile browsers. Use the single-value form for cross-browser compatibility.src/views/options/components/command-picker/command-picker.mjs-22-26 (1)
22-26: Memory leak: window resize listener is never removed.The resize event listener is added in the constructor but there's no corresponding removal in
disconnectedCallback. This can cause memory leaks when the element is removed from the DOM.constructor() { super(); this.attachShadow({ mode: 'open' }); - window.addEventListener('resize', this.#handleWindowSizeChange.bind(this)); + this.#boundHandleWindowSizeChange = this.#handleWindowSizeChange.bind(this); }Add the bound handler as a private field and manage the listener lifecycle:
+#boundHandleWindowSizeChange; +connectedCallback() { + window.addEventListener('resize', this.#boundHandleWindowSizeChange); // ... rest of connectedCallback +} disconnectedCallback() { + window.removeEventListener('resize', this.#boundHandleWindowSizeChange); this.shadowRoot.replaceChildren(); }Committable suggestion skipped: line range outside the PR's diff.
src/core/models/command-stack.mjs-16-25 (1)
16-25: Missing validation for unknown command names.If
item.namedoesn't match any exported class in the Commands module,Commands[item.name]will beundefined, causing a crypticTypeError: Commands[item.name] is not a constructorat runtime. Consider adding validation with a clearer error message.static fromJSON(json) { if (!Array.isArray(json)) throw 'Expects an array of JSON objects.'; return new CommandStack( json.map((item) => { - // note: a dedicated Command.fromJSON(item); function would easily make sense on the basic Command class, - // but this would create a cyclic import dependency between 'command.mjs' and 'commands.mjs' - return new Commands[item.name](item.settings) + const CommandClass = Commands[item.name]; + if (!CommandClass) { + throw new Error(`Unknown command type: "${item.name}"`); + } + return new CommandClass(item.settings); }) ); }src/views/options/components/command-card/command-card.mjs-67-84 (1)
67-84: UseclassNameinstead ofclassListfor string assignment.Same issue as in
gesture-card.mjs-classListis aDOMTokenListand cannot be assigned a string directly.#createHeader() { return Build('div', { - classList: 'command-header', + className: 'command-header', }, Build('div', { - classList: 'command-header-main', + className: 'command-header-main', }, Build('span', { textContent: this.#command.label, }), Build('button', { - classList: 'command-remove-button', + className: 'command-remove-button', onclick: this.#handleRemoveButtonClick.bind(this), }), ), Build('div', { - classList: 'command-header-secondary', + className: 'command-header-secondary', }), ); }src/views/options/components/command-card/command-card.mjs-94-103 (1)
94-103: SameclassListissue in the body container.const bodyContainer = Build('div', { - classList: 'command-body', + className: 'command-body', }, // build and insert the corresponding setting templates ...filteredTemplates.values().map(template => Build('div', { - classList: 'command-setting', + className: 'command-setting', }, document.importNode(template.content, true), )) );src/views/options/components/gesture-card/gesture-card.mjs-40-51 (1)
40-51: UseclassNameinstead ofclassListfor string assignment.The
classListproperty is aDOMTokenListobject and cannot be directly assigned a string. UseclassNameinstead to set class names as a string, or use the Build helper differently if it handles this conversion.this.#patternElement = Build('pattern-preview', { - classList: 'thumbnail', + className: 'thumbnail', pattern: this.#gesture.pattern, }), this.#commandElement = Build('div', { - classList: 'command', + className: 'command', textContent: this.#gesture.toString(), }), Build('button', { - classList: 'remove-button', + className: 'remove-button', onclick: this.#handleRemoveButtonClick.bind(this), }),src/core/services/base-event-listener.mjs-29-33 (1)
29-33:hasEventListenerdoesn’t return the result, alwaysundefinedThe docstring says “Checks if an event listener exists”, but
hasEventListenercalls.has(...)and ignores the return value, so callers will always getundefined:hasEventListener (event, callback) { this._validateEventParameter(event); this._validateCallbackParameter(callback); this._events.get(event).has(callback); // result unused }This should return the boolean:
hasEventListener (event, callback) { this._validateEventParameter(event); this._validateCallbackParameter(callback); - this._events.get(event).has(callback); + return this._events.get(event).has(callback); }You’ll probably want to mirror this fix in any older event‑manager implementations for consistency.
src/views/options/components/pattern-preview/pattern-preview.mjs-13-27 (1)
13-27: Guard against empty/degenerate patterns to avoid runtime errors and NaN geometryTwo edge cases can currently break the component:
- Undefined / non‑array pattern
#createSVGassumespattern.forEach(...), so if the element is constructed without a pattern (e.g. via markup<pattern-preview>) orpatternis not an array, you’ll get aTypeError.
- Zero‑area path (all points identical)
If all deltas sum to a single point, or there’s effectively no movement,
widthandheightin#fitPathToViewBoxbecome0, so:const scale = Math.min(viewBoxWidth / width, viewBoxHeight / height); // division by 0 // scale → Infinity, offsets become NaN, final coordinates become NaN/InfinityThat can make the path invalid and cause
getTotalLength()or rendering to fail.Suggested fix (minimal and defensive):
#createSVG(pattern) { @@ - // convert vector array to points starting by 0, 0 - const points = [ {x: 0, y: 0} ]; - pattern.forEach((vector, i) => points.push({ + if (!Array.isArray(pattern) || pattern.length === 0) { + // Return an empty SVG to avoid runtime errors for invalid/empty patterns + return document.createElementNS('http://www.w3.org/2000/svg', 'svg'); + } + + // convert vector array to points starting by 0, 0 + const points = [ { x: 0, y: 0 } ]; + pattern.forEach((vector, i) => points.push({ x: points[i].x + vector[0], y: points[i].y + vector[1] })); @@ - #fitPathToViewBox(path, viewBoxX, viewBoxY, viewBoxWidth, viewBoxHeight) { + #fitPathToViewBox(path, viewBoxX, viewBoxY, viewBoxWidth, viewBoxHeight) { @@ - const width = maxX - minX; - const height = maxY - minY; + const width = maxX - minX; + const height = maxY - minY; + + // If there is no movement, center the single point and bail out + if (width === 0 && height === 0) { + const cx = viewBoxX + viewBoxWidth / 2; + const cy = viewBoxY + viewBoxHeight / 2; + for (const point of path) { + point.x = cx; + point.y = cy; + } + return path; + } @@ - const scale = Math.min( + const scale = Math.min( viewBoxWidth / width, viewBoxHeight / height, );This keeps normal behavior intact while making the component resilient to edge cases.
Also applies to: 62-75, 174-201
src/views/shared/theme.css-82-95 (1)
82-95: Removingoutline: nonemay create accessibility issues.Setting
outline: noneon all elements removes the default focus indicator. While you have custom focus styles for links (lines 112-115), other focusable elements like buttons and inputs may lose visible focus indication, which is problematic for keyboard users.Consider adding explicit focus-visible styles for interactive elements:
* { margin: 0; padding: 0; - outline: none; border: none; } + +*:focus-visible { + outline: 2px solid var(--highlighted-color); + outline-offset: 2px; +}src/core/models/commands.mjs-1106-1114 (1)
1106-1114: Bug: Incorrect clamping logic for non-cycling left tab movement.When
cyclingis false and the tab is near the start,nextTabQueryIndexcan become negative. UsingMath.minhere doesn't prevent negative indices. This will causetabs[nextTabQueryIndex]to beundefined, leading to a runtime error when accessing.index.const shift = -(Number(this.settings["shift"]) || 1); let nextTabQueryIndex = currentTabQueryIndex + shift; if (this.settings["cycling"]) { // wrap index nextTabQueryIndex = ((nextTabQueryIndex % tabs.length) + tabs.length) % tabs.length; } else { - nextTabQueryIndex = Math.min(nextTabQueryIndex, tabs.length - 1); + nextTabQueryIndex = Math.max(0, Math.min(nextTabQueryIndex, tabs.length - 1)); }
🟡 Minor comments (15)
README.md-7-7 (1)
7-7: Add alt text to the decorative icon image.The embedded
<img>tag is missing analtattribute, which is required for accessibility and semantic HTML compliance.Apply this diff to add appropriate alt text:
-# <sub><img src="https://github.com/Robbendebiene/Gesturefy/blob/master/src/resources/img/icon.svg" height="38" width="38"></sub>esturefy +# <sub><img src="https://github.com/Robbendebiene/Gesturefy/blob/master/src/resources/img/icon.svg" height="38" width="38" alt="Gesturefy logo"></sub>esturefysrc/views/options/components/collapsible-item/collapsible-item.mjs-155-160 (1)
155-160: Remove the return statement from the setter.Setters in JavaScript should not return values; the return value is ignored. This is flagged by static analysis.
set group(value) { - return this.setAttribute('group', value); + this.setAttribute('group', value); }src/views/options/components/collapsible-item/collapsible-item.mjs-74-93 (1)
74-93: Wrap switch case declarations in blocks to prevent hoisting issues.The
constdeclarations inside the switch cases can be accessed from other cases due to JavaScript's switch statement scoping rules. This is flagged by static analysis.Apply this diff to fix the scoping:
attributeChangedCallback(name, oldValue, newValue) { // will also be called if any attribute is initially set switch (name) { - case 'collapsed': - const oldCollapsed = oldValue !== null; - const newCollapsed = newValue !== null; - if (oldCollapsed !== newCollapsed) { - this.#handleGroupCollapse(); - this.#handleCollapse(); - this.dispatchEvent(new CustomEvent('collapse', { - detail: newCollapsed, - })); - } + case 'collapsed': { + const oldCollapsed = oldValue !== null; + const newCollapsed = newValue !== null; + if (oldCollapsed !== newCollapsed) { + this.#handleGroupCollapse(); + this.#handleCollapse(); + this.dispatchEvent(new CustomEvent('collapse', { + detail: newCollapsed, + })); + } break; - case 'group': - // on group change remove from old group - if (CollapsibleItem.#groups.get(oldValue) === this) { - CollapsibleItem.#groups.delete(oldValue); } - this.#handleGroupCollapse(); + case 'group': { + // on group change remove from old group + if (CollapsibleItem.#groups.get(oldValue) === this) { + CollapsibleItem.#groups.delete(oldValue); + } + this.#handleGroupCollapse(); break; + } } }src/views/options/components/command-picker/command-picker.mjs-20-20 (1)
20-20: Missing semicolon.The
#filterInputdeclaration is missing a semicolon.- #filterInput + #filterInput;src/views/options/fragments/settings.inc-183-184 (1)
183-184: Invalid input type attribute.
type="input"is not a valid HTML input type. This should betype="text"for a text input field with pattern validation.- <input data-config="Settings.Gesture.Command.Style.fontSize" class="input-field" type="input" required pattern="^(0|[1-9][0-9]*)(\.[0-9]+)?(px|em|rem|ex|%|in|cm|mm|pt|pc|vh|vw|vmin|vmax)$"> + <input data-config="Settings.Gesture.Command.Style.fontSize" class="input-field" type="text" required pattern="^(0|[1-9][0-9]*)(\.[0-9]+)?(px|em|rem|ex|%|in|cm|mm|pt|pc|vh|vw|vmin|vmax)$">src/views/installation/index.html-6-6 (1)
6-6: Incorrect MIME type for SVG favicon.The
typeattribute specifiesimage/pngbut thehrefpoints to an SVG file. This mismatch may cause browsers to handle the icon incorrectly.- <link rel="icon" type="image/png" href="/resources/img/icon.svg" sizes="16x16"> + <link rel="icon" type="image/svg+xml" href="/resources/img/icon.svg">src/core/services/host-permission-service.mjs-17-21 (1)
17-21: Potential TypeError iforiginsis undefined.The optional chaining on
permissionsdoesn't protect againstoriginsbeing undefined or null. Ifpermissionsexists butoriginsdoesn't, accessing.lengthwill throw._permissionChangeHandler(permissions) { - if (permissions?.origins.length > 0) { + if (permissions?.origins?.length > 0) { this._events.get('change').forEach((callback) => callback(permissions.origins)); } }src/views/popup/main.mjs-118-119 (1)
118-119: Minor: Extra space in return statement.There's an extra space before
url.protocol.if (url.protocol === 'about:') { - return url.protocol + url.pathname; + return url.protocol + url.pathname; }src/views/options/data.mjs-41-52 (1)
41-52: Memory leak: Object URL is not revoked.The created Object URL should be revoked after use to prevent memory leaks.
function onFileBackupButton () { const manifest = browser.runtime.getManifest(); const linkElement = document.createElement("a"); linkElement.download = `${manifest.name} ${manifest.version} ${ new Date().toDateString() }.json`; // creates a json file with the current config - linkElement.href = URL.createObjectURL( + const blobUrl = URL.createObjectURL( new Blob([JSON.stringify(Config.get(), null, ' ')], {type: 'application/json'}) ); + linkElement.href = blobUrl; document.body.appendChild(linkElement); linkElement.click(); document.body.removeChild(linkElement); + URL.revokeObjectURL(blobUrl); }src/core/background.mjs-170-173 (1)
170-173: Potential race condition:handleBrowserActionIconcalled beforeExclusionsis loaded.Line 173 calls
handleBrowserActionIcon()immediately, butExclusions.loadedonly triggers the handler after resolution. If the initial call happens beforeExclusions._exclusionsis populated,isEnabledForwill always returntrue(empty array means no exclusions match).This may cause a brief incorrect icon state on startup. Consider awaiting both
Config.loadedandExclusions.loadedbefore the initial call.-Exclusions.loaded.then(handleBrowserActionIcon); Exclusions.addEventListener("change", handleBrowserActionIcon); -// on initial run -handleBrowserActionIcon(); +// on initial run, wait for both services to be loaded +Promise.all([Config.loaded, Exclusions.loaded]).then(handleBrowserActionIcon);src/views/options/components/command-picker/layout.css-97-108 (1)
97-108: Duplicatemin-widthon#filterInput(lint warning)
#filterInputdeclaresmin-widthtwice:#filterWrapper { … #filterInput { min-width: 0; flex: 1; min-width: 3rem; … } }The second declaration overrides the first, and Biome flags this as a suspicious duplicate.
If you don’t need the
0rule, simplify to the intended value:- #filterInput { - min-width: 0; - flex: 1; - min-width: 3rem; + #filterInput { + flex: 1; + min-width: 3rem;If both values serve a purpose (e.g., different fallbacks), please document that and consider restructuring to avoid linter complaints.
src/views/shared/theme.css-69-71 (1)
69-71: The!importantwith universal transition may cause unexpected behavior.Applying
transition: all .3s !importantto all descendants during theme transitions could interfere with other animations (like the orderable-collection reordering) and cause performance issues on complex pages.Consider being more selective about which properties transition:
.theme-transition:root * { - transition: all .3s !important; + transition: background-color .3s, color .3s, border-color .3s !important; }src/core/models/commands.mjs-158-159 (1)
158-159: Minor: Typo "tap ids" should be "tab ids".This typo appears in comments at lines 158, 182, and 203.
- // create array of tap ids + // create array of tab idssrc/core/models/commands.mjs-718-719 (1)
718-719: Fix copy-paste error in comment.The comment says "scrolled upwards" but this is the
ScrollPageDowncommand, so it should say "scrolled downwards".- // returns true if there exists a scrollable element in the injected frame - // which can be scrolled upwards else false + // returns true if there exists a scrollable element in the injected frame + // which can be scrolled downwards else falsesrc/core/models/commands.mjs-1272-1274 (1)
1272-1274: Handle invalid regex gracefully.User-provided regex patterns are passed directly to
RegExp()without validation. An invalid pattern will throw aSyntaxError, causing the command to fail. Consider wrapping in try-catch with user feedback.if (this.settings["regex"]) { - matchNumber = RegExp(this.settings["regex"]); + try { + matchNumber = new RegExp(this.settings["regex"]); + } catch (e) { + // Invalid regex - fall back to default pattern + console.warn("Invalid custom regex pattern:", e.message); + // Could also display a notification to the user + } } - else { + if (!matchNumber) {Apply the same pattern to
DecreaseURLNumberat line 1318-1319.Committable suggestion skipped: line range outside the PR's diff.
🧹 Nitpick comments (29)
src/views/options/components/popup-box/main.mjs (1)
3-3: Fix typographical errors in comments.There are a few typos in the documentation comments:
- Line 3: "sepcial" → "special"
- Line 15: "Construcor" → "Constructor"
- Line 295: "cofirm" → "confirm"
- * Accepts two sepcial attributes (and properties): + * Accepts two special attributes (and properties):- /** - * Construcor - * Create shadow root and load stylesheet by appending it to the shadow DOM - **/ + /** + * Constructor + * Create shadow root and load stylesheet by appending it to the shadow DOM + **/- /** - * Listenes for the enter key press and runs the cofirm button function - **/ + /** + * Listenes for the enter key press and runs the confirm button function + **/Also applies to: 15-15, 295-295
src/views/options/components/collapsible-item/collapsible-item.mjs (1)
130-134: Consider resetting height to 'auto' after transition completes.The
#unfoldmethod sets an explicit pixel height but never resets it toauto. If slotted content changes dynamically while expanded, the container won't resize. You could listen for thetransitionendevent to reset height.#unfold() { // use https://developer.mozilla.org/en-US/docs/Web/CSS/interpolate-size in the future this.#body.style.setProperty('height', this.#body.scrollHeight + 'px'); this.#body.style.setProperty('visibility', 'visible'); + this.#body.addEventListener('transitionend', () => { + if (!this.collapsed) { + this.#body.style.setProperty('height', 'auto'); + } + }, { once: true }); }src/views/popup/layout.css (2)
35-46: External favicon URLs may be blocked or change.These external URLs for favicons (mozilla.org, githubassets.com) could fail due to network issues, CSP restrictions, or URL changes. Consider bundling local fallback icons or using a generic fallback.
Verify the extension's CSP allows these external image requests and confirm the URLs remain stable.
52-55: Reset all borders onhrto avoid browser default styles leaking through.Setting only
border-bottomleaves the default top/left/right borders potentially visible depending on browser defaults.hr { + border: none; border-bottom: solid 1px var(--border-color); margin: 0.5em 0em; }src/views/popup/index.html (2)
33-40: Addrel="noopener"to external links for security.External links with
target="_blank"should includerel="noopener"to prevent the opened page from accessingwindow.opener. Also, "Github" should be "GitHub" for brand consistency.- <a href="https://addons.mozilla.org/firefox/addon/gesturefy" target="_blank" class="item"> + <a href="https://addons.mozilla.org/firefox/addon/gesturefy" target="_blank" rel="noopener" class="item"> <span class="favicon favicon-amo"></span> <span>Mozilla Add-ons</span> </a> - <a href="https://github.com/Robbendebiene/Gesturefy" target="_blank" class="item"> + <a href="https://github.com/Robbendebiene/Gesturefy" target="_blank" rel="noopener" class="item"> <span class="favicon favicon-github"></span> - <span>Github</span> + <span>GitHub</span> </a>
11-14: Addrel="noopener"to external link.Same security consideration applies here.
- <a hidden id="restrictedPageWarning" href="https://github.com/Robbendebiene/Gesturefy/wiki/Troubleshooting#gesturfy-is-not-working-at-all" class="item warning"> + <a hidden id="restrictedPageWarning" href="https://github.com/Robbendebiene/Gesturefy/wiki/Troubleshooting#gesturfy-is-not-working-at-all" rel="noopener" class="item warning">src/views/options/fragments/about.inc (2)
23-24: Addrel="noopener"to external links.External links with
target="_blank"should includerel="noopener"for security.- <a target="_blank" href="https://github.com/Robbendebiene/Gesturefy/graphs/contributors">Github</a>, - <a target="_blank" href="https://crowdin.com/project/gesturefy">Crowdin</a> + <a target="_blank" rel="noopener" href="https://github.com/Robbendebiene/Gesturefy/graphs/contributors">GitHub</a>, + <a target="_blank" rel="noopener" href="https://crowdin.com/project/gesturefy">Crowdin</a>
28-42: Locale-text migration looks good; addrel="noopener"to external links.The locale-text component usage is consistent. Apply the same security fix to these external links.
<li> - <a target="_blank" href="https://github.com/Robbendebiene/Gesturefy/issues"> + <a target="_blank" rel="noopener" href="https://github.com/Robbendebiene/Gesturefy/issues"> <locale-text key="aboutLinkBug"></locale-text> </a> </li> <li> - <a target="_blank" href="https://github.com/Robbendebiene/Gesturefy"> + <a target="_blank" rel="noopener" href="https://github.com/Robbendebiene/Gesturefy"> <locale-text key="aboutLinkCode"></locale-text> </a> </li> <li> - <a target="_blank" href="https://addons.mozilla.org/firefox/addon/gesturefy/"> + <a target="_blank" rel="noopener" href="https://addons.mozilla.org/firefox/addon/gesturefy/"> <locale-text key="aboutLinkAddon"></locale-text> </a> </li>src/core/utils/commons.mjs (1)
179-187: Consider guarding against an empty tabs query ingetActiveTab
browser.tabs.query({ active: true, currentWindow: true })should usually return one tab, but it can theoretically be empty (e.g. transient windows). Either document thatgetActiveTabmay returnundefinedor add a small guard (e.g. returnnullor throw) so callers don’t accidentally assume a tab is always present.src/core/utils/matching-algorithms.mjs (1)
16-54: Ensure all gesture objects expose a validpatternarrayThe switch to direct
gesture.patternaccess in all three algorithm branches is clean and simpler, but it assumes every gesture passed in has a non‑null pattern array. It’s worth confirming that gesture construction/migration guarantees this, or adding a lightweight validation/filter before the loop to avoid hard runtime errors if a malformed gesture slips in.src/resources/configs/defaults.mjs (1)
1-331: Defaults shape aligns with multi‑command model; consider stronger immutabilityThe new frozen defaults object and
commandsarrays look consistent with the refactored command/gesture model. If you rely on these as canonical read‑only defaults, consider either deep‑freezing (or always cloning on use) to avoid accidental mutation of nested objects leaking into the “defaults” instance over time.src/views/options/components/command-picker/command-groups.mjs (1)
1-134: Static command groups look consistent; watch for drift vs. command metadataThe grouped
new Command.*()instances cover the expected command set and match the templates at a glance. Longer‑term, you may want to derive these groups from a shared command metadata registry (or reuse whatever the picker already uses) so adding/removing commands in/core/models/commands.mjscan’t silently get out of sync with this configuration.src/views/options/components/gesture-card/layout.css (1)
1-66: Verify CSS nesting support (or preprocessing) for nested rulesThe nested rules like
#card:hover &and&::beforerely on CSS nesting support; if these stylesheets are shipped directly to Firefox without a build step, confirm that your minimum Firefox version fully supports this syntax in extension styles. Otherwise, you may want to flatten these selectors to avoid invalid CSS.src/views/options/index.html (1)
6-7: Align favicon type with SVG and confirm new Data wiringYou’re now using
/resources/img/icon.svgfor the favicon and sidebar logo, which is good. Consider updating the<link rel="icon">type fromimage/pngtoimage/svg+xml(or omitting it) for correctness. The new Data navigation entry and inclusion of/views/options/fragments/data.incanddata.mjslook consistent with the renamed section.Also applies to: 24-25, 65-70
src/views/options/components/command-picker/command-picker.mjs (1)
196-209: Assignment in loop condition.The
while (currentElement = previousElement.nextElementSibling)pattern works but can be confusing. Consider adding a comment or using a more explicit loop structure.This is a common pattern, but for readability you could alternatively use:
- while (currentElement = previousElement.nextElementSibling) { + while ((currentElement = previousElement.nextElementSibling) !== null) {src/views/options/fragments/gestures.inc (1)
32-35: Consider adding anameattribute to the label input for form data consistency.The input has an
idbut lacks anameattribute. If this form's data is collected viaFormData, the label value won't be included without aname.- <input id="gesturePopupLabelInput" class="input-field" maxlength="100"> + <input id="gesturePopupLabelInput" name="label" class="input-field" maxlength="100">src/views/shared/components/locale-option.mjs (1)
24-33: Consider logging a warning when translation key is not found.When
browser.i18n.getMessage(newValue)returns an empty string (key not found), the code silently does nothing. A console warning could help developers identify missing translations.attributeChangedCallback(name, oldValue, newValue) { // will also be called if attribute is initially set if (name === 'key') { const string = browser.i18n.getMessage(newValue); // insert text from language files if found if (string) { this.label = string; } + else if (newValue) { + console.warn(`Missing translation for key: ${newValue}`); + } } }src/views/options/main.mjs (1)
86-90: Consider adding null safety for the nested querySelector call.While all current navigation items do contain
locale-text[key]elements, accessing.keyon the querySelector result without a null check could cause an error if the HTML structure changes. Add a guard clause or optional chaining to handle the case where the selector returns null:const localeTextElement = nextItem.querySelector("locale-text[key]"); const sectionKey = localeTextElement?.key; if (sectionKey) { document.title = `Gesturefy - ${browser.i18n.getMessage(sectionKey)}`; }src/core/models/command-stack.mjs (1)
60-62:lastCommandreturnsundefinedfor empty stack.When
#commands.lengthis 0,nthCommand(-1)returnsundefined. This is likely acceptable behavior, but consider documenting it or returningnullexplicitly for clarity.src/core/content.mjs (1)
39-45: Inconsistent indentation style.The
Exclusions.addEventListenercall on line 40 uses unusual indentation that doesn't match the pattern used forConfig.addEventListeneron line 46. Consider aligning them for consistency.const Exclusions = new ExclusionService(); - Exclusions.addEventListener("change", main); +Exclusions.addEventListener("change", main); const Config = new ConfigManager({ defaults: DefaultConfig, autoUpdate: true }); - Config.addEventListener("change", main); +Config.addEventListener("change", main);src/views/options/components/gesture-card/gesture-card.mjs (1)
78-83: Consider storing and clearing the timeout to prevent potential stale callbacks.The
setTimeouttimer is not stored, so it cannot be cancelled indisconnectedCallback. While the?.optional chaining provides safety, storing the timer ID would be cleaner.+ #hoverTimeoutId; + #handlePointerenter(event) { // add delay so it only triggers if the mouse stays on the card - setTimeout(() => { + this.#hoverTimeoutId = setTimeout(() => { if (this.matches(":hover")) this.#patternElement?.playDemo(); }, 200); }And in
disconnectedCallback:disconnectedCallback() { + clearTimeout(this.#hoverTimeoutId); this.shadowRoot.replaceChildren(); this.#patternElement = null; this.#commandElement = null; }src/views/options/gestures.mjs (1)
87-129: Animation helpers look sound; be cautious with newer transform shorthand propertiesThe addition/update/removal animations are well structured and cache layout before mutating the DOM, which is good for performance.
Note that you’re using individual transform properties in Web Animations:
// Addition { translate: `${entry.x}px ${entry.y}px` }, { translate: '0 0' } { scale: '1.6', opacity: 0 }, { scale: '1', opacity: 1 } // Removal / update { transform: 'scale(1)', ... } // mixed usageSome browsers still have partial support for the
translate/scalelonghands in animations. If you see inconsistent behavior (especially in Firefox), consider normalizing totransform: 'translate(...)'/transform: 'scale(...)'for maximum compatibility.No change is strictly required if your target environment is verified to support these, but it’s something to keep in mind.
Also applies to: 152-179
src/core/models/gesture.mjs (1)
12-20: Gesture<->JSON / CommandStack integration looks good; consider empty-stack fallback intoStringThe new
Gesturemodel withCommandStackandfromJSON/toJSONis clean and well-typed:
- Constructor validates
patternis an array,commandsis aCommandStack,labelis string.fromJSONandtoJSONare symmetric and defer toCommandStack’s JSON methods.- Using private fields avoids accidental external mutation.
One small robustness concern:
toString() { return this.#label || this.commands.firstCommand.label; }If a
Gestureis ever constructed with an emptyCommandStack(e.g., from malformed data),firstCommandisundefinedand.labelwill throw.A defensive tweak would keep behavior while avoiding a hard crash:
toString() { - return this.#label || this.commands.firstCommand.label; + const first = this.commands.firstCommand; + return this.#label || first?.label || ''; }If you already guarantee non‑empty stacks at all call sites, this is optional, but it makes the model safer against bad data.
Also applies to: 22-31, 37-43, 49-51
src/views/options/components/orderable-collection/orderable-collection.mjs (3)
76-90: The#onDragEndmethod is markedasyncbut doesn't useawait.The
asynckeyword is unnecessary here since the method doesn't contain any awaited promises. This could be misleading to readers.- async #onDragEnd(event) { + #onDragEnd(event) {
204-213: OrderEvent class fields should be initialized in constructor only.The class field declarations for
oldIndexandnewIndexwithout initializers will be set toundefinedbefore the constructor assigns values. While this works, consider adding default values inline for clarity.class OrderEvent extends DragEvent { - oldIndex; - newIndex; + oldIndex = -1; + newIndex = -1; constructor(type, dragEventInit, oldIndex = -1, newIndex = -1) { super(type, dragEventInit); this.oldIndex = oldIndex; this.newIndex = newIndex; } }
316-327: Consider returning the animation for chaining or awaiting.The
afterDOMChangemethod returns anAnimationobject only when there's a shift, but returnsundefinedotherwise. The caller may want to await animation completion. Ensure the return type is consistent or document this behavior.src/core/models/command.mjs (1)
126-130: Missing semicolon afterObject.seal(descriptor.value).For consistency with the rest of the codebase, add a semicolon.
if (property === 'settings') { descriptor.writable = false; Object.assign(descriptor.value, target.settings); - Object.seal(descriptor.value) + Object.seal(descriptor.value); }src/core/models/commands.mjs (2)
30-46: Consider extracting duplicated tab position logic into a helper.The switch statement for calculating tab index based on position setting ("before", "after", "start", "end", "default") is repeated across 8+ command classes. Extracting this into a shared helper method would reduce duplication and improve maintainability.
Example helper in the Command base class or commons utility:
function getTabIndex(position, currentTabIndex) { switch (position) { case "before": return currentTabIndex; case "after": return currentTabIndex + 1; case "start": return 0; case "end": return Number.MAX_SAFE_INTEGER; default: return null; } }
2284-2319: Add error handling for network operations.The
fetch()call and image loading have no error handling. Network failures, CORS issues, or invalid URLs will cause unhandled promise rejections. Consider wrapping in try-catch with user notification.async execute(sender, data) { if (data.target.nodeName.toLowerCase() === "img" && data.target.src) { + try { const response = await fetch(data.target.src); + if (!response.ok) return; // silently fail on HTTP errors const mimeType = response.headers.get("Content-Type"); // ... rest of the logic + } catch (error) { + console.warn("Failed to copy image:", error); + return; // indicate failure by not returning true + } // confirm success return true; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
src/resources/fonts/icons.woffis excluded by!**/*.woffsrc/resources/img/icon.svgis excluded by!**/*.svgsrc/resources/img/icon_deactivated.svgis excluded by!**/*.svgsrc/resources/img/iconx32.pngis excluded by!**/*.pngsrc/resources/img/iconx48.pngis excluded by!**/*.pngsrc/resources/img/iconx96.pngis excluded by!**/*.png
📒 Files selected for processing (66)
README.md(1 hunks)src/_locales/en/messages.json(5 hunks)src/core/background.mjs(5 hunks)src/core/bundle/background.html(0 hunks)src/core/bundle/content.bundle.js(7 hunks)src/core/commands.mjs(0 hunks)src/core/content.mjs(4 hunks)src/core/migration.mjs(1 hunks)src/core/models/command-stack.mjs(1 hunks)src/core/models/command.mjs(1 hunks)src/core/models/commands.mjs(1 hunks)src/core/models/gesture-context-data.mjs(1 hunks)src/core/models/gesture.mjs(1 hunks)src/core/services/base-event-listener.mjs(1 hunks)src/core/services/config-manager.mjs(5 hunks)src/core/services/exclusion-service.mjs(1 hunks)src/core/services/host-permission-service.mjs(1 hunks)src/core/utils/commons.mjs(2 hunks)src/core/utils/matching-algorithms.mjs(3 hunks)src/manifest.json(3 hunks)src/resources/configs/defaults.mjs(1 hunks)src/resources/json/commands.json(0 hunks)src/views/installation/index.html(5 hunks)src/views/installation/layout.css(3 hunks)src/views/installation/main.mjs(0 hunks)src/views/options/components/collapsible-item/collapsible-item.mjs(1 hunks)src/views/options/components/color-picker/main.mjs(1 hunks)src/views/options/components/command-card/command-card.mjs(1 hunks)src/views/options/components/command-card/command-setting-templates.inc(1 hunks)src/views/options/components/command-card/layout.css(1 hunks)src/views/options/components/command-multi-select/layout.css(0 hunks)src/views/options/components/command-multi-select/main.mjs(0 hunks)src/views/options/components/command-picker/command-groups.mjs(1 hunks)src/views/options/components/command-picker/command-picker.mjs(1 hunks)src/views/options/components/command-picker/layout.css(1 hunks)src/views/options/components/command-select/layout.css(0 hunks)src/views/options/components/command-select/main.mjs(0 hunks)src/views/options/components/gesture-card/gesture-card.mjs(1 hunks)src/views/options/components/gesture-card/layout.css(1 hunks)src/views/options/components/orderable-collection/orderable-collection.mjs(1 hunks)src/views/options/components/pattern-preview/layout.css(1 hunks)src/views/options/components/pattern-preview/pattern-preview.mjs(1 hunks)src/views/options/components/popup-box/main.mjs(1 hunks)src/views/options/components/sortable-multi-select/layout.css(0 hunks)src/views/options/components/sortable-multi-select/main.mjs(0 hunks)src/views/options/data-management.mjs(0 hunks)src/views/options/data.mjs(1 hunks)src/views/options/exclusions.mjs(0 hunks)src/views/options/fragments/about.inc(1 hunks)src/views/options/fragments/command-setting-templates.inc(0 hunks)src/views/options/fragments/data.inc(1 hunks)src/views/options/fragments/exclusions.inc(0 hunks)src/views/options/fragments/extras.inc(2 hunks)src/views/options/fragments/gestures.inc(1 hunks)src/views/options/fragments/settings.inc(1 hunks)src/views/options/gestures.mjs(9 hunks)src/views/options/index.html(2 hunks)src/views/options/layout.css(19 hunks)src/views/options/main.mjs(5 hunks)src/views/popup/index.html(1 hunks)src/views/popup/layout.css(1 hunks)src/views/popup/main.mjs(1 hunks)src/views/shared/commons.mjs(1 hunks)src/views/shared/components/locale-option.mjs(1 hunks)src/views/shared/components/locale-text.mjs(1 hunks)src/views/shared/theme.css(1 hunks)
💤 Files with no reviewable changes (14)
- src/views/installation/main.mjs
- src/resources/json/commands.json
- src/views/options/components/command-multi-select/main.mjs
- src/views/options/components/command-select/main.mjs
- src/views/options/fragments/exclusions.inc
- src/views/options/fragments/command-setting-templates.inc
- src/views/options/components/sortable-multi-select/main.mjs
- src/core/bundle/background.html
- src/views/options/exclusions.mjs
- src/views/options/data-management.mjs
- src/views/options/components/sortable-multi-select/layout.css
- src/views/options/components/command-select/layout.css
- src/views/options/components/command-multi-select/layout.css
- src/core/commands.mjs
🧰 Additional context used
🧬 Code graph analysis (22)
src/views/options/main.mjs (6)
src/views/popup/main.mjs (1)
Config(11-13)src/core/background.mjs (1)
Config(22-25)src/core/bundle/content.bundle.js (1)
Config(2671-2674)src/core/content.mjs (1)
Config(42-45)src/core/migration.mjs (1)
Config(7-7)src/core/services/config-manager.mjs (1)
ConfigManager(9-244)
src/core/services/host-permission-service.mjs (1)
src/core/services/base-event-listener.mjs (1)
BaseEventListener(4-70)
src/views/options/components/command-picker/command-picker.mjs (3)
src/views/shared/commons.mjs (1)
Build(22-38)src/core/models/command-stack.mjs (1)
CommandStack(8-97)src/views/options/components/command-card/command-card.mjs (1)
CommandCard(13-149)
src/views/options/components/pattern-preview/pattern-preview.mjs (1)
src/views/shared/commons.mjs (1)
Build(22-38)
src/core/utils/matching-algorithms.mjs (2)
src/core/migration.mjs (1)
pattern(136-136)src/views/options/gestures.mjs (1)
gesture(46-46)
src/views/options/data.mjs (2)
src/views/options/main.mjs (4)
ContentLoaded(21-21)ContentLoaded(21-21)Config(7-9)Config(7-9)src/core/services/config-manager.mjs (1)
ConfigManager(9-244)
src/core/services/exclusion-service.mjs (2)
src/core/services/base-event-listener.mjs (1)
BaseEventListener(4-70)src/core/utils/commons.mjs (2)
i(144-144)isURL(54-62)
src/views/options/components/gesture-card/gesture-card.mjs (2)
src/views/options/gestures.mjs (1)
gesture(46-46)src/views/shared/commons.mjs (1)
Build(22-38)
src/views/popup/main.mjs (4)
src/core/services/config-manager.mjs (1)
ConfigManager(9-244)src/core/services/exclusion-service.mjs (1)
ExclusionService(11-118)src/core/services/host-permission-service.mjs (1)
HostPermissionService(6-74)src/core/utils/commons.mjs (3)
getActiveTab(182-187)url(81-81)url(101-101)
src/core/models/command-stack.mjs (1)
src/core/models/commands.mjs (3)
commands(2672-2674)command(2701-2701)returnValue(2702-2702)
src/views/options/components/command-card/command-card.mjs (1)
src/views/shared/commons.mjs (2)
fetchHTMLAsFragment(45-52)Build(22-38)
src/views/options/gestures.mjs (4)
src/core/models/gesture.mjs (1)
Gesture(7-79)src/views/options/components/gesture-card/gesture-card.mjs (1)
GestureCard(8-90)src/core/models/command-stack.mjs (1)
CommandStack(8-97)src/views/options/components/pattern-preview/pattern-preview.mjs (1)
PatternPreview(9-202)
src/core/models/gesture.mjs (1)
src/core/models/command-stack.mjs (1)
CommandStack(8-97)
src/core/services/config-manager.mjs (1)
src/core/utils/commons.mjs (1)
isObject(5-7)
src/views/shared/components/locale-option.mjs (1)
src/views/shared/components/locale-text.mjs (1)
string(24-24)
src/core/content.mjs (3)
src/core/services/exclusion-service.mjs (1)
ExclusionService(11-118)src/core/migration.mjs (1)
Config(7-7)src/core/services/config-manager.mjs (1)
ConfigManager(9-244)
src/views/shared/components/locale-text.mjs (1)
src/views/shared/components/locale-option.mjs (1)
string(27-27)
src/views/options/components/orderable-collection/orderable-collection.mjs (1)
src/views/options/gestures.mjs (1)
animation(105-111)
src/core/bundle/content.bundle.js (5)
src/core/utils/commons.mjs (5)
isURL(54-62)isObject(5-7)i(144-144)url(81-81)url(101-101)src/core/services/base-event-listener.mjs (1)
BaseEventListener(4-70)src/core/services/exclusion-service.mjs (7)
ExclusionService(11-118)exclusions(24-24)newExclusions(33-33)oldExclusions(34-34)tailoredExclusions(70-72)urlObj(86-86)globPattern(87-87)src/core/background.mjs (2)
Exclusions(29-29)Config(22-25)src/core/services/config-manager.mjs (1)
ConfigManager(9-244)
src/core/background.mjs (4)
src/core/models/gesture.mjs (1)
Gesture(7-79)src/core/models/command-stack.mjs (1)
CommandStack(8-97)src/core/utils/matching-algorithms.mjs (1)
bestMatchingGesture(10-10)src/core/utils/commons.mjs (1)
getActiveTab(182-187)
src/core/models/command.mjs (1)
src/core/models/commands.mjs (1)
Command(2670-2670)
src/core/models/commands.mjs (1)
src/core/utils/commons.mjs (9)
getClosestElement(193-201)scrollToY(210-238)displayNotification(155-176)isHTTPURL(79-90)isDomainName(71-73)isLegalURL(96-110)isURL(54-62)dataURItoBlob(137-148)sanitizeFilename(116-130)
🪛 Biome (2.1.2)
src/views/options/components/collapsible-item/collapsible-item.mjs
[error] 76-76: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 77-77: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 159-159: The setter should not return a value.
The setter is here:
Returning a value from a setter is ignored.
(lint/correctness/noSetterReturn)
src/views/options/components/command-picker/layout.css
[error] 100-100: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
min-width is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
🪛 markdownlint-cli2 (0.18.1)
README.md
7-7: Images should have alternate text (alt text)
(MD045, no-alt-text)
| hasEventListener (event, callback) { | ||
| this._validateEventParameter(event); | ||
| this._validateCallbackParameter(callback); | ||
| this._events.get(event).has(callback); |
There was a problem hiding this comment.
hasEventListener method doesn't return the result.
The method calls .has() but doesn't return the boolean result. This is also present in the BaseEventListener snippet from other files, suggesting a systematic bug.
hasEventListener (event, callback) {
this._validateEventParameter(event);
this._validateCallbackParameter(callback);
- this._events.get(event).has(callback);
+ return this._events.get(event).has(callback);
}🤖 Prompt for AI Agents
In src/core/bundle/content.bundle.js around lines 915 to 918, the
hasEventListener method calls this._events.get(event).has(callback) but does not
return the boolean result; change the method to return the boolean value and
guard against a missing event entry by retrieving the listener set into a local
variable and returning its .has(callback) result or false if the set is
undefined.
| async execute(sender, data) { | ||
| // get ref to Command class constructor | ||
| const Command = this.constructor; | ||
| // create Command objects | ||
| const commands = this.settings["commands"].map((commandObject) => { | ||
| return new Command(commandObject); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the PopupCustomCommandList class definition
rg -n "class PopupCustomCommandList" --type=js -A 20Repository: Robbendebiene/Gesturefy
Length of output: 49
🏁 Script executed:
# Find the Command base class definition
rg -n "class Command {" --type=js -A 15Repository: Robbendebiene/Gesturefy
Length of output: 160
🏁 Script executed:
# Search for factory methods and deserialization patterns
rg -n "fromJSON|createCommand|static create" --type=js -B 2 -A 5Repository: Robbendebiene/Gesturefy
Length of output: 49
🏁 Script executed:
# Check how settings["commands"] is populated and what structure it has
rg -n 'settings\["commands"\]|settings\.commands' --type=js -B 3 -A 3Repository: Robbendebiene/Gesturefy
Length of output: 49
🏁 Script executed:
# Look at CommandStack mentioned in the comment to understand the deserialization mechanism
rg -n "CommandStack" --type=js -B 2 -A 5Repository: Robbendebiene/Gesturefy
Length of output: 49
🏁 Script executed:
# Check if the file exists and get its size
stat src/core/models/commands.mjs
wc -l src/core/models/commands.mjsRepository: Robbendebiene/Gesturefy
Length of output: 160
🏁 Script executed:
# Read the specific section mentioned in the review (lines 2668-2674)
sed -n '2668,2674p' src/core/models/commands.mjsRepository: Robbendebiene/Gesturefy
Length of output: 331
🏁 Script executed:
# Look for PopupCustomCommandList class - start search
rg -n "PopupCustomCommandList" src/core/models/commands.mjs -B 2 -A 10Repository: Robbendebiene/Gesturefy
Length of output: 482
🏁 Script executed:
# Look for class Command definition with simpler pattern
rg -n "^class Command" src/core/models/commands.mjs -A 20Repository: Robbendebiene/Gesturefy
Length of output: 49
🏁 Script executed:
# Search for fromJSON or factory methods
rg -n "fromJSON\|createCommand" src/core/models/commands.mjs -B 2 -A 5Repository: Robbendebiene/Gesturefy
Length of output: 49
🏁 Script executed:
# Find Command class definition - look at the start of the class
head -n 100 src/core/models/commands.mjs | tail -n 50Repository: Robbendebiene/Gesturefy
Length of output: 885
🏁 Script executed:
# Search for "class Command" without special regex
rg -n "class Command" src/core/models/commands.mjs | head -20Repository: Robbendebiene/Gesturefy
Length of output: 49
🏁 Script executed:
# Look at the Command base class more broadly
sed -n '1,100p' src/core/models/commands.mjsRepository: Robbendebiene/Gesturefy
Length of output: 2030
🏁 Script executed:
# Read more of PopupCustomCommandList to understand its purpose
sed -n '2663,2711p' src/core/models/commands.mjsRepository: Robbendebiene/Gesturefy
Length of output: 1482
🏁 Script executed:
# Check if commands are stored as objects or as command instances
rg -n "settings\[.commands.\].*=" src/core/models/commands.mjs -B 2 -A 2Repository: Robbendebiene/Gesturefy
Length of output: 280
🏁 Script executed:
# Find and read the Command base class (imported from /core/models/command.mjs)
find . -name "command.mjs" -type fRepository: Robbendebiene/Gesturefy
Length of output: 96
🏁 Script executed:
# Read the Command class definition
cat /core/models/command.mjs 2>/dev/null || find . -path "*/core/models/command.mjs" -exec cat {} \;Repository: Robbendebiene/Gesturefy
Length of output: 4655
🏁 Script executed:
# Search for where settings["commands"] is populated to understand commandObject structure
rg -n "commands.*push\|commands.*=.*\[" src/core/models/commands.mjs -B 3 -A 3Repository: Robbendebiene/Gesturefy
Length of output: 49
🏁 Script executed:
# Look for Command instantiation patterns elsewhere in the file to understand the pattern
rg -n "new Command\|new \w+Command" src/core/models/commands.mjs | head -20Repository: Robbendebiene/Gesturefy
Length of output: 49
🏁 Script executed:
# Check if there's a factory or lookup mechanism for creating commands by name
rg -n "CommandStack|fromJSON|getCommand|lookupCommand" src/core/models/commands.mjsRepository: Robbendebiene/Gesturefy
Length of output: 49
🏁 Script executed:
# Search for how commands are typically created elsewhere in the codebase
rg -n "new \w+Command\(" src/core/models/commands.mjs | head -10Repository: Robbendebiene/Gesturefy
Length of output: 49
🏁 Script executed:
# Look for where PopupCustomCommandList.settings["commands"] is populated
rg -B 5 -A 5 "PopupCustomCommandList" src/core/models/commands.mjs | tail -30Repository: Robbendebiene/Gesturefy
Length of output: 215
🏁 Script executed:
# Check if there's any command registry or mapping
rg -n "commandRegistry\|commandMap\|COMMANDS\|commands.*=.*{" src/core/models/commands.mjs | head -15Repository: Robbendebiene/Gesturefy
Length of output: 236
🏁 Script executed:
# Look at how other parts of the codebase handle command deserialization
find . -name "*.mjs" -o -name "*.js" | xargs rg -l "CommandStack\|CommandStack.fromJSON" 2>/dev/null | head -5Repository: Robbendebiene/Gesturefy
Length of output: 49
🏁 Script executed:
# Search for how Command subclasses are typically stored or referenced
git ls-files src/core/models/ | head -20Repository: Robbendebiene/Gesturefy
Length of output: 226
🏁 Script executed:
# Check if there's an export list or index file that maps commands
cat src/core/models/commands.mjs | head -1 && echo "---" && tail -50 src/core/models/commands.mjsRepository: Robbendebiene/Gesturefy
Length of output: 1372
🏁 Script executed:
# Look for how settings["commands"] data is actually structured by examining the UI or config
rg -n "PopupCustomCommandList" . --type=js --type=mjs -B 5 -A 5 | head -40Repository: Robbendebiene/Gesturefy
Length of output: 94
🏁 Script executed:
# Read the command-stack.mjs file to understand proper command instantiation
cat src/core/models/command-stack.mjsRepository: Robbendebiene/Gesturefy
Length of output: 2526
Fix command instantiation logic in PopupCustomCommandList.execute().
The code uses this.constructor (which is PopupCustomCommandList) to instantiate commands from this.settings["commands"], but should deserialize commands by name instead. This creates PopupCustomCommandList instances rather than the intended command types (e.g., NewTab, CloseTab).
Reference the CommandStack.fromJSON() pattern in src/core/models/command-stack.mjs, which correctly instantiates commands as new Commands[item.name](item.settings). Apply the same approach here:
const commands = this.settings["commands"].map((commandObject) => {
return new Commands[commandObject.name](commandObject.settings);
});This ensures each command object is instantiated as its correct type, allowing proper execution and label generation.
🤖 Prompt for AI Agents
In src/core/models/commands.mjs around lines 2668 to 2674, the execute method
currently uses this.constructor to instantiate each command which creates
PopupCustomCommandList instances instead of the intended command types; change
the instantiation to use the Commands registry keyed by the command name
(following the CommandStack.fromJSON pattern) so each entry is created as new
Commands[commandObject.name](commandObject.settings) rather than new
Command(commandObject), ensuring correct command types for execution and label
generation.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/services/config-manager.mjs (1)
202-210: Bug:hasEventListenerdoesn't return a value.The method checks if a callback exists but discards the result. It should return the boolean from
has().hasEventListener (event, callback) { if (!this._events.hasOwnProperty(event)) { throw "The first argument is not a valid event."; } if (typeof callback !== "function") { throw "The second argument must be a function."; } - this._events[event].has(callback); + return this._events[event].has(callback); }
♻️ Duplicate comments (4)
src/core/services/exclusion-service.mjs (1)
31-46: Past issue has been addressed.The storage change handler now correctly normalizes
newValueandoldValueto arrays usingArray.isArray()checks (lines 35-36), preventing potential crashes when theExclusionskey is removed or set to a non-array value.src/core/services/config-manager.mjs (1)
87-87: Past issue has been addressed.The code now uses
globalThis.structuredClone()instead ofwindow.structuredClone(), which works correctly in MV3 service worker contexts.Also applies to: 112-112, 130-130
src/views/options/data.mjs (1)
194-203: Past issue has been addressed.The code now correctly accesses
gesture.commands(array) instead ofgesture.command(singular), and usesCommandStack.fromJSONto properly extract permissions.src/core/models/commands.mjs (1)
2668-2674: Critical: Command instantiation still uses wrong constructor (unchanged from previous review).
this.constructorisPopupCustomCommandList, not the baseCommandclass or the specific command type. This createsPopupCustomCommandListinstances instead of the intended command types (e.g.,NewTab,CloseTab).Additionally, line 2678 calls
command.toString()which will return"[object Object]"sinceCommandhas no customtoString(). This should likely usecommand.label.Follow the
CommandStack.fromJSON()pattern:async execute(sender, data) { - // get ref to Command class constructor - const Command = this.constructor; // create Command objects const commands = this.settings["commands"].map((commandObject) => { - return new Command(commandObject); + // instantiate correct command type by name lookup + const CommandClass = Commands[commandObject.name]; + if (!CommandClass) throw `Unknown command: "${commandObject.name}"`; + return new CommandClass(commandObject.settings); }); // map commands to popup data structure const dataset = commands.map((command, index) => ({ id: index, - label: command.toString(), + label: command.label, icon: null }));Note: You'll need to import
Commandsor use a self-reference pattern since this file exports the commands.
🧹 Nitpick comments (6)
src/views/options/components/command-card/command-setting-templates.inc (1)
1-289: Consider adding explicit label/ARIA wiring for better accessibilityMost controls rely on visual proximity to preceding
locale-textelements, but they are not associated via<label for>or ARIA attributes. This can make screen-reader narration and focus indication less clear. Long term, consider either:
- Turning
locale-textinto/inside actual<label for="...">elements, or- Adding
aria-labelledby/aria-describedbyon inputs pointing to the relevantlocale-text/description IDs.Not urgent, but would improve a11y across the options UI.
src/core/services/exclusion-service.mjs (2)
40-51: Memory leak: URL not revoked after download.The created object URL should be revoked after use to free memory.
document.body.appendChild(linkElement); linkElement.click(); document.body.removeChild(linkElement); + URL.revokeObjectURL(linkElement.href); }
100-105: Consider clearing event listeners indispose().The
dispose()method removes the storage listener but doesn't clear event listeners registered via the inheritedBaseEventListener. Depending on usage, this could prevent garbage collection of callback references.dispose() { browser.storage.onChanged.removeListener(this._listener); + this.clearEventListeners('change'); }src/core/services/config-manager.mjs (1)
49-53: Consider handling key removal in autoUpdate.When a storage key is removed,
newValueisundefined. Settingthis._storage[property] = undefinedkeeps the key in the object, which may cause unexpected behavior when iterating or checkinghasOwnProperty.if (this._autoUpdate === true) { for (let property in changes) { - this._storage[property] = changes[property].newValue; + if (changes[property].newValue === undefined) { + delete this._storage[property]; + } else { + this._storage[property] = changes[property].newValue; + } } }src/views/options/data.mjs (1)
40-51: Memory leak: URL not revoked after download.The created object URL should be revoked after use to free memory.
document.body.appendChild(linkElement); linkElement.click(); document.body.removeChild(linkElement); + URL.revokeObjectURL(linkElement.href); }src/core/models/command-stack.mjs (1)
16-25: Add validation for unknown command names infromJSON.If
item.nameis not a valid command class inCommands, the error "Commands[item.name] is not a constructor" provides poor debugging context. Consider adding explicit validation:static fromJSON(json) { if (!Array.isArray(json)) throw 'Expects an array of JSON objects.'; return new CommandStack( json.map((item) => { + const CommandClass = Commands[item.name]; + if (!CommandClass) throw `Unknown command: "${item.name}"`; // note: a dedicated Command.fromJSON(item); function would easily make sense on the basic Command class, // but this would create a cyclic import dependency between 'command.mjs' and 'commands.mjs' - return new Commands[item.name](item.settings) + return new CommandClass(item.settings); }) ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/core/models/command-stack.mjs(1 hunks)src/core/models/command.mjs(1 hunks)src/core/models/commands.mjs(1 hunks)src/core/services/config-manager.mjs(5 hunks)src/core/services/exclusion-service.mjs(1 hunks)src/core/services/host-permission-service.mjs(1 hunks)src/views/options/components/command-card/command-setting-templates.inc(1 hunks)src/views/options/components/gesture-card/gesture-card.mjs(1 hunks)src/views/options/data.mjs(1 hunks)src/views/options/layout.css(19 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/views/options/components/gesture-card/gesture-card.mjs
- src/core/services/host-permission-service.mjs
🧰 Additional context used
🧬 Code graph analysis (6)
src/core/services/config-manager.mjs (1)
src/core/utils/commons.mjs (1)
isObject(5-7)
src/core/services/exclusion-service.mjs (2)
src/core/services/base-event-listener.mjs (1)
BaseEventListener(4-70)src/core/utils/commons.mjs (2)
i(144-144)isURL(54-62)
src/core/models/command-stack.mjs (1)
src/core/models/commands.mjs (3)
commands(2672-2674)command(2701-2701)returnValue(2702-2702)
src/views/options/data.mjs (4)
src/views/options/main.mjs (4)
ContentLoaded(21-21)ContentLoaded(21-21)Config(7-9)Config(7-9)src/core/migration.mjs (1)
Config(7-7)src/core/services/config-manager.mjs (1)
ConfigManager(9-244)src/core/models/command-stack.mjs (1)
CommandStack(8-104)
src/core/models/command.mjs (3)
src/core/models/commands.mjs (1)
Command(2670-2670)src/core/content.mjs (1)
data(261-261)src/core/models/gesture.mjs (1)
obj(38-41)
src/core/models/commands.mjs (1)
src/core/utils/commons.mjs (10)
isScrollableY(257-290)getClosestElement(193-201)scrollToY(210-238)displayNotification(155-176)isHTTPURL(79-90)isDomainName(71-73)isLegalURL(96-110)isURL(54-62)dataURItoBlob(137-148)sanitizeFilename(116-130)
🔇 Additional comments (11)
src/views/options/components/command-card/command-setting-templates.inc (1)
69-80: Previously reported HTML syntax issues are now resolvedThe earlier stray
>on thenextFocus<select>and the incorrect closing tag for theExcludeDiscardedlocale-textare both fixed; the templates in these sections are now valid and should parse/render correctly.Also applies to: 179-183
src/views/options/layout.css (2)
533-570: Well-structured grid layout for gesture popup form.The new grid-based layout with named template areas (lines 533–570) is clean and maintainable. The modular token-driven approach using
grid-gap: 1.25rem,var(--shadow-color), and consistent spacing improves consistency with the broader refactor.
189-206: The:not()selector with complex selectors is valid and widely supported.Line 191 uses
:not(.ol-section .ol-section), which is valid CSS Selectors Level 4. Complex selectors inside:not()have been supported in Chrome since version 88 (2021), Firefox since version 84 (2020), and modern Safari. This is safe to use in contemporary projects targeting current browsers.If legacy browser support (IE, very old versions) is a requirement, use
@supports selector()to provide fallbacks. Otherwise, no change is needed here.Note: The suggested diff would alter the logic—removing
:not()would match all sibling ol-sections, defeating the intent to exclude nested sections.src/views/options/data.mjs (1)
175-178: Verifyoptional_permissionsexists in MV3 manifest.In Manifest V3,
optional_permissionsmay be undefined or empty. Add a guard to prevent passing undefined tobrowser.permissions.remove().if (proceed) { const manifest = browser.runtime.getManifest(); - await browser.permissions.remove( - { permissions: manifest.optional_permissions } - ); + if (manifest.optional_permissions?.length > 0) { + await browser.permissions.remove( + { permissions: manifest.optional_permissions } + ); + } Config.clear();src/core/models/command-stack.mjs (1)
1-14: LGTM on class structure, mutation methods, and execute logic.The constructor validation,
toJSONserialization, accessor methods, and theexecuteloop with early-exit on success are well-implemented.Also applies to: 27-84, 93-103
src/core/models/command.mjs (2)
54-58: Confirmed: Previous logic inversion has been fixed.The
requiresNewPermissions()now correctly returnstruewhen permissions are missing by negating the result ofbrowser.permissions.contains().
116-140: Proxy pattern is well-documented and correctly implemented.The proxy solution for preventing child class property initializers from overwriting settings is clever. The inline documentation explaining why this approach was chosen over alternatives is helpful for maintainability.
src/core/models/commands.mjs (4)
693-714: Confirmed: PreviouscanScrollUpbug has been fixed.Line 713 now correctly returns
canScrollDowninstead of the previously incorrectcanScrollUp.
2315-2316: Confirmed:canvas.toBlobtype argument is now correctly placed.The MIME type
"image/png"is now properly passed tocanvas.toBlob()within the Promise callback.
21-55: Command implementations follow a consistent, well-structured pattern.The tab management commands (
DuplicateTab,NewTab,CloseTab,RestoreTab) demonstrate good practices:
- Clear switch statements for position handling
- Proper use of browser APIs with
await- Consistent
return trueto confirm success- Optional settings with sensible defaults
Also applies to: 95-142, 212-235
498-548: The review comment is incorrect. The helper functions are available in the injected script context.The
isScrollableY,getClosestElement,scrollToYfunctions andTARGETvariable are intentionally exposed globally by the pre-injected content script (src/core/content.mjs, lines 29–35). The manifest loads this content script atdocument_starton all frames, ensuring these helpers are available in the global window scope before any commands are executed. Whenbrowser.scripting.executeScript()injects the scroll functions, they can access these globals without error.Likely an incorrect or invalid review comment.
|
|
||
| // catch error and free the blob for gc | ||
| if (browser.runtime.lastError) URL.revokeObjectURL(screenshotURL); | ||
| else browser.downloads.onChanged.addListener(function clearURL(downloadDelta) { | ||
| if (downloadId === downloadDelta.id && downloadDelta.state.current === "complete") { | ||
| URL.revokeObjectURL(screenshotURL); | ||
| browser.downloads.onChanged.removeListener(clearURL); | ||
| } | ||
| }); |
There was a problem hiding this comment.
browser.runtime.lastError check is ineffective after await.
With promise-based APIs, errors throw exceptions rather than setting runtime.lastError. This check on line 2213 will never be true because:
- If
browser.downloads.download()fails, it throws (caught by try/catch, which isn't present here) runtime.lastErroris only set in callback contexts, not after awaited promises
The blob URL may leak if the download fails. Consider wrapping in try/catch:
+ try {
const downloadId = await browser.downloads.download({
url: screenshotURL,
filename: sanitizeFilename(sender.tab.title) + '.png',
saveAs: true
});
-
- // catch error and free the blob for gc
- if (browser.runtime.lastError) URL.revokeObjectURL(screenshotURL);
- else browser.downloads.onChanged.addListener(function clearURL(downloadDelta) {
+ browser.downloads.onChanged.addListener(function clearURL(downloadDelta) {
if (downloadId === downloadDelta.id && downloadDelta.state.current === "complete") {
URL.revokeObjectURL(screenshotURL);
browser.downloads.onChanged.removeListener(clearURL);
}
});
+ }
+ catch (error) {
+ URL.revokeObjectURL(screenshotURL);
+ throw error;
+ }The same issue exists in SaveImage around lines 2383-2394.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/core/models/commands.mjs around lines 2211-2219 (and similarly in
SaveImage around lines 2383-2394), the check for browser.runtime.lastError after
an awaited browser.downloads.download() is ineffective and may leak the blob
URL; wrap the await call in a try/catch so any thrown error leads to
URL.revokeObjectURL(screenshotURL) in the catch, and keep the onChanged listener
logic for the success path (ensuring the listener is removed after revoking);
also ensure the listener is only added after a successful download call so you
don't rely on runtime.lastError.
| async function onFileRestoreButton (event) { | ||
| if (this.files[0].type !== "application/json") { | ||
| prompt("restoreAlertWrongFile"); | ||
| // terminate function | ||
| return; | ||
| } |
There was a problem hiding this comment.
Guard against empty file selection.
If the file dialog is canceled or no file is selected, this.files[0] is undefined and accessing .type will throw.
async function onFileRestoreButton (event) {
+ if (!this.files || this.files.length === 0) {
+ return;
+ }
if (this.files[0].type !== "application/json") {
prompt("restoreAlertWrongFile");
// terminate function
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function onFileRestoreButton (event) { | |
| if (this.files[0].type !== "application/json") { | |
| prompt("restoreAlertWrongFile"); | |
| // terminate function | |
| return; | |
| } | |
| async function onFileRestoreButton (event) { | |
| if (!this.files || this.files.length === 0) { | |
| return; | |
| } | |
| if (this.files[0].type !== "application/json") { | |
| prompt("restoreAlertWrongFile"); | |
| // terminate function | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/views/options/data.mjs around lines 58 to 63, the handler accesses
this.files[0].type without checking for an empty selection; update the guard to
first verify that this.files exists and has at least one item (e.g., this.files
&& this.files.length > 0) and if not, call the appropriate user prompt or
silently return, then only check the selected file's .type and continue; ensure
the function returns early when no file is selected or when the file type is
wrong.
| if (proceed) { | ||
| Config.clear(); | ||
| Config.set(json); | ||
| await prompt("restoreAlertSuccess"); | ||
| // reload option page to update the ui | ||
| window.location.reload(); | ||
| } |
There was a problem hiding this comment.
Await Config.clear() before setting new config.
Config.clear() returns a Promise. Calling Config.set() without awaiting could cause a race condition where the new config is written before the clear completes.
if (proceed) {
- Config.clear();
- Config.set(json);
+ await Config.clear();
+ await Config.set(json);
await prompt("restoreAlertSuccess");
// reload option page to update the ui
window.location.reload();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (proceed) { | |
| Config.clear(); | |
| Config.set(json); | |
| await prompt("restoreAlertSuccess"); | |
| // reload option page to update the ui | |
| window.location.reload(); | |
| } | |
| if (proceed) { | |
| await Config.clear(); | |
| await Config.set(json); | |
| await prompt("restoreAlertSuccess"); | |
| // reload option page to update the ui | |
| window.location.reload(); | |
| } |
🤖 Prompt for AI Agents
In src/views/options/data.mjs around lines 97 to 103, Config.clear() is async
and must be awaited to avoid a race where Config.set(json) runs before clearing
finishes; update the flow to await Config.clear() (e.g., await Config.clear())
before calling Config.set(json) and keep the subsequent await prompt(...) and
window.location.reload() after the awaited operations so the new config is
reliably written.
| if (proceed) { | ||
| Config.clear(); | ||
| Config.set(json); | ||
| await prompt("downloadAlertSuccess"); | ||
| // reload option page to update the ui | ||
| window.location.reload(); | ||
| } |
There was a problem hiding this comment.
Same race condition: await Config.clear() before setting.
Consistent with the file restore flow, await both operations to prevent race conditions.
if (proceed) {
- Config.clear();
- Config.set(json);
+ await Config.clear();
+ await Config.set(json);
await prompt("downloadAlertSuccess");
// reload option page to update the ui
window.location.reload();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (proceed) { | |
| Config.clear(); | |
| Config.set(json); | |
| await prompt("downloadAlertSuccess"); | |
| // reload option page to update the ui | |
| window.location.reload(); | |
| } | |
| if (proceed) { | |
| await Config.clear(); | |
| await Config.set(json); | |
| await prompt("downloadAlertSuccess"); | |
| // reload option page to update the ui | |
| window.location.reload(); | |
| } |
🤖 Prompt for AI Agents
In src/views/options/data.mjs around lines 158 to 164, there is a race condition
because Config.clear() and Config.set(json) are called without awaiting them;
update the code to await Config.clear() and then await Config.set(json) (i.e.,
await the clear to finish before calling and awaiting set) so both operations
complete in sequence before calling await prompt("downloadAlertSuccess") and
reloading the page.
| .toggle-button, | ||
| .input-field, | ||
| .select-field, | ||
| .button, | ||
| .color-select-field { | ||
| box-sizing: border-box; | ||
| padding: 0.4em 0.5em; | ||
| border: 1px solid var(--border-color); | ||
| border-radius: 0.2em; | ||
| background: var(--base-color, #FFF); | ||
| color: var(--text-color, #555555); | ||
| text-align: center; | ||
| font-size: 1rem; | ||
| font-family: inherit; | ||
| transition: background .5s; | ||
| /* use normal because line-height for text inputs cannot be override: https://www.456bereastreet.com/archive/201108/line-height_in_input_fields/ */ | ||
| line-height: normal; | ||
| text-align: center; | ||
| font-weight: normal; | ||
| text-decoration: none; | ||
| } |
There was a problem hiding this comment.
Consolidating input styles with text-align: center degrades UX for text inputs.
The rule applies text-align: center to all input types (.input-field, .select-field, .toggle-button, .button, .color-select-field). Text inputs and select fields should use left alignment for standard UX — center-aligned text in input fields is unexpected and breaks accessibility conventions.
Consider splitting the common base styles from alignment-specific rules:
.toggle-button,
.input-field,
.select-field,
.button,
.color-select-field {
box-sizing: border-box;
padding: 0.4em 0.5em;
border: 1px solid var(--border-color);
border-radius: 0.2em;
background: var(--base-color, #FFF);
color: var(--text-color, #555555);
font-size: 1rem;
font-family: inherit;
line-height: normal;
- text-align: center;
font-weight: normal;
text-decoration: none;
}
+ .toggle-button,
+ .button,
+ .color-select-field {
+ text-align: center;
+ }🤖 Prompt for AI Agents
In src/views/options/layout.css around lines 247 to 265, the shared rule group
applies text-align: center to inputs and selects which degrades UX; remove
text-align from the common selector block and split the styles so the shared
base (box-sizing, padding, border, background, color, font, line-height, etc.)
stays on .toggle-button, .input-field, .select-field, .button,
.color-select-field while adding a separate alignment-specific rule that applies
text-align: center only to controls that should be centered (e.g., .button and
.toggle-button) and text-align: left for text inputs and selects (.input-field,
.select-field, .color-select-field) to restore standard left-aligned input
behavior.
| * { | ||
| vertical-align: middle; | ||
| } |
There was a problem hiding this comment.
Overly broad wildcard selector inside .button may cause unintended styling.
The nested rule * { vertical-align: middle; } applies to all descendants of .button, including nested form controls, text nodes, and potentially nested components. This is too permissive and could cause unexpected layout shifts or styling conflicts.
Target specific child elements instead:
.button {
min-width: 9em;
cursor: pointer;
- * {
+ button > *,
+ .button-content {
vertical-align: middle;
}Alternatively, if the goal is to center inline content, use flexbox on the button itself:
.button {
min-width: 9em;
cursor: pointer;
+ display: inline-flex;
+ align-items: center;
+ justify-content: center;
- * {
- vertical-align: middle;
- }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/views/options/layout.css around lines 359-361, the nested rule ".button *
{ vertical-align: middle; }" is too broad and can affect unrelated descendants;
remove that wildcard rule and either (A) replace it with targeted child
selectors such as ".button img, .button svg, .button input, .button select,
.button .icon, .button span" and set vertical-align only on those, or (B) switch
the .button to use flexbox (e.g., set display: inline-flex or flex and use
align-items:center; justify-content:center) to center inline content without
applying styles to all descendants. Ensure you delete the wildcard rule and add
the chosen replacement in the same location.
…on chain The reason is that it otherwise causes unpredictable behaviour when calling .cancel() inside such a callback. For example registering code is executed after the .cancel() code
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/controllers/mouse-gesture-controller.mjs (1)
92-95:hasEventListenerdoesn't return the result.The method calls
.has()but doesn't return the boolean result, making it useless for callers.function hasEventListener (event, callback) { // if event exists check for listener - if (event in events) events[event].has(callback); + if (event in events) return events[event].has(callback); + return false; };src/core/bundle/content.bundle.js (1)
539-547:ConfigManager.hasEventListenerdoesn't return the result.Same issue as
BaseEventListener- the method calls.has()but doesn't return the boolean result.hasEventListener (event, callback) { if (!this._events.hasOwnProperty(event)) { throw "The first argument is not a valid event."; } if (typeof callback !== "function") { throw "The second argument must be a function."; } - this._events[event].has(callback); + return this._events[event].has(callback); }
♻️ Duplicate comments (3)
src/core/models/command-stack.mjs (1)
90-95: Critical: Iterator lacksflatMapmethod (duplicate finding).This issue was already flagged in a previous review. The
permissionsgetter callsflatMaponthis.commands, which returns an iterator from line 53. Iterators don't have aflatMapmethod, causing a runtime TypeError.Fix by using
this.#commandsdirectly or converting the iterator to an array:get permissions() { - return new Set(this.commands.flatMap(c => c.permissions)); + return new Set(this.#commands.flatMap(c => c.permissions)); }src/core/bundle/content.bundle.js (2)
915-919:BaseEventListener.hasEventListenerdoesn't return the result.This issue was already flagged in a previous review. The method needs to return the boolean result from
.has().
1161-1164: Missing return in bundledhasEventListener$2.This is the bundled version of the bug flagged in
mouse-gesture-controller.mjs. Fix the source file and rebuild.
🧹 Nitpick comments (2)
src/views/options/components/gesture-card/gesture-card.mjs (1)
80-85: Minor:setTimeoutcallback may fire after disconnect, but handled safely.The
setTimeoutin#handlePointerentercould fire after the component is disconnected. The:hovercheck and optional chaining (this.#patternElement?.playDemo()) provide adequate protection. Consider storing and clearing the timeout ID indisconnectedCallbackfor cleaner resource management, though this is optional given the safeguards.src/views/options/components/pattern-recorder/pattern-recorder.mjs (1)
126-126: Consider using strict equality.Using
== undefinedcatches bothnullandundefined. If that's intentional, consider making it explicit withthis.#pattern == null || this.#pattern === undefinedor just===if onlyundefinedis expected.- if (this.#pattern == undefined) { + if (this.#pattern === undefined) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/_locales/en/messages.json(6 hunks)src/core/bundle/content.bundle.js(11 hunks)src/core/controllers/mouse-gesture-controller.mjs(4 hunks)src/core/models/command-stack.mjs(1 hunks)src/views/options/components/command-card/command-card.mjs(1 hunks)src/views/options/components/command-picker/command-picker.mjs(1 hunks)src/views/options/components/command-picker/layout.css(1 hunks)src/views/options/components/command-stacker/command-stacker.mjs(1 hunks)src/views/options/components/command-stacker/layout.css(1 hunks)src/views/options/components/gesture-card/gesture-card.mjs(1 hunks)src/views/options/components/pattern-preview/pattern-preview.mjs(1 hunks)src/views/options/components/pattern-recorder/layout.css(1 hunks)src/views/options/components/pattern-recorder/pattern-recorder.mjs(1 hunks)src/views/options/index.html(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/views/options/index.html
🧰 Additional context used
🧬 Code graph analysis (8)
src/views/options/components/pattern-preview/pattern-preview.mjs (1)
src/views/shared/commons.mjs (1)
Build(22-38)
src/core/models/command-stack.mjs (1)
src/core/models/commands.mjs (9)
commands(2672-2674)command(2701-2701)index(28-28)index(65-65)index(1357-1357)index(1407-1407)index(1745-1745)index(2013-2013)returnValue(2702-2702)
src/views/options/components/command-picker/command-picker.mjs (1)
src/views/shared/commons.mjs (1)
Build(22-38)
src/core/controllers/mouse-gesture-controller.mjs (3)
src/core/bundle/content.bundle.js (1)
events(1823-1826)src/core/controllers/rocker-gesture-controller.mjs (1)
events(89-92)src/core/controllers/wheel-gesture-controller.mjs (1)
events(106-109)
src/views/options/components/command-card/command-card.mjs (1)
src/views/shared/commons.mjs (2)
fetchHTMLAsFragment(45-52)Build(22-38)
src/views/options/components/gesture-card/gesture-card.mjs (1)
src/views/shared/commons.mjs (1)
Build(22-38)
src/views/options/components/pattern-recorder/pattern-recorder.mjs (2)
src/views/shared/commons.mjs (1)
Build(22-38)src/core/utils/pattern-constructor.mjs (1)
PatternConstructor(8-126)
src/core/bundle/content.bundle.js (1)
src/core/utils/commons.mjs (5)
isURL(54-62)isObject(5-7)i(144-144)url(81-81)url(101-101)
🪛 Biome (2.1.2)
src/views/options/components/command-picker/layout.css
[error] 83-83: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
min-width is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
🔇 Additional comments (24)
src/_locales/en/messages.json (7)
292-332: Data management section: new labels and descriptions.The structured addition of data-related locale keys (dataLabel* and dataDescription* entries) aligns well with the new data management features. These follow the existing naming conventions and include both message and description fields. The entries appear complete and consistent.
355-390: Data notification keys: new additions for restore, upload, download, and reset operations.Comprehensive set of notification keys added to support data management workflows. All entries include both message and description fields. The keys follow a consistent naming pattern (
data[Operation]Notification*), making them maintainable.
1624-1645: New gesture form validation keys.Four new validation message keys added (
gestureFormValidationMissingGesture,gestureFormValidationInvalidGesture,gestureFormValidationMissingCommand,gestureFormValidationInvalidCommandSetting). The last key includes a placeholder for$COMMAND_NAME$, properly defined at lines 1640–1643 with content "$1" and example "New Tab". These keys enable user-facing error messages for form validation.
1765-1810: New popup-related keys: permissions, exclusions, and settings.Six new keys added to support popup UI enhancements:
popupProhibitedPageWarning(line 1765): warning for prohibited pagespopupMissingPermissionButtonand tooltip (lines 1769–1776): permission request UIpopupExclusionsToggleButtonand tooltips (lines 1777–1806): domain-level exclusion toggles with placeholder$DOMAIN$popupOpenSettingsButton(lines 1807–1809): settings accessAll entries are properly formatted with message and description fields. The exclusion-related keys include a
$DOMAIN$placeholder with example value. These additions support the new per-site gesture control feature mentioned in the PR objectives.
1597-1612: Gesture popup label and description renames.Keys renamed from
gesturePopupLabelCommandandgesturePopupLabelGestureDirectionstogesturePopupLabelMouseMovementandgesturePopupLabelCommandsrespectively. The description at lines 1609–1612 is updated to reference "main command name" instead of the original description. Verify all references to the old keys in UI code are updated.
1520-1565: Command picker placeholder argument order is correct.The
commandPickerAlternativeCommandDescriptionkey uses properly ordered placeholder arguments. Insrc/views/options/components/command-stacker/command-stacker.mjs, the message is called with[currentElement.command.label, previousElement.command.label], which correctly maps to$1(this_command) and$2(previous_command) respectively, aligning with the message template.
334-353: Remove old data action keys from all locale files.The old keys
aboutBackup,aboutRestore, andaboutResetare no longer referenced in the application code, but they remain defined in all locale files (src/_locales/*/messages.json). Remove these keys from all language variants to clean up unused message definitions.src/views/options/components/command-stacker/layout.css (1)
1-30: LGTM!The CSS structure and styling are clean and well-organized. The use of CSS variables with fallbacks and modern nesting syntax is appropriate.
src/views/options/components/command-picker/command-picker.mjs (1)
1-198: LGTM!The CommandPicker implementation is well-structured with proper permission checks, filtering logic, and UI state management. The keyword-based filtering and permission request flow are implemented correctly.
src/views/options/components/command-card/command-card.mjs (1)
1-162: LGTM!The CommandCard component is well-implemented with proper async template loading, setting initialization, and event handling. The stop propagation on the remove button (line 126) correctly prevents unwanted collapsing behavior.
src/core/controllers/mouse-gesture-controller.mjs (2)
246-260: LGTM!Storing
oldStateandoldBufferbeforereset()and conditionally dispatching 'end' callbacks only when the gesture wasACTIVEis a proper fix. This prevents spurious 'end' events from firing when the gesture was still inPENDINGorABORTEDstate.
235-239: LGTM!Setting
state = ABORTEDbefore dispatching 'abort' callbacks ensures listeners see the correct internal state when invoked.src/views/options/components/pattern-preview/pattern-preview.mjs (2)
50-57: LGTM!Good defensive coding using optional chaining (
this.#svgElement?.classList) in theanimationendcallback to handle the case where the component might be disconnected during animation. The{ once: true }option ensures proper cleanup.
9-46: LGTM!The component follows web component best practices: proper shadow DOM usage, cleanup in
disconnectedCallback, and the pattern setter correctly checksisConnectedbefore updating the DOM.src/views/options/components/gesture-card/gesture-card.mjs (1)
17-32: LGTM!The constructor properly initializes private fields and attaches the shadow DOM. The
pointerenterlistener binding and pattern pass-through are correctly implemented.src/core/bundle/content.bundle.js (3)
964-1071: LGTM!The
ExclusionServiceimplementation is well-structured:
- Proper extension of
BaseEventListenerfor event management- Storage change detection with array comparison
- Correct glob-to-regex conversion escaping special characters
- Clean
dispose()method for resource cleanup
2667-2679: LGTM!The initialization pattern using
Promise.allto wait for bothConfig.loadedandExclusions.loadedbefore callingmain()is correct. Both services add change listeners to trigger reconfiguration when settings or exclusions are modified.
2903-2957: LGTM!The
main()function properly usesExclusions.isEnabledFor(window.location.href)to gate gesture controller enablement. The synchronous signature is appropriate since no async operations are performed within the function.src/views/options/components/pattern-recorder/layout.css (1)
1-37: LGTM!Clean CSS layout using modern features (CSS nesting, grid, CSS variables). The stacked grid layout (
grid-area: 1/1) for canvas and pattern preview is an elegant approach. CSS nesting is well-supported in Firefox 117+, which is appropriate for this Firefox extension.src/views/options/components/pattern-recorder/pattern-recorder.mjs (5)
18-51: LGTM! Clean constructor and field declarations.The use of form-associated custom elements, private fields, and binding handlers in the constructor follows best practices.
100-120: LGTM! Proper form integration.The
formResetCallbackand property accessors correctly implement the form-associated custom element API, with appropriateisConnectedguards.
222-229: Verify browser support for iterator helper.flatMap()on iterators.Lines 223-229 use
events.values().flatMap(), which also depends on the Iterator Helpers proposal. The same browser support concerns apply as noted for.drop().If iterator helpers are unsupported, refactor to use traditional array methods:
- events - .values() - .flatMap(event => { + events.flatMap(event => {(Since
eventsis already an array,.values()is unnecessary here—events.flatMap()works directly.)
172-192: No action needed. Iterator helper methods (.drop(),.flatMap()) are now widely supported across all mainstream browsers as of 2025 and require no polyfill or transpilation for modern WebExtension environments.
77-82: The<pattern-recorder>component is never instantiated in the application. The gesture popup UI usesPatternPreviewcomponents instead (notPatternRecorder). While the component exports thePatternRecorderclass and registers it as a custom element, it is dead code and its lifecycle methods—including theMouseGestureController.enable()/disable()calls—are never executed. The concern about singleton conflicts does not apply here.Likely an incorrect or invalid review comment.
| "navigationData": { | ||
| "message": "Data", | ||
| "description": "Data" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "navigationExclusions" --type=js --type=ts --type=json --type=htmlRepository: Robbendebiene/Gesturefy
Length of output: 2102
🏁 Script executed:
rg -n "navigationData|navigationExclusions" --type=js --type=ts --type=htmlRepository: Robbendebiene/Gesturefy
Length of output: 180
Navigation key rename from navigationExclusions to navigationData is incomplete—only the English locale was updated.
The code in src/views/options/index.html:48 references the new key navigationData, and src/_locales/en/messages.json has been updated accordingly. However, all 33 other locale files (de, fr, es_ES, it_IT, ja_JP, ko_KR, pt_BR, ru, zh_CN, zh_TW, etc.) still contain the old navigationExclusions key. This will cause the extension to fail for non-English locales. Update all locale files to use navigationData instead of navigationExclusions.
🤖 Prompt for AI Agents
In src/_locales/en/messages.json lines 40-43 and across the other locale files
under src/_locales/*/messages.json, the localization key was partially renamed
to navigationData (English only) while the other 33 locales still use
navigationExclusions; update each locale file to replace the
navigationExclusions key with navigationData preserving the existing "message"
and "description" values and JSON structure (i.e., rename the key only, do not
change the messages), then run a quick JSON lint or project build to ensure no
syntax errors.
| #filterInput { | ||
| min-width: 0; | ||
| flex: 1; | ||
| min-width: 3rem; | ||
| box-sizing: border-box; | ||
| padding: 0.5rem 0 0.5rem 1rem; | ||
| font-size: inherit; | ||
| font-family: inherit; | ||
| background: none; | ||
| color: inherit; | ||
| font-weight: normal; | ||
| } |
There was a problem hiding this comment.
Remove duplicate min-width property.
Lines 81 and 83 both declare min-width. The second declaration (3rem) overrides the first (0), making line 81 redundant.
Remove the duplicate:
#filterInput {
- min-width: 0;
flex: 1;
min-width: 3rem;
box-sizing: border-box;Based on static analysis hints from Biome.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #filterInput { | |
| min-width: 0; | |
| flex: 1; | |
| min-width: 3rem; | |
| box-sizing: border-box; | |
| padding: 0.5rem 0 0.5rem 1rem; | |
| font-size: inherit; | |
| font-family: inherit; | |
| background: none; | |
| color: inherit; | |
| font-weight: normal; | |
| } | |
| #filterInput { | |
| flex: 1; | |
| min-width: 3rem; | |
| box-sizing: border-box; | |
| padding: 0.5rem 0 0.5rem 1rem; | |
| font-size: inherit; | |
| font-family: inherit; | |
| background: none; | |
| color: inherit; | |
| font-weight: normal; | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 83-83: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
min-width is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
🤖 Prompt for AI Agents
In src/views/options/components/command-picker/layout.css around lines 80 to 91,
there are two min-width declarations on #filterInput (min-width: 0 and
min-width: 3rem); remove the redundant min-width: 0 line so only the intended
min-width: 3rem remains, ensuring no other duplicate properties are left.
| connectedCallback() { | ||
| this.shadowRoot.append( | ||
| Build('link', { | ||
| rel: 'stylesheet', | ||
| href: import.meta.resolve('./layout.css'), | ||
| }), | ||
| this.#commandStackList = Build('orderable-collection', { | ||
| id: 'commandStackList', | ||
| }, (e) => { | ||
| e.addEventListener('orderend', this.#handleCommandReorder.bind(this)); | ||
| }, | ||
| ...this.#commandStack.commands.map((c) => { | ||
| const ele = new CommandCard( | ||
| c.clone(), true, this.#handleCommandRemoval.bind(this), | ||
| ); | ||
| ele.draggable = true; | ||
| return ele; | ||
| }) | ||
| ), | ||
| this.#commandPicker = Build('command-picker', {}, (ele) => { | ||
| ele.addEventListener('selection', this.#handleCommandSelection.bind(this)); | ||
| }), | ||
| ); | ||
|
|
||
| this.#updateButtonLabel(); | ||
| this.#updateCommandHintTexts(); | ||
| this.#updateFormValidity(); | ||
| } |
There was a problem hiding this comment.
Critical: Calling .map() on iterator will throw TypeError.
Line 43 calls .map() on this.#commandStack.commands, which returns an iterator (see command-stack.mjs line 53). Iterators don't have a .map() method.
Convert the iterator to an array before mapping:
this.#commandStackList = Build('orderable-collection', {
id: 'commandStackList',
}, (e) => {
e.addEventListener('orderend', this.#handleCommandReorder.bind(this));
},
- ...this.#commandStack.commands.map((c) => {
+ ...Array.from(this.#commandStack.commands).map((c) => {
const ele = new CommandCard(
c.clone(), true, this.#handleCommandRemoval.bind(this),
);
ele.draggable = true;
return ele;
})
),🤖 Prompt for AI Agents
In src/views/options/components/command-stacker/command-stacker.mjs around lines
32 to 59, the code calls .map() directly on this.#commandStack.commands which is
an iterator and will throw a TypeError; convert the iterator to an array (e.g.,
Array.from(this.#commandStack.commands) or [...this.#commandStack.commands])
before calling .map(), preserving the existing mapping logic and event bindings
so the rest of the connectedCallback remains unchanged.
| set commandStack(stack) { | ||
| this.#commandStack = stack; | ||
| this.#commandStackList.replaceChildren( | ||
| ...stack.commands.map((c) => { | ||
| const ele = new CommandCard( | ||
| c.clone(), true, this.#handleCommandRemoval.bind(this), | ||
| ); | ||
| ele.draggable = true; | ||
| return ele; | ||
| }) | ||
| ); | ||
| this.#updateButtonLabel(); | ||
| this.#updateCommandHintTexts(); | ||
| this.#updateFormValidity(); | ||
| } |
There was a problem hiding this comment.
Critical: Same iterator issue in setter.
Line 76 has the same issue as the connectedCallback - calling .map() on an iterator.
Apply the same fix:
set commandStack(stack) {
this.#commandStack = stack;
this.#commandStackList.replaceChildren(
- ...stack.commands.map((c) => {
+ ...Array.from(stack.commands).map((c) => {
const ele = new CommandCard(
c.clone(), true, this.#handleCommandRemoval.bind(this),
);
ele.draggable = true;
return ele;
})
);🤖 Prompt for AI Agents
In src/views/options/components/command-stacker/command-stacker.mjs around lines
73 to 87, the setter uses stack.commands.map(...) but stack.commands is an
iterator not an array; replace the .map-on-iterator usage by first materializing
the iterator into an array (e.g., Array.from(stack.commands) or
[...stack.commands]) and then map over that array when building the CommandCard
elements, leaving the rest of the setter (assigning this.#commandStack,
replacing children, calling update methods) unchanged.
| this.#patternElement = Build('pattern-preview', { | ||
| classList: 'thumbnail', | ||
| pattern: this.#gesture.pattern, | ||
| }), | ||
| this.#commandElement = Build('div', { | ||
| classList: 'command', | ||
| textContent: this.#gesture.toString(), | ||
| }), | ||
| Build('button', { | ||
| classList: 'remove-button', | ||
| onclick: this.#handleRemoveButtonClick.bind(this), | ||
| }), |
There was a problem hiding this comment.
classList property assignment won't work as expected.
classList is a read-only DOMTokenList. Using Object.assign(ele, { classList: 'thumbnail' }) via the Build helper won't set the class. Use className instead.
this.#patternElement = Build('pattern-preview', {
- classList: 'thumbnail',
+ className: 'thumbnail',
pattern: this.#gesture.pattern,
}),
this.#commandElement = Build('div', {
- classList: 'command',
+ className: 'command',
textContent: this.#gesture.toString(),
}),
Build('button', {
- classList: 'remove-button',
+ className: 'remove-button',
onclick: this.#handleRemoveButtonClick.bind(this),
}),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.#patternElement = Build('pattern-preview', { | |
| classList: 'thumbnail', | |
| pattern: this.#gesture.pattern, | |
| }), | |
| this.#commandElement = Build('div', { | |
| classList: 'command', | |
| textContent: this.#gesture.toString(), | |
| }), | |
| Build('button', { | |
| classList: 'remove-button', | |
| onclick: this.#handleRemoveButtonClick.bind(this), | |
| }), | |
| this.#patternElement = Build('pattern-preview', { | |
| className: 'thumbnail', | |
| pattern: this.#gesture.pattern, | |
| }), | |
| this.#commandElement = Build('div', { | |
| className: 'command', | |
| textContent: this.#gesture.toString(), | |
| }), | |
| Build('button', { | |
| className: 'remove-button', | |
| onclick: this.#handleRemoveButtonClick.bind(this), | |
| }), |
🤖 Prompt for AI Agents
In src/views/options/components/gesture-card/gesture-card.mjs around lines 42 to
53, the Build helper is assigning classList as a string which won't work because
classList is a read-only DOMTokenList; replace those property assignments to use
className (e.g. className: 'thumbnail', className: 'command', className:
'remove-button') so the classes are actually set on the created elements and
leave onclick/textContent/pattern assignments unchanged.
| const gesturePathLength = patternPathElement.getTotalLength(); | ||
| const svgElement = document.createElementNS('http://www.w3.org/2000/svg', 'svg'); | ||
| // scales the svg elements to always fit the svg canvas | ||
| svgElement.setAttribute('viewBox', `${viewBoxX} ${viewBoxY} ${viewBoxWidth} ${viewBoxHeight}`); | ||
| // add path length and scale as css variables for animations and styling | ||
| svgElement.style.setProperty('--pathLength', gesturePathLength); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd src/views/options/components/pattern-preview && cat -n pattern-preview.mjs | sed -n '60,105p'Repository: Robbendebiene/Gesturefy
Length of output: 2343
🌐 Web query:
SVG getTotalLength() detached element Firefox behavior compatibility
💡 Result:
Short answer: Browser behavior is inconsistent — Firefox only computes getTotalLength() for rendered/attached SVG geometry (it can throw “non‑rendered element”), while other engines (Chromium/WebKit) have different/unstable behavior. Call getTotalLength() only after the element is inserted into a rendered document (or use an attached offscreen-but-rendered SVG as a measurement host). Sources: MDN (getTotalLength), a StackOverflow report of the error, and Mozilla bug/WPT notes on non‑rendered/detached SVG differences. [1][2][3]
References
- MDN: SVGGeometryElement.getTotalLength(). [1]
- StackOverflow: "Failed to execute 'getTotalLength' … This element is non-rendered element." [2]
- Mozilla Bug/WPT discussion re: detached/non‑rendered SVG behavior and cross‑browser differences. [3]
getTotalLength() will throw an error on detached SVG elements in Firefox.
At line 94, getTotalLength() is called on patternPathElement before it is attached to a rendered document. Firefox throws a "non-rendered element" error in this scenario, while other browsers may return 0 or incorrect values. The element must be inserted into the live DOM or an offscreen-but-rendered SVG context before calling getTotalLength().
Move the getTotalLength() call to after the SVG is appended to the document, or use a temporary rendered SVG container for measurement.
🤖 Prompt for AI Agents
In src/views/options/components/pattern-preview/pattern-preview.mjs around lines
94 to 99, calling patternPathElement.getTotalLength() on a detached SVG element
will throw in Firefox; move the measurement to after the SVG is inserted into a
rendered document (or temporarily append the SVG to an offscreen but rendered
container) so getTotalLength() runs on a rendered element, then read
gesturePathLength and set the CSS variable; ensure the SVG is removed from the
temporary container if using one.
| #fitPathToViewBox(path, viewBoxX, viewBoxY, viewBoxWidth, viewBoxHeight) { | ||
| // find min/max values for both axes | ||
| let minX = Infinity, minY = Infinity; | ||
| let maxX = -Infinity, maxY = -Infinity; | ||
| for (const {x, y} of path) { | ||
| minX = Math.min(minX, x); | ||
| minY = Math.min(minY, y); | ||
| maxX = Math.max(maxX, x); | ||
| maxY = Math.max(maxY, y); | ||
| } | ||
| // calculate original dimensions and scaling factors | ||
| const width = maxX - minX; | ||
| const height = maxY - minY; | ||
| // uniform scaling factor (maintain aspect ratio) | ||
| const scale = Math.min( | ||
| viewBoxWidth / width, | ||
| viewBoxHeight / height, | ||
| ); | ||
| // calculate the translation needed to center the path | ||
| const offsetX = viewBoxX + (viewBoxWidth - width * scale) / 2; | ||
| const offsetY = viewBoxY + (viewBoxHeight - height * scale) / 2; | ||
| // scale and translate all points | ||
| for (const point of path) { | ||
| point.x = (point.x - minX) * scale + offsetX; | ||
| point.y = (point.y - minY) * scale + offsetY; | ||
| } | ||
| return path; | ||
| } |
There was a problem hiding this comment.
Potential division by zero when pattern has single point or collinear points.
If width or height is 0 (e.g., single-point pattern or all points on a horizontal/vertical line), Math.min(viewBoxWidth / 0, ...) yields Infinity, causing incorrect scaling.
#fitPathToViewBox(path, viewBoxX, viewBoxY, viewBoxWidth, viewBoxHeight) {
// find min/max values for both axes
let minX = Infinity, minY = Infinity;
let maxX = -Infinity, maxY = -Infinity;
for (const {x, y} of path) {
minX = Math.min(minX, x);
minY = Math.min(minY, y);
maxX = Math.max(maxX, x);
maxY = Math.max(maxY, y);
}
// calculate original dimensions and scaling factors
const width = maxX - minX;
const height = maxY - minY;
+ // handle edge case where width or height is 0
+ if (width === 0 && height === 0) {
+ // single point - center it
+ for (const point of path) {
+ point.x = viewBoxX + viewBoxWidth / 2;
+ point.y = viewBoxY + viewBoxHeight / 2;
+ }
+ return path;
+ }
// uniform scaling factor (maintain aspect ratio)
const scale = Math.min(
- viewBoxWidth / width,
- viewBoxHeight / height,
+ width > 0 ? viewBoxWidth / width : Infinity,
+ height > 0 ? viewBoxHeight / height : Infinity,
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #fitPathToViewBox(path, viewBoxX, viewBoxY, viewBoxWidth, viewBoxHeight) { | |
| // find min/max values for both axes | |
| let minX = Infinity, minY = Infinity; | |
| let maxX = -Infinity, maxY = -Infinity; | |
| for (const {x, y} of path) { | |
| minX = Math.min(minX, x); | |
| minY = Math.min(minY, y); | |
| maxX = Math.max(maxX, x); | |
| maxY = Math.max(maxY, y); | |
| } | |
| // calculate original dimensions and scaling factors | |
| const width = maxX - minX; | |
| const height = maxY - minY; | |
| // uniform scaling factor (maintain aspect ratio) | |
| const scale = Math.min( | |
| viewBoxWidth / width, | |
| viewBoxHeight / height, | |
| ); | |
| // calculate the translation needed to center the path | |
| const offsetX = viewBoxX + (viewBoxWidth - width * scale) / 2; | |
| const offsetY = viewBoxY + (viewBoxHeight - height * scale) / 2; | |
| // scale and translate all points | |
| for (const point of path) { | |
| point.x = (point.x - minX) * scale + offsetX; | |
| point.y = (point.y - minY) * scale + offsetY; | |
| } | |
| return path; | |
| } | |
| #fitPathToViewBox(path, viewBoxX, viewBoxY, viewBoxWidth, viewBoxHeight) { | |
| // find min/max values for both axes | |
| let minX = Infinity, minY = Infinity; | |
| let maxX = -Infinity, maxY = -Infinity; | |
| for (const {x, y} of path) { | |
| minX = Math.min(minX, x); | |
| minY = Math.min(minY, y); | |
| maxX = Math.max(maxX, x); | |
| maxY = Math.max(maxY, y); | |
| } | |
| // calculate original dimensions and scaling factors | |
| const width = maxX - minX; | |
| const height = maxY - minY; | |
| // handle edge case where width or height is 0 | |
| if (width === 0 && height === 0) { | |
| // single point - center it | |
| for (const point of path) { | |
| point.x = viewBoxX + viewBoxWidth / 2; | |
| point.y = viewBoxY + viewBoxHeight / 2; | |
| } | |
| return path; | |
| } | |
| // uniform scaling factor (maintain aspect ratio) | |
| const scale = Math.min( | |
| width > 0 ? viewBoxWidth / width : Infinity, | |
| height > 0 ? viewBoxHeight / height : Infinity, | |
| ); | |
| // calculate the translation needed to center the path | |
| const offsetX = viewBoxX + (viewBoxWidth - width * scale) / 2; | |
| const offsetY = viewBoxY + (viewBoxHeight - height * scale) / 2; | |
| // scale and translate all points | |
| for (const point of path) { | |
| point.x = (point.x - minX) * scale + offsetX; | |
| point.y = (point.y - minY) * scale + offsetY; | |
| } | |
| return path; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/views/options/layout.css (1)
462-474: Add accessibility label to the icon-only search toggle button.The search toggle button at
src/views/options/fragments/gestures.incis missing an accessibility label. The button element<button id="gestureSearchToggleButton" class="gl-search-input-toggle" type="button"></button>has noaria-label,title, or visible text content. Add either anaria-label="Toggle search"attribute or wrap the icon content with accessible text for screen reader users.
🤖 Fix all issues with AI agents
In @src/core/models/command-stack.mjs:
- Around line 90-95: The permissions getter calls flatMap on this.commands which
is an iterator (from Array.values()) and causes a runtime error; fix it by
materializing the iterator into an array before using flatMap (e.g., use
Array.from(this.commands) or [...this.commands]) or replace flatMap with a loop
that collects permissions into a Set; update the permissions getter to use the
chosen approach so it returns new Set(Array.from(this.commands).flatMap(c =>
c.permissions)) (or equivalent).
- Around line 16-25: In CommandStack.fromJSON, validate each array item before
dynamically constructing with Commands[item.name]: ensure json is an array, each
item has a non-empty string name, Commands has an entry for that name and that
entry is a constructor/function, and provide a safe default for missing settings
(e.g., {}). If any validation fails, throw a clear error mentioning the failing
item and reason; otherwise instantiate via new Commands[item.name](settings).
Refer to CommandStack.fromJSON, Commands and item.name/item.settings when making
these checks.
In @src/views/options/components/pattern-recorder/pattern-recorder.mjs:
- Around line 216-235: The code in #handleGestureEnd uses
events.values().flatMap(), which relies on iterator helpers not supported in
older browsers; replace that chain with standard Array methods by converting the
events iterable to an array (e.g., Array.from(events)) and then use map + flat
(or a single reduce/concat) to expand coalesced events before calling
patternConstructor.addPoint; keep the existing logic of calling
event.getCoalescedEvents?.() and falling back to [event], but ensure the
conversion uses Array.from(events) and standard Array.prototype methods rather
than iterator helpers.
- Around line 78-84: The component calls MouseGestureController.enable() in
connected logic but unconditionally calls MouseGestureController.disable() in
disconnectedCallback, which breaks other PatternRecorder instances; fix by
adding reference counting for controller usage: increment a shared counter when
PatternRecorder connects (e.g., in connectedCallback where you add listeners and
call MouseGestureController.enable()) and decrement it in disconnectedCallback
(where you remove listeners), and only call MouseGestureController.disable()
when the counter reaches zero; store the counter as a static property on the
PatternRecorder class or a module-level variable and keep using the existing
handler names (#gestureRegisterHandler, #gestureStartHandler,
#gestureUpdateHandler, #gestureAbortHandler, #gestureEndHandler) so listener
add/remove calls remain unchanged.
- Around line 173-193: The iterator usage events.values().drop(1) in the private
method #handleGestureStart is not widely supported; replace it by converting
events to a real array and slicing off the first element (e.g.,
Array.from(events).slice(1) or events.slice(1) if events is already an Array)
and then iterate that array for the lineTo calls, ensuring the logic still uses
firstEvent and lastEvent as before and preserving the subsequent
beginPath/moveTo behavior.
In @src/views/options/fragments/gestures.inc:
- Around line 17-18: The save button(s) inside the form with id
"gesturePopupForm" currently rely on the default button type (submit) which can
cause unintended dialog-closing due to method="dialog"; update the save button
elements (the button(s) referenced around lines 17 and 36-38 in this diff) to
explicitly declare type="button" if the save is handled by JS click handlers, or
type="submit" (and include a value attribute) if you intend a form
submit-to-close flow—pick the behavior matching the existing JS and make the
type explicit.
- Around line 16-20: The dialog with id "gesturePopup" lacks an accessible name
and the close button is icon-only; update the markup so the title container with
id "gesturePopupHeading" becomes a proper heading element (e.g., h2/h3) and
reference it from the dialog using aria-labelledby="gesturePopupHeading" (or
supply aria-label on the dialog if you prefer a string), and add an accessible
label to the close button (e.g., aria-label="Close" driven by your localization
mechanism) while keeping the existing command="request-close" attribute.
- Around line 16-21: The dialog close button lacks a JS fallback for older
Firefox; add a click event listener on the element with class
"popup-close-button" that invokes
document.getElementById('gesturePopup').close() (or calls gesturePopup.close()
if you store the element) so the button closes the dialog when
command="request-close" is unsupported; attach the listener after the DOM
element exists (e.g., on DOMContentLoaded or inline script) and target the
specific "popup-close-button" and "gesturePopup" ids to preserve behavior across
Firefox 128–143.
In @src/views/options/gestures.mjs:
- Around line 203-209: The code assumes morph(...) returns a defined from
animation and does await from.finished which can throw if from is undefined;
update the gesture popup teardown in gestures.mjs to defensively check for from
before awaiting (e.g., if (from && from.finished) await from.finished; else if
(to && to.finished) await to.finished) and ensure
sourceElement.style.removeProperty('z-index') and gesturePopup.close() still run
(use a finally-style flow if needed) — reference the destructuring const {from,
to} = morph(...) and the await from.finished call.
- Line 47: The code uses Iterator Helpers (calls to .toArray()) which are
unsupported on the declared minimum Firefox 128; replace all .toArray() usages
with compatible alternatives (e.g., use Array.from(...) or spread into an array)
so the calls in gestures.mjs — specifically the expression using
Gestures.keys().toArray().reverse() (line with gestureList.append), and the
other two .toArray() usages referenced around lines 84 and 136 — become
standard-array conversions before calling reverse/append/iteration;
alternatively, if supporting Firefox 131+ is acceptable, update manifest.json
strict_min_version to "131.0".
In @src/views/options/layout.css:
- Around line 545-561: The .popup-close-button uses CSS-generated "X" content
which screen readers may not announce; update the actual <button> element using
that class (popup-close-button) to include an accessible label such as
aria-label="Close" (or a localized equivalent) so assistive tech can describe
its purpose; ensure you add the attribute on the button element where
class="popup-close-button" (optionally also add a title attribute or use
aria-labelledby if you prefer a visible label).
- Around line 279-291: The CSS currently uses __MSG_toggleButtonOff__ and
__MSG_toggleButtonOn__ which Firefox won't replace; remove those raw strings and
switch to using data attributes plus dynamic JS: change the ::after rules (the
selectors using ::after and &:checked ::after) to use content: attr(data-label);
add data-label-off and data-label-on attributes on the toggle element in HTML
(or a single data-label you update), and in the code that renders/initializes
the toggle call browser.i18n.getMessage("toggleButtonOff") and
getMessage("toggleButtonOn") to populate the element's data-label (and update it
on change/checked events) so the ::after pseudoelement shows localized text.
Ensure the same element referenced by the CSS selectors (&::after / &:checked)
is the one you set attributes on and update the attribute when the checked state
changes.
🧹 Nitpick comments (12)
src/_locales/en/messages.json (1)
1773-1802: Consider revising the toggle button text to better reflect state.The
popupExclusionsToggleButtonuses the same text "Allow gestures on$DOMAIN$ " regardless of state, relying only on tooltips to convey whether gestures are currently enabled or disabled. This could confuse users who don't hover to read tooltips (especially on touch devices).Consider using state-specific text, such as:
- When OFF: "Enable gestures on
$DOMAIN$ "- When ON: "Disable gestures on
$DOMAIN$ "Or indicate current state:
- When OFF: "Gestures disabled on
$DOMAIN$ "- When ON: "Gestures enabled on
$DOMAIN$ "This would make the toggle's state immediately clear without requiring tooltip interaction.
src/core/models/command-stack.mjs (3)
11-14: Throw Error objects instead of strings.Throwing strings is a code smell. Throw proper Error objects for better stack traces and error handling.
♻️ Proposed fix
constructor (commands = []) { - if (!Array.isArray(commands)) throw 'The first argument must be an array of commands.'; + if (!Array.isArray(commands)) throw new TypeError('The first argument must be an array of commands.'); this.#commands = commands; }
68-84: Add input validation to mutation methods.The mutation methods lack input validation, which could lead to:
- Invalid commands being added to the stack
- Out-of-bounds array access with invalid indices
- Runtime errors when executing invalid commands
Consider adding validation for:
addCommand: verify command is a Command instancereplaceCommand: verify index is valid and command is a Command instanceremoveCommand: verify index is validmoveCommand: verify both indices are valid♻️ Example validation for addCommand
addCommand(command) { + if (!command || typeof command.execute !== 'function') { + throw new TypeError('Invalid command: must have an execute method.'); + } this.#commands.push(command); }
56-62: Document edge case behavior for empty stacks.When the stack is empty,
firstCommandandlastCommandreturnundefined. If this is intentional, consider documenting it. Alternatively, you could returnnullexplicitly or throw an error to make the behavior clearer.src/views/options/gestures.mjs (2)
51-72: Consider extracting duplicate persistence logic.The pattern
Array.from(Gestures.values().map((g) => g.toJSON()))is repeated in all three CRUD functions. A small helper would reduce duplication.♻️ Suggested refactor
+function persistGestures() { + Config.set("Gestures", Array.from(Gestures.values().map((g) => g.toJSON()))); +} + function addGesture(gesture) { const gestureListItem = new GestureCard(gesture, {onRemove: removeGesture}); gestureListItem.addEventListener('click', onGestureCardClick); Gestures.set(gestureListItem, gesture); - Config.set("Gestures", Array.from(Gestures.values().map((g) => g.toJSON()))); + persistGestures(); animateGestureAddition(gestureListItem); } function updateGesture(gestureListItem, gesture) { gestureListItem.gesture = gesture; Gestures.set(gestureListItem, gesture); - Config.set("Gestures", Array.from(Gestures.values().map((g) => g.toJSON()))); + persistGestures(); animateGestureUpdate(gestureListItem); } function removeGesture(gestureListItem) { Gestures.delete(gestureListItem); - Config.set("Gestures", Array.from(Gestures.values().map((g) => g.toJSON()))); + persistGestures(); animateGestureRemoval(gestureListItem); }
211-223: Minor inconsistency:scaleproperty vstransform: 'scale(...)'.The closing animation uses the CSS
scaleproperty (line 215), while the opening animation usestransform: 'scale(0.8)'(line 180). Both work, but using consistent property access improves readability.♻️ Suggested consistency fix
else { await gesturePopup.animate([ { opacity: 0, - scale: 0.8, + transform: 'scale(0.8)', } ], { id: 'closing', duration: 300, easing: 'ease' }).finished; gesturePopup.close(); }src/views/options/layout.css (4)
1-3: Performance: Avoidtransition: allon universal selector.Transitioning all properties on every element can cause layout thrashing and poor performance during theme switches. Limit transitions to specific visual properties.
⚡ Proposed performance optimization
.theme-transition:root * { - transition: all .3s !important; + transition: background-color .3s, color .3s, border-color .3s, box-shadow .3s !important; }Alternatively, consider applying transitions only to elements that actually need them rather than using a universal selector.
260-261: Outdated comment reference.The comment references a 2011 article about line-height limitations in input fields. Modern browsers fully support
line-heightin text inputs, making this comment potentially misleading. Consider updating or removing it.
316-316: Consider converting fixed pixel height to rem units.The
height: 42pxis inconsistent with the rem-based spacing system used throughout the file. For consistency and scalability, consider converting to rem units.📏 Proposed consistency improvement
.color-select-field { width: 3.2rem; - height: 42px; + height: 2.625rem; padding: 0.3rem; cursor: pointer; }
509-509: Consider using moderngapproperty instead of deprecatedgrid-gap.While
grid-gapstill works, the modern CSS specification recommends usinggapwhich works for both grid and flexbox layouts.🔧 Modernize gap property
.gesture-list { display: grid; - grid-gap: 1.25rem; + gap: 1.25rem; grid-template-columns: repeat(auto-fill, minmax(170px, 1fr));#gesturePopupContent { display: grid; grid-template-areas: "a b" "c d"; grid-template-columns: 5fr minmax(17rem, 3fr); grid-template-rows: 0fr auto; - grid-gap: 1.25rem; + gap: 1.25rem; background-color: var(--base-color);Also applies to: 593-593
src/views/options/components/pattern-recorder/pattern-recorder.mjs (1)
123-142: Clarify validation logic to prevent potential null reference.Line 127 uses loose equality (
==) which matches bothundefinedandnull. However, ifthis.#patternisnull, line 133 would throw when accessing.lengthon a null value.Consider using strict equality and explicit checks for better clarity and safety.
♻️ Proposed refactor for clearer validation
- if (this.#pattern == undefined) { + if (this.#pattern === undefined || this.#pattern === null) { this.#internals.setValidity( { valueMissing: true }, browser.i18n.getMessage('gestureFormValidationMissingGesture'), ); }src/views/options/fragments/gestures.inc (1)
10-14: List semantics may regress after switching fromul/lito adiv.If the gesture entries are still conceptually a list, consider restoring list semantics via ARIA (or keep
ul/li) so screen readers preserve expectations.Proposed tweak
-<div id="gestureContainer" class="gesture-list"> +<div id="gestureContainer" class="gesture-list" role="list"> <button id="gestureAddButton" type="button" class="gl-add-button" > <locale-text key="gestureCreateNewButton"></locale-text> </button> </div>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/_locales/en/messages.jsonsrc/core/models/command-stack.mjssrc/views/options/components/pattern-recorder/pattern-recorder.mjssrc/views/options/fragments/gestures.incsrc/views/options/gestures.mjssrc/views/options/layout.csssrc/views/shared/commons.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/views/shared/commons.mjs
🧰 Additional context used
🧬 Code graph analysis (3)
src/core/models/command-stack.mjs (1)
src/core/models/commands.mjs (3)
commands(2672-2674)command(2701-2701)returnValue(2702-2702)
src/views/options/gestures.mjs (4)
src/core/models/gesture.mjs (1)
Gesture(7-79)src/views/options/components/gesture-card/gesture-card.mjs (1)
GestureCard(10-92)src/views/shared/commons.mjs (1)
morph(60-109)src/core/utils/matching-algorithms.mjs (1)
getClosestGestureByPattern(9-59)
src/views/options/components/pattern-recorder/pattern-recorder.mjs (2)
src/views/shared/commons.mjs (1)
Build(22-38)src/core/utils/pattern-constructor.mjs (1)
PatternConstructor(8-126)
🔇 Additional comments (18)
src/core/models/command-stack.mjs (3)
27-33: LGTM!The serialization logic is clean and correctly delegates to individual command
toJSONmethods.
97-107: LGTM!The execute method correctly implements the sequential execution pattern with early exit on success, and properly handles async commands.
109-114: LGTM!The clone method correctly creates a deep copy by cloning each individual command.
src/views/options/gestures.mjs (5)
78-105: LGTM!The FLIP animation pattern (cache positions → mutate DOM → calculate deltas → animate) is correctly implemented for smooth gesture list reordering.
229-244: LGTM!Search implementation correctly uses CSS class toggling for visibility and respects the current filter when items are updated.
284-308: LGTM!Event handlers correctly manage the popup lifecycle and gesture CRUD operations. The use of
thisinonGestureCardClickcorrectly references the GestureCard element due to addEventListener binding semantics.
345-354: Good defensive programming!The animation guard at lines 347-351 prevents race conditions when users rapidly trigger close actions. The comment explaining why a reverse animation approach wasn't taken is also helpful for future maintainers.
401-411: LGTM!Clean generator utility for sibling traversal with optional inclusion of the initial element.
src/views/options/layout.css (2)
623-623: The image resource file exists atsrc/resources/img/dashed_border_gray.pngand will be correctly bundled with the extension. The CSS path reference is valid.
149-151: The extension's minimum Firefox version is 128.0, which exceeds the CSS nesting support requirement of Firefox 117+. No browser compatibility issue exists.src/views/options/components/pattern-recorder/pattern-recorder.mjs (7)
1-7: LGTM!The imports are well-organized and necessary for the component's functionality.
19-39: LGTM!The static properties and private field declarations follow best practices for form-associated custom elements.
41-52: LGTM!The constructor properly initializes the form-associated custom element and binds event handlers.
87-99: LGTM!The cleanup logic properly removes event listeners and clears references, preventing memory leaks.
101-121: LGTM!The form reset callback and property accessors correctly manage state and conditionally trigger updates when the element is connected to the DOM.
238-238: LGTM!The custom element registration follows the standard pattern.
152-171: MouseGestureController.cancel() method exists and is properly exported.Line 156 correctly calls
MouseGestureController.cancel(), which is defined insrc/core/controllers/mouse-gesture-controller.mjs(lines 30–33) and exported in the module's default export.src/views/options/fragments/gestures.inc (1)
24-31: Both custom elements are properly form-associated and will participate in form submission.Both
<pattern-recorder>and<command-stacker>havestatic formAssociated = trueand callattachInternals()in their constructors, so they correctly integrate with the form's submit handler. No regression risk from missing pattern/commands data.
| static fromJSON(json) { | ||
| if (!Array.isArray(json)) throw 'Expects an array of JSON objects.'; | ||
| return new CommandStack( | ||
| json.map((item) => { | ||
| // note: a dedicated Command.fromJSON(item); function would easily make sense on the basic Command class, | ||
| // but this would create a cyclic import dependency between 'command.mjs' and 'commands.mjs' | ||
| return new Commands[item.name](item.settings) | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add validation to prevent runtime errors.
Line 22 uses dynamic property access without validation, which can cause runtime errors if:
item.nameis missing or undefined →Commands[undefined]failsCommands[item.name]doesn't exist → constructor call failsitem.settingsis missing → command constructor may fail
🐛 Proposed fix with validation
static fromJSON(json) {
- if (!Array.isArray(json)) throw 'Expects an array of JSON objects.';
+ if (!Array.isArray(json)) throw new TypeError('Expects an array of JSON objects.');
return new CommandStack(
json.map((item) => {
+ if (!item || typeof item.name !== 'string') {
+ throw new TypeError('Each item must have a valid "name" property.');
+ }
+ const CommandClass = Commands[item.name];
+ if (!CommandClass) {
+ throw new Error(`Unknown command: ${item.name}`);
+ }
// note: a dedicated Command.fromJSON(item); function would easily make sense on the basic Command class,
// but this would create a cyclic import dependency between 'command.mjs' and 'commands.mjs'
- return new Commands[item.name](item.settings)
+ return new CommandClass(item.settings ?? {});
})
);
}🤖 Prompt for AI Agents
In @src/core/models/command-stack.mjs around lines 16 - 25, In
CommandStack.fromJSON, validate each array item before dynamically constructing
with Commands[item.name]: ensure json is an array, each item has a non-empty
string name, Commands has an entry for that name and that entry is a
constructor/function, and provide a safe default for missing settings (e.g.,
{}). If any validation fails, throw a clear error mentioning the failing item
and reason; otherwise instantiate via new Commands[item.name](settings). Refer
to CommandStack.fromJSON, Commands and item.name/item.settings when making these
checks.
| /** | ||
| * Returns a deduplicated Set of permissions required by this command stack. | ||
| */ | ||
| get permissions() { | ||
| return new Set(this.commands.flatMap(c => c.permissions)); | ||
| } |
There was a problem hiding this comment.
Fix runtime error: flatMap cannot be called on an iterator.
Line 94 calls flatMap on this.commands, but this.commands returns an iterator (from Array.values() on line 53), and iterators don't have a flatMap method. This will cause a runtime error.
🐛 Proposed fix
get permissions() {
- return new Set(this.commands.flatMap(c => c.permissions));
+ return new Set(this.#commands.flatMap(c => c.permissions));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Returns a deduplicated Set of permissions required by this command stack. | |
| */ | |
| get permissions() { | |
| return new Set(this.commands.flatMap(c => c.permissions)); | |
| } | |
| /** | |
| * Returns a deduplicated Set of permissions required by this command stack. | |
| */ | |
| get permissions() { | |
| return new Set(this.#commands.flatMap(c => c.permissions)); | |
| } |
🤖 Prompt for AI Agents
In @src/core/models/command-stack.mjs around lines 90 - 95, The permissions
getter calls flatMap on this.commands which is an iterator (from Array.values())
and causes a runtime error; fix it by materializing the iterator into an array
before using flatMap (e.g., use Array.from(this.commands) or [...this.commands])
or replace flatMap with a loop that collects permissions into a Set; update the
permissions getter to use the chosen approach so it returns new
Set(Array.from(this.commands).flatMap(c => c.permissions)) (or equivalent).
| MouseGestureController.addEventListener("register", this.#gestureRegisterHandler); | ||
| MouseGestureController.addEventListener("start", this.#gestureStartHandler); | ||
| MouseGestureController.addEventListener("update", this.#gestureUpdateHandler); | ||
| MouseGestureController.addEventListener("abort", this.#gestureAbortHandler); | ||
| MouseGestureController.addEventListener("end", this.#gestureEndHandler); | ||
| MouseGestureController.enable(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for pattern-recorder element instantiation and usage (excluding pattern-recorder.mjs itself)
echo "=== PatternRecorder instantiation/usage ==="
rg -n 'pattern-recorder' --type=js -g '!**/pattern-recorder.mjs' | head -30
echo ""
echo "=== MouseGestureController class definition ==="
rg -n 'class MouseGestureController|MouseGestureController\s*=' --type=js -A 10Repository: Robbendebiene/Gesturefy
Length of output: 864
🏁 Script executed:
#!/bin/bash
# Search for MouseGestureController addEventListener to see how many components use it
echo "=== MouseGestureController addEventListener usage ==="
rg -n 'MouseGestureController\.addEventListener|MouseGestureController\.enable|MouseGestureController\.disable' --type=js
echo ""
echo "=== Check if MouseGestureController is a singleton/shared instance ==="
rg -n 'const.*MouseGestureController\s*=|export.*MouseGestureController' --type=jsRepository: Robbendebiene/Gesturefy
Length of output: 885
🏁 Script executed:
#!/bin/bash
# Check the pattern-recorder.mjs file more thoroughly
echo "=== Full context of pattern-recorder.mjs ==="
wc -l src/views/options/components/pattern-recorder/pattern-recorder.mjs
echo ""
echo "=== Check connectedCallback and disconnectedCallback implementation ==="
sed -n '54,90p' src/views/options/components/pattern-recorder/pattern-recorder.mjsRepository: Robbendebiene/Gesturefy
Length of output: 1448
🏁 Script executed:
#!/bin/bash
# View the full disconnectedCallback method to see if it removes listeners
sed -n '85,120p' src/views/options/components/pattern-recorder/pattern-recorder.mjsRepository: Robbendebiene/Gesturefy
Length of output: 1089
🏁 Script executed:
#!/bin/bash
# Also check if pattern-recorder is instantiated in HTML files
echo "=== Search for pattern-recorder in HTML files ==="
rg -n 'pattern-recorder' --type=html
echo ""
echo "=== Search for element instantiation via createElement ==="
rg -n 'createElement.*pattern-recorder|pattern-recorder.*createElement' --type=jsRepository: Robbendebiene/Gesturefy
Length of output: 312
Fix unbalanced enable/disable calls for shared MouseGestureController.
Event listeners are correctly added and removed, but the component calls MouseGestureController.disable() in disconnectedCallback without reference counting. If multiple PatternRecorder instances exist simultaneously, disconnecting the first instance disables the controller globally while the second instance still has active listeners, breaking the second instance.
Either ensure only one instance exists at a time, or implement reference counting for enable()/disable() calls (e.g., track how many instances are active and only disable when the last one disconnects).
🤖 Prompt for AI Agents
In @src/views/options/components/pattern-recorder/pattern-recorder.mjs around
lines 78 - 84, The component calls MouseGestureController.enable() in connected
logic but unconditionally calls MouseGestureController.disable() in
disconnectedCallback, which breaks other PatternRecorder instances; fix by
adding reference counting for controller usage: increment a shared counter when
PatternRecorder connects (e.g., in connectedCallback where you add listeners and
call MouseGestureController.enable()) and decrement it in disconnectedCallback
(where you remove listeners), and only call MouseGestureController.disable()
when the counter reaches zero; store the counter as a static property on the
PatternRecorder class or a module-level variable and keep using the existing
handler names (#gestureRegisterHandler, #gestureStartHandler,
#gestureUpdateHandler, #gestureAbortHandler, #gestureEndHandler) so listener
add/remove calls remain unchanged.
| #handleGestureStart(event, events) { | ||
| // get first event and remove it from the array | ||
| const firstEvent = events[0]; | ||
| const lastEvent = events[events.length - 1] ?? firstEvent; | ||
|
|
||
| this.#canvasContext.beginPath(); | ||
| this.#canvasContext.moveTo( | ||
| firstEvent.clientX, | ||
| firstEvent.clientY | ||
| ); | ||
| for (let event of events.values().drop(1)) this.#canvasContext.lineTo( | ||
| event.clientX, | ||
| event.clientY | ||
| ); | ||
| this.#canvasContext.stroke(); | ||
| this.#canvasContext.beginPath(); | ||
| this.#canvasContext.moveTo( | ||
| lastEvent.clientX, | ||
| lastEvent.clientY | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Firefox WebExtensions Manifest V3 minimum browser version requirements support
💡 Result:
Short answer: Firefox added baseline Manifest V3 support in Firefox 109 (Jan 2023). Subsequent MV3 enhancements landed in later releases (notably Firefox 127/128 for additional scripting/content-script MAIN‑world features). You can require a minimum Firefox version for your add-on using browser_specific_settings.gecko.strict_min_version in manifest.json. [1][2][3][4]
Sources
- Firefox 109 release notes (Manifest V3 supported). [1]
- Mozilla Add‑ons blog: Manifest V3 updates landed in Firefox 127 and 128. [2][3]
- MDN: browser_specific_settings (strict_min_version) manifest key. [4]
🌐 Web query:
ES2024 Iterator helpers drop() method browser support compatibility
💡 Result:
Short answer — partial support in modern browsers; use a polyfill for broader compatibility.
- Current support: implemented in Chromium-based browsers (Chrome 122+, Edge 122+, Chrome for Android 122+), Firefox (131+), and Safari (18.4 / iOS Safari 18.4+). See feature status and release versions. [1][2]
- Node / Deno / older browsers: not universally available — use a polyfill (es-shims / core-js) or transpile for compatibility. MDN links polyfills. [1][3]
- Baseline/widely-available timeline: feature marked "Newly available (baseline 2025)"; wider availability expected later. [2]
Sources: MDN (Iterator.prototype.drop) and Web Platform / feature status. [1][2][3]
References
[1] MDN — Iterator.prototype.drop.
[2] Web Features Explorer — Iterator methods (support matrix).
[3] core-js / es-shims polyfills referenced on MDN.
🏁 Script executed:
# Check the actual code file to understand the context
head -200 src/views/options/components/pattern-recorder/pattern-recorder.mjs | tail -50Repository: Robbendebiene/Gesturefy
Length of output: 1930
Use Array.prototype.slice() for iterator traversal to maintain browser compatibility.
Line 183 uses events.values().drop(1), which relies on Iterator helper methods from ES2024. While Firefox added Manifest V3 support in Firefox 109 (January 2023), the drop() method is only available in Firefox 131+ (October 2024), creating a 22-month compatibility gap. Similarly, this pattern requires Chrome 122+, Edge 122+, and Safari 18.4+.
Users running the extension on Firefox 109–130 with MV3 support will encounter a runtime error when recording gestures.
Proposed fix
- for (let event of events.values().drop(1)) this.#canvasContext.lineTo(
+ for (let event of events.slice(1)) this.#canvasContext.lineTo(
event.clientX,
event.clientY
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #handleGestureStart(event, events) { | |
| // get first event and remove it from the array | |
| const firstEvent = events[0]; | |
| const lastEvent = events[events.length - 1] ?? firstEvent; | |
| this.#canvasContext.beginPath(); | |
| this.#canvasContext.moveTo( | |
| firstEvent.clientX, | |
| firstEvent.clientY | |
| ); | |
| for (let event of events.values().drop(1)) this.#canvasContext.lineTo( | |
| event.clientX, | |
| event.clientY | |
| ); | |
| this.#canvasContext.stroke(); | |
| this.#canvasContext.beginPath(); | |
| this.#canvasContext.moveTo( | |
| lastEvent.clientX, | |
| lastEvent.clientY | |
| ); | |
| } | |
| #handleGestureStart(event, events) { | |
| // get first event and remove it from the array | |
| const firstEvent = events[0]; | |
| const lastEvent = events[events.length - 1] ?? firstEvent; | |
| this.#canvasContext.beginPath(); | |
| this.#canvasContext.moveTo( | |
| firstEvent.clientX, | |
| firstEvent.clientY | |
| ); | |
| for (let event of events.slice(1)) this.#canvasContext.lineTo( | |
| event.clientX, | |
| event.clientY | |
| ); | |
| this.#canvasContext.stroke(); | |
| this.#canvasContext.beginPath(); | |
| this.#canvasContext.moveTo( | |
| lastEvent.clientX, | |
| lastEvent.clientY | |
| ); | |
| } |
🤖 Prompt for AI Agents
In @src/views/options/components/pattern-recorder/pattern-recorder.mjs around
lines 173 - 193, The iterator usage events.values().drop(1) in the private
method #handleGestureStart is not widely supported; replace it by converting
events to a real array and slicing off the first element (e.g.,
Array.from(events).slice(1) or events.slice(1) if events is already an Array)
and then iterate that array for the lineTo calls, ensuring the logic still uses
firstEvent and lastEvent as before and preserving the subsequent
beginPath/moveTo behavior.
| #handleGestureEnd(event, events) { | ||
| this.#canvasContext.reset(); | ||
| // setup pattern extractor | ||
| const patternConstructor = new PatternConstructor(0.12, 10); | ||
| // gather all events in one array | ||
| // calling getCoalescedEvents for an event other then pointermove will return an empty array | ||
| // build gesture pattern | ||
| events | ||
| .values() | ||
| .flatMap(event => { | ||
| const events = event.getCoalescedEvents?.(); | ||
| // if events is null/undefined or empty (length == 0) return plain event | ||
| return (events?.length > 0) ? events : [event]; | ||
| }) | ||
| .forEach(event => patternConstructor.addPoint(event.clientX, event.clientY)); | ||
| // update current pattern | ||
| this.pattern = patternConstructor.getPattern(); | ||
| // dispatch change event | ||
| this.dispatchEvent(new CustomEvent('change')); | ||
| } |
There was a problem hiding this comment.
Replace iterator helper with compatible Array methods.
Lines 223-230 use events.values().flatMap(), which relies on ES2024 iterator helpers that are not available in older browsers. This will cause the same runtime error as in line 183.
♻️ Proposed fix using standard Array methods
- events
- .values()
- .flatMap(event => {
+ events
+ .flatMap(event => {
const events = event.getCoalescedEvents?.();
// if events is null/undefined or empty (length == 0) return plain event
return (events?.length > 0) ? events : [event];
})
.forEach(event => patternConstructor.addPoint(event.clientX, event.clientY));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #handleGestureEnd(event, events) { | |
| this.#canvasContext.reset(); | |
| // setup pattern extractor | |
| const patternConstructor = new PatternConstructor(0.12, 10); | |
| // gather all events in one array | |
| // calling getCoalescedEvents for an event other then pointermove will return an empty array | |
| // build gesture pattern | |
| events | |
| .values() | |
| .flatMap(event => { | |
| const events = event.getCoalescedEvents?.(); | |
| // if events is null/undefined or empty (length == 0) return plain event | |
| return (events?.length > 0) ? events : [event]; | |
| }) | |
| .forEach(event => patternConstructor.addPoint(event.clientX, event.clientY)); | |
| // update current pattern | |
| this.pattern = patternConstructor.getPattern(); | |
| // dispatch change event | |
| this.dispatchEvent(new CustomEvent('change')); | |
| } | |
| #handleGestureEnd(event, events) { | |
| this.#canvasContext.reset(); | |
| // setup pattern extractor | |
| const patternConstructor = new PatternConstructor(0.12, 10); | |
| // gather all events in one array | |
| // calling getCoalescedEvents for an event other then pointermove will return an empty array | |
| // build gesture pattern | |
| Array.from(events.values()) | |
| .flatMap(event => { | |
| const events = event.getCoalescedEvents?.(); | |
| // if events is null/undefined or empty (length == 0) return plain event | |
| return (events?.length > 0) ? events : [event]; | |
| }) | |
| .forEach(event => patternConstructor.addPoint(event.clientX, event.clientY)); | |
| // update current pattern | |
| this.pattern = patternConstructor.getPattern(); | |
| // dispatch change event | |
| this.dispatchEvent(new CustomEvent('change')); | |
| } |
🤖 Prompt for AI Agents
In @src/views/options/components/pattern-recorder/pattern-recorder.mjs around
lines 216 - 235, The code in #handleGestureEnd uses events.values().flatMap(),
which relies on iterator helpers not supported in older browsers; replace that
chain with standard Array methods by converting the events iterable to an array
(e.g., Array.from(events)) and then use map + flat (or a single reduce/concat)
to expand coalesced events before calling patternConstructor.addPoint; keep the
existing logic of calling event.getCoalescedEvents?.() and falling back to
[event], but ensure the conversion uses Array.from(events) and standard
Array.prototype methods rather than iterator helpers.
| <form id="gesturePopupForm" method="dialog"> | ||
| <div class="popup-head"> |
There was a problem hiding this comment.
Save button type is ambiguous; make it explicit (likely a behavior bug).
Inside a form, <button> defaults to type="submit". With method="dialog", that can close the dialog as a side effect. If saving is handled by JS on click (as the summary implies), set type="button". If you do want submit-to-close, set type="submit" (and ideally a value).
Option A: click-handled save (no implicit submit)
- <button id="gesturePopupSaveButton" class="button">
+ <button id="gesturePopupSaveButton" class="button" type="button">
<locale-text key="buttonSave"></locale-text>
</button>Option B: submit-to-close (explicit submit)
- <button id="gesturePopupSaveButton" class="button">
+ <button id="gesturePopupSaveButton" class="button" type="submit" value="save">
<locale-text key="buttonSave"></locale-text>
</button>Also applies to: 36-38
🤖 Prompt for AI Agents
In @src/views/options/fragments/gestures.inc around lines 17 - 18, The save
button(s) inside the form with id "gesturePopupForm" currently rely on the
default button type (submit) which can cause unintended dialog-closing due to
method="dialog"; update the save button elements (the button(s) referenced
around lines 17 and 36-38 in this diff) to explicitly declare type="button" if
the save is handled by JS click handlers, or type="submit" (and include a value
attribute) if you intend a form submit-to-close flow—pick the behavior matching
the existing JS and make the type explicit.
| gestureList.dataset.noResultsHint = browser.i18n.getMessage('gestureHintNoSearchResults'); | ||
| // add mouse gesture controller event listeners | ||
| mouseGestureControllerSetup(); | ||
| gestureList.append(...Gestures.keys().toArray().reverse()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for Iterator Helpers usage patterns
echo "=== Searching for .toArray() usage ==="
rg -n '\.toArray\(\)' --type=js
echo -e "\n=== Searching for manifest.json to check browser compatibility ==="
find . -name "manifest.json" -type f | head -5Repository: Robbendebiene/Gesturefy
Length of output: 193
🏁 Script executed:
#!/bin/bash
# Check for browser_minimum_version or similar in manifest
cat ./manifest.json 2>/dev/null || echo "manifest.json not found in root"Repository: Robbendebiene/Gesturefy
Length of output: 98
🌐 Web query:
Firefox Iterator Helpers support minimum version
💡 Result:
Firefox supports the synchronous Iterator Helpers starting in Firefox 131 (released October 1, 2024). [1][2]
Sources:
- Firefox 131 release notes (JavaScript changes). [1]
- MDN: Iterator.prototype.filter (Iterator helpers documentation). [2]
🏁 Script executed:
#!/bin/bash
# Read the manifest to check browser_minimum_version
cat ./src/manifest.json | head -50Repository: Robbendebiene/Gesturefy
Length of output: 1233
🏁 Script executed:
#!/bin/bash
# Search for .toArray() including .mjs files
echo "=== Searching for .toArray() in all files ==="
rg -n '\.toArray\(\)' . --type-list | grep -i mjs
rg -n '\.toArray\(\)' .
echo -e "\n=== Check specific file lines mentioned in review ==="
sed -n '47p' ./src/views/options/gestures.mjs
sed -n '82,84p' ./src/views/options/gestures.mjs
sed -n '131,136p' ./src/views/options/gestures.mjsRepository: Robbendebiene/Gesturefy
Length of output: 1290
Fix Iterator Helpers compatibility issue - incompatible with minimum Firefox version 128.
The code uses Iterator Helpers (.toArray() at lines 47, 84, and 136), which are ES2024+ features available only in Firefox 131+. However, the extension's manifest.json specifies strict_min_version: "128.0", meaning the code will fail at runtime on Firefox 128, 129, and 130.
Replace Iterator Helpers usage with compatible alternatives, such as Array.from() or [...iterator] spread syntax:
- Line 47:
Gestures.keys().toArray().reverse()→Array.from(Gestures.keys()).reverse() - Line 84:
.toArray()→ UseArray.from()or array spread - Line 136:
.toArray()→ UseArray.from()or array spread
Alternatively, increase strict_min_version to "131.0" if Firefox 128-130 support is not required.
🤖 Prompt for AI Agents
In @src/views/options/gestures.mjs at line 47, The code uses Iterator Helpers
(calls to .toArray()) which are unsupported on the declared minimum Firefox 128;
replace all .toArray() usages with compatible alternatives (e.g., use
Array.from(...) or spread into an array) so the calls in gestures.mjs —
specifically the expression using Gestures.keys().toArray().reverse() (line with
gestureList.append), and the other two .toArray() usages referenced around lines
84 and 136 — become standard-array conversions before calling
reverse/append/iteration; alternatively, if supporting Firefox 131+ is
acceptable, update manifest.json strict_min_version to "131.0".
| const {from, to} = morph(gesturePopup, sourceElement, { | ||
| duration: 300, | ||
| fade: 'from', | ||
| }); | ||
| await from.finished; | ||
| sourceElement.style.removeProperty('z-index'); | ||
| gesturePopup.close(); |
There was a problem hiding this comment.
Potential null dereference if morph doesn't return from animation.
Line 207 accesses from.finished without a null check. If morph unexpectedly returns undefined for from, this will throw a TypeError.
Consider adding defensive handling similar to line 172's pattern:
🛡️ Suggested defensive fix
const {from, to} = morph(gesturePopup, sourceElement, {
duration: 300,
fade: 'from',
});
- await from.finished;
+ await (from ?? to)?.finished;
sourceElement.style.removeProperty('z-index');
gesturePopup.close();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const {from, to} = morph(gesturePopup, sourceElement, { | |
| duration: 300, | |
| fade: 'from', | |
| }); | |
| await from.finished; | |
| sourceElement.style.removeProperty('z-index'); | |
| gesturePopup.close(); | |
| const {from, to} = morph(gesturePopup, sourceElement, { | |
| duration: 300, | |
| fade: 'from', | |
| }); | |
| await (from ?? to)?.finished; | |
| sourceElement.style.removeProperty('z-index'); | |
| gesturePopup.close(); |
🤖 Prompt for AI Agents
In @src/views/options/gestures.mjs around lines 203 - 209, The code assumes
morph(...) returns a defined from animation and does await from.finished which
can throw if from is undefined; update the gesture popup teardown in
gestures.mjs to defensively check for from before awaiting (e.g., if (from &&
from.finished) await from.finished; else if (to && to.finished) await
to.finished) and ensure sourceElement.style.removeProperty('z-index') and
gesturePopup.close() still run (use a finally-style flow if needed) — reference
the destructuring const {from, to} = morph(...) and the await from.finished
call.
| &::after { | ||
| content: "__MSG_toggleButtonOff__"; | ||
| } | ||
|
|
||
| &:checked { | ||
| border-color: var(--highlighted-color, #00AAA0); | ||
| background: var(--highlighted-color, #00AAA0); | ||
| color: var(--textSecond-color, #FFF); | ||
|
|
||
| &::after { | ||
| content: "__MSG_toggleButtonOn__"; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, explore the repository structure
git ls-files | head -30Repository: Robbendebiene/Gesturefy
Length of output: 930
🏁 Script executed:
# Look for build configuration and package.json
fd -e package.json -e webpack.config.js -e rollup.config.js -e gulpfile.js -e vite.config.jsRepository: Robbendebiene/Gesturefy
Length of output: 49
🏁 Script executed:
# Search for message files
fd -e json | rg "_messages|locale|i18n" | head -20Repository: Robbendebiene/Gesturefy
Length of output: 714
🏁 Script executed:
# Look for manifest files (extension config)
fd manifest.jsonRepository: Robbendebiene/Gesturefy
Length of output: 84
🏁 Script executed:
# Search for any build or preprocessing of CSS
rg -i "css|content.*msg|processCSS|i18n" --max-count=5 -t js -t jsonRepository: Robbendebiene/Gesturefy
Length of output: 9059
🏁 Script executed:
# Search for toggleButton messages in the messages files
rg "toggleButton" src/_locales/en/messages.jsonRepository: Robbendebiene/Gesturefy
Length of output: 111
🏁 Script executed:
# Read the layout.css file context around the specified lines
wc -l src/views/options/layout.cssRepository: Robbendebiene/Gesturefy
Length of output: 99
🏁 Script executed:
# Check how the CSS file is loaded
rg -i "layout.css" src/Repository: Robbendebiene/Gesturefy
Length of output: 1315
🏁 Script executed:
# Search for __MSG_ replacements in JavaScript/HTML
rg "__MSG_" --max-count=10Repository: Robbendebiene/Gesturefy
Length of output: 707
🏁 Script executed:
# Look for i18n or message replacement functions
rg "getMessage|__MSG_|i18n" -t js src/ | head -30Repository: Robbendebiene/Gesturefy
Length of output: 49
🏁 Script executed:
# Read the options/index.html to see how CSS is loaded
cat src/views/options/index.htmlRepository: Robbendebiene/Gesturefy
Length of output: 3393
🏁 Script executed:
# Look for i18n or message processing code
rg "i18n|getMessage|textContent|innerHTML" -t js -t mjs -A 2 src/views/ | head -50Repository: Robbendebiene/Gesturefy
Length of output: 94
🏁 Script executed:
# Check if there's any build or preprocessing step documented
cat README.md | head -100Repository: Robbendebiene/Gesturefy
Length of output: 4247
🏁 Script executed:
# Search for extension runtime message handling
rg "chrome\.|browser\.|runtime" -t js -t mjs | head -20Repository: Robbendebiene/Gesturefy
Length of output: 94
🏁 Script executed:
# Look for CSS variable processing or i18n utilities
fd -e mjs -e js src/views/ | xargs rg -l "i18n|locali|message" | head -10Repository: Robbendebiene/Gesturefy
Length of output: 749
🏁 Script executed:
# Read the locale-text.mjs component to understand how it handles __MSG_ placeholders
cat src/views/shared/components/locale-text.mjsRepository: Robbendebiene/Gesturefy
Length of output: 1275
🏁 Script executed:
# Also check locale-option.mjs
cat src/views/shared/components/locale-option.mjsRepository: Robbendebiene/Gesturefy
Length of output: 1777
🏁 Script executed:
# Search for any CSS processing related to __MSG_
rg -t js -t mjs "__MSG_|content.*__" src/ -B 3 -A 3 2>/dev/null | head -40Repository: Robbendebiene/Gesturefy
Length of output: 49
🏁 Script executed:
# Check if there's CSS preprocessing in package.json
cat package.json 2>/dev/null || echo "No package.json found"Repository: Robbendebiene/Gesturefy
Length of output: 88
🏁 Script executed:
# Look at the manifest.json to understand how i18n is configured
cat src/manifest.json | head -50Repository: Robbendebiene/Gesturefy
Length of output: 1233
🏁 Script executed:
# Search for any CSS processing or replacement of __MSG_ in content
rg "\.css|content:" -t js -t mjs -B 2 -A 2 src/ | head -50Repository: Robbendebiene/Gesturefy
Length of output: 94
🏁 Script executed:
# Check if the CSS content is actually being used and working
grep -n "toggle" src/views/options/layout.css | head -20Repository: Robbendebiene/Gesturefy
Length of output: 345
🏁 Script executed:
# Look for JavaScript that might be setting CSS content or ::before/::after dynamically
rg "::before|::after|content|setAttribute" -t js -t mjs src/views/ | head -20Repository: Robbendebiene/Gesturefy
Length of output: 94
🏁 Script executed:
# Check the actual messages.json to see what the toggle button values are
cat src/_locales/en/messages.json | rg -A 3 "toggleButton"Repository: Robbendebiene/Gesturefy
Length of output: 253
🏁 Script executed:
# Let's check the actual HTML structure around the toggle button to see if it's being used
rg -i "toggle.*button" -B 5 -A 5 src/views/options/ | head -60Repository: Robbendebiene/Gesturefy
Length of output: 3147
🏁 Script executed:
# Check if there's any JavaScript modifying CSS or setting text content for these elements
rg "toggle" -t js -A 5 src/views/options/ | head -40Repository: Robbendebiene/Gesturefy
Length of output: 49
🏁 Script executed:
# Search the entire codebase for any CSS processing or sheet manipulation
rg "textContent|innerHTML|content|stylesheet" -t js | head -30Repository: Robbendebiene/Gesturefy
Length of output: 1275
🏁 Script executed:
# Let's verify by looking at how the toggle button is actually rendered in HTML
fd "\.html" src/views/options/ -exec cat {} \; 2>/dev/null | grep -i "toggle" -B 3 -A 3Repository: Robbendebiene/Gesturefy
Length of output: 49
🏁 Script executed:
# Check the fragment files
cat src/views/options/fragments/settings.inc | head -100Repository: Robbendebiene/Gesturefy
Length of output: 4939
🏁 Script executed:
# Let's verify if the toggle button actually displays the __MSG_ text or if something else handles it
# Check if there's any dynamic content setting for these elements
rg "toggle-button|toggleButton" -A 10 -B 5 src/views/options/main.mjsRepository: Robbendebiene/Gesturefy
Length of output: 49
🏁 Script executed:
# Let's also look at the complete toggle button CSS section to see all its styling
sed -n '247,295p' src/views/options/layout.cssRepository: Robbendebiene/Gesturefy
Length of output: 1109
🏁 Script executed:
# One final check - let's verify by searching for any Firefox-specific CSS processing or
# any other mechanism that might handle __MSG_ in CSS
rg -i "firefox|gecko|manifest.*i18n|chrome\.i18n" src/ 2>/dev/null | head -20Repository: Robbendebiene/Gesturefy
Length of output: 2136
🏁 Script executed:
# Let's also check if there's a build script in .github or elsewhere
find .github -type f -name "*.yml" -o -name "*.yaml" 2>/dev/null | xargs cat 2>/dev/null | head -100Repository: Robbendebiene/Gesturefy
Length of output: 164
Replace CSS content placeholders with JavaScript-based localization.
The __MSG_toggleButtonOff__ and __MSG_toggleButtonOn__ in content properties won't be replaced at runtime. Firefox's i18n API only processes __MSG_ in manifest fields, not CSS. Use data-* attributes with browser.i18n.getMessage() to set the text dynamically instead.
🤖 Prompt for AI Agents
In @src/views/options/layout.css around lines 279 - 291, The CSS currently uses
__MSG_toggleButtonOff__ and __MSG_toggleButtonOn__ which Firefox won't replace;
remove those raw strings and switch to using data attributes plus dynamic JS:
change the ::after rules (the selectors using ::after and &:checked ::after) to
use content: attr(data-label); add data-label-off and data-label-on attributes
on the toggle element in HTML (or a single data-label you update), and in the
code that renders/initializes the toggle call
browser.i18n.getMessage("toggleButtonOff") and getMessage("toggleButtonOn") to
populate the element's data-label (and update it on change/checked events) so
the ::after pseudoelement shows localized text. Ensure the same element
referenced by the CSS selectors (&::after / &:checked) is the one you set
attributes on and update the attribute when the checked state changes.
| .popup-close-button { | ||
| margin-left: auto; | ||
| background: none; | ||
| text-align: right; | ||
| font-size: 12px; | ||
| font-family: Icons; | ||
| cursor: pointer; | ||
| color: var(--text-color, #555555); | ||
|
|
||
| &:hover { | ||
| color: var(--warning-color, #FF6347); | ||
| } | ||
|
|
||
| &::before { | ||
| content: "X"; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for popup-close-button implementation in HTML and JS files
fd -e html -e js -e mjs | xargs rg -l "popup-close-button|popup.*close" | head -20Repository: Robbendebiene/Gesturefy
Length of output: 279
🏁 Script executed:
# More specific search for popup structure and close button
rg -A 10 "popup-close-button" --type html --type jsRepository: Robbendebiene/Gesturefy
Length of output: 49
🏁 Script executed:
# Search for popup container definitions
rg "class.*popup" -A 5 --type html --type js | head -100Repository: Robbendebiene/Gesturefy
Length of output: 49
🏁 Script executed:
# Check the popup-command-view.mjs file
cat -n src/core/views/popup-command-view/popup-command-view.mjsRepository: Robbendebiene/Gesturefy
Length of output: 6944
🏁 Script executed:
# Also check the iframe version
cat -n src/core/views/popup-command-view/iframe-popup-command-view.mjsRepository: Robbendebiene/Gesturefy
Length of output: 7060
🏁 Script executed:
# Search for popup-close-button content in these specific files
rg "popup-close-button" -B 5 -A 5 src/core/views/popup-command-view/Repository: Robbendebiene/Gesturefy
Length of output: 49
🏁 Script executed:
# Find the HTML file for the popup
fd "popup-command-view.html"Repository: Robbendebiene/Gesturefy
Length of output: 124
🏁 Script executed:
# Read the HTML file
cat -n "src/core/views/popup-command-view/popup-command-view.html"Repository: Robbendebiene/Gesturefy
Length of output: 396
🏁 Script executed:
# Search for popup and close in all relevant files
rg "popup.*close|close.*button" -i --type js --type mjs --type htmlRepository: Robbendebiene/Gesturefy
Length of output: 94
🏁 Script executed:
# Search for popup-close-button in all file types
rg "popup-close-button" -type-listRepository: Robbendebiene/Gesturefy
Length of output: 99
🏁 Script executed:
# Search without specifying file type
rg "popup-close-button"Repository: Robbendebiene/Gesturefy
Length of output: 277
🏁 Script executed:
# Find and read HTML files in options directory
fd "\.html$" | grep -i optionsRepository: Robbendebiene/Gesturefy
Length of output: 95
🏁 Script executed:
# Read the options page HTML
cat -n "src/views/options/options.html"Repository: Robbendebiene/Gesturefy
Length of output: 129
🏁 Script executed:
# Read the gestures.inc file to see the button in context
cat -n "src/views/options/fragments/gestures.inc"Repository: Robbendebiene/Gesturefy
Length of output: 2206
🏁 Script executed:
# Also check if there are any related ARIA labels or accessible descriptions
rg "gesturePopup|popup-close" -B 5 -A 5 "src/views/options/fragments/gestures.inc"Repository: Robbendebiene/Gesturefy
Length of output: 1544
Add accessible label to the close button for screen readers.
The close button is properly implemented as a semantic <button> element, which handles keyboard accessibility correctly. However, it lacks an aria-label to describe its purpose to screen readers. The "X" content is only provided via CSS and may not be reliably announced. Add aria-label="Close" (or similar) to the button element.
🤖 Prompt for AI Agents
In @src/views/options/layout.css around lines 545 - 561, The .popup-close-button
uses CSS-generated "X" content which screen readers may not announce; update the
actual <button> element using that class (popup-close-button) to include an
accessible label such as aria-label="Close" (or a localized equivalent) so
assistive tech can describe its purpose; ensure you add the attribute on the
button element where class="popup-close-button" (optionally also add a title
attribute or use aria-labelledby if you prefer a visible label).
Requirements:
allow disabling Gesturefy for certain websites (https://bugzilla.mozilla.org/show_bug.cgi?id=1497075, https://bugzilla.mozilla.org/show_bug.cgi?id=1711787)This will probably not happen in the near future so we should stick to the existing exclusions.