WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.35k stars 4.13k forks source link

Group blocks inside row loose fixed width setting when converted to synced pattern #64332

Open simonwammerfors opened 1 month ago

simonwammerfors commented 1 month ago

Description

If I turn a group block inside a row block (like a contact card of something similar) into a synced pattern, the group block looses its settings for width (Dimensions -> Width). This in turn breaks the layout in the containing row block since the synced pattern tries to fill the whole width of the parent. This only happens if the pattern is synced though, unsynced patterns preserve the width setting.

It is possible to edit the synced pattern and then use the controls in Layout -> Content width to achieve a fixed width that will work in a row block, but it seems more intuitive that the block should simple preserve its own settings when it is converted to a synced pattern.

Step-by-step reproduction instructions

  1. Add a row block containing group blocks with fixed width.
  2. Turn one of the group blocks into a synced pattern.
  3. The width setting is removed from the synced pattern and the layout breaks.

Screenshots, screen recording, code snippet

No response

Environment info

WordPress 6.6.1, Gutenberg 18.9.0.

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

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

annezazu commented 1 month ago

Hey there! Thanks for opening. I tried to replicate this but wasn't able to using GB 18.9 and WP 6.6.1:

https://github.com/user-attachments/assets/3c00b8c8-a607-44d6-b580-f1fa4206d962

What am I doing wrong here or what am I missing?

simonwammerfors commented 1 month ago

Thanks for trying to replicate this. I started a site in Wordpress playground, put some blocks together and recorded a video. Hopefully this will show the issue more clearly.

https://github.com/user-attachments/assets/d7747487-f1bc-48dd-8100-6372116658b1

annezazu commented 1 month ago

Ah ha! Thank you. That’s much clearer. @aaronrobertshaw you previously helped resolve an issue with blocks losing their width when converting to synced patterns. If you have time, do you have a sense of what might be going on here? This feels both similar yet different πŸ˜„

aaronrobertshaw commented 1 month ago

@aaronrobertshaw you previously helped resolve an issue with blocks losing their width when converting to synced patterns

Thanks for the ping πŸ‘

From memory (which is cloudy at the moment), the issue I worked on was blocks losing their wide or full alignment rather than fixed width when within the context of a parent container. That does touch on what could be part of the issue though.

A pattern is intended to be reused across a site. In most scenarios, it probably doesn't make sense to include the instance-specific fixed width in the created pattern. I'm happy to be corrected on that of course, it's just my current understanding.

The pattern's creation is only one part of the equation though. In this scenario, once the pattern is created and replaces the existing blocks, the fixed width of those blocks is lost. That is unexpected and not the desired experience.

There's probably a couple of directions we could go:

  1. Maintain fixed widths by bringing that data into the created pattern, then see what impact that has if any on general pattern creation and management.
  2. Keep pattern creation as it is and update how the existing blocks get replaced by the pattern to reinstate fixed widths

This only happens if the pattern is synced though, unsynced patterns preserve the width setting.

I'll need to double check the following but it should be close πŸ˜…

Synced and unsynced patterns are handled quite differently. When creating synced patterns, the blocks are replaced with a reference to the pattern, hence the loss of instance-specific settings on the wrapper.

Upon creation of an unsynced pattern, the pattern is created separately while the existing blocks are left untouched. Each time an unsynced pattern is inserted, the pattern's blocks are added to the page with no container or reference to the pattern.

I also suspect it could be the case that if you create an unsynced pattern following the test instructions and then attempt to insert that new pattern, it would also be missing the fixed width data. I'll dig into this and update accordingly.

aaronrobertshaw commented 1 month ago

Quick update:

I was a little off with my understanding expressed above.

When the synced pattern is created, the Group block in the new parent does contain the fixed width size setting. This can be seen if you edit the pattern, wrap the group block in a row block, and then check the Group's fixed width.

After creating the synced pattern, if the page or post is saved and the frontend viewed, the fixed width will be applied correctly. This is because there is no pattern wrapper on the frontend, only the pattern's contents.

It is possible to edit the synced pattern and then use the controls in Layout -> Content width to achieve a fixed width that will work in a row block

I tested this and it makes no difference, in the editor, where the pattern is inserted. This makes sense as the pattern still has the wrapper around it in the editor preventing the Layout > Content Width style from applying, in the same way it does the fixed width.

  1. Maintain fixed widths by bringing that data into the created pattern, then see what impact that has if any on general pattern creation and management.

As noted above, the fixed width data is on the Group block in the pattern already. So this isn't a valid approach.

  1. Keep pattern creation as it is and update how the existing blocks get replaced by the pattern to reinstate fixed widths

This suggestion wasn't quite right either. The issue is more about how do we apply an inner block's child layout styles to the pattern block, similar to how alignments can be inferred from pattern contents and applied. See https://github.com/WordPress/gutenberg/pull/54416 for some history.

The issue I'm still wrestling with is that child layout styles are rendered via a useBlockProps hook that enqueues CSS via useStyleOverride to apply things like fixed width. These hooks are intended to run on a specific block with its current props. I'll need some time to see if the hook can be safely reused and called directly from the pattern block.

cc/ @andrewserong, as a layout expert, I might need to pick your brain on this one πŸ€”

andrewserong commented 1 month ago

Thanks for the ping @aaronrobertshaw! The direction you're heading in in your comment follows my immediate thoughts β€” it's a bit unfortunate for the way that our styling behaves that we need to have a pattern wrapper element for synced patterns in the editor, but I doubt there's much we can do to work around that.

I'll need some time to see if the hook can be safely reused and called directly from the pattern block.

That was my first thought, whether there's a way we can safely apply these styles on the parent wrapper / grab the values from the first child if there's only one child. That said, a couple of other things for us to keep in mind when investigating this:

