Use custom attack ratio slider for consistent cross-browser rendering#3422
Open
hkio120 wants to merge 3 commits intoopenfrontio:mainfrom
Open
Use custom attack ratio slider for consistent cross-browser rendering#3422hkio120 wants to merge 3 commits intoopenfrontio:mainfrom
hkio120 wants to merge 3 commits intoopenfrontio:mainfrom
Conversation
Contributor
WalkthroughThe change replaces the native range input with a custom pointer-driven attack ratio slider in the control panel, adding pointer event handlers, keyboard accessibility, clamped persistence to localStorage via setAttackRatio, and wiring to onAttackRatioChange and UI updates. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CP as ControlPanel
participant LS as localStorage
participant UI as Renderer
participant App as onAttackRatioChange
User->>CP: Pointer/keyboard input on slider
CP->>CP: compute newRatio (clamp 0.01..1)
CP->>LS: persist attackRatio
CP->>App: call onAttackRatioChange(newRatio)
CP->>UI: request UI update / re-render
UI->>User: visual thumb/values updated
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/graphics/layers/ControlPanel.ts (1)
1-1:⚠️ Potential issue | 🟡 MinorPipeline reports formatting issues.
CI flagged code style issues. Run
npx prettier --write src/client/graphics/layers/ControlPanel.tsto fix before merging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/ControlPanel.ts` at line 1, CI flagged formatting issues in ControlPanel.ts; run the formatter and commit the result: run `npx prettier --write src/client/graphics/layers/ControlPanel.ts` (or your repo-wide prettier command), review the formatted changes in the ControlPanel class (the file that imports LitElement and html and likely declares the ControlPanel component), stage and push the updated file so the import/formatting problems are resolved for the PR.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/graphics/layers/ControlPanel.ts`:
- Around line 176-188: The custom slider div (used with handlers like
handleRatioSliderPointerDown, handleRatioSliderPointerMove,
handleRatioSliderPointerUp and state attackRatio/setAttackRatio) lacks keyboard
accessibility—add tabindex="0" to the slider element and implement a keydown
handler (e.g., handleRatioSliderKeyDown) wired via `@keydown` that handles
ArrowLeft/ArrowRight/ArrowUp/ArrowDown/Home/End to call setAttackRatio with
appropriate steps (use Shift for larger steps), preventDefault on handled keys,
and ensure aria-valuenow updates when attackRatio changes.
---
Outside diff comments:
In `@src/client/graphics/layers/ControlPanel.ts`:
- Line 1: CI flagged formatting issues in ControlPanel.ts; run the formatter and
commit the result: run `npx prettier --write
src/client/graphics/layers/ControlPanel.ts` (or your repo-wide prettier
command), review the formatted changes in the ControlPanel class (the file that
imports LitElement and html and likely declares the ControlPanel component),
stage and push the updated file so the import/formatting problems are resolved
for the PR.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45878400-dc03-4fbe-b240-5ca97cdd3856
📒 Files selected for processing (1)
src/client/graphics/layers/ControlPanel.ts
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/graphics/layers/ControlPanel.ts`:
- Line 207: Replace the hardcoded aria-label "Attack ratio" with a localized
string via translateText(), i.e. call translateText with a new key (e.g.
"attackRatio") where the aria-label is set (the Slider/element in
ControlPanel.ts that currently has aria-label="Attack ratio"), and add that key
with the English value "Attack ratio" into en.json so the i18n system picks it
up.
- Around line 127-133: The setAttackRatio method must reject non-finite inputs
and avoid redundant side effects: first check Number.isFinite(newRatio) and
return early if not to prevent persisting "NaN"; compute clamped = Math.min(1,
Math.max(0.01, newRatio)) and compare to this.attackRatio, and if unchanged
return early to avoid unnecessary localStorage.setItem("settings.attackRatio",
...), this.onAttackRatioChange(...) and this.requestUpdate(); only when the
clamped value is finite and different from the current this.attackRatio, assign
it, persist the stringified value, call onAttackRatioChange(this.attackRatio)
and requestUpdate().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f460ae9c-789f-4998-ad6a-d1d4d8f7a78f
📒 Files selected for processing (1)
src/client/graphics/layers/ControlPanel.ts
Comment on lines
+127
to
+133
| private setAttackRatio(newRatio: number) { | ||
| const clamped = Math.min(1, Math.max(0.01, newRatio)); | ||
| this.attackRatio = clamped; | ||
| localStorage.setItem("settings.attackRatio", String(this.attackRatio)); | ||
| this.onAttackRatioChange(this.attackRatio); | ||
| this.requestUpdate(); | ||
| } |
Contributor
There was a problem hiding this comment.
Guard setAttackRatio against non-finite input and redundant writes.
Line 128 clamps range, but NaN still passes through as NaN, then Line 130 persists "NaN" and UI math can break. Also, unchanged values still trigger storage/event/update churn.
Suggested hardening
private setAttackRatio(newRatio: number) {
- const clamped = Math.min(1, Math.max(0.01, newRatio));
- this.attackRatio = clamped;
- localStorage.setItem("settings.attackRatio", String(this.attackRatio));
- this.onAttackRatioChange(this.attackRatio);
- this.requestUpdate();
+ const safeRatio = Number.isFinite(newRatio) ? newRatio : 0.2;
+ const clamped = Math.min(1, Math.max(0.01, safeRatio));
+ if (clamped === this.attackRatio) return;
+ this.attackRatio = clamped;
+ localStorage.setItem("settings.attackRatio", String(clamped));
+ this.onAttackRatioChange(clamped);
+ this.requestUpdate();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/layers/ControlPanel.ts` around lines 127 - 133, The
setAttackRatio method must reject non-finite inputs and avoid redundant side
effects: first check Number.isFinite(newRatio) and return early if not to
prevent persisting "NaN"; compute clamped = Math.min(1, Math.max(0.01,
newRatio)) and compare to this.attackRatio, and if unchanged return early to
avoid unnecessary localStorage.setItem("settings.attackRatio", ...),
this.onAttackRatioChange(...) and this.requestUpdate(); only when the clamped
value is finite and different from the current this.attackRatio, assign it,
persist the stringified value, call onAttackRatioChange(this.attackRatio) and
requestUpdate().
| class="relative w-full ${heightClass} rounded-full bg-slate-700/70 border border-slate-300/25 shadow-inner cursor-pointer touch-none select-none" | ||
| role="slider" | ||
| tabindex="0" | ||
| aria-label="Attack ratio" |
Contributor
There was a problem hiding this comment.
Localize the slider accessibility label.
Line 207 uses a hardcoded English string ("Attack ratio"). This should go through translateText() and be added to en.json to match the repo i18n pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/layers/ControlPanel.ts` at line 207, Replace the
hardcoded aria-label "Attack ratio" with a localized string via translateText(),
i.e. call translateText with a new key (e.g. "attackRatio") where the aria-label
is set (the Slider/element in ControlPanel.ts that currently has
aria-label="Attack ratio"), and add that key with the English value "Attack
ratio" into en.json so the i18n system picks it up.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/graphics/layers/ControlPanel.ts (1)
53-57:⚠️ Potential issue | 🟠 MajorInitialize attack ratio via the validated setter.
Line 53-Line 56 reads from
localStorageand writes directly touiStatewithout validation. If storage containsNaNor out-of-range values, attack calculations can start invalid.Suggested fix
init() { - this.attackRatio = Number( - localStorage.getItem("settings.attackRatio") ?? "0.2", - ); - this.uiState.attackRatio = this.attackRatio; + const storedRatio = Number( + localStorage.getItem("settings.attackRatio") ?? "0.2", + ); + this.setAttackRatio(Number.isFinite(storedRatio) ? storedRatio : 0.2); this.eventBus.on(AttackRatioEvent, (event) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/ControlPanel.ts` around lines 53 - 57, The code reads settings.attackRatio from localStorage and writes it directly to uiState.attackRatio without validation; instead parse the stored value (fallback "0.2") and assign it to the validated property this.attackRatio so the class setter can clamp/validate and update uiState appropriately (i.e., replace direct assignment this.uiState.attackRatio = ... with this.attackRatio = parsedValue and keep the existing eventBus.on(AttackRatioEvent, ...) hookup unchanged).
♻️ Duplicate comments (3)
src/client/graphics/layers/ControlPanel.ts (3)
127-133:⚠️ Potential issue | 🟠 MajorHarden
setAttackRatiofor non-finite input and no-op updates.Line 128 clamp does not protect against
NaN(clampedstaysNaN), and unchanged values still trigger storage/event/render churn.Suggested hardening
private setAttackRatio(newRatio: number) { + if (!Number.isFinite(newRatio)) return; const clamped = Math.min(1, Math.max(0.01, newRatio)); + if (clamped === this.attackRatio) return; this.attackRatio = clamped; localStorage.setItem("settings.attackRatio", String(this.attackRatio)); this.onAttackRatioChange(this.attackRatio); this.requestUpdate(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/ControlPanel.ts` around lines 127 - 133, Harden setAttackRatio to handle non-finite inputs and avoid no-op work: first coerce newRatio into a finite clamped value (e.g., if !isFinite(newRatio) or isNaN(newRatio) treat as the min 0.01), then clamp between 0.01 and 1; if the resulting clamped value equals this.attackRatio, return early to skip localStorage.setItem("settings.attackRatio"), this.onAttackRatioChange, and this.requestUpdate(); otherwise assign this.attackRatio, persist the setting, call this.onAttackRatioChange(this.attackRatio) and this.requestUpdate().
177-188:⚠️ Potential issue | 🟠 MajorAdd keyboard support to keep slider accessible.
With
role="slider"but notabindexand no keyboard handler, keyboard-only users cannot adjust this control.Suggested keyboard wiring
<div class="relative w-full ${heightClass} rounded-full bg-slate-700/70 border border-slate-300/25 shadow-inner cursor-pointer touch-none select-none" role="slider" + tabindex="0" aria-label="Attack ratio" aria-valuemin="1" aria-valuemax="100" aria-valuenow=${Math.round(ratioPercent)} `@pointerdown`=${this.handleRatioSliderPointerDown} `@pointermove`=${this.handleRatioSliderPointerMove} `@pointerup`=${this.handleRatioSliderPointerUp} `@pointercancel`=${this.handleRatioSliderPointerUp} + `@keydown`=${this.handleRatioSliderKeyDown} >private handleRatioSliderKeyDown = (e: KeyboardEvent) => { const step = e.shiftKey ? 0.1 : 0.01; if (e.key === "ArrowRight" || e.key === "ArrowUp") { this.setAttackRatio(this.attackRatio + step); e.preventDefault(); } else if (e.key === "ArrowLeft" || e.key === "ArrowDown") { this.setAttackRatio(this.attackRatio - step); e.preventDefault(); } else if (e.key === "Home") { this.setAttackRatio(0.01); e.preventDefault(); } else if (e.key === "End") { this.setAttackRatio(1); e.preventDefault(); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/ControlPanel.ts` around lines 177 - 188, The slider markup lacks keyboard focus and handlers: make the slider element focusable by adding tabindex="0" and wire a keydown handler (e.g., `@keydown`=${this.handleRatioSliderKeyDown}); implement a private method handleRatioSliderKeyDown that reads e.key (ArrowRight/ArrowUp to increase, ArrowLeft/ArrowDown to decrease, Home to set 0.01, End to set 1), uses a larger step when e.shiftKey is true, calls the existing setAttackRatio(this.attackRatio ± step) accordingly, and calls e.preventDefault() for handled keys so keyboard users can adjust the control while keeping the existing pointer handlers (handleRatioSliderPointerDown/Move/Up) unchanged.
180-180:⚠️ Potential issue | 🟡 MinorLocalize the slider ARIA label.
Line 180 uses a hardcoded English label. Please route this through
translateText()and add the key toen.json.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/ControlPanel.ts` at line 180, Replace the hardcoded aria-label "Attack ratio" in ControlPanel.ts with a call to translateText(), e.g. translateText('control.attackRatio'), and add the corresponding "control.attackRatio": "Attack ratio" entry to en.json so the label is localized; ensure the same translation key is used where aria-label is set and that translateText is imported/available in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/client/graphics/layers/ControlPanel.ts`:
- Around line 53-57: The code reads settings.attackRatio from localStorage and
writes it directly to uiState.attackRatio without validation; instead parse the
stored value (fallback "0.2") and assign it to the validated property
this.attackRatio so the class setter can clamp/validate and update uiState
appropriately (i.e., replace direct assignment this.uiState.attackRatio = ...
with this.attackRatio = parsedValue and keep the existing
eventBus.on(AttackRatioEvent, ...) hookup unchanged).
---
Duplicate comments:
In `@src/client/graphics/layers/ControlPanel.ts`:
- Around line 127-133: Harden setAttackRatio to handle non-finite inputs and
avoid no-op work: first coerce newRatio into a finite clamped value (e.g., if
!isFinite(newRatio) or isNaN(newRatio) treat as the min 0.01), then clamp
between 0.01 and 1; if the resulting clamped value equals this.attackRatio,
return early to skip localStorage.setItem("settings.attackRatio"),
this.onAttackRatioChange, and this.requestUpdate(); otherwise assign
this.attackRatio, persist the setting, call
this.onAttackRatioChange(this.attackRatio) and this.requestUpdate().
- Around line 177-188: The slider markup lacks keyboard focus and handlers: make
the slider element focusable by adding tabindex="0" and wire a keydown handler
(e.g., `@keydown`=${this.handleRatioSliderKeyDown}); implement a private method
handleRatioSliderKeyDown that reads e.key (ArrowRight/ArrowUp to increase,
ArrowLeft/ArrowDown to decrease, Home to set 0.01, End to set 1), uses a larger
step when e.shiftKey is true, calls the existing setAttackRatio(this.attackRatio
± step) accordingly, and calls e.preventDefault() for handled keys so keyboard
users can adjust the control while keeping the existing pointer handlers
(handleRatioSliderPointerDown/Move/Up) unchanged.
- Line 180: Replace the hardcoded aria-label "Attack ratio" in ControlPanel.ts
with a call to translateText(), e.g. translateText('control.attackRatio'), and
add the corresponding "control.attackRatio": "Attack ratio" entry to en.json so
the label is localized; ensure the same translation key is used where aria-label
is set and that translateText is imported/available in the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f6d8c44e-5ebb-4baf-b73d-7dc9e88e9bf8
📒 Files selected for processing (1)
src/client/graphics/layers/ControlPanel.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)
It was an issue but Idk the number
Bug: the attack-ratio slider looked and behaved differently across browsers (Firefox vs Chrome/Opera GX) due to native input[type=range] rendering differences.
Fix: replaced the native range input with a custom slider (track + thumb + pointer handling), using a single shared style and interaction model to ensure consistent visuals and behavior across browsers.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
HulKiora
How it looked in Opera GX:
How it look after the fix :