Hosted video support in video section #2248

kmeleta · 2023-01-23T16:57:10Z

PR Summary:

Added support for Shopify-hosted video to the existing Dawn video section. Also added a video looping setting and other video section improvements.

Why are these changes introduced?

Fixes #2247

Previously our video section has only supported video_url, effectively allowing embedded youtube or vimeo videos. Now video files can be uploaded directly to the admin and surfaced in editor using the new video setting type.

We've also added a video looping setting that will apply to both video types and updated some setting content.

With these changes we've also fixed the issue where youtube and vimeo videos would require 2 clicks to play. Now they should correctly autoplay (on desktop at least) after clicking the cover image to load the video.

What approach did you take?

Implemented video type setting in the video section alongside the existing video_url setting. Because the new and existing settings need to exist at the same time, the hosted video values will override anything from the existing settings, including the video source, description (alt text), and poster image. From a UX perspective this is not ideal, but is technically necessary for the time being. The hosted video is rendered in the liquid via the video_tag.

To minimize complexity and redundant code in the section I captured some values in variables that are shared between the hosted video and video url cases, like poster image, alt text, id, etc. I also cleaned up the poster image aspect ratio code to ensure consistent display between video types.

Decision log

#	Decision	Alternatives	Rationale	Downsides
1	Don't allow alt text and cover image settings to override hosted video at all	- Allow hosted video to inherit alt text if empty - Allow cover image to explicitly override cover from admin	- In the future, if we're able to offer only one video type at a time, there will be no override capabilities - Allowing the video url settings to have an effect on a hosted video could add confusion and break the single source of truth from the admin	- Less flexibility in poster image, if a video is used in different contexts and overriding the image for the video section might be desirable - Allowing the video url desc setting serve as a fallback could make it more likely that alt text is added even if from the wrong place, but this could enforce a bad pattern

Visual impact on existing themes

New settings will appear in the editor, but there should be no visual impact on existing themes.

Testing steps/scenarios

Test video section for regressions to previous behavior
Test a variety of section settings and global media styling
Add video section using video uploaded from the admin (I've uploaded one test file)
Verify hosted video, poster image, and alt text overrides those from the video url section settings
Add custom poster to hosted video in different aspect ratio and ensure video player scales properly (as it always has for video url)
Verify new video looping settings works as expected

Demo links

Editor

Checklist

Added PR summary for release notes
Requested review from UX (Only for changes that are affecting the experience or perceivable visual details)
Created a ticket for the help.shopify.com documentation team about updates to theme settings. (Internal-only task)
Followed theme code principles
Linted with Theme Check
Tested on mobile
Tested on multiple browsers
Tested for accessibility

YoannJailin · 2023-01-25T18:02:07Z

Hello @kmeleta, we add some little updates on the settings. They are listed here.

Enable video looping would be above the shopify hosted video picker.
We would replace "Or import a video url" by "Or add a video url"
Remove the "Video plays in the page" under the url field.
Cover image setting would be next.

Also this might be tricky so if this is too complicated, don't hesitate to flag. But it feels like the preview from the default youtube URL gives a lot of focus on this option vs the shopify hosted video option. is it possible to remove the default url in the URL field, and the preview under? We would still need to keep the placeholder image in the page.
The preview under the field would still work once the user has pasted his video url. What do you think?

sections/video.liquid

YoannJailin · 2023-01-31T11:03:23Z

Hello, some notes and a few changes (sorry about that Ken)

Agree with @ludo, let’s add a heading « SHOPIFY-HOSTED VIDEO » below « Enable video looping »
« Or add a video URL » should be « Or add a video from URL »
When URL field is empty, message inside the field is « Paste a link »
Help text below is « Use a YouTube or Vimeo URL. »
Cover image should be below URL field.

@ludo I think settings right above Shopify Hosted Video should stay functional, like Enable video looping. Ken is exploring a setting to play the video automatically when loading the page, if we add it it would be also there. Full-width and color options are most likely layout settings, we would likely keep them together below the content. Once we have the new semantic settings, we will have a layout heading to make them more obvious! Also the new color setting is gonna be a lot more prominent, we don’t want it to high once we have it.

After discussion with Kim and Dan, we don’t want to add tons of settings. If video is autoplayed, sound is off by default. Or else, sound is on since you decided to start it!

For the autoplay setting / positon / naming, I am still checking with Katy.

kmeleta · 2023-01-31T18:06:10Z

👋 We're exploring additional functionality here. No need for further reviewing until updated. cc @ludoboludo

sections/video.liquid

kmeleta · 2023-02-14T18:12:33Z

Reviews can resume. Recent changes have mostly been to settings content, so please give attention to those before I request translations.

ludoboludo · 2023-02-14T20:14:41Z

assets/video-section.css

  position: relative;
-  padding-bottom: 56.25%;
+  padding-bottom: calc(var(--ratio-percent) - var(--media-border-width));


This doesn't seem to be perfect in all scenarios:

Screenshot

But not a blocker. I think when decimals are involved it's tricky to get the perfect effect.

Yeah I remember playing with that. I do think it's a partial pixel issue so not sure what we can do. I'll revisit quickly though and see if a trivial fix presents itself.

Actually I found out it's because of very slight differences in aspect ratio between the cover and video. Which apparently can happen even with the default cover images generated in the admin. I decided to compare the two ratios and if the difference between them is insignificant I set apply an object-fit: cover to hide any difference. We wouldn't want to apply the cover for larger differences as we don't want significant portions of the video cropped. Curious what you think about this approach (see video.liquid) 3141a40

sections/video.liquid

assets/video-section.css

sections/video.liquid

stufreen

🎩 on MacOS Chrome, Firefox and Safari + iOS Safari

assets/video-section.css

sections/video.liquid

stufreen · 2023-02-14T21:40:38Z

sections/video.liquid

-          ></iframe>
+          {{ section.settings.video
+            | video_tag:
+              image_size: "2048x",


This could be a big image (by file size) potentially. Is there any way to serve the poster responsively? Do we even need this poster if we're using the deferred-media component?

One idea might be if the image size could be page width if the full_width setting is false.

Hmm.. honestly I was hoping that wouldn't have to be loaded.. I see now that it is, even if I remove image_size and pass poster: "".

It renders this dom but that fallback image still loads even when not displayable..

<video> <source> <img />  </video>

So I guess the 2 options I see
A) Keep our current responsive cover image implementation and pass a very small, inconsequential image_size to the video_tag for now.
B) Refactor the hosted video cover image to use the same fixed size that the video_tag and poster attribute will. Though this will probably require I not use image_tag or else we'll probably end up loading both /myimage.png&width=2000 and /myimage_2000x.png

It's unfortunate there doesn't appear to be a way to support dynamic image sizes, but option B may be the way to go as it might have the benefit of preventing the flashing you mentioned in another comment. I'll probably go ahead and experiment with that, but let me know what you think or if it seems like I'm missing something.

Edit: not actually sure how/if I can ensure the image sources always match..

cc @ludoboludo ^

Ended up just reducing the image size passed to the video_tag (value is debatable). I'm ok with this (for now) because that image won't impact initial load with deferred rendering. That said, I don't have too much of a problem not using the video_tag if would allow us to better control and repurpose the images used. So if anyone else agrees with that I can look at making that change. Though it's probably worth noting that the poster attr on video I believe requires one fixed image src afaik.

Also happy to open a follow up issue to further consider other options regardless of the outcome here.

stufreen · 2023-02-14T21:50:53Z

sections/video.liquid

+          {{ section.settings.video
+            | video_tag:
+              image_size: "2048x",
+              autoplay: true,


Might be related to above comment, but do you notice a little flicker when you click the play button? It happens very quickly but seems like the screen goes white for a frame. Comments above are more nits/style stuff but this seems noticeable for buyers.

I think you had identified this already, but during tophatting I found that on Safari I had to click play twice: once on the deferred media and again on the player. True on MacOS and iOS safari.

The flicker appears to be from a small delay between when the player is rendered and when the player can show the first frame or poster image. I mention a possible fix in your comment about the poster image.

Otherwise we'd probably need to make an exception for this type of video and not defer rendering the fragment so that it can load that first frame before the cover is clicked. Or possibly delay the removal of the cover image until the player fires the loadeddata event. But then you get into a scenario where there could be a perceived lag upon action.

But then you get into a scenario where there could be a perceived lag upon action.

I'd like to catch up on this if I can help to test if we could use some of our perceived performance UI (loader) in this use case?

What does the deferred media behaviour get us in terms of performance for hosted video?

The deferred media pattern is great for youtube and vimeo since their video players are very large (lots of JS). For hosted video though I believe we are using the default browser controls. Do we need it for hosted video? It might solve some problems with the flicker and double-clicking play on Safari.

The deferred media pattern is great for youtube and vimeo since their video players are very large

Correct. Using deferred media was mostly to take advantage of existing functionality and styling. If I were to not defer the rendering, I'd also use preload="metadata" on the video to avoid any flicker. This will load that poster/fallback image I can't exclude from the video_tag and it will preload a certain amount of the video itself so it can at least show the first frame (usually ~1kb but sometimes it just starts buffering for some reason). So inherently there will be a slight performance hit that deferred media aims to avoid. If I don't preload anything, the flicker will occur regardless.

@melissaperreault it could be worth us pairing on this and considering the severity and our options. We could also just mitigate the flicker as much as I can for now and follow up with an enhancement if we feel we need to. Youtube and vimeo also flash a blank background before the first frame loads. Here's a throttled/slow-mo comparison https://screenshot.click/hosted-yt-comparison.mp4 of the best case scenario.

Paired with @melissaperreault on this. We decided the flicker isn't blocking, but we did set background of the video player to pure black. We find this slightly less jarring and should be a familiar experience to the youtube video loading sequence.

I experimented with non-deferred loading but didn't feel the benefit was worth the performance impact or the code exceptions.

I also tried changing the z-index of the cover image instead of hiding it. This actually worked well, but only if the aspect ratio of the video and cover image were the same.

I will likely be opening an issue for follow up investigations/improvements as a result of this PR and will make mention all this.

assets/video-section.css

kmeleta · 2023-02-15T21:04:29Z

sections/video.liquid

-          ></iframe>
+          {{ section.settings.video
+            | video_tag:
+              image_size: "1100x",


Not sure what makes sense for this fixed value. In the video section, you'd most likely only see this image displayed if your browser or device prevents the autoplay from happening, or if your browser doesn't support html video or the provided formats. So I'm not sure how much it's worth increasing the file size.

The only other use of native video in the theme we pass 2048x as I originally had. Not actually sure what was used to determine that value.

Maybe it was tied to the max-width we could get to 🤔 But since the max is 1600 that would be 32000 😬
We defaulted to 1024 x 2 for most common resolutions maybe.

Yeah I could see that. That could probably be smarter logic now, but I also wouldn't think we should go higher than what it is anyway..

bakura10 · 2023-02-16T00:57:07Z

Hi !

I did not read everything, but we have implemented that on our themes as well. here, I decided to use preload="metadata" everywhere. This indeed improves the user experience by making the video faster to load as first few frames are already loaded. However we have, as alwyas, received complains from merchants using ton shit of video and having their performance scores "apparently" impacted.

What I am thinking to do is switching to preload none, and using an InteresectionObserver to switch to metadata like 500 or 600px before the video appears. Have you considered this approach?

kmeleta · 2023-02-16T16:14:33Z

What I am thinking to do is switching to preload none, and using an InteresectionObserver to switch to metadata like 500 or 600px before the video appears. Have you considered this approach?

Thanks for that insight @bakura10. I could certainly see an approach like that being useful. I briefly thought about preloading the video metadata upon hovering on the section, to get even a small jump on the loading. We'll just want to fully consider the options and decide on something that will apply well to product media and potential future uses of video.

stufreen

LGTM! 🎩 Let's make that follow-up ticket to look into skipping the deferred-media component for hosted videos.

kmeleta · 2023-02-16T20:29:14Z

Meli approved the content at least, so I've requested translations.

I also went ahead and opened an issue that captures all the video improvement topics above #2312 and another to investigate using aspect-ratio in Dawn #2313

ludoboludo

Looking good 👍
Just some small comments/nitpicks but ready to approve after that

assets/component-deferred-media.css

ludoboludo · 2023-02-17T21:48:44Z

sections/video.liquid

+              controls: true,
+              muted: false
+          }}
+        {%- endif -%}


Pretty cool that without JS the native video still works 👍

ludoboludo · 2023-02-17T21:53:47Z

sections/video.liquid

+          {{ section.settings.video
+            | video_tag:
+              image_size: "1100x",
+              autoplay: true,


The video does autoplay though. It doesn't seem like it happen all the time but it will. I wonder if we should default not to autoplay here.
I know we don't want to overthink the no js behaviour, but I think it's a safer bet to not autoplay since it's also not muted.

Video

Oh for just the no-js, yeah you're right, good call. Removed.

melissaperreault

Thank you Ken!! 🎉

Sharing some observations that are not blockers. Mainly notes for us to acknowledge and one that I'll create an issue for to follow up on.

I am sad to remember that Youtube URL videos require users to tap twice on touch devices. 😢
When the "cover image" don't have the same aspect ratio of the video, only the Vimeo video will wear the light grey background. I think it's okay due to how the player and control will align.
I'd love to have the ability to replace back my cover image from my Shopify-hosted video instead of being required to re-upload it again, if I tested a new cover. We offer no way to dismiss that change. 😢
We need to fix how an image will preview at the section UI thumbnail level. Right now, it doesn't display the "cover image" for the Shopify-hosted video when it does for the embed one. And if you add a cover image under the embed cover image setting, the preview won't match the right video displayed. Video reference 1, video reference 2

ludoboludo · 2023-02-17T22:23:26Z

I'd love to have the ability to replace back my cover image from my Shopify-hosted video instead of being required to re-upload it again, if I tested a new cover. We offer no way to dismiss that change. 😢

@melissaperreault I think it is doable, right here:
video

melissaperreault · 2023-02-20T02:17:52Z

I think it is doable, right here:

@ludoboludo yeah, thanks for the video. That process works great, but I meant giving the ability to pick back that first cover image that is automatically pulled from the Shopify-hosted video. We can't revert back if you replace that original cover image. The only workaround I found with Ken's help is downloading the video from the Files and re-uploading it to get back that original cover image. It's known and shared with the team. We'll keep waiting to see how merchants will use. 🙏

bakura10 · 2023-02-20T04:49:30Z

For info, I have experimented with a preload none + intersection observer to change to preload meta when the video is nearing the viewport. It works like a charm. Best of both worlds ;). This really added only 3 lines of code to our own video component, so it is worthwhile.

kmeleta · 2023-02-21T15:10:32Z

Thanks @melissaperreault

I am sad to remember that Youtube URL videos require users to tap twice on touch devices. 😢

Unfortunately that appears the case. I believe I found it could be possible to avoid this, just not with our current embed approach. Which we chose for its lightweight. Youtube is fairly opinionated about mobile autoplay. On the bright side, I was able to improve this behavior for both vimeo and youtube which previously required a second click even on desktop.

When the "cover image" don't have the same aspect ratio of the video, only the Vimeo video will wear the light grey background. I think it's okay due to how the player and control will align.

Yeah vimeo handles that case a little differently. That light grey background on the iframe is from the vimeo styles, just as the youtube player uses black, but vimeo keeps the player ui tied to the actual playable area so it creates the appearance that maybe that grey background isn't a part of the embedded video. Not ideal, but hopefully an edge case that won't be noticed too often.

I'd love to have the ability to replace back my cover image from my Shopify-hosted video instead of being required to re-upload it again, if I tested a new cover. We offer no way to dismiss that change. 😢

I agree, but it's also possible we have more of a use case for that flow as testers than most users would in real world use. Would be interested in what that team thinks about that.

ludoboludo

One small nitpick but otherwise 👍

ludoboludo · 2023-02-21T20:24:24Z

sections/video.liquid

      {% endif %}
    >
      <button
-        id="Deferred-Poster-Modal-{{ section.settings.video_url.id }}"
+        id="Deferred-Poster-Modal-{{ video_id }}"


Nitpick:

Suggested change

id="Deferred-Poster-Modal-{{ video_id }}"

id="Deferred-Poster-{{ video_id }}"

No need to make this change, I was just noticing that in our JS the selector is this.querySelector('[id^="Deferred-Poster-"]') and since it's not a modal we could adjust it.

Not going to change it now, but I added this to the video section improvements ticket so it can get updated in follow ups.

Hosted video support in video section (Shopify#2248)

* shopify/main: update version and release notes (Shopify#2327) add zoom on hover and show spinner when image loading (Shopify#2314) Try more universal rich text controls (Shopify#2085) Hosted video support in video section (Shopify#2248)

* New hosted video setting and liquid improvements * Setting and content updates * Settings updates * More setting/content changes * Another one * Fix mute and border radius * Aspect ratio fixes and general improvements * Safari autoplay fix * Update no-js experience * Update 20 translation files * Whitespace * Prevent no-js autoplay --------- Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

kmeleta added the Settings change label Jan 23, 2023

kmeleta force-pushed the video-section-updates branch from 04b93dc to 1fd71bb Compare January 25, 2023 15:42

github-actions bot added the cla-needed label Jan 25, 2023

kmeleta force-pushed the video-section-updates branch from 0355cdb to 1fd71bb Compare January 25, 2023 17:21

github-actions bot removed the cla-needed label Jan 25, 2023

ludoboludo reviewed Jan 30, 2023

View reviewed changes

sections/video.liquid Outdated Show resolved Hide resolved

sections/video.liquid Outdated Show resolved Hide resolved

sections/video.liquid Show resolved Hide resolved

stufreen reviewed Feb 1, 2023

View reviewed changes

sections/video.liquid Show resolved Hide resolved

kmeleta added 3 commits February 13, 2023 09:32

New hosted video setting and liquid improvements

cce5d3c

Setting and content updates

34436a3

Settings updates

d6b66a0

kmeleta force-pushed the video-section-updates branch from b7901ee to d6b66a0 Compare February 13, 2023 17:58

More setting/content changes

b8f121a

kmeleta force-pushed the video-section-updates branch from f066c84 to b8f121a Compare February 14, 2023 17:46

kmeleta requested review from stufreen, danielvan, melissaperreault and ludoboludo February 14, 2023 18:13

kmeleta assigned stufreen and ludoboludo Feb 14, 2023

Another one

1e21117

ludoboludo reviewed Feb 14, 2023

View reviewed changes

Fix mute and border radius

3228fe5

stufreen suggested changes Feb 14, 2023

View reviewed changes

kmeleta force-pushed the video-section-updates branch from 91b3232 to 588d4cd Compare February 15, 2023 19:28

Aspect ratio fixes and general improvements

3141a40

kmeleta force-pushed the video-section-updates branch from 588d4cd to 3141a40 Compare February 15, 2023 19:29

Safari autoplay fix

3778c9e

kmeleta commented Feb 15, 2023

View reviewed changes

Update no-js experience

51d731a

stufreen self-requested a review February 16, 2023 19:12

stufreen previously approved these changes Feb 16, 2023

View reviewed changes

kmeleta mentioned this pull request Feb 16, 2023

Investigate hosted video improvements #2312

Open

kmeleta requested a review from ludoboludo February 16, 2023 20:32

ludoboludo reviewed Feb 17, 2023

View reviewed changes

melissaperreault previously approved these changes Feb 17, 2023

View reviewed changes

Update 20 translation files

eac31bf

translation-platform bot dismissed stale reviews from melissaperreault and stufreen via eac31bf February 21, 2023 10:01

kmeleta added 2 commits February 21, 2023 08:19

Whitespace

80a93d4

Prevent no-js autoplay

5f032e8

kmeleta requested review from ludoboludo and stufreen February 21, 2023 15:26

stufreen approved these changes Feb 21, 2023

View reviewed changes

ludoboludo approved these changes Feb 21, 2023

View reviewed changes

kmeleta merged commit 0b58992 into main Feb 21, 2023

kmeleta deleted the video-section-updates branch February 21, 2023 22:34

chris-kreidl added a commit to uniorusa/unior-usa-b2b that referenced this pull request Feb 22, 2023

Merge pull request #14 from Shopify/main

31f5758

Hosted video support in video section (Shopify#2248)

	id="Deferred-Poster-Modal-{{ video_id }}"
	id="Deferred-Poster-{{ video_id }}"

Hosted video support in video section #2248

Hosted video support in video section #2248

Conversation

kmeleta commented Jan 23, 2023 • edited Loading

PR Summary:

Why are these changes introduced?

What approach did you take?

Decision log

Visual impact on existing themes

Testing steps/scenarios

Demo links

Checklist

YoannJailin commented Jan 25, 2023

YoannJailin commented Jan 31, 2023 • edited Loading

kmeleta commented Jan 31, 2023

kmeleta commented Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stufreen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmeleta Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmeleta Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

stufreen Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmeleta Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmeleta Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakura10 commented Feb 16, 2023

kmeleta commented Feb 16, 2023

stufreen left a comment

Choose a reason for hiding this comment

kmeleta commented Feb 16, 2023

ludoboludo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melissaperreault left a comment

Choose a reason for hiding this comment

ludoboludo commented Feb 17, 2023

melissaperreault commented Feb 20, 2023 • edited Loading

bakura10 commented Feb 20, 2023 • edited Loading

kmeleta commented Feb 21, 2023

ludoboludo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmeleta commented Jan 23, 2023 •

edited

Loading

YoannJailin commented Jan 31, 2023 •

edited

Loading

kmeleta commented Feb 14, 2023 •

edited

Loading

kmeleta Feb 15, 2023 •

edited

Loading

kmeleta Feb 15, 2023 •

edited

Loading

stufreen Feb 14, 2023 •

edited

Loading

kmeleta Feb 15, 2023 •

edited

Loading

kmeleta Feb 15, 2023 •

edited

Loading

melissaperreault commented Feb 20, 2023 •

edited

Loading

bakura10 commented Feb 20, 2023 •

edited

Loading