So, from my perspective it sounds like the use case here (a pattern that includes width settings on the outer most / root block) is kind of a workaround for the bigger problem which is that we don't (yet) have universal width and height controls for blocks that isn't dependent on the presence of the parent. In such a case, then we could perhaps more reliably have a way for that sizing to be applied consistently wherever a patterns is used.

That said, getting to the point of universal width and height controls is probably a long ways off, so I think it'd still be good to figure out a way to pass the child layout rules "up" from the outer-most block within the pattern, or retrieve them from the first child from the perspective of the pattern block, in order to do a best effort to display the layout sizing as expected within the editor. I'm not sure how easy that would be to do, though, but sounds like a good idea for a path forward to me!

This comment wound up much wordier than I'd intended it to be, so please let me know if any of that didn't make sense, or if you'd like me to try to simplify πŸ™‚

Here is how this bug is looking to me in a site downgraded to WP 6.5.4, without Gutenberg running:

https://github.com/user-attachments/assets/bfabae27-51ec-4bb6-8042-c6f49ff0769e

In terms of guidelines for creating patterns or synced patterns, I think it's probably going to be more reliable for themes or sites to try to create patterns that include the wrapping Row block where possible, to avoid this bug / the absence of universal width and height controls. Depending on the desired layout, this might involve a bit more nesting than desired, but could be a user-based way to work around the issue in the short-term?

aaronrobertshaw commented 1 month ago

Thanks for sharing your thoughts @andrewserong, you articulated the point of patterns being reused across the site in different contexts better than I did πŸ™‚

Explicit Widths

is kind of a workaround for the bigger problem which is that we don't (yet) have universal width and height controls for blocks that isn't dependent on the presence of the parent.

A while ago we were very close to having the ability to define explicit widths for blocks via a width block support. See https://github.com/WordPress/gutenberg/pull/32502 for details.

The TL;DR there was that it was preferred if widths were intrinsic and determined by the parent. I believe a major factor in this thinking was that it was more flexible and robust across different device sizes. It's hard to see that decision being reversed without enhanced responsive design tooling.

Inferring Child Layout Styles From Inner Block

I think it'd still be good to figure out a way to pass the child layout rules "up" from the outer-most block within the pattern

Getting the appropriate data from the pattern's inner block is pretty straightforward. There's already a hook in the edit component that infers the inner layout and alignment. We can augment that to return the child layout style data easy enough.

After thinking on this further though, I'm not sold on migrating the child layout style data to the pattern wrapper.

As discussed, patterns are intended to be reusable across a site regardless of context and what layout their parent might enforce. Trying to implement some logic in the pattern block to determine when these fixed width styles should be brought "up" from the pattern's inner block, or not, could be complex and brittle. There could also be cases where a pattern is to be reused in two different Row blocks but the width isn't desired in one and there's no way to remove it. In other words, there's a risk of making the UX worse.

Recommendations

In terms of guidelines for creating patterns or synced patterns, I think it's probably going to be more reliable for themes or sites to try to create patterns that include the wrapping Row block where possible

I'd agree with this.

It could also probably be approached from the bottom up as well. For example, create the pattern from the Group block's content not the Group block itself. From there create the card containers as desired and use the pattern to insert the synced content. A block style variation could also be used to quickly apply styling to the card container, enforce a width etc.

It's not lost on me that this isn't very intuitive but I'm throwing it out there in case it can help.

andrewserong commented 1 month ago

Trying to implement some logic in the pattern block to determine when these fixed width styles should be brought "up" from the pattern's inner block, or not, could be complex and brittle. There could also be cases where a pattern is to be reused in two different Row blocks but the width isn't desired in one and there's no way to remove it. In other words, there's a risk of making the UX worse.

That's a very good point β€” it would be unfortunate to attempt to add logic to handle something when know it'd be brittle and not always work, and would likely require further up-keep.

For another potential long-term solution (not an immediate fix) there was also an exploration over in https://github.com/WordPress/gutenberg/pull/59195 to look into adding Width and Height controls to children of flow and constrained layouts. I'm just thinking out-loud, but once we're in a situation where all layout types allow those sorts of controls, we might be in a better position to re-evaluate inferring sizes in other situations (i.e. in wrapper Pattern structures)?

It could also probably be approached from the bottom up as well.

But yes, for now I agree with you about this. Either going more granular with building a pattern based on the content, or more broad and encapsulating the container block that "owns" the layout used by the child layout controls.

simonwammerfors commented 1 month ago

I see the intricacies involved in this! And also that it might just be better to construct my synced patterns with regard to these limitations. A the same time the experience in the back-end now is quite surprising. I'm thinking about if there is some way to communicate these limitations to the user somehow? Maybe when the pattern is created, in the modal? At least give a hint that the appearance of the synced pattern might vary depending on context and then getting pointed to relevant documentation?

aaronrobertshaw commented 1 month ago

Thanks for the continued feedback and ideas @simonwammerfors πŸ‘

I'm thinking about if there is some way to communicate these limitations to the user somehow? Maybe when the pattern is created, in the modal?

I've added the Needs Design Feedback label to this issue. There's a careful balance to strike between providing useful information, in a timely manner, and overwhelming the user.

Ultimately, I suspect we'll need to resolve, or mitigate, the root problem somehow rather than rely on warnings.

Maybe an exploration could be done into the following and see how far it gets us:

  1. When creating a new synced pattern, check if there is a single root block with a fixed width size
  2. If there is, create the synced pattern as normal however when replacing the selected blocks with the pattern, wrap the pattern in an appropriate group to restore the previous size.

When inserting the synced block elsewhere, it would still probably have to behave as it does now given it may be inserted to a context where the fixed width doesn't make sense.