Skip to content
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

Simplify react-dom patch #294

Merged
merged 1 commit into from
Sep 17, 2024
Merged

Simplify react-dom patch #294

merged 1 commit into from
Sep 17, 2024

Conversation

diegodlh
Copy link
Owner

Commit 6e2018e fixed the PID row text input bug where we were getting activeElement.detachEvent is not a function errors when clicking on text inputs in PID rows.

The fix involved importing createRoot dynamically. Otherwise, if imported at the top of the file, because window would be undefined, canUseDOM would be false in react-dom's react-dom.js, which in turn makes isInputEventSupported be false, which then makes handleEventsForInputEventPolyfill be called, which in the end makes activeElement.attachEvent() and activeElement.detachEvent() be called, which are both undefined.

However, as a result of importing createRoot dynamically, now canUseDOM is true, and the following line in react-dom-js is run:

if (navigator.userAgent.indexOf('Chrome') > -1 && navigator.userAgent.indexOf('Edge') === -1 || navigator.userAgent.indexOf('Firefox') > -1) {

However, navigator is not defined and an error is thrown. This is why a patch to react-dom was applied in f1792e7. I think a simpler patch could be applied that simply points navigator to window.navigator if undefined.

Anyways, I feel this whole fix is a little difficult to follow. Apparently a more straightforward fix may be possible (based on capricorn86/happy-dom/issues/534) which simply patches react-dom by replacing activeElement.attachEvent() and activeElement.detachEvent() calls with activeElement.addEventListener() and activeElement.removeEventListener() calls, respectively.

I have tested it and it seems to work. But because I'm not sure what the "PID row text input bug" before the original fix implied exactly, I would appreciate @Dominic-DallOsto's comments before merging.

Copy link
Collaborator

@Dominic-DallOsto Dominic-DallOsto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This simplifies things a lot, thanks!

I tested it and it works for me too. We were getting this error whenever a PIDRow got or lost focus, so it was easy to notice haha.

@Dominic-DallOsto Dominic-DallOsto merged commit 0de092f into zotero7 Sep 17, 2024
2 checks passed
@Dominic-DallOsto Dominic-DallOsto deleted the react-dom-patch branch September 17, 2024 18:22
Copy link

🚀 This ticket has been resolved in v1.0.0-beta.1. See Release 1.0.0-beta.1 for release notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants