-
Notifications
You must be signed in to change notification settings - Fork 278
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
Fix double tap issue on Mobile #90
Conversation
src/lite-yt-embed.js
Outdated
playerVars: { 'autoplay': 1, 'playsinline': 1 }, | ||
events: { | ||
'onReady': (event) => { | ||
event.target.mute(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why mute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed. It was used for testing.
Thanks!
Currently, you must to double tab a video and is very annoying for the users. Fetch YouTube API script on demand The client fetch the YouTube API Library only when the user click a placeholder video by the first time. It doesn't use iframes directly iframes are using internally through the YouTube API Library Keep the performance stunning It doesn't impact to the perfomance because the library is not being fetched at the same time that the page is being loaded
Tested it out and it's not working with safari on iOS Edit: seems it worked while you had it muted |
HI Oskar,
Which version of iOS have you tested it out on?
…On Wed, 21 Jul 2021 at 20:05, Oskar Schöldström ***@***.***> wrote:
Tested it out and it's not working with safari on iOS
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#90 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAVPE5GA62P7WGJ4CRL47RLTY5HEJANCNFSM5ALIGC2Q>
.
--
Dan Tovbein
|
iOS 14.6 |
@oxyc I tested on IOs 14.6 Safari and works fine 🤔 |
@@ -40,6 +39,19 @@ class LiteYTEmbed extends HTMLElement { | |||
// TODO: support dynamically setting the attribute via attributeChangedCallback | |||
} | |||
|
|||
fetchYoutubeAPI(cb) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not make this a promise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it could be.
But honestly, I don't see any blocker in this case.
onYouTubeIframeAPIReady() { | ||
const videoWrapper = document.createElement('div') | ||
videoWrapper.id = this.videoId; | ||
document.body.appendChild(videoWrapper); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto on only be manipulating the custom element structure. Not the main body. Please use 'this.appendChild' or reference a node within the element to bind to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point 👍
var tag = document.createElement('script'); | ||
tag.src = 'https://www.youtube.com/iframe_api'; | ||
tag.async = true; | ||
var firstScriptTag = document.getElementsByTagName('script')[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be manipulating things outside of the element itself. Please just add the script within the element's scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree in this case, because you're fetching the library on demand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking forward to this PR. 👏
I tested on Chrome on Android and it works well, however on iOS Simulator (iPhone 5s 10.3.1 and iPhone X 14.4) and it doesn't work - video still plays but requires double tap.
Also there is an issue that it doesn't work if there are multiple embeds on the same page. I am not sure if it will be solved after addressing @Garbee 's comments. Perhaps a page should be added to the repo with two videos to cover it during testing.
You may want to fetch the latest changes from upstream. Left you some minor comments. Please let me know if you would like a hand tackling the requested changes, happy to assist.
this.insertAdjacentHTML('beforeend', iframeHTML); | ||
onYouTubeIframeAPIReady() { | ||
const videoWrapper = document.createElement('div') | ||
videoWrapper.id = this.videoId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I understand this line is not needed.
@@ -40,6 +39,19 @@ class LiteYTEmbed extends HTMLElement { | |||
// TODO: support dynamically setting the attribute via attributeChangedCallback | |||
} | |||
|
|||
fetchYoutubeAPI(cb) { | |||
var tag = document.createElement('script'); | |||
tag.src = 'https://www.youtube.com/iframe_api'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a preconnect
to https://www.youtube-nocookie.com
that is no longer relevant and should possible be replaced by:
LiteYTEmbed.addPrefetch('preconnect', 'https://www.youtube.com');
Heads up that following this change, it will connect to youtube.com not the privacy-enhanced youtube-nocookie.com.
Fixed dobule tap issue on mobile with this PR, but now multiple videos won't play on the same page. |
Does this definitely work in iOS and for non-muted videos? I've tried a similar approach to this in the past and didn't get anywhere close to it working. I've not tried this PR but it doesn't look different from the code I tried that didn't work on mobile devices. If we look at https://developers.google.com/youtube/iframe_api_reference#Autoplay_and_scripted_playback we can see this quote:
This PR uses The only hack I found to get around this issue on mobile was to have the |
You are right. I tested this solution on iPadOS Safari and it still requires double tapping. However, it does work on MacOS Safari which wasn't working before, so I would say it's still an improvement. |
What is done
Fix double tap issue on Mobile
Currently, you must to double tab a video and is very annoying for the users.
Fetch YouTube API script on demand
The client fetch the YouTube API Library only when the user click a placeholder video by the first time.
It doesn't use iframes directly
iframes are using internally through the YouTube API Library
Keep the performance stunning
It doesn't impact to the performance because the library is not being fetched at the same time that the page is being loaded. You can validate against the two attached reports below.
Lighthouse
The reports are almost the same, in fact, you can see on the first screenshot that the video placeholder is being shown more faster than the previous implementation.
This is the audit related to the new implementation
/variants/solo
:This is the audit related to the previous implementation
![Screenshot 2021-07-14 at 08 11 22](https://user-images.githubusercontent.com/2814580/125612730-2b8f0775-f7d3-41e9-ac67-64d8d55c7f46.png)
/variants/solo
: