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.

Sign up for GitHub

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

Fix for double paste in composer after function component mig #25419

Merged
merged 1 commit into from
Aug 17, 2023
Merged

Fix for double paste in composer after function component mig #25419

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/components/Composer/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,18 +363,18 @@ function Composer({
}

if (textInput.current) {
textInput.current.addEventListener('paste', handlePaste);
document.addEventListener('paste', handlePaste);
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is original code before function component refactor.
But I think it's better to set textInput as target as long as it doesn't cause any issues.
@bondydaa what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I was wondering about using document as well.

my gut says having it on the dom element instead of the document object is probably better, in the event we add other global paste type handlers in the future so it won't create a conflict in that event.

Copy link
Contributor

Choose a reason for hiding this comment

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

asked in open source here https://expensify.slack.com/archives/C01GTK53T8Q/p1692292251654389

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm previously I fixed an error where multiple composers existed at the same time like editing multiple messages and that's why handlePaste haves some logic of visibility. I'm not sure if maybe there is some logic behind that but I would prefer not to mess with it in case it does. Although if we move it to textInput we coudla void that logic of focus.
cc @mountiny in case you know there is logic about this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI Here is the issue of multiple composers #23502

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can go with this solution (document listener) for now as deploy blocker and discussion may not end up very soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 thanks for that extra context and yeah agree let's go with this for now, we can always fix it later if needed too. this stuff isn't being carved into stone 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

YEah probably that would've been a possible solution for #22803 maybe @Santhosh-Sellavel can give us a clue on why it was fixed in document instead of textInput

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a regression from #23359

Copy link
Collaborator

Choose a reason for hiding this comment

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

We used document only earlier

textInput.current.addEventListener('wheel', handleWheel);
}

return () => {
unsubscribeFocus();
unsubscribeBlur();
document.removeEventListener('paste', handlePaste);
// eslint-disable-next-line es/no-optional-chaining
if (!textInput.current) {
return;
}
document.removeEventListener('paste', handlePaste);
textInput.current.removeEventListener('wheel', handleWheel);
};
// eslint-disable-next-line react-hooks/exhaustive-deps
Expand Down