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

Fix height of Video blocks when native video is used #6762

Merged
merged 2 commits into from
Dec 6, 2021

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Dec 4, 2021

Summary

Builds on top of #6761.

Add height:auto to video in Video Block

First of all, this addresses findings from @dhaval-parekh in #6443 (comment) whereby when a page is in the loose sandboxing level where a Video block's video is not not converted into amp-video, the result is the video having excessive height due to the width and height being supplied when the dimensions are absent. As noted in #6443 (comment) this is something I've started to explore as a fix in Gutenberg via WordPress/gutenberg#30092, but for now the issue can be fixed by adding the height:auto style to .wp-block-video video in the amp-default.css stylesheet.

Non-AMP AMP L1 w/ video Before 👎 AMP w/ video After 👍 AMP with amp-video after 👍
non-amp-horizontal-video amp-native-video-without-height-auto amp-native-video-with-height-auto amp-video-component-with-height-auto

Add aspect-ratio to native video in Video Block

In WordPress/gutenberg#37052 (comment) I discovered that even though a video has its width and height defined, it will still have layout shifting happening. In Chromium it appears that the default aspect-ratio has only been implemented for img and not for video. In fact, it doesn't seem that the width and height on video do anything in Chromium at the moment. Therefore, in order to prevent layout shift from happening for video, it is currently necessary to add an explicit aspect-ratio style to the element.

Non-AMP

non-amp-without-width-or-height.mov

Before: AMP L1 with native video and width/height but no aspect-ratio 👎

amp-l1-with-width-or-height-on-native-video.mov

After: AMP L1 with native video and width/height and aspect-ratio 👍

amp-l1-with-width-or-height-and-aspect-ratio-on-native-video.mov

This renders exactly in AMP L3 with amp-video: no layout shift!

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@westonruter westonruter added this to the v2.2 milestone Dec 4, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Dec 4, 2021

Plugin builds for 5c75c5b are ready 🛎️!

Copy link
Collaborator

@delawski delawski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! When testing locally with L1 sandboxing, the videos are sized correctly. Also, portrait videos are properly sized thanks to the aspect-ratio property in the inline style.

Base automatically changed from update/video-intrinsic-layout to develop December 6, 2021 15:35
@westonruter westonruter merged commit ba291c3 into develop Dec 6, 2021
@westonruter westonruter deleted the add/video-aspect-ratio branch December 6, 2021 15:36
@milindmore22
Copy link
Collaborator

QA Passed

Tested with Sandboxing Loose level with both portrait and landscape videos looks good

@milindmore22
Copy link
Collaborator

One more thing I noticed there is an AMP setting for Video block with no option in it.

image

@westonruter
Copy link
Member Author

One more thing I noticed there is an AMP setting for Video block with no option in it.

Yes, that's a known issue. See #6508.

@westonruter westonruter changed the title Fix height of video in Video blocks when in loose sandboxing level Fix height of Video blocks when native video is used Dec 15, 2021
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes. Sandboxing Experiment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants