facebook
fix: calling onblur event on unmount
Working wit React, there was a small bug i discovered, related to unmounting off a particular element in the DOM, in vanilla JS whenever a particular item gets removed from the DOM, an onBlur event is triggered automatically, as said in MDN
MDN LINK
React simply does not seem to call this
onBlur event when a particular element is unmounted, i might be wrong to judge why the same wasn't implemented in the context of react, anyways this would mean a learning opportunity for me to understand react design principles more better.
here is a Vanilla Js repro of the onBlur being called
https://github.com/facebook/react/assets/72331432/71764d95-8cf1-46f4-870c-7260496eb4ed
here is the React Codesandbox example, check console as the onBlur is not called
https://github.com/facebook/react/assets/72331432/f12b66bf-5bb9-4a0b-a0f5-aa37c57c3119
The fix i proposed here is if the deletedFiber object has memoizedProps and if those memoizedProps contain an onBlur property. If both conditions are true, it calls the onBlur function with a new blur event.
I have added a test for the same, to not allow regression to occur
Tested on the fixtures file after running yarn build
Passing Test
https://github.com/facebook/react/assets/72331432/badedda2-94fa-46b0-a1e7-3c24e9dd26ae
This Behaviour falls under the category where React and Vanilla JS act differently , earlier i raised #27016 , reviewed by sophie , which concluded to refer away from custom event logic as of now, as the future of react seems to get rid of React event system logic and rely on browser event system.
tagging @sebmarkbage @sophiebits @acdlite @rickhanlonii , would love to know all of your thoughts and what you feel about this.
Comparing: 01ab35a9a731dec69995fbd28f3ac7eaad11e183...2e8c2a1537a574762d0aaf5001a7067efd73b85c
Critical size changes
Includes critical production bundles, as well as any change greater than 2%:
| Name | +/- | Base | Current | +/- gzip | Base gzip | Current gzip |
|---|---|---|---|---|---|---|
| oss-stable/react-dom/cjs/react-dom.production.min.js | +0.06% | 176.89 kB | 176.99 kB | +0.09% | 55.14 kB | 55.18 kB |
| oss-experimental/react-dom/cjs/react-dom.production.min.js | +0.06% | 179.00 kB | 179.10 kB | +0.07% | 55.80 kB | 55.84 kB |
| facebook-www/ReactDOM-prod.classic.js | +0.02% | 594.04 kB | 594.19 kB | +0.05% | 104.94 kB | 105.00 kB |
| facebook-www/ReactDOM-prod.modern.js | +0.03% | 577.33 kB | 577.47 kB | +0.05% | 101.96 kB | 102.01 kB |
| test_utils/ReactAllWarnings.js | Deleted | 66.40 kB | 0.00 kB | Deleted | 16.26 kB | 0.00 kB |
Significant size changes
Includes any change greater than 0.2%:
Expand to show
| Name | +/- | Base | Current | +/- gzip | Base gzip | Current gzip |
|---|---|---|---|---|---|---|
| test_utils/ReactAllWarnings.js | Deleted | 66.40 kB | 0.00 kB | Deleted | 16.26 kB | 0.00 kB |
Generated by :no_entry_sign: dangerJS against 2e8c2a1537a574762d0aaf5001a7067efd73b85c
@sophiebits could you provide some feedback on this? mostly i feel this might not be appropriate though i don have much clarity compared to the core team!
hi @Biki-das — in a future React release we'll likely add React event listeners directly to DOM nodes so we would inherit the browser behavior with no additional work. it sounds like in this case we have an onBlur event that is fired by the browser after we remove the node, which means that by the time React processes it, the tree is fully unmounted and so we ignore the event
a proper fix here would need to somehow change that so that the event gets through — the implementation as you have it (a) would not bubble the event correctly to any ancestor nodes, (b) is DOM-specific but is in a file that is shared across renderers, so it definitely can't be landed in this state. as a result, I'm going to close this PR since it would need to be implemented in a very different way to make sense with the current codebase and I'm not sure offhand if/how we could do that
thanks for taking a look!
got it and thanks for the detailed feedback, so the concept of synthetic events are kinda non existent now @sophiebits ?