Fix: Improve date input UX and prevent manual editing issues#591
Open
gurusatsangi wants to merge 1 commit intofossasia:mainfrom
Open
Fix: Improve date input UX and prevent manual editing issues#591gurusatsangi wants to merge 1 commit intofossasia:mainfrom
gurusatsangi wants to merge 1 commit intofossasia:mainfrom
Conversation
Contributor
Reviewer's GuideThis PR refines the date range inputs in the popup to rely solely on the native date picker, switching date change handling from Sequence diagram for date input click and change handlingsequenceDiagram
actor User
participant startingDateInput
participant endingDateInput
participant NativeDatePicker as NativeDatePicker
participant scrumDateRangeUtils
participant browserStorageLocal
User->>startingDateInput: click()
startingDateInput->>startingDateInput: showPicker()
startingDateInput->>NativeDatePicker: open picker
User->>NativeDatePicker: select starting date
NativeDatePicker-->>startingDateInput: set value (change event)
startingDateInput->>scrumDateRangeUtils: normalizeSyncAndPersistDateRange(startingDateInput, endingDateInput)
scrumDateRangeUtils->>browserStorageLocal: set normalized date range
User->>endingDateInput: click()
endingDateInput->>endingDateInput: showPicker()
endingDateInput->>NativeDatePicker: open picker
User->>NativeDatePicker: select ending date
NativeDatePicker-->>endingDateInput: set value (change event)
endingDateInput->>scrumDateRangeUtils: normalizeSyncAndPersistDateRange(startingDateInput, endingDateInput)
scrumDateRangeUtils->>browserStorageLocal: set normalized date range
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
github-actions
Bot
added
javascript
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- Setting the date inputs to
readonlyand relying onclick+showPicker()may degrade keyboard accessibility (e.g., no typing or arrow-key changes); consider adding keyboard event handling (Enter/Space) or an alternative accessible control path for non-pointer users. - The new
maxconstraint limiting both dates totodaychanges behavior by disallowing future dates; verify this aligns with the intended use cases and, if needed, allow configuring or relaxing this constraint.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Setting the date inputs to `readonly` and relying on `click` + `showPicker()` may degrade keyboard accessibility (e.g., no typing or arrow-key changes); consider adding keyboard event handling (Enter/Space) or an alternative accessible control path for non-pointer users.
- The new `max` constraint limiting both dates to `today` changes behavior by disallowing future dates; verify this aligns with the intended use cases and, if needed, allow configuring or relaxing this constraint.
## Individual Comments
### Comment 1
<location path="src/scripts/popup.js" line_range="1030" />
<code_context>
+ }
});
+
+ const today = new Date().toISOString().split('T')[0];
+ startingDateInput.max = today;
+ endingDateInput.max = today;
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `toISOString()` can produce an off-by-one day value in some timezones.
Because the `<input type="date">` value is interpreted in local time, the max value should be built from local date parts instead of UTC. Consider something like:
```js
const now = new Date();
const today = `${now.getFullYear()}-${String(now.getMonth() + 1).padStart(2, '0')}-${String(now.getDate()).padStart(2, '0')}`;
```
This keeps the max constraint aligned with the user’s actual local date.
</issue_to_address>
### Comment 2
<location path="src/scripts/popup.js" line_range="1018-1001" />
<code_context>
+ });
+
+
+ startingDateInput.addEventListener('click', () => {
+ if (startingDateInput.showPicker) {
+ startingDateInput.showPicker();
+ }
+ });
+
+ endingDateInput.addEventListener('click', () => {
</code_context>
<issue_to_address>
**issue (bug_risk):** Combining `readonly` with `showPicker` can make the field unusable where `showPicker` is unsupported.
Because the inputs are `readonly` and only open via `showPicker`, environments without `HTMLInputElement.prototype.showPicker` will leave them non-interactive, so users can’t set dates. Consider conditionally removing `readonly` when `showPicker` is unavailable or providing an alternate date input UI in that case.
</issue_to_address>
### Comment 3
<location path="src/scripts/popup.js" line_range="1003-1006" />
<code_context>
- endingDateInput,
- );
+
+ startingDateInput.addEventListener('change', () => {
+ window.scrumDateRangeUtils.normalizeSyncAndPersistDateRange(
+ startingDateInput,
+ endingDateInput
+ );
});
</code_context>
<issue_to_address>
**suggestion:** Consider also handling keyboard-based interactions for opening the date picker.
At the moment the picker only opens on `click`. For keyboard users—especially with a `readonly` input—this may not reliably trigger, depending on the browser. To make this more robust and accessible, also handle `keydown` (e.g., Enter/Space) on the focused input or use `focus` to trigger `showPicker` where appropriate.
Suggested implementation:
```javascript
startingDateInput.addEventListener('change', () => {
window.scrumDateRangeUtils.normalizeSyncAndPersistDateRange(
startingDateInput,
endingDateInput,
);
});
// Improve keyboard accessibility for the date picker on the starting date input
startingDateInput.addEventListener('keydown', (event) => {
if (event.key === 'Enter' || event.key === ' ') {
// Some browsers expose showPicker on input[type="date"]
if (typeof startingDateInput.showPicker === 'function') {
event.preventDefault();
startingDateInput.showPicker();
}
}
});
```
1. If the ending date input also opens a date picker via click elsewhere in this file, consider adding a similar `keydown` handler for `endingDateInput` to keep behavior consistent.
2. If there's already a shared function that handles opening the picker on click (e.g. `openDatePicker(input)`), you may want to reuse it in the `keydown` handler instead of calling `showPicker` directly, to keep all open-picker logic in one place.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| }); | ||
|
|
||
| const today = new Date().toISOString().split('T')[0]; |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Using toISOString() can produce an off-by-one day value in some timezones.
Because the <input type="date"> value is interpreted in local time, the max value should be built from local date parts instead of UTC. Consider something like:
const now = new Date();
const today = `${now.getFullYear()}-${String(now.getMonth() + 1).padStart(2, '0')}-${String(now.getDate()).padStart(2, '0')}`;This keeps the max constraint aligned with the user’s actual local date.
| @@ -999,20 +999,39 @@ document.addEventListener('DOMContentLoaded', () => { | |||
| yesterdayRadio.addEventListener('change', () => { | |||
| browser.storage.local.set({ yesterdayContribution: yesterdayRadio.checked }); | |||
| }); | |||
Contributor
There was a problem hiding this comment.
issue (bug_risk): Combining readonly with showPicker can make the field unusable where showPicker is unsupported.
Because the inputs are readonly and only open via showPicker, environments without HTMLInputElement.prototype.showPicker will leave them non-interactive, so users can’t set dates. Consider conditionally removing readonly when showPicker is unavailable or providing an alternate date input UI in that case.
Comment on lines
+1003
to
+1006
| startingDateInput.addEventListener('change', () => { | ||
| window.scrumDateRangeUtils.normalizeSyncAndPersistDateRange( | ||
| startingDateInput, | ||
| endingDateInput |
Contributor
There was a problem hiding this comment.
suggestion: Consider also handling keyboard-based interactions for opening the date picker.
At the moment the picker only opens on click. For keyboard users—especially with a readonly input—this may not reliably trigger, depending on the browser. To make this more robust and accessible, also handle keydown (e.g., Enter/Space) on the focused input or use focus to trigger showPicker where appropriate.
Suggested implementation:
startingDateInput.addEventListener('change', () => {
window.scrumDateRangeUtils.normalizeSyncAndPersistDateRange(
startingDateInput,
endingDateInput,
);
});
// Improve keyboard accessibility for the date picker on the starting date input
startingDateInput.addEventListener('keydown', (event) => {
if (event.key === 'Enter' || event.key === ' ') {
// Some browsers expose showPicker on input[type="date"]
if (typeof startingDateInput.showPicker === 'function') {
event.preventDefault();
startingDateInput.showPicker();
}
}
});- If the ending date input also opens a date picker via click elsewhere in this file, consider adding a similar
keydownhandler forendingDateInputto keep behavior consistent. - If there's already a shared function that handles opening the picker on click (e.g.
openDatePicker(input)), you may want to reuse it in thekeydownhandler instead of callingshowPickerdirectly, to keep all open-picker logic in one place.
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.
🔧 Fixes
Fixes #590
📌 Summary of Changes
readonlyinputevent withchangeeventfocuswithclickto ensure picker opens every time🎯 Problem
Date inputs had poor UX:
✅ Solution
📸 Demo
❌ Before (Bug)
2026-04-21.21-17-20.online-video-cutter.com.mp4
✅ After (Fix)
2026-04-22.00-15-53.online-video-cutter.com.mp4
🧪 Testing
📝 Reviewer Notes
This change removes unreliable manual editing behavior caused by native date inputs and ensures a consistent, bug-free user experience.