-
Notifications
You must be signed in to change notification settings - Fork 58
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
Pasting external content - known issues #624
Comments
Adding some notes here after my findings today while working on This one's tricky, as we gotta implement a mechanism to update the caret position from JS to Native, meaning we also have to implement a mechanism similar to the one used for the content (through This is something I don't feel very comfortable trying to rush for tomorrow, so I'll pause now. |
I tested pasting on the lists branch #704 and it seems to work fine 🎉 so I checked that item from the list. |
I can confirm this one happens with the post mentioned although other images work. It was interesting to notice that if I just copied an image from the post, it worked, but if the selection also contains some text before the image, the pasting skipped the image. There seems to be something going on in |
Still happening on Android: the keyboard is dismissed but the caret remains at the beginning of the paragraph. There are several issues currently with selection, so it might or might not be related. Worth a separate issue to track it. |
About this one, it's not clear to me if it always happens or only for some cases, and if we know which cases. From what I understand it's native specific and due to our DOM implementation. I know we're using JSDOM Level 1, maybe @hypest knows why exactly that level, and style is defined in level 2. So maybe just switching to JSDOM level 2 or 3 would help with this and other DOM issues? Besides the technical details, do we have any examples were this would be a problem? |
When I initially explored this, I had the same thought (maybe we can just solve some issues by using a different level). For the changes I needed to get the basics working, merely increasing the levels was insufficient, as some methods / functions were still missing, and there is still an exception being thrown that is outdated. But, now that those changes have been patched in, I think it is definitely worth re-exploring what gains can be easily made by simply changing the levels. Do you think it's worth waiting until we have some tests in place? I'm not entirely sure what other areas of the code make use of these DOM implementations.
This, I am unsure of... |
Thanks for the context. I think that without having a real world example of this, it doesn't feel like a big priority at the moment. I'd put this on hold until we get some reports of missing styling on pasting, or there's another issue that demands looking at JSDOM levels. |
As far as I recall from back then, I only wanted to add access to some constants defined in the core.Node, without needing to manually recreate them in our global.js. I think there is probably no reason to stick to Level 1. We can try 2 or 3. |
We've got a report of some pasting errors here: |
Thanks for reporting this @supernovia 👍 Do you know, offhand, whether a message was logged in the console while reproducing this? Also, any steps you can list to reproduce the issue would be very helpful (e.g. is there particular content from google docs that causes the issue, or just anything from that app?). |
Ah sorry I missed the ping, @mkevins . The user wasn't using console. I was not able to duplicate it on my end. I can ask them for specifically what they're pasting so I can try again to duplicate if you'd like. |
@mkevins they are not finding console errors. They did share extensive details about what they are trying to paste and which apps / formats give them problems, though. |
Thank you @supernovia for gathering more information. 😃 Regarding the console message, in particular, I was wondering whether there was a message about the paste handling fallback in the mobile app. The message is logged here. The mobile app has a try/catch to catch errors while parsing pasted content, and it falls back to plain-text when it encounters an error. The web version will not show that particular log message though. I'm not sure the user would see that, but I originally had the impression that you were also encountering the same issue:
Can you clarify whether you encountered the same issue? (also, on web, or mobile)? Thanks for your help on this! |
Hey @mkevins, I can try to duplicate this now, but how do I view the console in the app? |
Hey @supernovia 👋 😄 Are you using a development version of the app, or from the Play Store? Also, do you have Android Studio or |
I don't have Android Studio or adb. |
Hi @supernovia 👋 Without |
This came up in 13.6 beta testing. Steps to reproduce:
Result: cannot paste content into the title area. (iOS: 1m36s, Android: 39s) /hat tip @koke for the report (internal reference: p5T066-14K-p2#comment-3790) App logs from iOS test:
App logs from Android test:
|
Thanks @designsimply for all the detailed info! 😄 I've had a chance to look into this a bit, and I have a small change locally which seems to fix the issue, but it may be best to confirm with either @SergioEstevao and @ellatrix before drafting a PR. @SergioEstevao and @ellatrix 👋 I noticed the use of the Use block-editor RichText in post-titlediff --git a/packages/editor/src/components/post-title/index.native.js b/packages/editor/src/components/post-title/index.native.js
index a47de19cf..d63401698 100644
--- a/packages/editor/src/components/post-title/index.native.js
+++ b/packages/editor/src/components/post-title/index.native.js
@@ -8,13 +8,12 @@ import { isEmpty } from 'lodash';
* WordPress dependencies
*/
import { Component } from '@wordpress/element';
-import { __experimentalRichText as RichText } from '@wordpress/rich-text';
+import { RichText } from '@wordpress/block-editor';
import { decodeEntities } from '@wordpress/html-entities';
import { withDispatch, withSelect } from '@wordpress/data';
import { withFocusOutside } from '@wordpress/components';
import { withInstanceId, compose } from '@wordpress/compose';
import { __, sprintf } from '@wordpress/i18n';
-import { pasteHandler } from '@wordpress/blocks';
/**
* Internal dependencies
@@ -89,7 +88,6 @@ class PostTitle extends Component {
onSelectionChange={ () => { } }
onEnter={ this.props.onEnterPress }
disableEditingMenu={ true }
- __unstablePasteHandler={ pasteHandler }
__unstableIsSelected={ this.props.isSelected }
>
</RichText> |
@mkevins while your approach works I think it's best to use the base rich-text component instead of the block/editor aware one. Here is a PR with my proposed solution: WordPress/gutenberg#18479 |
I think we can check off and close the corresponding ticket for this one. See comment - #839 (comment) |
This issue is a follow-up to #238. First steps have been taken to implement pasting with the merging of #553 and WordPress/gutenberg#13841, however, additional work is required. Known issues and additional work needed are listed below.
Known issues
TypeError: this.props.value.trimLeft is not a function
(was working before merging recent changes to RichText - may be related togetRecord
) - (fixed with PR Fixes a red sceen when pasting. #638)Fix pasting into code blocks (see known issues in PR First steps to implement pasting external content #553)-> Code block #822Additional work
jsdom-patches.js
(related comment) (PR Add comments to clarify functions in jsdom-patches file. #826)jsdom-jscore
to support missing features from web codesplitContent
to a common interface with web, consider using options object (related comment)onSplit
as discussed here. Related: RichText: mark onSplit prop as unstable #394<span>
tags (details here)The text was updated successfully, but these errors were encountered: