Skip to content
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

Pattern overrides: Image captions appear to not be handled correctly #62287

Closed
afercia opened this issue Jun 4, 2024 · 11 comments · Fixed by #62747
Closed

Pattern overrides: Image captions appear to not be handled correctly #62287

afercia opened this issue Jun 4, 2024 · 11 comments · Fixed by #62747
Assignees
Labels
[Feature] Block bindings [Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented Jun 4, 2024

Description

In a joined testing session with @carolinan and @SergeyBiryukov we wanted to get familiar with and test the Pattern overrides feature unblocked in #62011 and planned to be released in WordPress 6.6.

Our testing was limited to the Image and Paragraph blocks in a pattern so it was a very partial testing with only two blocks. We noticed a few unexpected behaviors especially with the Image block. We are concerned there may be more unexpected behaviors with other blocks at the point we shared the feeling this feature appears to not be fully ready to be released.

I'd like to point out we're not very familiar with the implementation and we tested this feature as 'advanced users'. Users expectations are important though and the general perception we got is that it doesn't feel polished enough to be 'ready'.

Personally, I would argue that content that disappears in an unexpected way or that is not replaced as users would expect doesn't contribute to perceive this feature as reliable and doesn't help users feel confident enough to use it.

Teh reproduction steps are a little complex, I'll try to simplify to only two very evident cases of unexpected behavior.

Step-by-step reproduction instructions

Scenario 1:

  • Add an image block picking an image from the Media Library that does have an alt attribute and does not have a caption.
  • Add a paragraph block.
  • Select the two blocks and click Create a pattern from the block toolbar ellipsis button.
  • Give a pattern a name, keep it synced, and save.
  • Select the Pattern and choose 'Edit original'.
  • In the pattern editor, select the Image block.
  • In the settings panel, expand 'Advanced' and click 'Enable overrides'.
  • In the modal dialog that oopens, giv ethe override a name and click 'Enable'.
  • Click 'Save' at the top right of the editor.
  • Go back to the post editor.
  • Select the overridable image in the Pattern.
  • Replace the image with one from the Media Library that does have a caption.
  • Save.
  • Observe the alt attribute has been replaced with the one of the new image.
  • Observe the image block does show a caption.
  • Refresh the page.
  • Observe the caption disappeared.
  • Expected: the caption of the new image to persist.

Scenario 2:

  • Repeat the steps above in a new post and creating a new pattern but this time start with an image that does have a caption.
  • At the end, replace the image with one from the Media Librady that does not have a caption.
  • Observe the caption of the old image persists.
  • Expected: the caption to disappear, as the new image doesn't have one.

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@afercia afercia added [Type] Bug An existing feature does not function as intended [Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) labels Jun 4, 2024
@talldan
Copy link
Contributor

talldan commented Jun 5, 2024

The heart of the problem is that the caption attribute isn't supported by block bindings, and the pattern overrides feature is built upon block bindings. So I've added the 'Block Bindings' label.

I agree, it's not ideal, I had really hoped it would be supported for the beta. I'm sure there are still options for solving this though.

The first issue is tracked on the iteration - #59819 (edit: I've now updated it to link to this issue). When I tested the other day I thought it had been solved, I'll do some more testing today to establish what's happening.

I think the expected behavior you mentioned is an interesting one. If the original pattern creator creates the pattern without a caption, then should the user of the pattern be able to add one? It should match the way the pattern (and by proxy the image) is expected to be used.

Similarly, perhaps pattern creators should almost be able to enforce a caption for other types of patterns where one is expected.

Edit: Would it be ok to rename the issue to be about image captions? If other issues could be created for other bugs you encounter that would be great. Thanks for testing btw!

@afercia
Copy link
Contributor Author

afercia commented Jun 5, 2024

@talldan thanks for your feedback.

I think the expected behavior you mentioned is an interesting one. If the original pattern creator creates the pattern without a caption, then should the user of the pattern be able to add one? It should match the way the pattern (and by proxy the image) is expected to be used.

Yes, when testing we were wondering about this. I would say captions are specific to a specific image, similarly to the alt and title attributes. I see a pattern as a 'suggested' layout and content. The user of the pattern should alwqays be able to edit / add / delete content. For example, the alt text can be added, edited, emptied. The caption should work the same way. Instead, making these properties work in a different way would be very confusing for users, I think.

No objections to renaming. It can always be re-renamed, if need be.

@talldan talldan changed the title Pattern overrides: Some overridable blocks properties appear to not be handled correctly Jun 5, 2024
@afercia
Copy link
Contributor Author

afercia commented Jun 5, 2024

After some more testing: it appears more properties of an image are not overridable>

  • Made a pattern that contains a linked image; A regular link to https://wordpress.org
  • Made the pattern synced and the image overridable.
  • When reusing the pattern, how do I edit the original image link? I couldn't find a way to do it.

Not sure a pattern with a linked image can be of any use if the pattern users can't edit the link?

@carolinan
Copy link
Contributor

@afercia the supported attributes are listed in the linked issue:

We currently maintain a hard-coded list of supported blocks and attributes:

core/paragraph: content
core/heading: content
core/button: text, url, linkTarget, rel
core/image: id, url, title, alt

I don't think there are plans to support more attributes in 6.6. So there needs to be short-term solutions for the unsupported attributes.

They should probably be removed when an image is replaced.

@afercia
Copy link
Contributor Author

afercia commented Jun 5, 2024

@carolinan found this Draft PR: #61255 @talldan should that be added to the priority list in #59819 ?

@talldan
Copy link
Contributor

talldan commented Jun 5, 2024

Hadn't seen that one before. Seems like it's missed 6.6 unfortunately, and it'd be a big change during the beta, though potentially parts of it could be merged as bug fixes? Not sure. I feel a little uncomfortable about the use of regexes to replace content in the PR, but we can continuing to look into solutions for this.

@talldan
Copy link
Contributor

talldan commented Jun 11, 2024

Observe the image block does show a caption.
Refresh the page.
Observe the caption disappeared.

I looked into the reason this happens, and it's due to the way block bindings updates the values for non-bound attributes (in this case, those that aren't supported by pattern overrides). This bit of code:

