Fix keyboard-handling regressions + regression tests (focus-trap preserved)#28
Draft
gnbm wants to merge 3 commits into
Draft
Fix keyboard-handling regressions + regression tests (focus-trap preserved)#28gnbm wants to merge 3 commits into
gnbm wants to merge 3 commits into
Conversation
Restores behavior dropped when input keydown handling was split out of
the shared onKeyDown into onInputKeyDown/onTimeKeydown, and fixes an
onBlur misfire predating that change:
- onInputKeyDown never called triggerEvent("onKeyDown", e), so
user-registered onKeyDown callbacks stopped firing for keys pressed
in the input.
- onInputKeyDown/onTimeKeydown matched only e.key, silently no-oping
for callers that only set the legacy e.keyCode.
- onTimeKeydown only handled Shift+Tab out of the hour element; the
original Tab-cycling across hour/minute/second/AM-PM/plugin elements
had been commented out with no replacement.
- onBlur hardcoded valueChanged = true for configs without altInput,
so blurring an empty, untouched input always fired onChange.
Locks the three regressions fixed in the previous commit plus the opposite-direction behavior so a future refactor can't silently reintroduce them: - onKeyDown hook fires exactly once for a keydown on the input. - ArrowDown on the input opens the calendar (e.keyCode path). - Enter closes when allowInput is set, via e.key as well as e.keyCode. - Time picker tabs forward through hour/minute/second/AM-PM to the input. - onBlur still fires onChange when a value is entered without altInput (companion to the existing "doesn't misfire" test). Verified each regression-catcher fails against the pre-fix source.
The earlier onTimeKeydown fix reinstated manual JS Tab-cycling across the time elements that the accessibility work (3599774) had intentionally removed. That cycling called preventDefault + focus() and, on Tab from the last time element, moved focus to self._input - which lives outside the calendar container, escaping the focus trap and overriding the a11y-managed native tab order. This regressed keyboard navigation for all time-enabled pickers. Revert onTimeKeydown to the accessibility-designed behavior: native tab order (elements are tabindex=0 while open) plus the focus-trap elements keep focus inside the open dialog; only Shift+Tab out of the hour element is handled in JS (back into the calendar days / altInput). Realign the time-picker tests with the focus-trap model instead of the old "tab out to the input" expectation, which only failed under jsdom because jsdom does not simulate native Tab traversal: - keeps keyboard focus trapped within the calendar (trap wrap + tabindex) - leaves forward tab navigation between time elements to the browser (guards against re-introducing the JS cycling that broke the trap) - Shift+Tab from the hour element returns focus to the calendar
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.
Summary
Fixes three pre-existing regressions in
src/index.ts(all already onmaster) surfaced by failing unit tests, and adds regression tests that lock the fixed behavior. The time-picker Tab fix is implemented so it does not break the accessibility focus-trap introduced in35997745.Regressions fixed
onKeyDownhook stopped firing for input keydowns. When input keydown handling was split intoonInputKeyDown, the handler never re-fired the publiconKeyDownhook, and it matched onlye.key(ignoring the legacye.keyCode). Restored the hook and made key-matching accept both. FixesAPI › destroy()andAPI › onKeyDown closes flatpickr on Enter when allowInput set to true.UI › time-picker focuses out onto input. See the accessibility note below.onBlurmisfiredonChange. For configs withoutaltInput,valueChangedwas hardcodedtrue, so blurring an empty, untouched input firedonChange. Restored the historical guard (selectedDates.length > 0 || _input.value.length > 0). Fixesevents + hooks › onBlur › doesn't misfire.Accessibility: focus-trap preserved
The initial fix for (2) reinstated manual JS Tab-cycling that the accessibility work (
35997745) had intentionally removed. That cycling calledpreventDefault()+focus()and, on Tab from the last time element, moved focus toself._input— outside the calendar container — escaping the focus trap and overriding the a11y-managed native tab order.This PR instead keeps
onTimeKeydownat the accessibility-designed behavior: native tab order (all interactive elements aretabindex=0while open) plus the focus-trap elements keep focus inside the open dialog; only Shift+Tab out of the hour element is handled in JS. The time-picker tests were realigned to assert the focus-trap model rather than the old "tab out to the input" expectation (which only failed under jsdom because jsdom does not simulate native Tab traversal).Tests
time-picker leaves forward tab navigation between time elements to the browsertest was verified to fail if the JS cycling is re-introduced, guarding the focus trap.Notes for reviewers
ROU-12898timezone/onChangefix — this branch andROU-12898branch from the samemasterwith disjoint changes and should be reconciled before merge.