Add looping videos to articles #14843
Conversation
|
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
|
(AI suggestions sent me down the wrong path) |
|
This appears to be a CORS issue rather than a code issue. When developing locally we test on The white space appears when the poster image fails to upload for some reason? This is an intermittent issue (because: see screen shot above which loaded the poster image fine 2 mins after failing and 2 mins before failing again): 
I noticed this comment in the last PR to touch the LoopVideoPlayer file: |
|
Can confirm that the video src/url strings are correct (pasting the url into the browser address bar loads the video) |
|
Viewing the article locally, using http://r.thegulocal.com:3030/Article/https://www.theguardian.com/media/2025/oct/08/conservative-late-night-talkshow URL. doesn't solve the issue: |
|
tl;dr: Looping videos now need to be served with a Chat messages: AnnaB, MarjanK Anna: We introduced (and rolled back) a crossOrigin="anonymous" header to looping videos so that we can retrieve the subtitle vtt file from uploads.guim. This change meant that both the video and the subtitle files are now requested as cross-origin CORS fetches, requiring fastly to include Access-Control-Allow-Origin headers. Unsuprisingly, uploads.guim already has https://www.theguardian.com as an allowed origin but some users were getting the following error Does anyone have any ideas as to why some users would be hitting this error? I'm wondering if some users could be getting a cached mp4 response that does not have the headers attached because we did not ask for them before this change was rolled out and if we therefore need to purge the cache for some videos. Does that sound plausible? Are we able to purge certain items in the uploads.guim cache? The fastly VCL is available here if anyone is interested in looking through it https://manage.fastly.com/configure/services/2TmfkSoyUoNo8aFNe6Htjs/versions/29/vcl/snippets/CORS%20headers Marjan [...etc] |
dotcom-rendering/src/components/LoopVideoInArticle.importable.tsx
Outdated
Show resolved
Hide resolved
| @@ -168,6 +168,8 @@ export const LoopVideo = ({ | |||
| const [hasBeenPlayed, setHasBeenPlayed] = useState(false); | |||
| const [hasTrackedPlay, setHasTrackedPlay] = useState(false); | |||
|
|
|||
| const [devicePixelRatio, setDevicePixelRatio] = useState(1); | |||
There was a problem hiding this comment.
Introducing state feels a bit unnecessary here, what was your thinking?
There was a problem hiding this comment.
This is a bug in the new LoopVideo player. The bug tries to access the browser window environment for the DPR value before the element has been mounted. So I decided to move the value to state and do the window check in a useUpdate hook. Thus once we get the correct DPR the component will rectify itself.
There was a problem hiding this comment.
I don't think that the getOptimisedPosterImage function will run before the element has been mounted, since this function is only called if showPosterImage is true, which is only set to true in a useEffect.
However I can see why it looks that way as the code is confusing as it is and could be tidied up.
Maybe optimisedPosterImage could replace showPosterImage in state, and the call to getOptimisedPosterImage() could live in this useEffect. The function itself could remain as you have it and we access the window to obtain devicePixelRatio within the useEffect specified, then pass it into the function.
This would have the benefit of all the logic around the poster image living in one place, the accessing of the window object would explicitly live in the useEffect instead of a function.
Happy to chat through this further.
There was a problem hiding this comment.
Thanks for this, I'll let @RikRootsGuardian respond when he's back
simonbyford
force-pushed
the
rjr-investigate-looping-video-in-articles
branch
from
19644a1 to
e8451e3
Compare
simonbyford
force-pushed
the
rjr-investigate-looping-video-in-articles
branch
from
979a78b to
db38c0d
Compare
github-actions
bot
removed
the
run_chromatic
| fallbackImageLoading="lazy" | ||
| fallbackImageSize="small" | ||
| height={400} | ||
| linkTo="Article-embed-MediaAtomBlockElement" |
There was a problem hiding this comment.
This needs to be considered. It's used for tracking events eg when video first comes into view. This was developed with Fronts in mind, so I don't know if that tracking is useful for progression through the article? I put it in as a hardcoded text, but maybe a better approach would be to use the article URL here?
| subtitleSource={getSubtitleAsset(element.assets)} | ||
| videoStyle="Loop" | ||
| uniqueId={element.id} | ||
| width={500} |
There was a problem hiding this comment.
Is it definitely the case that looping videos will always be 5:4?
As it stands, videos with other aspect ratios will get cropped:
There was a problem hiding this comment.
Looping videos are 5:4, as mandated by Editorial and Design. All other videos currently displaying in articles (youtube + legacy self-hosted) play in 16:9. There's been no consideration of how to display 4:5 or 9:16 videos in Articles (at least we've not been told of these plans). Similarly no news on the aspect ratios of the new "Cinemascope" videos. There's also a wider issue around moving normal images to 5:4 AR in articles. This is where we stare at Calvin and ask for guidance.
There was a problem hiding this comment.
Looping videos are 5:4, as mandated by Editorial and Design
Cool, this gives me confidence 👍 thanks!
There was a problem hiding this comment.
Humans being humans… Not safer to do something like this?
There was a problem hiding this comment.
We are going to be rolling out loops in other aspect ratios soon, including vertical - see @paperboyo's link above.
There was a problem hiding this comment.
There's an argument that Articles should render aspect ratios different to Fronts. On Fronts, we have cards of fixed sizes, and so the video needs to fit into that size - with grey bars used to fill out the remaining space. On Articles, we don't have that same restriction.
So we could choose to allow any aspect ratio in Articles, with the player expanding or contracting to fit the size of the video. In practice, we would mostly be using fixed aspect ratios (e.g. 4/5, 5/4), but this would allow for example Editorial Design to create interactives which implement players in their preferred sizing.
This might leave a lot of whitespace in certain situations, e.g. a vertical player, justified to the left like text, will have a lot of space to the right. And there are problems with the inverse situation, e.g. a superwide player on a mobile device would not fare well.
There was a problem hiding this comment.
@pippinpen @HarryFischer do you have any thoughts on the above?
There was a problem hiding this comment.
As per @Fweddi’s comment, I think the only sensible (possible, even) course of action for articles it to allow any aspect ratio and that the decision of what it should be to be left to creators.
Anecdotally, creators are quite good at making all front loops 5:4.
There was a problem hiding this comment.
I think from our perspective we'd want the aspect ratio to be controlled by the video asset and not fixed sizes
There was a problem hiding this comment.
Thanks all. I've given this a crack here: 3463988
github-actions
bot
removed
the
run_chromatic
|
Is there anything specific you want WebEx to look at here, otherwise we are happy for you to review and merge |
|
|
||
| return ( | ||
| <> | ||
| <SelfHostedVideo |
There was a problem hiding this comment.
Does this need to be an island? Since SelfHostedVideo is an island, I don't think this needs to be. I don't believe there's any harm to it, since this PR, but it might be unnecessary.
| - We can do this by interrogating the atom's metadata, which includes the new attribute `videoPlayerFormat` | ||
| - Note: we'll probably extend this functionality to handle new 'Cinemagraph' videos | ||
| - These may use the looping video, or yet another new, video player |
There was a problem hiding this comment.
You should be able to pass in videoStyle="Cinemagraph" here and it'll just work.
There was a problem hiding this comment.
Great, thanks. I've removed this comment now the path forward is clear.
simonbyford
force-pushed
the
rjr-investigate-looping-video-in-articles
branch
from
07cd67d to
3463988
Compare
What does this change?
Displays looping self-hosted videos (ones with
videoPlayerFormat=Loop) in the appropriate player.Why?
Looping videos currently render correctly on fronts, but not in articles.
How to test
Deploy to CODE. Create an article in CODE Composer and include a self-hosted video atom. When you preview the article, the video should display in the appropriate player, according the "Video Player Format" setting from Media Atom Maker:
If set to "Standard", it should display in the normal player
If set to "Loop", it should display in the looping player
Screenshots
Related PRs