Post Content Block: Render readonly content as blocks to preserve block supports styles #35863
Conversation
andrewserong
added
[Type] Bug
|
Size Change: +3.05 kB (0%) Total Size: 1.09 MB
ℹ️ View Unchanged
|
andrewserong
changed the title
andrewserong
changed the title
andrewserong
changed the title
|
Here's what I'm seeing:
✅ The duotone filter, social icons links and custom paragraph link colors are rendering correctly. I also inserted a bunch of other blocks just to see. All looked as expected. It'd be interesting to get the opinions of Site Editor experts, but based on these comments I think you've landed on a very nice workaround 🥇 |
| .block-editor-block-list__block[data-empty="true"] { | ||
| display: none; | ||
| } | ||
| } |
There was a problem hiding this comment.
Are the styles here specific to the post content or block or styles that should be added to all "block preview"?
There was a problem hiding this comment.
The fix should be in the preview and noticed this in this PR: https://github.com/WordPress/gutenberg/pull/35932/files#r735831633
--edit: I was only thinking of the appenders though.. I now noticed the placeholder styles here..
There was a problem hiding this comment.
I've moved these styles over to the live variation of the BlockPreview component. In addition to the placeholder styles, we also need the one for the buttons component so that Social Links buttons don't render greyed out.
So that this PR still tests well in isolation, I've left in the appender styling rule, but the approach to switch off the appender in #35932 looks like a better way to do it. I think we'll still need the other styles, though.
| return content?.protected && ! userCanEdit ? ( | ||
| <div { ...blockProps }> | ||
| <Warning>{ __( 'This content is password protected.' ) }</Warning> | ||
| </div> | ||
| ) : ( | ||
| <div { ...blockProps }> | ||
| <RawHTML key="html">{ content?.rendered }</RawHTML> | ||
| <BlockPreview |
There was a problem hiding this comment.
This is going to add an addition div container that might break alignments and things like that, I wonder if there's a way to render a preview without container or reuse the container above. cc @ntsekouras
There was a problem hiding this comment.
I've had a go at removing the wrapper div in BlockPreview by adding an __experimentalAsButton prop. It seems to work so far in testing, but let me know if you can think of a better approach.
| <RawHTML key="html">{ content?.rendered }</RawHTML> | ||
| <BlockPreview | ||
| blocks={ blocks } | ||
| tabIndex={ -1 } |
There was a problem hiding this comment.
Why do we need the tabIndex prop?
There was a problem hiding this comment.
This one's pretty subtle, but the tabIndex={ -1 } is required because the live BlockPreview sets tabIndex={ 0 } by default, which makes the block preview itself reachable when you press the up and down keys to navigate between blocks. By adding the -1 we remove its div from being reached by the up/down navigation. Here's a gif of what it looks like when we move between blocks if we remove this line:
In the above screengrab:
- The group block is selected
- I press the UP arrow once, and the Post Content block is selected, but the preview within it is highlighted
- I press the UP arrow again, and the Post Content block is still selected (instead of moving up to the next block), but the focus now moves to the parent container
There was a problem hiding this comment.
Update: I've added an __experimentalAsButton prop which will allow us to switch off the div wrapper that acted as a button, so I could remove the tabIndex prop.
|
Thanks for the review @youknowriad and the input @ntsekouras! I think I've managed to address the three pieces of feedback by doing the following:
There's probably still some overlap between this PR and #35932, so let me know if the above changes need tweaking further (or if it looks like the approach for removing the BlockPreview wrapper isn't going to work). Thanks! |
| __experimentalAsButton = true, | ||
| } ) { | ||
| const blockList = ( | ||
| <Disabled className="block-editor-block-preview__live-content"> |
There was a problem hiding this comment.
While this removes on container div, I believe <BlockList> also adds its own container which means we'd still break alignments rules if the post content block has a layout. right?
There was a problem hiding this comment.
Ah, yes, of course, I see now. I think it might be possible for us to pass just the .wp-container- class down to the BlockList to get alignments working. I had a hack around today and managed to get it working, but ran into an issue with withDataAlign not returning the data-align props for the Group blocks within the block preview (e.g. within post content). I could hard-code it to always show, but I'll need to do a little more digging to get it working cleanly.
I'll keep exploring tomorrow!
There was a problem hiding this comment.
What about finding a way to use BlockListItems instead of BlockList kind of like useInnerBlocksProps does.
There was a problem hiding this comment.
Oh, that's a good idea, thanks! I'll have a play with that tomorrow 👍
|
@youknowriad, I've managed to get this working in 8b4c0d8 by removing the final two layers of div nesting. Firstly, I've done the following, but I'm sure this will need some cleaning up / re-organising to neaten it up. I'd love feedback on where best to place the
The end result is that I believe this PR is now working as we expect it to, with the Post Content block correctly rendering custom content widths in the editor:
I'm at the end of my week, and have only just pushed the code change, so I'm sure I've missed things in the implementation (I also haven't checked to see if this introduces regressions elsewhere). I'll be away on Monday/Tuesday next week, but can pick this up again on Wednesday. Of course, if it's needed to get this in sooner, feel free to commit in the PR if there's any urgency. Otherwise I'll dig in again first thing Wednesday. Thanks! 🙇 |
There was a problem hiding this comment.
Thanks for your work, I think we're going in the right direction but this PR turned out a lot more complex than I was expecting initially :)
Reading the PR, it became clearer to me that what we really want here is a hook useBlockPreview that is very similar to useInnerBlocksProps hook but for previews (do not insert the blocks into the block store but relies on an alternative BlockEditorProvider) and the BlockPreview component would just rely on this hook internally
I think this way we won't have any issues with the classes needed to be manually applied to the wrapper... and no need for extra isButton prop...
| const layout = { | ||
| type: 'default', | ||
| alignments: [ ...DEFAULT_CONTROLS, ...WIDE_CONTROLS ], | ||
| }; |
There was a problem hiding this comment.
It seems the layout should be inherited from the parent block here, potentially it could be something completely different (like a flex block...)
There was a problem hiding this comment.
I've switched to inheriting the layout in the post-content block and passing it down to the new useBlockPreview hook
| 'TEXTAREA', | ||
| ]; | ||
|
|
||
| export function useDisabled() { |
There was a problem hiding this comment.
I think the right place for it might be @wordpress/compose WDYT?
There was a problem hiding this comment.
Good idea, I've moved it over.
|
Thanks again for the helpful feedback, Riad, much appreciated! 🙇
Funny how these things go! All it takes is a couple of little edge cases sometimes, and then the scope creeps up on us 😀
That's a terrific idea, that's just the kind of abstraction I was hoping we'd hone in on, thanks for the suggestion! I'll dig into that next week, hopefully it should make things much cleaner. Cheers! |
…erve block supports styles
…hide placeholders
…tyles to block-preview
andrewserong
force-pushed
the
try/swap-post-content-read-only-rendering-to-use-live-preview
branch
from
8b4c0d8
to
b06b374
Compare
…to the compose package
|
Thanks again @youknowriad, I've moved the BlockPreview behaviour for the Post Content block to a separate It also wasn't hard to pass the
I'm sure this'll need some more cleaning / tidying and testing, but since I'm at the end of my day, I've pushed where I'm up to. Let me know if there are any other immediate changes you think we should make. Otherwise, tomorrow I'll take another look with fresh eyes and see what I've missed / what else needs testing. I thought I'd leave the refactoring of the existing BlockPreview component to use the |
|
Thanks for the ping. I'm not entirely sure what to test for, so just taking the site editor for a quick spin seemed to work as intended. In the TT2 templates, the post content block is set to inherit styles by default, but tweaking those also worked as they should. |
|
|
||
| const blockPreviewProps = useBlockPreview( blockProps, { | ||
| blocks, | ||
| __experimentalLayout: themeSupportsLayout ? usedLayout : undefined, |
There was a problem hiding this comment.
Is the logic for the layout something that could be moved to useBlockPreview? You could potentially read the layout attribute from the block edit context.
There was a problem hiding this comment.
In theory, useBlockProps can be called inside useBlockPreview, too. We might need the same hook for Comment Content block one day so I'm thinking about how to make this hook easier to use in other places.
There was a problem hiding this comment.
That's a neat idea — I think for the moment, I'm leaning toward keeping this particular layout logic outside of the useBlockPreview hook, in case it's possible for us to use the hook in other circumstances where useBlockProps isn't appropriate. I'm thinking of arbitrary block previews that are used elsewhere in the repo, for example in the block switcher or for block patterns. They currently use the BlockPreview component directly, but it could be good to retain some flexibility for the hook in case we wanted to roll it out there, (or internally in the BlockPreview component itself?
There was a problem hiding this comment.
You are absolutely right. I didn't think about the original use case of <BlockPreview /> that was hidden in the changes for this PR. I guess the usage of the preview inside the edit is rather an edge case so we shouldn't expect that the block edit context is set here 👍
There was a problem hiding this comment.
Thanks for your work here @andrewserong !
My main concern is the security related one with content.raw. We need to be sure not to expose data where we shouldn't be.
| const [ , , content ] = useEntityProp( | ||
| 'postType', | ||
| postType, | ||
| 'content', | ||
| postId | ||
| ); | ||
| const blockProps = useBlockProps(); | ||
|
|
||
| const themeSupportsLayout = useSelect( ( select ) => { |
There was a problem hiding this comment.
Since now both EditableContent and ReadOnlyContent use the layout we can remove this logic from both and add it only to Content.
There was a problem hiding this comment.
Thanks, I've moved the layout logic up to the Content component 👍
| const defaultLayout = useSetting( 'layout' ) || {}; | ||
| const usedLayout = !! layout && layout.inherit ? defaultLayout : layout; | ||
|
|
||
| const rawContent = content?.raw; |
There was a problem hiding this comment.
I'm little hesitant with this change from content.rendered to content.raw. I think that this might lead to security issues with private content.. @peterwilsoncc can you share your thoughts?
There was a problem hiding this comment.
That's correct: previews need to use the rendered version as the raw version may not be available to all users. For example authors don't have raw rights for others' posts, contributors for any published post.
There was a problem hiding this comment.
Thanks for raising this @ntsekouras and for the feedback @peterwilsoncc! In the use case in this PR, unfortunately I don't think it'll work to use content.rendered as we need the raw markup of blocks in order to be able to parse them and have the styles be rendered properly by the preview.
As a bit of background, this PR is an alternative / workaround for the issue of rendering block supports styles on the server in order to display styles correctly in the editor. We currently have no way of retrieving the block supports styles from the server via an API request, so the next best option in this PR was to allow the editor to generate the styles from the block markup.
In the case of the Post Content block, in most cases, I imagine that users will see this block in the editor when editing template parts rather than in an isolated post. Because author and contributor roles don't have access to the site editor, in practice, I'm wondering if it isn't too much of an issue if we have the Post Content preview depend on access to content.raw. We already only render the blocks if content.raw is available, otherwise this falls back to rendering an empty preview, so on balance I think it could be more beneficial to prioritise the correct rendering for the admin view of the block, rather than focusing on author/contributor roles being able to view it in the editor.
|
Thanks for the feedback, everyone! 🙇 I've done the following small changes:
I think the last remaining question is whether or not we can comfortably use |
|
Thanks for all your work here @andrewserong !
I think this is the case here unfortunately and we should try to find some other way to resolve this issue. |
|
Per discussion above about this PR being blocked, closing it off so an alternative approach can be considered. |
peterwilsoncc
deleted the
try/swap-post-content-read-only-rendering-to-use-live-preview
branch
Description
This is an exploratory PR to come up with a shorter-term fix for #35376. By rendering the post content block as a live preview within the editor, we resolve the following issues:
We achieve the live block preview by doing the following:
useDisabledhook to provide a hook-based approach that achieves most of the functionality of theDisabledcomponent, without adding a wrapper element to the DOM.useBlockPreviewhook, that behaves similarly to theuseInnerBlocksPropshook, by allowing the consuming component to add the behaviour of a block preview to a wrapper element. This turns a component into a block preview, without adding an additional wrapper DOM element.useBlockPreviewhook usesuseDisabledinternally. The Post Content block is then updated to useuseBlockPreviewfor the live block preview.To-do
How has this been tested?
Screenshots
Note the following includes blocks that use Layout spacing, Duotone, and a custom link color (block supports that we need to ensure render correctly within the read only mode of the Post Content block):
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).