Ensure AV does not cause reflows

[jamesdonoh]

Is your feature request related to a problem? Please describe.
As described in our coding standards:

we never reflow page layout, i.e. we always reserve vertical space and never change the height of elements during the render of the page.

We need to verify that this does not happen
a) when audio/video content is on the page but the user has not interacted with it
b) when the user clicks to play audio/video content

This currently does reflow, see asset https://www.test.bbc.co.uk/news/articles/c3wmq4d1y3wo. The media player should reverse height meaning the text should not need to move once the player loads in.

We should reserve height based on the placeholder image supplied with the video, taking into account the width we are display the player at (using ratio's). Also we should check this works for landscape and portrait videos.

Describe the solution you'd like

• Verify the above using browser dev tools (disable caching, throttle network and refresh the page)
• If reflows are observed then implement solution to avoid them as far as possible

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Testing notes
Visual regression on canonical article (no AMP video currently exists). Test there is no reflow (visible jumping of components). No non-JS testing will be required.

Dev insight: Visual regression of the asset c3wmq4d1y3wo

Additional context

• This likely can be achieved using a placeholder element.

[pjlee11]

https://ares-broker.test.api.bbci.co.uk/api/article/c3wmq4d1y3wo is the payload for the AV test page. It currently seems that the portrait AV player does not have a width and height values for the rawImage that it supplies.

@jimjohnsonrollings could you confirm if Ares will supply this information? Also do we have a square video asset yet, or should that be kept seperate from this work?

[jimjohnsonrollings]

1. Yes Ares will supply that. I'm just republishing that video which should push it through. It's because the AV source didn't supply those fields when we originally created this - but now it's been updated to include them so the republished asset should have them (caveat - republishing didn't work for two other pieces of AV last week :( )

2. Workflow tools do not currently have the capability to provide us with square video, so that should not be a concern for now.

[chris-hinds]

This PR could be of use here BBC-archive/psammead#2582

[HarveyPeachey]

I’ve been testing the mega av test article and https://www.test.bbc.co.uk/news/articles/cn7k01xp8kxo and I’m not seeing any reflows with the media player even with slow 3g throttling and cache disabled.

Also checked the amp version of the mega test article, vertical video and there are no reflows at all. Our snapshot coverage for both amp and canonical should cover visual regression if this changes. So I will be closing this issue as no further work is needed.

[HarveyPeachey]

Testing out contentAnchor component to solve this issue results in a fix for the reflow's that we were seeing for the AV player:

Before Implementation:

After Implementation:

However to be able to use this component it needs to be merged, it may be worthwhile to block this issue until the content anchor component has been tested and merged BBC-archive/psammead#2582 as this issue isn't a huge priority as the reflow doesn't have any outstanding visual issues.

I was also receiving a warning message in the server console when using the component:

Warning: useLayoutEffect does nothing on the server, because its effect cannot be encoded into the server renderer's output format. This will lead to a mismatch between the initial, non-hydr
ated UI and the intended UI. To avoid this, useLayoutEffect should only be used in components that render exclusively on the client. See https://fb.me/react-uselayouteffect-ssr for common fi
xes.

[chris-hinds]

@HarveyPeachey this is exactly what the thinking behind the the contentAnchor component was. So Id be included to wait for that component to be completed or if the content reflows are not that bad maybe we can release without.

We should catchup with @jroebu14 to see where he is on this component and what is left to do.

[jroebu14]

@hindsc52 the content anchor passed testing however @PriyaKR wanted a few examples in Storybook where the content anchor component wraps some iframe because that would be more representative of what it will be used for in the wild. I've been too busy getting up to speed with my new team to look into this but I can pick it up if it's blocking you. We should maybe set up a call so you can explain better the problem you're trying to solve and if the content anchor component will do what you want it to do.

[jamesdonoh]

Thanks @HarveyPeachey for the writeup and @jroebu14 for the update.

Rather than create some artificial components in Storybook to use as test scenarios for ContentAnchor first, would another way to approach it be to merge psammead#2582 as it stands (assuming there is no obvious outstanding work needed on it) - since it adds a new component rather than modifying anything existing - and then create a full PR out of Harvey's test above? Effectively the media player would then becomes our test scenario for ContentAnchor. How does that sound?

[jroebu14]

I think that's a good idea. If @PriyaKR is ok with that then I'll work on getting it merged. Would still be good to know what exactly the problem is you're trying to solve. Is it the slight height adjustment when clicking play?

[HarveyPeachey]

I think that's a good idea. If @PriyaKR is ok with that then I'll work on getting it merged. Would still be good to know what exactly the problem is you're trying to solve. Is it the slight height adjustment when clicking play?

The problem that was being solved can be seen in the before GIF. James told me how to use the layout shift region tool in the dev tools to show where discrepancies in the page render were happening. In the GIF when the video is played the blue area that flashes shows the video is causing shifting in the page even though it's not visible to the end-user, there is something happening on the page. The after GIF shows that your component fixes this non-user facing reflow.

Hope I explained that alright 😃

[jroebu14]

Ah ok that explains it well, thanks. I wonder what causes the layout shifting in the first place.

[simonsinclair]

I did a bit of digging and the Layout Shifting is caused by SMP and its controls - it's nothing to do with our application. This can be confirmed by enabling Layout Shifting Regions on https://polling.test.bbc.co.uk/ws/av-embeds/articles/c3wmq4d1y3wo/p01k6msp/en-GB and inspecting them with a padding-top applied to body.

Is this an issue? It doesn't cause content to reflow or shift within Simorgh in the browsers I've tested it in.

[amywalkerdev]

Putting this in the backlog until we can use BBC-archive/psammead#2582