if ( Object.keys( keptAttributes ).length ) {
setAttributes( keptAttributes );
}

So when you upload an image, it sets a range of attributes, only some of them are supported by bindings/pattern overrides and are updated via the bindings. The unsupported attributes like caption are applied to the block via a normal call to setAttributes. In the case of a pattern instance, you're not editing the pattern source, so the change is only stored in memory and disappears after refreshing.

This is something that I don't think should happen for pattern overrides, as the unsupported attributes should never be modifiable from a pattern instance, they can only be edited in the original pattern.

The behavior does make sense for other types of block bindings where non-bound attributes can still be edited, so it's a tricky one to solve technically. Perhaps we could say that the presence of the __default binding means (#60694) means those other attributes shouldn't be updated (that the binding applies to all attributes even if only some are supported?).

@afercia
Copy link
Contributor Author

afercia commented Jun 11, 2024

Thanks @talldan for looking into it. Worth noting the Image block caption appears to be buggy also for normal, non-bound blocks. I reported it in a new issue: Image block: caption persists when replacing image #62468

@talldan
Copy link
Contributor

talldan commented Jun 11, 2024

I've made a PR that addresses the appearing/dissapearing caption - #62471.

I've also had a quick look at what it would take to support image captions for block bindings fully (but no PR yet). I think a first step could be making a similar change to the one we just did for the button block - #62220.

There's some client side code that conditionally renders the caption element for the image block:

{ ! RichText.isEmpty( caption ) && (
<RichText.Content
className={ __experimentalGetElementClassName( 'caption' ) }
tagName="figcaption"
value={ caption }
/>
) }

This could be moved to the block's render_callback to make it dynamic, just like that PR did for the button block. After that the block bindings code that runs on the server could replace the caption text dynamically and there's less concern over whether the figcaption element is present or not. The replacement of the text is the bit that's more difficult, the HTML Tag Processor doesn't support inner text, so a rock solid regular expression could be required.

@SantosGuillamot
Copy link
Contributor

Thanks for working on it! I'll take a look and give any feedback there.

I've also had a quick look at what it would take to support image captions for block bindings fully (but no PR yet). I think a first step could be making a similar change to the one we just did for the button block - #62220.

Regarding this, I plan to work again on supporting image attributes in this pull request. The original idea was to use something similar to what was done for the button block, although I have to review the logic again.

@ellatrix
Copy link
Member

Let's try to do this bandaid instead for 6.6: #62747. Adding support for caption seems a bit too late now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block bindings [Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
5 participants