Run <Slate />'s focus event listeners after <Editable />'s focus handlers in React >= 17 #4920
Conversation
See facebook/react#19186 for details on changes to `onFocus` and `onBlur`
🦋 Changeset detectedLatest commit: 96538b6 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 |
adri1wald
changed the title
There was a problem hiding this comment.
Please add a changeset and then I'll land this. Thanks @adri1wald!
|
Don't really have the time to test this thoroughly at the moment, but the reasoning makes sense to me. Nice catch @adri1wald and thanks for the detailed explanation! |
| return () => { | ||
| document.removeEventListener('focus', fn, true) | ||
| document.removeEventListener('blur', fn, true) | ||
| if (IS_REACT_VERSION_17_OR_ABOVE) { |
There was a problem hiding this comment.
My only remark would be to add a little summary of your explanation in the PR here just to make it easier to figure out why this code is here in the future
There was a problem hiding this comment.
Have added a summary of my explanation!
Description
Run
<Slate />'s focus event listeners after<Editable />'s focus handlers in React >= 17Issue
Fixes: #4754
Example
See #4754 / #4755 - happy to provide more evidence if necessary
Context
In #4754, @jhurwitz noted that "the
useFocusedhook displays old values". I noticed this issue in my project as well, and the fix in #4755 solved the problem for me. However as noted by @bryanph, this change caused a regression when selecting between two different editors. Thus #4755 was reverted in #4874.By chance, I noticed that running @jhurwitz's code sandbox in slate's examples does not cause the "inverted" focus behaviour, and after some digging I realized that the difference is that the code sandbox uses React 17 whereas slate's examples use React 16. To validate this, I cloned the sandbox and downgraded to React 16.
After some debugging, I noticed that the order of execution of
<Editable />'s onFocus handler and<Slate />'s focus event listener were reversed in React 17. This led me to look at the React changelog, and it became clear that all this comes down to a change in howonFocusandonBlurare implemented in React 17 (see facebook/react#19186). As I understand it,onFocusandonBlurnow listen to thefocusinandfocusoutevents during the bubbling phase.Based on the above, I reasoned that listening to the
focusinandfocusoutevents withoutuseCapturein<Slate />should fix the order of execution. I'm happy to report that this is indeed the case!For backwards compatibility, I added a React version check, and only use the
focusinandfocusoutevents if React >= 17. However, I've since tested using thefocusinandfocusoutevents no matter the React version and it doesn't seem to cause any issues (the focus behaviour in React 16 and 17 look identical and all tests still passing). Let me know whether it would be preferable to remove the version check.Lastly, it would be good to get some input from @bryanph to verify that this doesn't cause the same issues as #4755 for them.
Checks
yarn test.yarn lint. (Fix errors withyarn fix.)yarn start.)yarn changeset add.)