Remove disableKeyboard and preventDefault workarounds from DatePicker#84523
Closed
MelvinBot wants to merge 2 commits into
Closed
Remove disableKeyboard and preventDefault workarounds from DatePicker#84523MelvinBot wants to merge 2 commits into
MelvinBot wants to merge 2 commits into
Conversation
Instead of using disableKeyboard and event.preventDefault() to prevent cursor/keyboard when clicking the DatePicker, use readOnly on the TextInput and skip focus in BaseTextInput when the input is readOnly. Also change the DatePicker role from PRESENTATION to COMBOBOX for better screen reader semantics. Co-authored-by: Rushat Gabhane <rushatgabhane@users.noreply.github.com>
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
Fixes no-use-before-define ESLint error by moving the isReadOnly const declaration above the onPress function that references it. Co-authored-by: Rushat Gabhane <rushatgabhane@users.noreply.github.com>
Contributor
Author
|
Fixed the failing |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
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
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.
Explanation of Change
This PR removes the
disableKeyboardandevent.preventDefault()workarounds from the DatePicker component, as suggested in the review on PR #79892.Problem: PR #79892 removed
readOnlyfrom the DatePicker's TextInput (to fix the screen reader announcing "read-only" on an interactive field) and replaced it withdisableKeyboard+event.preventDefault()as workarounds to prevent the cursor and keyboard from appearing.Solution: Instead of these workarounds, this PR:
Modifies
BaseTextInput(web + native) to skip focusing the input on press whenreadOnlyis true. This is a sensible behavior change — a read-only input should not acquire focus when clicked via the component's press handler.Updates DatePicker to:
readOnlyinstead ofdisableKeyboard(the BaseTextInput change ensures no cursor/keyboard appears)event.preventDefault()fromhandlePress(no longer needed)rolefromPRESENTATIONtoCOMBOBOXfor better screen reader semantics — a combobox is the correct ARIA pattern for a field that opens a selection popupFixed Issues
$ #74882
Tests
Offline tests
N/A — this is a UI interaction change with no network dependency.
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web: Chrome
Tested on web — date picker opens on click, no cursor in input, date selection works correctly, no console errors.