-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
run focus event listener after existing onFocus handlers #4755
Conversation
🦋 Changeset detectedLatest commit: b885d86 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
711cd9c
to
bc28754
Compare
I believe the integration test failure is a flaky test. That test passes locally, and it's failing on this line which suggests to me that maybe the wait just isn't long enough?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually @jhurwitz , I wonder if we really want to use window.setTimeout
inside useIsomorphicLayoutEffect
instead of some other timeout mechanism that doesn't rely on the presence of the window
object?
Ah, good call. Would changing to just |
Thanks for the comment. Can you please give more details? Are you running this out of the Codesandbox or locally? I suspect you might be running it locally, because the gray background in your screen capture does not appear in the Codesandbox. And just to confirm -- are you testing on the
The newest what, exactly?
Are you saying you're using a different version of React.js (if so, which version? the codesandbox uses 17.0.2, the latest version), or are you saying that the reaction of the useFocused hook is different?
What browser and OS are you using? |
my steps
Environmentrunning on locally Slate Version: 0.72.3 |
This causes a regression for me somehow when selecting between two different editors. It moves the cursor back to the previous selection instead. Trying to investigate why exactly but I narrowed it down to the setTimeout introduced here. Perhaps a better way to solve this generally is to set the |
…lor#4755) * run focus event listener after existing onFocus handlers * change window.setTimeout to setTimeout
Description
As described in the issue, the
useFocused
hook was returning incorrect/unexpected values. I debugged and realized it was displaying old values because of the order in which event listeners were running.Issue
Fixes: #4754
Example
Here I repro the exact two behaviors I described in the linked issue, but with this fix in place. (View the GIFs in the issue to see what they looked like before this fix.)
Context
Based on my testing, I think there was a race condition between the focus/blur event listeners in
components/slate.tsx
(added in #3987) and the onFocus/onBlur handlers incomponents/editable.tsx
. TheIS_FOCUSED
map was eventually consistent/had the correct values after all handlers ran, but the event listeners inslate.tsx
were running before the handlers ineditable.tsx
and therefore theisFocused
hook was displaying the old values, from before the focus/blur handlers ran.Adding a
setTimeout
with a delay of 0 cause the event listeners inslate.tsx
to run after the handlers ineditable.tsx
, thereby fixing the issue.Checks
yarn test
.yarn lint
. (Fix errors withyarn fix
.)yarn start
.)yarn changeset add
.)