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

Cover block not working when changing the aspect ratio #60224

Open
westnz opened this issue Mar 26, 2024 · 6 comments
Open

Cover block not working when changing the aspect ratio #60224

westnz opened this issue Mar 26, 2024 · 6 comments
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Type] Bug An existing feature does not function as intended

Comments

@westnz
Copy link

westnz commented Mar 26, 2024

Description

When I use the 6.5 Beta Tester plugin and change the aspect ratio of the Cover block, the text does not always display correctly when selecting mobile view. For example, when I select the 'wide' aspect ratio with three sentences of text, the text cuts off in mobile view. A second issue I found was that when I changed the aspect ratio back to 'Original', the block did not display correctly.

I don't have any other plugins installed, only Beta Tester. I am using Local and the Twenty Twenty-Four theme.
The expected output when changing the aspect ratio is that the text will change accordingly for mobile view and that you can revert back to 'Original'.

Step-by-step reproduction instructions

  1. Add a Cover block
  2. Add image
  3. Change the aspect ratio to Wide
  4. Add four sentences
  5. View on mobile
  6. Change the aspect ratio back to Original

Screenshots, screen recording, code snippet

Screenshot 2024-03-27 at 8 56 43 AM Screenshot 2024-03-27 at 8 57 06 AM

Environment info

Gutenberg not installed - only Beta Tester plugin. I did test it with Gutenberg 17.9.0 turned on as well.

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

@westnz westnz added the [Type] Bug An existing feature does not function as intended label Mar 26, 2024
@andrewserong andrewserong added the [Block] Cover Affects the Cover Block - used to display content laid over a background image label Mar 26, 2024
@andrewserong
Copy link
Contributor

Thanks for opening this issue @westnz! I've added it to the tracking issue for follow-up tasks over in #58230

For example, when I select the 'wide' aspect ratio with three sentences of text, the text cuts off in mobile view.

