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

Jump to bottom

Keyboard & Right-Click Menu Copy + Paste #3083

Merged
merged 113 commits into from
Sep 12, 2019
Merged

Keyboard & Right-Click Menu Copy + Paste #3083

merged 113 commits into from
Sep 12, 2019

Conversation

miina
Copy link
Contributor

@miina miina commented Aug 23, 2019 •
edited
Loading

Closes #2996
Closes #2990
Closes #2989

Todo:

  • Handle onPaste in Page
  • Ensure only allowed blocks are copied
  •  Verify that all the blocks work
  • Include Right-click menu copy-cut, etc.
  • Workaround for Pasting.
  • Paste Handling for Chrome
  • Fix popover positioning.
  • Make sure it's accessible.
  • Refactor code.
  • E2E tests.

@googlebot googlebot added the cla: yes Signed the Google CLA label Aug 23, 2019
@miina miina changed the title Keyboard Copy + Paste [WIP] Keyboard Copy + Paste Aug 23, 2019
@swissspidy
Copy link
Collaborator

Nice, can‘t wait to test this!

@miina
Copy link
Contributor Author

miina commented Aug 28, 2019

There is an issue with keyboard copying, namely, the CopyHandler doesn't always trigger for many blocks, looks like it triggers only if the block is selected in some specific way (so far first dragging the block and then copying seems to work the best). This seems to be an issue with regular post and the default block editor as well, e.g. adding an image block, clicking on it, and then pasting into empty paragraph sometimes copies the block into clipboard but not always.

The following blocks seem to work OK:

  • text
  • title
  • author
  • date
  • Embed

The rest of the blocks are not reliably copied to clipboard. If the block gets copied then everything works as expected.

Debugged the upstream CopyHandler to verify it's not reliably triggering. Also tried adding a custom copy handler by using onCopyCapture instead, also tried adding the ` handler directly wrapping each block, still not triggering for most of the blocks.

@swissspidy Any thoughts on this?

Otherwise, looks like the Right-Click menu might be the way to reliably implement this feature.

@swissspidy
Copy link
Collaborator

Haven't gotten around to test this myself yet, but what you describe sounds like an upstream bug (or perhaps even a browser issue) that we can't work around ourselves. Is there any related issue in the Gutenberg repository about the copy handler?

Otherwise, looks like the Right-Click menu might be the way to reliably implement this feature.

👍

@miina
Copy link
Contributor Author

miina commented Aug 28, 2019

Is there any related issue in the Gutenberg repository about the copy handler?

Couldn't find it as much as I checked at least, can look some more. I'll try testing it again later with clean WP Install (with AMP Plugin completely disabled) to see if that's still the case and then open an issue if yes.

@miina miina changed the title [WIP] Keyboard Copy + Paste [WIP] Keyboard & Right-Click Copy + Paste Aug 28, 2019
@miina miina changed the title [WIP] Keyboard & Right-Click Copy + Paste [WIP] Keyboard & Right-Click Menu Copy + Paste Aug 28, 2019
spacedmonkey and others added 5 commits September 10, 2019 16:42
@spacedmonkey
Allow blocks to be copied many times.
ddce164
@spacedmonkey
Fix pasting text elements will now paste them when the cusor is.
66953f1
@spacedmonkey @swissspidy
Use templates
15c7731 
Co-Authored-By: Pascal Birchler <pascalb@google.com>
@spacedmonkey
Change variable names.
c4f3b2b
@spacedmonkey
Merge remote-tracking branch 'origin/add/2996-keyboard-copy-paste' in…
3cff1e2 
…to add/2996-keyboard-copy-paste

# Conflicts:
#	assets/src/stories-editor/blocks/amp-story-page/edit.js
#	assets/src/stories-editor/helpers/index.js
#	assets/src/stories-editor/index.js
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no Has not signed the Google CLA and removed cla: yes Signed the Google CLA labels Sep 10, 2019
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes Signed the Google CLA and removed cla: no Has not signed the Google CLA labels Sep 10, 2019
swissspidy
swissspidy approved these changes Sep 11, 2019
View reviewed changes
Copy link
Collaborator

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

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

Everything is working as expected:

  • Keyboard copy/paste
  • right click context menu for copying and pasting
  • browser clipboard permission dialog

@spacedmonkey spacedmonkey force-pushed the add/2996-keyboard-copy-paste branch from 27023d1 to 8dbf87b Compare September 11, 2019 16:24
@spacedmonkey spacedmonkey force-pushed the add/2996-keyboard-copy-paste branch from ecf6b79 to a425d0b Compare September 11, 2019 17:28
@spacedmonkey spacedmonkey force-pushed the add/2996-keyboard-copy-paste branch from c686037 to 593086a Compare September 11, 2019 18:01
@spacedmonkey
Copy link
Contributor

E2E tests are passing. Can someone re-review this?

spacedmonkey
spacedmonkey approved these changes Sep 12, 2019
View reviewed changes
Copy link
Contributor

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

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

👍

@swissspidy swissspidy merged commit c406f2a into develop Sep 12, 2019
@swissspidy swissspidy deleted the add/2996-keyboard-copy-paste branch September 12, 2019 09:58
westonruter added a commit that referenced this pull request Sep 14, 2019
@westonruter
Merge branch 'develop' of github.com:ampproject/amp-wp into fix/3137-…
9b52059 
…keep-attributes-with-mustache-placeholders-intact

* 'develop' of github.com:ampproject/amp-wp: (113 commits)
  This converts the keyboard cut handler to equal a copy handler to avoid bugs
  Fix aria-label adding helper function
  Remove extra line I added in resolving merge conflict
  Fix alignment of arrow
  Fix custom CTA text for page attachment
  Fix cut handler by shortcut
  Cleanup of duplicated comment
  Add unit testing to `addVideoAriaLabel`
  Remove unused piece of code
  Remove Cloudflare AMP cache since deprecated
  Handle cut keyboard requests. (#3231)
  Page Attachment block (#3035)
  Keyboard & Right-Click Menu Copy + Paste (#3083)
  Animation Previews (#3104)
  Make internal methods inaccessible
  Omit the ecosystem link also when using a core theme
  Add skipped e2e test for the video block
  Add array_colum() pollyfill for PHP < 5.5
  Add asserts to make sure we are not enqueueing both versions of dashicons
  Remove useless variable
  ...
@swissspidy swissspidy mentioned this pull request Sep 16, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spacedmonkey spacedmonkey spacedmonkey approved these changes

@swissspidy swissspidy swissspidy approved these changes

@barklund barklund Awaiting requested review from barklund

@dero dero Awaiting requested review from dero

Assignees
No one assigned
Labels
cla: yes Signed the Google CLA
Projects
None yet
Milestone
v1.3
5 participants
@miina @swissspidy @spacedmonkey @googlebot @barklund