Render html in post titles in visual mode and edit HTML in post title in code view #54718
Conversation
|
Size Change: +1.75 kB (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
This looks like a really good way forward. Great idea. |
|
Flaky tests detected in f902fd3. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6532037168
|
| __unstableDisableFormats: true, | ||
| __unstableDisableFormats: false, |
There was a problem hiding this comment.
This is what allows formats (HTML) to render in the field when in Visual mode.
There was a problem hiding this comment.
Yeh it can. Leaving it in for explanation purposes but now it can go.
| const richEditorRef = useMergeRefs( [ richTextRef, ref ] ); | ||
|
|
||
| // Setting to "2" will not render raw HTML. | ||
| const plainTextVersion = 1; |
There was a problem hiding this comment.
@ellatrix I don't know why but using v2, caused the HTML not to render at all. The tags were stripped from the content. Whereas in v1, the tags are preserved in raw form (which is what we want in "Code view" mode).
There was a problem hiding this comment.
PlainText is not really meant to be used outside of blocks btw. I'll have a look into why this happens though. If it's a bug, it should happens in some blocks
getdave
changed the title
|
I think this is a great change, much better experience that aligns what you see in the editor to what you see in the frontend |
|
As there are no objections I'm going to refine the implementation a bit and ensure the "strip HTML on paste" gets reinstated. |
|
Another idea I had is to alert users to the fact that their title contains HTML using a tooltip which appears on hover: 
|
getdave
requested review from
kevin940726,
Mamaduka and
alexstine
as code owners
getdave
force-pushed
the
fix/allow-render-html-in-post-titles
branch
from
5880cc6
to
e6e3080
Compare
|
@ironprogrammer @azaozz As we recently discussed the topic of HTML in post titles (away from Github) I am curious to your thoughts on this experience. Specifically
Thank you 🙇 |
|
Also pinging @audrasjb as he was involved in a related discussion in Core and I would very much appreciate his perspective insight. |
getdave
added
Needs Design Feedback
|
Added tags to request review from Design team. Specifically because this will impact the display of the title in the case that the post contains HTML. |
|
Also pinging @Rottinator who raised the original Issue and @MadtownLems who was involved in discussion. |
|
In response to @getdave's comment:
Yes. I think that in the editor representing the visual display of how the title will ultimately render in a post is important for consistency, even though it differs from how this is treated in the classic editor. Anyone adding markup via code view should expect seeing the "final product" in visual mode.
I agree with your caution here, and feel we should avoid accepting pasted HTML for now. The markup could be a "Trojan horse" of tags that weren't intended (reminds me of visually unexpected examples given in #38637). Markup in the title should be intentional, which would mitigate copy-paste surprises. As a reference for "legacy" behavior, I've attached a short video that shows how markup in titles is handled with the classic editor. This PR's treatment of markup through code editor mode is a good adaptation to maintain this support 👍🏻. [Click to Expand] Adding title markup with the classic editorclassic-editor-title-markup.mp4 |
|
@kevin940726 I wonder do you think my change in a35ade3 to amend the util to be more "aware" of the current state of the menu before it tries to open/close it is a good idea. The purpose is to avoid situations whereby the menu is already closed but the code is acting to make it "closed" which ironically then toggles it to be open again (which was the cause of this Issue). |
Seems like a safe addition 👍. Ideally though, we should be able to control the state throughout the test so we don't have to do manual checking. This type of checks are often only needed when the actions are not declarative and predictable enough for regular readers, which can be seen as a potential source of flakiness or bad practice. |
|
Not sure if it's been discussed before, but the post title block should also be updated to reflect these changes. |
Do you feel that should be in this PR or can I start a follow up PR? |
| ( firstBlock.name === 'core/heading' || | ||
| firstBlock.name === 'core/paragraph' ) | ||
| ) { | ||
| onUpdate( stripHTML( firstBlock.attributes.content ) ); |
There was a problem hiding this comment.
Shouldn't this be reverted? (Introduced in #35825)
HTML is now allowed, so why strip?
There was a problem hiding this comment.
It's counterintuitive but it's intended. Why? The idea is that if you are pasting into the title you are very unlikely to want HTML to go into it.
A common action is copy/paste and accidentally including HTML in the pasted content. Then it ends up in the title.
With this PR at least you can see that HTML, but this way it avoids it being added entirely.
I'm not strongly wedded to this change so if you feel it would be better to simply allow HTML and have the user remove it manually then I'd be happy to remove it.
There was a problem hiding this comment.
Improved code comments with explanation as to why this remains in place.
Related to that, can't we somehow use/embed the site title block in the post editor so we also have the rich text toolbar? It feels a bit strange that I can see the HTML visually, but can't change it. |
There was a problem hiding this comment.
Just wondering about #54718 (comment), otherwise it looks good
I intentionally didn't want the rich text toolbar here because I felt it was too large a change for a single PR. Rather I thought let's get the support for rendering HTML merged so that at least users can perceive the presence of HTML. After that we can then have a separate (smaller) PR which enables the toolbar and allows for rich text formatting to be updated via the editor. I had thought that perhaps discouraging the use of HTML would be a good idea, but maybe that's not necessarily the best route. I'm not sure yet. |
|
Two 👍 on this PR so I've set to auto merged. |
|
After this PR was merged, I noticed that the title field has a double border in the code editor and is resizable. Is this the intended spec layout?
|
|
@t-hamano Ah yes I hadn't noticed that. It could be tidied up to look more like it was yes. |
| const { ref: richTextRef } = useRichText( { | ||
| value: title, | ||
| onChange, | ||
| placeholder: decodedPlaceholder, | ||
| decodedPlaceholder, |
There was a problem hiding this comment.
@getdave This change makes the placeholder text now show in the post editor.
What?
Updates the Post Title so that:
Note: is is intended that you cannot add/edit HTML in visual mode (i.e. there is not toolbar). This could be added as a followup, but the goal of this PR is an iterative step to ensure users can perceive the presence of HTML in the title.
Closes #38668
Why?
HTML is allowed in post titles in WordPress.
A recent consensus in Core is that HTML should be displayed and visible to the user.
Currently in the Block Editor, there is no way for users to perceive or edit HTML that is in the Post Title when in the editor. This is because the HTML is not rendered at all but it is persisted on save.
This provides a poor experience and leads to titles with hidden HTML 👎
How?
Updates the Post to:
Note
PostTitlecomponent remains "as was" with no functionality changes. For the "raw text" version a new component is available calledPostTitleRaw. This is so as to ensure backwards compat for any consumers of the PostTitle block.Testing Instructions
http://gutenberg.run/54718
Code editor).Visual editor).Now check that you cannot paste HTML:
Testing Instructions for Keyboard
Screenshots or screencast
Screen.Capture.on.2023-09-22.at.09-53-17.1.mp4