Patterns: disable image caption if part of synced pattern #58916
Conversation
This file contains 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
glendaviesnz
added
[Type] Bug
glendaviesnz
changed the title
|
Size Change: +68 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in a75bb56. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7908441575
|
glendaviesnz
requested review from
artemiomorales and
michalczaplinski
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
glendaviesnz
changed the title
49 tasks
talldan
reviewed
Feb 12, 2024
| /> | ||
|
|
||
| { lockCaption && ( | ||
| <figcaption>{ attributes.caption?.toHTMLString() }</figcaption> |
There was a problem hiding this comment.
Any HTML will be escaped by React (try using formatting or adding a link to the caption).
Is there another approach?
There was a problem hiding this comment.
TIL about .toHTMLString() 🙂 . The only issue is that if the caption text in the pattern is anything other than plain text, the HTML tags will show up in the caption in the editor:
output_d5797f.mp4
Could we pass it to dangerouslySetHTML and escape it like:
diff --git a/packages/block-library/src/image/image.js b/packages/block-library/src/image/image.js
index 4b46d085a77..3405c21007d 100644
--- a/packages/block-library/src/image/image.js
+++ b/packages/block-library/src/image/image.js
@@ -35,6 +35,7 @@ import { switchToBlockType } from '@wordpress/blocks';
import { crop, overlayText, upload } from '@wordpress/icons';
import { store as noticesStore } from '@wordpress/notices';
import { store as coreStore } from '@wordpress/core-data';
+import { safeHTML } from '@wordpress/dom';
/**
* Internal dependencies
@@ -795,7 +796,11 @@ export default function Image( {
{ img }
{ lockCaption && (
- <figcaption>{ attributes.caption?.toHTMLString() }</figcaption>
+ <figcaption
+ dangerouslySetInnerHTML={ {
+ __html: safeHTML( attributes.caption?.toHTMLString() ),
+ } }
+ ></figcaption>
) }
{ ! lockCaption && (I see that there's precedent for this in https://github.com/WordPress/gutenberg/pull/33814/files
There was a problem hiding this comment.
ah, yeh, html escaping 🤦. Instead of using dangerouslySetInnerHTML we could pass through props to the RichText component to disable editing. We can do this either with an explicit disableEditing and use that to set the same shouldDisableEditing flag that block bindings lockAttributesEditing is using - as here.
Or we could pass contentEditable and aria-readonly props from the image caption and plumb these through the Caption component with a ...props, as here .
The second one does not alter the external API of the RichText component so maybe that is the approach to take? Happy to hear others thoughts on this.
There was a problem hiding this comment.
Or we could pass contentEditable and aria-readonly props from the image caption and plumb these through the Caption component with a ...props
It might be risky, as I think there's some programmatic modification of the contenteditable attribute directly via the DOM API elsewhere in the codebase (for example - https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/components/writing-flow/use-arrow-nav.js#L265).
So you could set it via React, but it might not stay that way.
We can do this either with an explicit disableEditing and use that to set the same shouldDisableEditing flag that block bindings lockAttributesEditing is using
This could be a good idea, though it may need to be private initially, which means going through the dance of making a private version of the RichText component.
There was a problem hiding this comment.
yeh, I don't like relying on ...props drilling for something critical as it can easily get broken somewhere in the component chain, as well as by the external DOM manipulations, so will follow up on the disableEditing approach even though it is trickier in terms of altering the API.
fabiankaegy
added
the
Backport to WP 6.7 Beta/RC
|
I think this qualifies as a bugfix that should get included in WordPress 6.5 if time allows. So I added the relevant label and added it to the project board. |
|
Agreed. Thanks for adding Fabian! |
glendaviesnz
requested review from
ellatrix,
fluiddot,
dcalhoun and
ajitbohra
as code owners
|
I have added a new private prop to RichText, |
glendaviesnz
commented
Feb 14, 2024
| @@ -109,6 +109,7 @@ export function RichTextWrapper( | |||
| __unstableDisableFormats: disableFormats, | |||
| disableLineBreaks, | |||
| __unstableAllowPrefixTransformations, | |||
| privateDisableEditing, | |||
There was a problem hiding this comment.
I suspect I have taken the private API docs too literally here - they show an example with the new prop prefixed with private, but I suspect that negates a bit of the value of the private API, in that once the API is made public we can just unwrap it and all the existing props will just work without having to be renamed?
There was a problem hiding this comment.
have removed the prefix
talldan
reviewed
Feb 14, 2024
This reverts commit e1019b5.
Co-authored-by: Daniel Richards <daniel.richards@automattic.com>
glendaviesnz
force-pushed
the
fix/image-caption-in-pattern-overrides
branch
from
e8cf4f6 to
03a8d89
Compare
|
@ellatrix given your background with the richtext component just checking if you have any concerns about this before we merge? |
getdave
pushed a commit
that referenced
this pull request
Feb 27, 2024
Co-authored-by: glendaviesnz <glendaviesnz@git.wordpress.org> Co-authored-by: talldan <talldanwp@git.wordpress.org> Co-authored-by: michalczaplinski <czapla@git.wordpress.org> Co-authored-by: fabiankaegy <fabiankaegy@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org>
|
I just cherry-picked this PR to the cherry-pick-wp-6-5-beta-3 branch to get it included in the next release: c87e94c |
getdave
added
Backported to WP Core
9 tasks
getdave
pushed a commit
that referenced
this pull request
Feb 27, 2024
Co-authored-by: glendaviesnz <glendaviesnz@git.wordpress.org> Co-authored-by: talldan <talldanwp@git.wordpress.org> Co-authored-by: michalczaplinski <czapla@git.wordpress.org> Co-authored-by: fabiankaegy <fabiankaegy@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org>
ellatrix
reviewed
Feb 27, 2024
| <Caption | ||
| attributes={ attributes } | ||
| setAttributes={ setAttributes } | ||
| isSelected={ isSingleSelected } | ||
| insertBlocksAfter={ insertBlocksAfter } | ||
| label={ __( 'Image caption text' ) } | ||
| showToolbarButton={ isSingleSelected && hasNonContentControls } | ||
| disableEditing={ lockCaption } |
There was a problem hiding this comment.
This doesn't seem scalable. What about rich text everywhere else?
There was a problem hiding this comment.
@ellatrix I don't see it as something that needs to be scalable. At the moment caption is disabled because it's an edge case, one of the few content attributes that block bindings can't process (due to the lack of a replace_inner_html function in the html processor).
In the future when caption is supported by block bindings it'll be possible to revert this PR.
dcalhoun
added a commit
that referenced
this pull request
Mar 28, 2024
This export was added for interoperability with the web version modified in #58916.
dcalhoun
added a commit
that referenced
this pull request
Mar 28, 2024
* test: Define missing `logException` native module mock Prevent cryptic test failure messages originating from invoking this undefined method, which prevented the original error message from surfacing in the test failure log. * refactor: Export private API Rich Text module This export was added for interoperability with the web version modified in #58916.
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.
What?
Disables the image caption if the image is part of a sync pattern.
Why?
Currently it is not possible to add a binding for the caption as the HTML processor is not capable of replacing the innerHTML on the frontend. But currently in the editor an bound image in a pattern still allows the caption to edited, but these changes are not saved or displayed on the frontend.
How?
Shows a standard
figcaptionfor caption instead of a rich text input if the image is the child of a synced pattern.Testing Instructions
Allow instance overrideson the image, and add a caption to the imageRichText, eg.Paragraph, and check that it works as expected stillScreenshots or screencast
captions.mp4