Skip to content

Support full-width looping videos in articles #15056

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.

Sign up for GitHub

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

Merged
simonbyford merged 3 commits into from
Jan 6, 2026
+40 −16
Merged

Support full-width looping videos in articles #15056

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6652c71
Set containerAspectRatio
simonbyford Dec 23, 2025
26a0906
Allow SelfHostedVideo to take up the full width
simonbyford Dec 23, 2025
dd67624
Refactor
simonbyford Dec 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions dotcom-rendering/src/components/Card/Card.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1013,6 +1013,7 @@ export const Card = ({
}
subtitleSize={subtitleSize}
enableHls={enableHls}
letterboxed={true}
Copy link
Contributor

@RikRootsGuardian RikRootsGuardian Jan 5, 2026

Choose a reason for hiding this comment

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

I don't know why we're touching the Fronts card here? Is calculating the aspect ratio from the video itself not enough?

Another thought - what happened with the rumours that people were looking to add an aspect ratio attribute to media atoms?

Copy link
Contributor Author

@simonbyford simonbyford Jan 6, 2026

Choose a reason for hiding this comment

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

Fair question, I've bundled together 2 distinct changes in this PR which is causing confusion. It probably should have been 2 PRs.

The first change uses the aspect ratio of the video to calculate the size of the video container for the purposes of reducing CLS. It has no impact on whether or not the video displays letterboxes. It just follows on from this PR where it was missed: #15029

The second change introduces the concept of letterboxes which can be enabled or disabled. Since the default state is to disable letterboxes (see Mat's comment), we need to change the Fronts card to enable them again.

Copy link
Contributor Author

@simonbyford simonbyford Jan 6, 2026

Choose a reason for hiding this comment

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

Another thought - what happened with the rumours that people were looking to add an aspect ratio attribute to media atoms?

If I understand you correctly, such a field exists now. But it's a string (e.g. "5:4") and we need a numerical value so I opted to use the dimensions instead.

/>
</Island>
)}
Expand Down
12 changes: 12 additions & 0 deletions dotcom-rendering/src/components/LoopVideoInArticle.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,17 @@ export const LoopVideoInArticle = ({
return null;
}

const calculateAspectRatio = (): number => {
const dimensions = firstVideoAsset?.dimensions;

if (dimensions) {
return dimensions.width / dimensions.height;
}

// Default aspect ratio if dimensions are not available
return 5 / 4;
};

return (
<>
<Island priority="critical" defer={{ until: 'visible' }}>
Expand All @@ -39,6 +50,7 @@ export const LoopVideoInArticle = ({
fallbackImageAspectRatio={
(firstVideoAsset?.aspectRatio ?? '5:4') as FEAspectRatio
}
containerAspectRatio={calculateAspectRatio()}
fallbackImageLoading="lazy"
fallbackImageSize="small"
height={firstVideoAsset?.dimensions?.height ?? 400}
Expand Down
30 changes: 18 additions & 12 deletions dotcom-rendering/src/components/SelfHostedVideo.importable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,20 +55,23 @@ const videoContainerStyles = (
}
`;

const figureStyles = (aspectRatio: number) => css`
const figureStyles = (aspectRatio: number, letterboxed: boolean) => css`
position: relative;
aspect-ratio: ${aspectRatio};
height: 100%;
max-height: 100vh;
max-height: 100svh;
max-width: 100%;
${from.tablet} {
/**
* The value "80" is derived from the aspect ratio of the 5:4 slot.
* When other slots are used for self-hosted videos, this will need to be adjusted.
*/
max-width: ${aspectRatio * 80}%;
}
${letterboxed &&
css`
max-height: 100vh;
max-height: 100svh;
max-width: 100%;
Comment on lines +64 to +66
Copy link
Contributor

@domlander domlander Jan 8, 2026

Choose a reason for hiding this comment

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

Why are these three lines of CSS not useful for video on articles?

Copy link
Contributor Author

@simonbyford simonbyford Jan 8, 2026

Choose a reason for hiding this comment

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

The requirement was that looping videos should take up the full width of an article, regardless of their aspect ratio. If you introduce a max-height then some very tall videos cannot take up the full width. For example:

Image

Source: https://viewer.code.dev-gutools.co.uk/preview/music/2025/dec/17/simon-looping-video-test

In reality this is an edge case and will probably never happen but that's what motivated me to make this change.

max-width: 100% should be fine though and can be moved out of the conditional.

${from.tablet} {
/**
* The value "80" is derived from the aspect ratio of the 5:4 slot.
* When other slots are used for self-hosted videos, this will need to be adjusted.
*/
max-width: ${aspectRatio * 80}%;
}
`}
`;

/**
Expand Down Expand Up @@ -150,6 +153,7 @@ type Props = {
subtitleSource?: string;
subtitleSize: SubtitleSize;
enableHls: boolean;
letterboxed?: boolean;
Copy link
Contributor

@domlander domlander Jan 8, 2026

Choose a reason for hiding this comment

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

What does letterboxed mean? Is it where the video can appear with grey bars at its left and right?

Copy link
Contributor Author

@simonbyford simonbyford Jan 8, 2026

Choose a reason for hiding this comment

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

That's right. I appreciate it isn't a standard or obvious term though and I'm open to changing it if you have any ideas? I originally went with fullWidth (to mean the opposite of letterboxed) but after some feedback I changed it.

Copy link
Contributor

@paperboyo paperboyo Jan 8, 2026

Choose a reason for hiding this comment

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

My fault for choosing the term itself (and, in our case, more correct would be, even more obscure, pillarbox). But while I don’t care for the term much, I think the reasoning behind my earlier comment still stands? (whatever works, ofc, though!)

Copy link
Contributor Author

@simonbyford simonbyford Jan 9, 2026

Choose a reason for hiding this comment

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

The reasoning is solid. The name is obscure but perhaps just a comment explaining it would clear things up?

};

export const SelfHostedVideo = ({
Expand All @@ -170,6 +174,7 @@ export const SelfHostedVideo = ({
subtitleSource,
subtitleSize,
enableHls,
letterboxed = false,
}: Props) => {
const adapted = useShouldAdapt();
const { renderingTarget } = useConfig();
Expand Down Expand Up @@ -703,7 +708,7 @@ export const SelfHostedVideo = ({
>
<figure
ref={setNode}
css={figureStyles(aspectRatio)}
css={figureStyles(aspectRatio, letterboxed)}
className={`video-container ${videoStyle.toLocaleLowerCase()}`}
data-component="gu-video-loop"
>
Expand Down Expand Up @@ -737,6 +742,7 @@ export const SelfHostedVideo = ({
subtitleSize={subtitleSize}
activeCue={activeCue}
enableHls={enableHls}
letterboxed={letterboxed}
/>
</figure>
</div>
Expand Down
13 changes: 9 additions & 4 deletions dotcom-rendering/src/components/SelfHostedVideoPlayer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,16 @@ import { VideoProgressBar } from './VideoProgressBar';

export type SubtitleSize = 'small' | 'medium' | 'large';

const videoStyles = (aspectRatio: number) => css`
const videoStyles = (aspectRatio: number, letterboxed: boolean) => css`
position: relative;
display: block;
height: auto;
width: 100%;
max-height: 100vh;
max-height: 100svh;
${letterboxed &&
css`
max-height: 100vh;
max-height: 100svh;
`}
cursor: pointer;
/* Prevents CLS by letting the browser know the space the video will take up. */
aspect-ratio: ${aspectRatio};
Expand Down Expand Up @@ -127,6 +130,7 @@ type Props = {
/* used in custom subtitle overlays */
activeCue?: ActiveCue | null;
enableHls: boolean;
letterboxed: boolean;
};

/**
Expand Down Expand Up @@ -169,6 +173,7 @@ export const SelfHostedVideoPlayer = forwardRef(
subtitleSize,
activeCue,
enableHls,
letterboxed,
}: Props,
ref: React.ForwardedRef<HTMLVideoElement>,
) => {
Expand Down Expand Up @@ -199,7 +204,7 @@ export const SelfHostedVideoPlayer = forwardRef(
<video
id={videoId}
css={[
videoStyles(aspectRatio),
videoStyles(aspectRatio, letterboxed),
showSubtitles && subtitleStyles(subtitleSize),
]}
crossOrigin="anonymous"
Expand Down
Loading