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

Video block: Add aspect ratio control #60911

Open
Drivingralle opened this issue Apr 19, 2024 · 10 comments · May be fixed by #66946 or #67047
Open

Video block: Add aspect ratio control #60911

Drivingralle opened this issue Apr 19, 2024 · 10 comments · May be fixed by #66946 or #67047
Assignees
Labels
[Block] Video Affects the Video Block [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.

Comments

@Drivingralle
Copy link

What problem does this address?

By adding a portrait video natively within a video block it can happen that the video gets very big and leads to very ugly layouts. Fixing the aspect ratio makes layouts stable and predictable.

What is your proposed solution?

Port the size controls from image block to videos.

@Drivingralle Drivingralle added the [Type] Enhancement A suggestion for improvement. label Apr 19, 2024
@t-hamano t-hamano added the [Block] Video Affects the Video Block label Apr 20, 2024
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Nov 12, 2024
@up1512001
Copy link
Member

@Drivingralle @jasmussen raised PR for this, can you please review 🙇‍♂
#66946

@t-hamano
Copy link
Contributor

In order to move this issue forward, I would like to summarize my current understanding and opinions.

Two PRs have been filed for this issue:

I think there are two points that need to be discussed here:

  • In this block, should it be possible to change scale(object:fit) in addition to aspectRatio?
  • Do we need to add Scale as a block support? My understanding is that the Scale option should not be part of aspectRatio, but a separate block support. There are cases where aspectRatio is required, but scale support is not. In fact, the Cover block supports aspectRatio, but the child is composed of inner blocks, so scale support should not be applicable.

cc @aaronrobertshaw @ramonjd @up1512001

@ramonjd
Copy link
Member

ramonjd commented Nov 24, 2024

I think some sort of aspect ratio control would be useful for videos, especially for folks who want to preserve the aspect ratio of the video across devices without quality degradation.

Improving tools for video content can only be a good thing in my opinion.

I'm not sure exactly what the right way to implement would be, but off the top of my head I think a modern content management system should be able to:

  • offer common aspect ratios as presets (e.g., 16:9, 4:3, 1:1, 21:9)
  • allow custom aspect ratios using width-to-height inputs (e.g., 3:2)
  • aspect ratios should adapt to different screen sizes (responsive). Here's where object-fit could come in handy
  • commonsense fallbacks, e.g., centering the video with letterboxing
  • a bonus would be to automatically detect the video's native aspect ratio from metadata and apply default settings

@aaronrobertshaw
Copy link
Contributor

I don't think there's any argument that aspect ratio support shouldn't be added to the Video block. The question of approach boils down to the desired use cases and priority.

@up1512001 noted on his PR a desire for control over object-fit, at least more options than simply object-fit: cover. I agree that support for aspect ratio does not mean the block would also blindly support scale options, so a separate block support would be preferred.

Given the expressed desire for scale options on the Video block alongside aspect ratio control, I think it deserves to be considered fully, as an independent block support or not.

I'm not familiar with the desired use case for #66946 but it doesn't take much to come up with one. Perhaps a theme wants to have a grid of video blocks on a page, with all those blocks represented in the same aspect ratio. The videos to be displayed in those blocks could be in different orientations etc. and the theme author doesn't want to say clip a portrait video due to object-fit: cover but still wants a consistent aspect ratio for the blocks within the grid 🤷

My view is our options haven't really changed during the discussion:

  1. If we're going to take a while to settle on the ideal block support implementation, we could proceed with the ad hoc solution from the original PR if it is done in a way to be forward compatible with the block supports
  2. Land Video: Add aspectRatio block support and utilties #67047. Either as a temporary first step to addressing the intent of the original PR, or because there's been a decision that object-fit support is a "won't do".
  3. Decide between either an object-fit block support or ad hoc control if it is only useful on Video block. Then implement that alongside adopting Video: Add aspectRatio block support and utilties #67047.

So to me the only real questions are;

  • Do we want object-fit options or not?
  • If so, should we deliver them at the same time as the aspect ratio support or incrementally?

I don't have strong opinions other than to make sure we are supporting how contributors and extenders wish to leverage core blocks.

@ramonjd
Copy link
Member

ramonjd commented Nov 25, 2024

Apologies, I didn't read up on the context and answered pretty generically.

Decide between either an object-fit block support or ad hoc control if it is only useful on Video block.

object-fit might be handy for making media responsive in grid layouts. Are there downsides to constraining it to the video block, and deciding later? Deprecation nightmares, perhaps?

Did folks add text columns only for the columns block? I'm fishing for precedents 😄

If so, should we deliver them at the same time as the aspect ratio support or incrementally?

I'd vote incrementally. Which is more useful to the user is all I'd ask.

@aaronrobertshaw
Copy link
Contributor

Did folks add text columns only for the columns block?

There were plans to include it for Post Excerpt and Paragraph blocks. It was also something extenders had designs on for third party blocks. So it was supposed to be more than a single-block block support. The adoption there though stalled as priorities shifted.

If you're looking for a precedent for a block support that has no, or only planned, adoption in core blocks you found it 😅

@jasmussen
Copy link
Contributor

In this block, should it be possible to change scale(object:fit) in addition to aspectRatio?

This issue came on my radar because of how the videos embedded on the 6.7 release site are put together. For the moment, the thumbnails are square, but the videos are 16:9. The Video block already supports this, but in some browsers you'll see a jump in height when pressing play, and the square thumbnail collapses to a landscape video.

Adding object-fit set to contain, would solve it for the release site, allowing the video to remain square, and uncropped when playing. Without the object-fit control, the aspect ratio control would not be useful in this case, as it would crop the video, with object-fit defaulting to cover.

To that end, I'd say yes: definitely aspect ratio should be paired with an object-fit control. In fact I would argue that whenever you set dimensions of a block that do not match intrinsic dimensions from the object itself (image, video, SVG come to mind), the object fit control should become available. But certainly for image and video, which come with dimensions from the object itself, object-fit feels appropriate. Possibly Audio as well, depending on how that behaves when a thumbnail is embedded.

@t-hamano
Copy link
Contributor

Thank you for your feedback!

From what I've read so far, I'm beginning to agree that the Video block needs an object-fit option.

I think the important thing is whether to provide the object-fit option as block support. Because object-fit is only available for a limited number of elements, I'm not sure if providing it as an independent block support would be useful for other blocks.

With that in mind, the following approaches seem ideal to me:

Either way, I think it would be better to add the aspect ratio option as block support rather than ad-hoc, since that would allow us to remove the default option or add our own custom option via theme.json.

@jasmussen
Copy link
Contributor

I'm limited in my technical insights, so I'm likely missing nuance that you can hopefully help fill in. But responding to this part:

Because object-fit is only available for a limited number of elements, I'm not sure if providing it as an independent block support would be useful for other blocks.

Exactly, and even for the elements that it's available to, it's doesn't do anything on its own. A video or image with default dimensions, no matter what property you set, it's not going to visually do anything (give or take few exceptions). So it's a control that only ever works when paired with something else, and that's either width/height, or aspect ratio. Can we tie it to that support? Can we make it a property of those supports?

Expanding a bit, object-fit is sort of a counterpart to background-size as well, specifically:

background-size: cover;
background-size: contain;

In that vein, the UI is already the same, and similar to object-fit, giving the values cover and contain are meaningless unless tied to dimensions. In discussing supports, not sure if that's meaningful or not, but wanted to mention the overlap.

@aaronrobertshaw
Copy link
Contributor

Do we want object-fit options or not?

Sounds to me like we have an agreement that object-fit is desired 👌

If so, should we deliver them at the same time as the aspect ratio support or incrementally?

I also believe we have consensus that the aspect ratio should be supplied as a block support. This should mean that the aspect ratio can be applied to a supported block type via Global Styles (including theme.json). Given that, we probably need object-fit as a block support too so that it can also be applied via theme.json/global styles. An ad-hoc approach to object-fit would create a gap between global styles and block instance options.

If there is time pressure on adding the aspect ratio and object-fit support to the video block, I'm not opposed to a temporary ad-hoc solution that's forward compatible with the proposed block support. That said, I think it might be better to avoid that re-work and pursue the object-fit block support angle first.

So it's a control that only ever works when paired with something else, and that's either width/height, or aspect ratio. Can we tie it to that support?

I believe we only have min-height and aspect ratio block supports at the moment. They're all separate at this stage but it should be possible to determine object-fit support conditionally. Rather than checking a single block support flag, it'd also check that it has at least one of the dimensions-related supports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Video Affects the Video Block [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.
Projects
None yet
6 participants