Unfortunately this is a limitation of using aspect ratio with the Cover block. When using an aspect ratio, the aspect ratio is preserved even if text extends beyond the bounds of the block. There was some initial discussion about this on the original PR (#56897) and more recently on a Cover block bug fix PR (#59388) that restored the original overflow rules. I'll just ping @jasmussen, @jameskoster and @tellthemachines for visibility since they were involved in some of those discussions. In short, I think the discussion was summed up in this comment:

I think we can explore an overflow design tool as a separate thing, and leave it to an editorial responsibility to ensure things look good here.

What this means is that for WP 6.5, when using the aspect ratio control for Cover blocks, it will preserve the aspect ratio at all screen sizes. This makes it suitable for simple Cover blocks with, say, a single heading or a button, but likely not for cover blocks containing paragraph-like content.

For future WP versions, I think it'd be great for us to explore extending the options for how overflowing content is handled: e.g. if it extends the container, if it switches off the aspect ratio, or if it displays a scrollbar, etc. Any exploration there will need to be careful to avoid regressions, as was brought up in #59388.


A second issue I found was that when I changed the aspect ratio back to 'Original', the block did not display correctly.

This is an interesting one. To work around it in 6.5, to clear out the value, you can either set a "minimum height of cover value" or go to the ellipsis menu for Dimensions and then click RESET next to Aspect ratio:

image

What's happening on trunk is that if you deliberately change aspect ratios and then select Original, it'll output auto for the aspect ratio, and switch off the minimum height for the Cover block, so that the Cover block is just the size of its content. I imagine most of the time that's not what's desired, as folks are just wanting to clear the value. One potential use case for deliberately outputting auto is in order to override an aspect ratio that is set in global styles. I.e. if the Cover block in global styles is set to 16/9, it might be good for individual cover blocks to be able to override that value and set the block to auto explicitly?

The confusion surrounding the "Original" option is also flagged in the tracking issue (#58230).


To sum up: the aspect ratio support in 6.5 is in its first state, which is useful for some but not all use cases. I think it'd be good to come up with more options for how we handle overflowing content, but I doubt there'll be any solutions ready in time for 6.5. It would be a good thing to expand and enhance for 6.6, though.

For the issue of switching to Original and it overriding the minimum height of the Cover block, that does look worth fixing if we can. Since it's possible for a user to reset via the ellipsis menu or by setting a minimum height of the cover block, I think it's a good idea to fix it, but IMO can wait until WP 6.5.1.

@westnz
Copy link
Author

westnz commented Mar 27, 2024

Thank @andrewserong you very much for shedding more light and connecting all the dots. ⭐ Glad these issues are being looked at by all the right folks.

@andrewserong andrewserong added the [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi label Mar 27, 2024
@jameskoster
Copy link
Contributor

It could be nice to start with some basic overflow controls to handle vertical scrolling (most common). Perhaps something like:

overflow-vertical

A notice could appear to warn the user if scroll behavior is incompatible.

This format could easily graduate to accommodate horizontal overflow controls via tools panel:

overflow-unlinked

@eric-michel
Copy link

I have been experimenting with the aspect-ratio tool as we commonly use custom CSS to define an aspect-ratio on Cover blocks. Unfortunately, the current implementation is not usable for how we implement aspect-ratio and I wonder if others feel the same.

We use the min-height value in tandem with aspect-ratio such that the min-height takes precedence. This allows us to take advantage of the min-height to ensure the Cover block (which is often the hero image of a page) stays a reasonable height on mobile, then as the screen gets larger, the aspect-ratio takes over to ensure the Cover block is a reasonably proportional height on larger screens.

The current implementation of aspect-ratio completely disabled min-height, which I think is a mistake. min-height should remain a usable tool in tandem with aspect-ratio so that we can tool our Cover blocks across a variety of browser sizes in a way that makes sense.

The overflow issue also seems like a strange choice when it comes to aspect-ratio priority. We use aspect-ratio as a "if possible, stay at this aspect ratio, otherwise follow whatever makes sense for the content or min-height". Prioritizing aspect-ratio over all other considerations results in some really jank layout issues (like hidden text on small screens).

@andrewserong
Copy link
Contributor

Thanks for sharing your thoughts @eric-michel!

We use aspect-ratio as a "if possible, stay at this aspect ratio, otherwise follow whatever makes sense for the content or min-height".

That was the original intention in the PR that implemented the feature. Unfortunately we ran into a bunch of issues when any width, height, or min height values were in use, where the element with the aspect ratio set would no longer smoothly reduce down in size on narrower viewports, and would sometimes break out of its container. Or, there were also situations where full width layouts broke when an aspect ratio was set. The compromise for the initial version of the support was to make the settings mutually exclusive. Longer-term, it'd be great to figure out how to make these controls play well with one another, as what you describe sounds ideal to me.

If you're comfortable, would you mind sharing the custom CSS you commonly use? It could be helpful to have an example of common custom CSS to use as a guideline for further development on the feature.

@eric-michel
Copy link

Hey @andrewserong, happy to! Here's a quick Pen I did that I think approximates how the Cover block works: https://codepen.io/ericmichel/full/oNOyBBw. The image should follow the aspect-ratio property of 2.1 as you scale the window down until it hits the point at which it would be shorter than 400px - then the min-height property takes precedence.

We started doing this probably a couple years ago because using the built-in min-height setting alone resulted in some pretty bad looking Cover blocks on wide screen monitors. If you take out that aspect-ratio property in the Pen and expand the window to be, say, 2560px wide, it's still only 400px tall which clips the image to the point that it's practically unrecognizable.

Generally speaking, I have found it to be super reliable to simply add a single-property class to any cover block with the aspect ratio defined and it just works right out of the box. Best I can tell, aspect-ratio takes a backseat to any other height or width properties that would conflict with it.

So for instance, I could usually get away with just adding something like:

.ar-2-1 {
  aspect-ratio: 2.1;
}

to a Cover block and be off to the races!

Interestingly, normally content breaking out of the aspect-ratio will also force the aspect-ratio to be ignored to avoid a blowout. But when a min-height is set, the aspect-ratio is preserved, even at the cost of content overflow. See https://css-tricks.com/almanac/properties/a/aspect-ratio/#aa-when-content-breaks-out-of-the-ratio.

If I had my druthers, aspect-ratio in the Cover block would function in these same ways: if no min-height is set, the aspect-ratio would give way to the content, and if min-height is set, then aspect-ratio would give way to the min-height.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

4 participants