Strip meta tags from pasted links in Chromium #36356
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
getdave
added
[Type] Bug
getdave
requested review from
cameronvoell,
ellatrix and
guarani
as code owners
Contributor
Author
|
@gVee Would you be able to verify this fixes the issue? |
|
Size Change: +36 B (0%) Total Size: 1.09 MB
ℹ️ View Unchanged
|
Contributor
Author
|
Also pinging @kevin940726 for review as he is the regex guru 😄 |
getdave
force-pushed
the
fix/strip-utf-meta-tag-from-pasted-links
branch
from
905c331 to
def7e39
Compare
kevin940726
reviewed
Nov 11, 2021
Member
kevin940726
left a comment
kevin940726
left a comment
There was a problem hiding this comment.
Yeh, there could be some false positives here. Since this is only happening in chromium and mac, should we add some UA detection to opt-in to this function?
Member
There was a problem hiding this comment.
Since that we have the exact source in chromium, I don't think we have to do regex search here. We can simply do:
const metaTag = "<meta charset='utf-8'>";
if (html.startsWith(metaTag)) {
return html.slice(metaTag.length);
}and maybe it's a little bit faster.
Contributor
Author
There was a problem hiding this comment.
Will that match on both single and double quotes around the Update: I checked and it does work 🥳 .charset attribute?
I'm sure I tried this as my first implementation and it didn't work but maybe I made a typo first time around.
Thanks.
Member
There was a problem hiding this comment.
Ahh I made a typo in the comment, I updated it.
Will that match on both single and double quotes around the charset attribute?
I don't think we have to worry about double quotes, no? Since we can clearly see that it's being hard-coded as single quotes in the source.
Contributor
Author
I don't think we need to do that. If the tag isn't there it won't be removed. The overhead is so minimal as to make the UA detection less valuable. |
getdave
force-pushed
the
fix/strip-utf-meta-tag-from-pasted-links
branch
from
b36ac88 to
62bb136
Compare
kevin940726
approved these changes
Nov 11, 2021
Contributor
Author
|
Moved discussion to #36392. @ellatrix I was looking at adding an integration test to cover this. Initially it seemed like we already have one but upon closer testing it doesn't assert on the same problem. According to the ...then all the filters in gutenberg/packages/block-editor/src/components/rich-text/use-paste-handler.js Lines 128 to 131 in 7a41d02 These filters are those within Therefore the integration test which exclusively uses It looks like we can't pass inline pasted content through If we don't do something it's going to be very difficult to assert on the handling of pasting rich inline content. For example, I tired adding an e2e tests to cover the Do you have any thoughts on the best way forward here? |
Contributor
Author
|
Going to merge this in order to fix the bug and we can circle back to integration tests and improving the actual implementation. |
noisysocks
removed
the
Backport to WP 6.9 Beta/RC
noisysocks
pushed a commit
that referenced
this pull request
Nov 15, 2021
* Strip meta tag * Corrections for accuracy * Simply by avoiding regex
|
sorry for late reply, and thanks @getdave @kevin940726 . It's working. Hope this back to WP soon. By the way, should it provide a way clearing script that dealing with superfluous meta tag in DB ? As mentioned like @isaumya here ? |
|
Hi @getdave & @kevin940726, Now after this patch though it won't be added again in the future contents but the old contents will still have the same artifacts present. So, naturally users needs to keep using functions like: add_filter( 'the_content', fn($content) => preg_replace('#</?meta(\s[^>]*)?>#i', '', $content) );in their I think the way WooCommerce has some asynchronous upgrader which runs when you upgrade to a major version, in this case Gutenberg should so something similar. What are your thoughts on this? |
Contributor
Author
|
I've raised this for the Open Floor of the next Core Editor meeting tomorrow https://make.wordpress.org/core/2021/11/22/editor-chat-agenda-24-november-2021/#comment-42139 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
In #33585 we learnt that Chromium appends a meta tag to the start of pasted HTML. This means when you copy and paste a link (even one you created within Gutenberg) the underlying post's HTML contains a meta tag.
This PR fixes that by stripping out that meta tag but only if it's at the very beginning of the pasted string.
Edge case - this will of course mean that if someone is trying to paste a string with that exact meta tag then it will be impossible, but I'm not sure we can work around that one and it is very much an edge case.
How has this been tested?
Contrast that with
trunkwhere the bug is still manifesting itself.Fixes #33585
Screenshots
Screen.Capture.on.2021-11-09.at.17-59-18.mp4
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).