fix: Copy address to clipboard on mobile devices - #220
Conversation
ArtemKolichenkov
left a comment
ArtemKolichenkov
left a comment
There was a problem hiding this comment.
First of all - if somebody reads this and wonders why the hell we need textarea to copy address - there are couple of interesting points about WebView and specifically Trust Wallet Browser mentioned in the original issue + issue in Trust Wallet repo.
Overall quite OK, you can ignore comments about html tags, it's not that important. The comment about not prioritizing queryCommandSupported would be great to fix though.
| const copyToClipboard = (content: string) => { | ||
| const el = document.createElement("textarea"); | ||
| el.value = content; | ||
| document.body.appendChild(el); |
There was a problem hiding this comment.
Was going to comment that we should specify el.style.display = "none" or el.style.visibility = "hidden" just to be safe. But - apparently el.select() doesn't work if the element is not actually visible on the screen.
Thankfully this happens quick enough so it is completely not noticeable for user.
| const [isTooltipDisplayed, setIsTooltipDisplayed] = useState(false); | ||
|
|
||
| const copyToClipboard = (content: string) => { | ||
| const el = document.createElement("textarea"); |
There was a problem hiding this comment.
This is a nitpick - but we could perhaps use input instead of textarea. Even though in 99% of the cases it is too quick to notice anything - in case where user has super slow computer they might notice something in theory and input just takes less space on the screen.
| if (document.queryCommandSupported("copy")) { | ||
| copyToClipboard(toCopy); | ||
| displayTooltip(); | ||
| } else if (navigator.clipboard) { | ||
| navigator.clipboard.writeText(toCopy); | ||
| setIsTooltipDisplayed(true); | ||
| setTimeout(() => { | ||
| setIsTooltipDisplayed(false); | ||
| }, 1000); | ||
| displayTooltip(); |
There was a problem hiding this comment.
- This should be the fallback only for WebViews, iFrames and weird browsers, not the primary way to copy the address, not to mention it is (getting) deprecated so it's a bit icky to prioritize it.
navigator.clipboardcheck should be first. And since technically Trust Wallet browser hasnavigator.clipboarddefined, it fails because its a WebView wherenavigator.permissionsisundefined(thus not having permission to copy), so we should check for that too. - I know that it "works" as it is right now, but technically
navigator.clipboard.writeText(toCopy)returns a promise, so we'd better handle it appropriately. Otherwise uses would see "Copied!" tooltip even if it didn't actually copied anything. Would be also nice to somehow handle error in.catch()but I guess (a) unless user uses extremely excotic browser it shouldn't happen (b) showing user "sorry we couldn't copy the address, browser is poop" is questionable 馃槃
EDIT: re-posted the comment to address multiple lines
| if (document.queryCommandSupported("copy")) { | |
| copyToClipboard(toCopy); | |
| displayTooltip(); | |
| } else if (navigator.clipboard) { | |
| navigator.clipboard.writeText(toCopy); | |
| setIsTooltipDisplayed(true); | |
| setTimeout(() => { | |
| setIsTooltipDisplayed(false); | |
| }, 1000); | |
| displayTooltip(); | |
| if (navigator.clipboard && navigator.permissions) { | |
| navigator.clipboard.writeText(toCopy).then(() => displayTooltip()); | |
| } else if (document.queryCommandSupported("copy")) { | |
| copyToClipboard(toCopy); | |
| displayTooltip(); | |
| } |
|
Migrated to #221 |
2 participants
No description provided.