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

[Master Feature] amp-ima-video: Use native controls. #8841

Open
aghassemi opened this Issue Apr 19, 2017 · 18 comments

Comments

@aghassemi
Member

aghassemi commented Apr 19, 2017

With using native controls we will get UX consistency, built-in accessibility,Volume, Chromecast, AirPlay, Closed Caption, etc..

There are issues with using native controls in IMA, that need addressing however:

1- Initially native controls need to be hidden and a custom "play" icon for click to play need to be present to capcture user-intent and initialize IMA.
2- During Ad play, controls need to be hidden since there is no way to disable "seek" otherwise.
3- Native controls Fullscreen is a big issue, outside of iOS, IMA needs to have a different Ad container rather than the <video> itself to go to fullscreen so various Ad formats can be served. There are two approaches for this:

  • In Chrome, we can hide the native fullscreen with video::-webkit-media-controls-fullscreen-button { display: none } and provide our own button.
  • In FF/IE, since we are in an iframe, we can toggle iframe's allowfullscreen attribute which automatically removes the native fullscreen control from all <video>s inside the iframe.
@shawnbuso

This comment has been minimized.

Contributor

shawnbuso commented Jun 2, 2017

I'm not sure if this one is technically feasible. So far I have exactly what we need working great on desktop and Android, but iPhone is giving me all kinds of problems. First, it won't let me disable controls during ads - I'm calling videoPlayer.setAttribute('controls', false), but it seems to just be ignored. I still see controls while ads are playing, which interferes with our ad UI.

Second, and this is the bigger issue, I cannot get the fullscreen button to work. I have a fullscreen button overlayed on my player, and for some reason I can tap this once and only once. If I enter fullscreen then exit fullscreen, I cannot re-enter fullscreen. Either something is interfering with my ability to tap the button (which should be the case because I have z-index set to max int) or my event listener is being removed, ignored, or interrupted. This works fine on desktop and Android - it's only iPhone that's giving me problems. Have you seen anything remotely like this before (something that can be tapped once but future taps are ignored)? I'm not getting any errors (related to this) in my console.

You can see my work in progress at https://github.com/shawnbuso/amphtml/tree/ima_controls.

@shawnbuso

This comment has been minimized.

Contributor

shawnbuso commented Jun 5, 2017

OK after a full weekend of shower-levels of thinking I think I've resolved both of these. For anyone else stumbling upon this with a similar issue:

For the first issue, I had to change videoPlayer.setAttribute('controls', false); to videoPlayer.removeAttribute('controls'); It looks like maybe iPhone just looks for the presence of the controls attribute and disregards the value associated with it.

For the second issue, this was just an error in my logic - when I exited fullscreen, I forgot to flip the bit that said I was back in regular playback mode, so subsequent attempts to enter fullscreen just re-ran the exit fullscreen logic. Also my debugger was disregarding my breakpoints for some reason - this is why I thought my event handler was being interrupted or removed. I resolved that by using the iPhone simulator on my Mac instead of the physical iPod touch device I was using previously to debug.

I still have other issues to resolve before submitting - like styling the fullscreen button so it's not just a big red square, and figuring out how to sync hiding it with the video player controls, but I'll hack away at that more before asking for help :)

@aghassemi aghassemi added the Needs UX label Jun 5, 2017

@aghassemi

This comment has been minimized.

Member

aghassemi commented Jun 5, 2017

@shawnbuso Awesome, thanks for powering through quirks. Behaviour of binary attributes are certainly not consistent across browsers. I ran into a similar issue with muted.

@spacedino can help with design of the fullscreen button. Added Needs UX to the issue.

I might be worth bringing up the IMA requirements for fullscreen functionality with the Web Platform folks (/cc @mounirlamouri) so it can be considered for in the spec.

@mounirlamouri

This comment has been minimized.

mounirlamouri commented Jun 5, 2017

Glad to hear that you want to use native controls :) /CC @beaufortfrancois @avayvod

  1. I guess nothing on the spec side here.
  2. Would you be interested to have a way to request controls without a seek bar?
  3. We launched recently an API to disable a few buttons on the HTMLMediaElement, see https://developers.google.com/web/updates/2017/03/chrome-58-media-updates#controlslist Among other things, you can hide the fullscreen button. If you want to target older versions of Chrome, you can use the CSS selector solution but I wouldn't recommend this solution in the long term.
@aghassemi

This comment has been minimized.

Member

aghassemi commented Jun 5, 2017

@mounirlamouri controlslist is great! Didn't know about it.
RE: fullscreen, I believe what would help here is ability to change the onclick behaviour of the native fullscreen button (My understanding is that IMA likes to take a totally different element to fullscreen rather than the video itself @shawnbuso correct me if I am wrong). Current workaround is to hide the native fullscreen button and show our own but that is not great on the UX side as the custom fullscreen button will look different/ be in different location than the native.

@shawnbuso

This comment has been minimized.

Contributor

shawnbuso commented Jun 5, 2017

