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

Make Video Poster Attribute Lazy? #32

Closed
DanThePainter opened this issue Dec 13, 2018 · 8 comments
Closed

Make Video Poster Attribute Lazy? #32

DanThePainter opened this issue Dec 13, 2018 · 8 comments

Comments

@DanThePainter
Copy link

What to do when video poster attribute used?

Poster image is not lazy loaded.
How about data-poster included?

@leeoniya
Copy link
Collaborator

leeoniya commented Dec 13, 2018

i was literally just about to open an issue like this :)

just add "data-poster" to this whitelist [1]. and yallFlipDataAttrs(element); ahead of [2].

[1] https://github.com/malchata/yall.js/blob/master/src/yall.js#L117
[2] https://github.com/malchata/yall.js/blob/master/src/yall.js#L45

@DanThePainter
Copy link
Author

Poster is now lazy. Thanks.

@malchata malchata self-assigned this Dec 13, 2018
@malchata
Copy link
Owner

A video's poster attribute is designed to be a placeholder until the video's source is loaded. Lazy loading the placeholder itself is not beneficial, particularly because it adds the possible overhead of an additional image transferred.

Feel free to fork this and modify it for your own purposes, but it's not a feature I intend to add to this script, which I'm already seeking to finalize and advise of deprecation (per #29) in favor of native lazy loading which is already available as a feature behind a flag in Chrome.

@leeoniya
Copy link
Collaborator

leeoniya commented Dec 13, 2018

A video's poster attribute is designed to be a placeholder until the video's source is loaded.

i don't understand this reasoning, except for the narrowest/purist interpretation. functionally, it is loaded as an image. if you have a video gallery below the fold, you could be loading 10 posters which can add to 1mb or more. it [semantically] being a placeholder is quite irrelevant.

in favor of native lazy loading which is already available as a feature behind a flag in Chrome.

seems very premature. we still see 6℅ IE11 traffic (US, ecommerce, home remodeling industry). desktop and iOS safari still lack IntersectionObserver support. it will be years before it's widely available. if you no longer want to maintain yall, that's perfectly fine but just call a spade a spade.

anyways, thanks for the lib!

@malchata
Copy link
Owner

First, I want to apologize for being flippant. I didn't think I was being so as I wrote my reply, but if that is how my response was taken, my intentions are irrelevant, and I own that.

That said, my concerns for this feature request are as follows:

  1. I want to keep complexity to a minimum. Bloat is not necessarily a concern for me with this script as it's already quite small, but testability. For every new feature added, a test has to be created for the local server, and has to be verified to work on a slew of available browsers. BrowserStack helps with this, but there's a lot of possible points for failure. Lazy loading is hard to do, but I think yall generally does a good job of it.
  2. For lazy loading video in particular, taking this on this request involves layering two potential lazy loading events. The first one (which yall already supports) is lazy loading the video itself for autoplaying use cases. The other is the one being proposed. Either of these use cases can occur independently of one another (i.e., lazy loading autoplaying video or lazy loading a poster image for a non-autoplaying video) or together (i.e., lazy loading autoplaying video and lazy loading a poster image). In the latter case, we want to ensure we do the right thing, as the video asset would already be incoming, and lazy loading a poster image, which can cause bandwidth contention. This may simply be resolved by advising users on a proper markup pattern.
  3. You are correct in a sense that yall is something I don't want to keep maintaining, but I do acknowledge that—for reasons I never anticipated—yall has entered into some amount of use in production websites. While I feel this is kind of cool on one hand, I'm concerned about the use of JavaScript to lazy load media assets in general, because issues. The current lazyload spec seems to be the best possible candidate to address this, but as you rightfully say, it could take a while to propagate. It could be quick (as we saw with CSS grid), or it could languish and take some time. Therefore, there is still a need and desire for libraries like mine. So maybe I need to re-evaluate and potentially close/delete #29.

I'll reopen this and take a stab at it (along with some other issues that have piled up), but I can't guarantee anything. In the short term, I would advocate serving lossy low quality placeholders (<q50) to mitigate how much is being sent down the wire.

And again, I apologize. I didn't intend to be flippant, but it came off that way, and that's what really matters.

@malchata malchata reopened this Dec 13, 2018
@leeoniya
Copy link
Collaborator

leeoniya commented Dec 13, 2018

No need to apologize.

I was just baffled by the drive-by "no one needs this, native lazyload is just around the corner" & wontfix stance, which completely doesnt match reality.

let's talk a bit more about <video> and lazy loading (and autoplay).

autoplay is widely considered a shit UX anti-pattern for the same reasons you'd want to lazy-load anything - it sucks bandwidth and distracts from static content. google, apple and others have been preaching against it for a long time. IMO autoplay is only acceptable in 2 cases. 1) the user came to the page to watch the video (news channel, youtube, netflix) or 2) the user clicked on something to cause a modal and you need to load the video dynamically in there and start playing it. in these causes, there is no need to lazy-load anything (neither the poster nor the content).

for videos that don't auto-play, we already have preload="none" to lazy-load the content.

poster, on the other hand is rarely used as a technique to save bandwidth (like low res images/thumbs). it is there to be the representation of the video content (so you can decide whether to watch it or not). therefore, it is not a placeholder in the same sense as the bandwidth-saving others. IMO it should be treated exactly like img[src] for the purpose of yall.js.

the current default behavior of lazy-loading video content caters more to below-the-fold autoplaying videos (a huge anti-pattern) that can already be done with preload rather than the better UI pattern of lazy-loading a poster for a non-autoplaying videos.

For lazy loading video in particular, taking this on this request involves layering two potential lazy loading events.

because of the above reasoning, i don't think this combination makes sense to support. it's unfortunate that yall caters to lazy-loading video content which has been possible for a while [1] (except IE, i guess). i would much rather have the posters be lazy-loaded with no special handling for the content.

/$0.02

[1] https://www.w3schools.com/tags/att_video_preload.asp

@leeoniya
Copy link
Collaborator

leeoniya commented Dec 13, 2018

you could actually instead toggle "data-preload" and "data-poster" at the <video> level rather than doing anything with video[src] or source[src]. i don't know if modifying preload after-the-fact will have the desired effect though.

malchata added a commit that referenced this issue Dec 15, 2018
- Added linting via eslint.
- Updated Babel to version 7.
- Added @babel/preset-env. May add a separate leaner build for mjs in the future.
- Resolved bugs #24 and #28 by removing async decoding via `Image.decode()`.
- Added features requested in #19 and #32.
- Updated documentation.
@leeoniya
Copy link
Collaborator

thanks @malchata!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants