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

Fix: Copy event and add/refactor tests #8777

Merged
merged 13 commits into from Mar 8, 2023
Merged

Fix: Copy event and add/refactor tests #8777

merged 13 commits into from Mar 8, 2023

Conversation

sprocketc
Copy link
Collaborator

@sprocketc sprocketc commented Mar 6, 2023

While trying to change the copy/paste functionality as discussed on #8636, I realized that I can't handle this by modifying usePaste.ts. The root cause seems to be the fact that (js/document.execCommand "copy") is firing twice on mod+c. :misc/copy always executes a document copy and then :editor/copy calls editor-handler/shortcut-copy, which also executes (js/document.execCommand "copy") on certain conditions.

As far as I understand, mod+c was introduced here 57d0436 to handle copying on the search panel on this PR #1191. If we remove this binding, using mod+c on the search input stops working. The changes on shortcut-copy were introduced on the same PR here e218ef7. I believe we can revert those changes, and just let editor-handler/shortcut-copy overwrite the clipboard when needed. mod+shift+c worked as expected in #8636, because (js/document.execCommand "copy") was called by shortcut-copy-text only.

Another cause of minor inconsistencies was calling navigator.clipboard.write directly, instead of using util/copy-to-clipboard! that handles some scenarios gracefully.

I added some copy/paste tests related to whiteboards. I also used modKey when possible to simplify some of our existing tests. Worth noting that there is no clipboard isolation between tests in playwright yet, so we may need to keep an eye on those tests. Also note that we cannot copy whiteboard shapes on CI, because ClipboardItem is not supported (see #5033).

@sprocketc sprocketc changed the title [wip] fix: copy Fix: copy event, add and refactor tests Mar 7, 2023
@sprocketc sprocketc changed the title Fix: copy event, add and refactor tests Fix: Copy event, add and refactor tests Mar 7, 2023
@sprocketc sprocketc marked this pull request as draft March 7, 2023 15:49
@sprocketc sprocketc changed the title Fix: Copy event, add and refactor tests Fix: Copy event and add/refactor tests Mar 7, 2023
@sprocketc sprocketc marked this pull request as ready for review March 7, 2023 18:56
@logseq-cldwalker
Copy link
Collaborator

@sprocketc Should I QA anything besides copy on the search input? Unclear if whiteboard copy behavior is changing

@sprocketc
Copy link
Collaborator Author

@logseq-cldwalker Yeah, copy/pasting shapes on whiteboards should now create a clone element instead of pasting a reference of the item. Mod+C should now behave the same way as Mod+Shift+C.

Copy link
Collaborator

@logseq-cldwalker logseq-cldwalker left a comment

Choose a reason for hiding this comment

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

@sprocketc Works as described! 👍 🚢

} else {
await page.keyboard.press('Control+c')
}
await page.keyboard.press(modKey + '+c')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice refactor. Makes tests easier to read

@@ -88,6 +88,63 @@ test('draw a rectangle', async ({ page }) => {
).not.toHaveCount(0)
})

test('copy/paste url to create an iFrame shape', async ({ page }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

❤️

Copy link
Contributor

@tiensonqin tiensonqin left a comment

Choose a reason for hiding this comment

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

Works great! 👍

@tiensonqin tiensonqin merged commit e52bda8 into master Mar 8, 2023
@tiensonqin tiensonqin deleted the fix/copy branch March 8, 2023 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants