-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Strip HTML from Post Title when pasting multiline title containing HTML #35825
Conversation
Size Change: +109 B (0%) Total Size: 1.29 MB
ℹ️ View Unchanged
|
7c2f74f
to
6237ea6
Compare
@@ -169,7 +170,7 @@ function PostTitle( _, forwardedRef ) { | |||
( firstBlock.name === 'core/heading' || | |||
firstBlock.name === 'core/paragraph' ) | |||
) { | |||
onUpdate( firstBlock.attributes.content ); | |||
onUpdate( stripHTML( firstBlock.attributes.content ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the plaintext
for if we need to stripHTML here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plainText
is a non-block representation of the content without any HTML, whereas content
may be an array of blocks whose attributes.content
may contain HTML.
So in the example provided in the description...
content
is:
[
{
"clientId": "ac523474-8aa7-4fa8-b732-2863e95f03d5",
"name": "core/paragraph",
"isValid": true,
"attributes": {
"content": "<strong>Puerto Rican Power: Pedraza Dominates Molina</strong><strong></strong>",
"dropCap": false
},
"innerBlocks": []
},
{
"clientId": "49782d58-9f9d-4617-bcfb-7ce93a6a34cb",
"name": "core/paragraph",
"isValid": true,
"attributes": {
"content": "<strong>Ajagba Decisions Rice in Heavyweight Co-Feature</strong>",
"dropCap": false
},
"innerBlocks": []
}
]
plainText
is:
Puerto Rican Power: Pedraza Dominates Molina
Ajagba Decisions Rice in Heavyweight Co-Feature
That's why this PR is stripping HTML from the attributes.content
of the pasted (parsed) blocks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should add that we can't just use plainText
here because the title seems to be designed to break into new blocks within the post-content. So you'll notice that when pasted Ajagba Decisions Rice in Heavyweight Co-Feature
becomes a block within the post content and not part of the post title.
6237ea6
to
a1248a3
Compare
Going to re-test as I think this may now be fixed. |
This is still a bug: Screen.Capture.on.2022-11-09.at.10-44-26.mp4 |
This fix strips the HTMl from the content field of the parsed blocks to avoid ending up with HTML in the title. Bear in mind that the title doesn't render HTML so you won't even know HTML has been pasted until you publish the post and find out the title/slug contains HTML. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works well for me.
const newValue = insert( value, create( { html: content } ) ); | ||
const newValue = insert( | ||
value, | ||
create( { html: stripHTML( content ) } ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I copy/pasted it resulted in a string, so I added this line to catch that case too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@scruffian can you remember what exactly you were talking about here? #54718 proposes reverting this change but I'm unsure as to whether it's a good idea.
Remember the goal is/was to avoid HTML being inadvertently added to post titles when pasting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what was happening was if I pasted HTML into the Post Title it could convert the HTML into the equivalent characters, so <
would become <
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 👍
Why is that not ok? Post titles have always allowed HTML tags, even though it's just a text field. The current post title field in a post editor is rich text but with formatting disabled, which is essentially plain text (text area), but that doesn't mean HTML tags are not allowed.
Sounds like a problem that needs to be address in slug generation? |
For me, the issue is that the HTML tags are invisible to the content provider. There's absolutely no indication that the post title contains something like . As a result, we've seen a huge uptick in people accidentally (and totally unknowingly) publishing posts with bold titles. If you go to edit the post, there's still no clear way to make it unbold. |
This reverts commit 6d804a9. # Conflicts: # package-lock.json
This reverts commit 6d804a9. # Conflicts: # package-lock.json
This reverts commit 6d804a9. # Conflicts: # package-lock.json
This reverts commit 6d804a9. # Conflicts: # package-lock.json
…iew (#54718) * Revert "Strip HTML from Post Title when pasting (#35825)" This reverts commit 6d804a9. * Preserve changes * Fix util to be more aware of menu expanded state before acting * Remove redundant “rich” component Addresses feedback in #54718 (comment) * Add explaination around stripping HTML tags on paste
Description
In this forum post, the reporter explains that pasting a multi-line string containing HTML into the Post Title field causes the following:
This is a particular problem because in the editor it appears to the user that the Post Title contains no HTML, but when you publish you get a surprise when you see the HTML formatting applied.
This PR addresses this by manually stripping HTML from the string before updating the editor.
stripHTML
method we will need to wait on #35539 otherwise it does mean that all leading spaces will be stripped from the post title.How has this been tested?
On Trunk - replicate the bug
Puerto Rican Power...
.<strong>
tags.On this PR - verify the fix
Go through the steps above again but this time verify that
Screenshots
Before
Screen.Capture.on.2021-10-21.at.10-02-43.mp4
After
Screen.Capture.on.2021-10-21.at.10-03-48.mp4
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.js
files for terms that need renaming or removal).