Skip to content

revert: remove anti-flicker clipboard styling #12227

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

Jump to bottom
Merged
merged 1 commit into from
Feb 20, 2024
Merged

revert: remove anti-flicker clipboard styling #12227

merged 1 commit into from
Feb 20, 2024

Conversation

Parkreiner
Copy link
Member

@Parkreiner Parkreiner commented Feb 20, 2024 •
edited
Loading

Changes made

  • Reverts logic for dummy input for clipboard-copying fallback behavior (triggers in non-secure contexts).

Notes

  • The current fallback for injecting text into the clipboard involves creating a separate input element that is filled with the clipboard text, and then manually copied. This element is always injected outside the React app (in fact, it has to be because it's a value that React doesn't know about), which can cause some layout shifts and potential UI flickers (particularly for full-screen layouts)
    • The CSS changes were intended to remove this issue by making the input "invisible" and removing it from the flow, while keeping it programmatically available for text manipulation
    • I'm still working on the automated testing, but no amount of manual testing I did could get the fallback to fail. I manually triggered errors to make logic flow to the fallback, but regardless of whether the input was using the old/new styling, the text was always getting copied.
    • Figured it'd be safest to revert to something that has been proven to work for most users, and then hurry up the migration to clipboard.js

@Parkreiner
revert: remove anti-flicker clipboard styling
68d5521 
- These CSS changes were for making sure there weren't layout shifts
  when using the non-secure clipboard fallback, which could cause janky
  UI flickers. It seems to be breaking things for some users on HTTP-only
  connections, though.
@Parkreiner Parkreiner self-assigned this Feb 20, 2024
cdr-bot[bot]
cdr-bot bot approved these changes Feb 20, 2024
View reviewed changes
Copy link

@cdr-bot cdr-bot bot left a comment

Choose a reason for hiding this comment

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

This PR is a hotfix and has been automatically approved.

  • ✅ Base is main
  • ✅ Has hotfix label
  • ✅ Head is from coder/coder
  • ✅ Less than 100 lines

Parkreiner
Parkreiner commented Feb 20, 2024
View reviewed changes
site/src/hooks/useClipboard.ts
Comment on lines +25 to +32
const input = document.createElement("input");
input.value = textToCopy;
document.body.appendChild(input);
input.focus();
input.select();
const isCopied = document.execCommand("copy");
document.body.removeChild(input);

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just the code that we had before all my changes, copied verbatim

@Parkreiner Parkreiner enabled auto-merge (squash) February 20, 2024 14:09
@Parkreiner Parkreiner merged commit d6ae9d8 into main Feb 20, 2024
@Parkreiner Parkreiner deleted the mes/clipboard-revert branch February 20, 2024 14:14
@github-actions github-actions bot locked and limited conversation to collaborators Feb 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cdr-bot cdr-bot[bot] cdr-bot[bot] approved these changes

Assignees

@Parkreiner Parkreiner

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@Parkreiner