@mounirlamouri @aghassemi Correct, changing the behavior of the existing native fullscreen button would be most ideal. I currently have my own button that works as needed, and I was able to hide the fullscreen button from the native controls by removing 'allowfullscreen' from the parent iframe attributes. The thing I'm wrestling with now is syncing showing/hiding my fullscreen button with the native controls - it looks like there's no way for me to detect when the native controls are shown or hidden, so I'm not sure how to best sync the two. If I could just re-purpose the native controls' fullscreen button I could avoid this issue (and all the UX work too, which would be nice :) )

@shawnbuso

This comment has been minimized.

Contributor

shawnbuso commented Jun 15, 2017

Any additional insight on this one? Like I mentioned earlier I've got this more or less working, I just need to figure out how to best sync showing and hiding the fullscreen button with the rest of the player controls. Are there some hard and fast rules about when the video player will show and hide controls that I can mimic in my own code? It seems to be browser and platform dependent:

on desktop the controls show when I mouse over and hide immediately on mouse out
on Android Chrome the controls show on click and hide after about 3 seconds
on iPhone and iPad they show on click but hide after about 4 seconds

I could write a decision tree that changes my show/hide behavior based on the platform and browser, but it would be much cleaner if I could just ask the player to tell me when it shows and hides controls :/

@aghassemi

This comment has been minimized.

Member

aghassemi commented Jun 16, 2017

@shawnbuso for now I think we should special case the popular platforms: Android-Chrome, iOS-* (iOS should be the same regardless of browser) and for everything else do a reasonable default (show/hide on tap/hover and do 3 second timeout)

@shawnbuso

This comment has been minimized.

Contributor

shawnbuso commented Jul 18, 2017

Can we re-assign this? Thanks :)

@aghassemi

This comment has been minimized.

Member

aghassemi commented Jul 18, 2017

Closing in favor of #10502

@aghassemi aghassemi closed this Jul 18, 2017

@aghassemi aghassemi moved this from TODO to DONE in IMA Launch Aug 2, 2017

@aghassemi aghassemi removed this from DONE in IMA Launch Aug 2, 2017

@aghassemi

This comment has been minimized.

Member

aghassemi commented Jul 20, 2018

Reopening this. With both iOS and Chrome updating their UI controls, I see much less reasons to support custom designed controls in AMP.

Would be ideal if we can switch IMA to use native controls. Current blockers to do so are listed in the description of the issue.

@curseagain

This comment has been minimized.

Contributor

curseagain commented Sep 26, 2018

@aghassemi, what is the status on this? We spoke earlier and it would be great to just make a quick fix to add some missing controls in amp-ima-video.

There are some other buggy things about amp-ima-video that may or may not need to have issues assigned.

  • missing controls (mute/unmute, replay)
  • unable to loop - and unable to determine when video or ad has ended so that we can trigger replay
  • when you click on video or ad the sound comes on and there is no mute button
  • when in fullscreen and ad is playing there is no way to exit the ad or mute

You should be able to replicate all of these issues in amp-by-example:
amp-ima-video

cc @torch2424

@torch2424

This comment has been minimized.

Member

torch2424 commented Sep 26, 2018

This was brought up during AMP Contributor Summit 😄

@curseagain is interested in splitting this up to smaller tasks that we can split amongst our teams to ease the bandwidth.

@aghassemi Let us know if this sounds good.

@alanorozco

This comment has been minimized.

Member

alanorozco commented Sep 27, 2018

@curseagain @torch2424

Our current plans is to support IMA via a third-party video plugin, and be able to use something like Video.JS. That way we ease our maintenance load and we also provide a much better player at the same time.

Maybe adding mute/unmute would be fine, since that seems simple enough for a temporary fix. Be aware that the approach will change to use Video.JS (which will include mute/unmute as well).

@torch2424

This comment has been minimized.

Member

torch2424 commented Sep 28, 2018

@alanorozco

After the contributor summit, we think it would be good to get this in a good shape. I offered during our sprint meeting to make this my "side" task in between other work and help out with the component. If you get the chance, can we sync on this? Feel free to ping me.

@torch2424

This comment has been minimized.

Member

torch2424 commented Oct 3, 2018

So I've created three issues to Granularlize new requests from @curseagain:

#18531
#18532
#18533

Let's continue discussion there, and figure out what we can do fix small bugs on the existing amp-ima-video.

@aghassemi @alanorozco

Is there any issue open for the rebuild? I feel like it would be good to close this, and just start referring new discussion of <amp-ima-video> there, since we now have this dialed down.

@aghassemi

This comment has been minimized.

Member

aghassemi commented Oct 8, 2018

Rebuilt of amp-ima-video is a not sure thing yet, really comes down to priority. With amp-video-iframe launching soon, now there will be at least a decent work-around. I will leave it with @jasti to prioritize support and/or rebuilt of AMP IMA video. Either by us or the IMA team.

@torch2424 torch2424 added this to TODO in amp-ima-video Oct 11, 2018

@jasti

This comment has been minimized.

Collaborator

jasti commented Oct 15, 2018

It was decided offline that the AMP team will continue to add support to the amp-ima-video player instead of a complete rewrite.

@jasti jasti changed the title from amp-ima-video: Use native controls. to [Master Feature] amp-ima-video: Use native controls. Oct 15, 2018

@jasti jasti assigned torch2424 and unassigned shawnbuso Oct 15, 2018

@jasti jasti added this to In Development in AMP HTML Project Roadmap Oct 15, 2018

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