🐛 Fix #5084: Fixfox Tab switch issue #5179
Conversation
…the blur operation to the next event loop Close Hacker0x01#5084
…n to the next event loop - This issue is caused due to the defer blur made in the commit 3682222 is getting executed after the focus operation for the ESC key press Close Hacker0x01#5084
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ This pull request was sent to the PullRequest network for review. Expert reviewers are now being matched to your request based on the code's requirements. Stay tuned!
What to expect from this code review:
- Comments posted to any areas of potential concern or improvement.
- Detailed feedback or actions needed to resolve issues that are found.
- Turnaround times vary, but we aim to be swift.
@balajis-qb you can click here to see the review status or cancel the code review job.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5179 +/- ##
=======================================
Coverage 96.91% 96.91%
=======================================
Files 29 29
Lines 3366 3370 +4
Branches 1396 1410 +14
=======================================
+ Hits 3262 3266 +4
+ Misses 104 102 -2
- Partials 0 2 +2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I have no blocking concerns; the change looks correct. All I see is a minor nit where code can be simplified using modern features.
Reviewed with ❤️ by PullRequest
src/index.tsx
Outdated
| safeFocus = () => { | ||
| setTimeout(() => { | ||
| if (this.input && this.input.focus) { | ||
| this.input.focus({ preventScroll: true }); |
src/index.tsx
Outdated
| safeBlur = () => { | ||
| setTimeout(() => { | ||
| if (this.input && this.input.blur) { | ||
| this.input.blur(); |
There was a problem hiding this comment.
I just moved the existing code into these new methods for the fix. I have updated the code as per your suggestion.
Close #5084
Description
As @avazhenin mentioned in the attached ticket, we're getting an issue while we try to switch to the next input control via Tab Key on Firefox browser.
To Reproduce
Even though @avazhenin shared his code sample along with a video explaining the issue, I'll share a simple way to recreate the issue. Goto
https://reactdatepicker.com/via FireFox and click on any date input, now try to press tab when the calendar is getting rendered. We expect it to auto-close the calendar and switch to the next code sample. But instead of that it'll refocus the same date input again. To make it work, we have to press ESC first (which will close the opened calendar popup) and then press Tab, it'll work. In this case alone it'll work, because as the datepicker is already closed, it won't disturb the tab key press.Try the same thing in Chrome Browser, it'll auto-close the opened calendar popup and switch to the next code sample editor.
Reason behind this issue
The reason why this issue occurs whenever we press Tab key over the input, we call
setOpen(false)to close the open popup so that the browser default focus switch will happen to the next control.But as I highlighted in the above screenshot, while performing the state update we manually call blur operation on the date input. We do a state update and along with setting blur to the DOM element. This issue arises from a combination of
1. JavaScript Event Loop & React's Rendering Lifecycle:
setStateassuming the blur will happen after the update of state. But React's state updates are asynchronous and may not complete immediately aftersetState(). The callback of setState runs after the state update, but not guarantee that the DOM has been fully updated and rendered (unlike theuseEffectcall).2. Browser-Specific Focus Behaviour:
blur()call made by us in-between the state update.Changes/Solution
Wrapping the blur() call inside a setTimeout defers it to the next macrotask, ensuring that React's render and browser focus management complete before the blur operation is executed.
As we wrapped
setBlutcall inside the setTimeout, we need to move the focus logic also to setTimeout, as in the case of Esc key press we kind of blur and send focus back to the input. So in this PR I moved the focus and blur call inside setTimeout.I created 2 new methods and moved our existing blur and focus call to it instead of updating the existing methods names. Updating the existing method names make me update many lines as these functions are used in many places. So to keep my fix simple and at the same time to emphasize the change I made, I created these 2 new helpers.
I updated the existing test cases to adapt with my fix and also added a new test case to check the Tab order.
Contribution checklist