Don't use a fullscreen view for HLS

VERIFIED FIXED in Firefox 50

Status

()

P2
normal
VERIFIED FIXED
2 years ago
2 months ago

People

(Reporter: snorp, Assigned: snorp)

Tracking

(Depends on: 1 bug)

50 Branch
Firefox 51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec50+, firefox50 verified, firefox51 verified)

Details

Attachments

(3 attachments, 2 obsolete attachments)

It's jarring.
Depends on: 1286133
Assignee: nobody → snorp
tracking-fennec: --- → ?
Created attachment 8780708 [details] [diff] [review]
Don't use fullscreen mode for the HLS video view on Android
Attachment #8780708 - Flags: review?(s.kaspari)
Comment on attachment 8780708 [details] [diff] [review]
Don't use fullscreen mode for the HLS video view on Android

Review of attachment 8780708 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
@@ +965,5 @@
>          if (hideFirstrunPager(TelemetryContract.Method.BACK)) {
>              return;
>          }
>  
> +        if (mVideoLayout.haveVideo()) {

Is it really haveVideo and not hasVideo? I'm confused.

@@ +4122,5 @@
>              Telemetry.sendUIEvent(TelemetryContract.Event.LAUNCH, method, "restricted");
>          }
>      }
> +
> +    static class VideoFrameLayout extends FrameLayout {

Can we move this to a separate file in org.mozilla.gecko.widget? The line count of BrowserApp is too damn high.

@@ +4183,5 @@
> +        }
> +
> +        @Override
> +        public boolean onTouchEvent(MotionEvent event) {
> +            mVideo.onTouchEvent(event);

* Doesn't the video view already get the touch events?
* Why don't we return the boolean from mVideo.onTouchEvent()?

@@ +4190,5 @@
> +
> +        @Override
> +        public boolean onTrackballEvent(MotionEvent event) {
> +            mVideo.onTrackballEvent(event);
> +            return true;

* return mVideo.onTrackballEvent(..) ?
Attachment #8780708 - Flags: review?(s.kaspari) → review+
(In reply to Sebastian Kaspari (:sebastian) from comment #2)
> Comment on attachment 8780708 [details] [diff] [review]
> Don't use fullscreen mode for the HLS video view on Android
> 
> Review of attachment 8780708 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
> @@ +965,5 @@
> >          if (hideFirstrunPager(TelemetryContract.Method.BACK)) {
> >              return;
> >          }
> >  
> > +        if (mVideoLayout.haveVideo()) {
> 
> Is it really haveVideo and not hasVideo? I'm confused.

Could be either. I guess hasVideo() seems better.

> 
> @@ +4122,5 @@
> >              Telemetry.sendUIEvent(TelemetryContract.Event.LAUNCH, method, "restricted");
> >          }
> >      }
> > +
> > +    static class VideoFrameLayout extends FrameLayout {
> 
> Can we move this to a separate file in org.mozilla.gecko.widget? The line
> count of BrowserApp is too damn high.

Yup, fair enough.

> 
> @@ +4183,5 @@
> > +        }
> > +
> > +        @Override
> > +        public boolean onTouchEvent(MotionEvent event) {
> > +            mVideo.onTouchEvent(event);
> 
> * Doesn't the video view already get the touch events?
> * Why don't we return the boolean from mVideo.onTouchEvent()?

It does, but we want touches in the containing FrameLayout to be eaten and not pass through to the underlying LayerView. That's why we always return true since that means the event has been handled.
Created attachment 8782081 [details]
error shown altought users can view the video
Tried it out on http://www.newshub.co.nz/business/powerball-winners-cash-in-tickets-2016071211#axzz4EA1uumyJ

I noticed a few things
- the overlay feels somehow not very accurate, spanning across the whole width of the screen (see attachment)
- it shows an error message for the video if it's not playing indicating to the user that they can't view the video (see attachment)

I'm currently worried this experience is actually worse than the full screen version.

Thoughts?
(In reply to Barbara Bermes [:barbara] from comment #6)
> Tried it out on
> http://www.newshub.co.nz/business/powerball-winners-cash-in-tickets-
> 2016071211#axzz4EA1uumyJ
> 
> I noticed a few things
> - the overlay feels somehow not very accurate, spanning across the whole
> width of the screen (see attachment)

That's by design here -- we aren't trying to position it on top of the real video.

> - it shows an error message for the video if it's not playing indicating to
> the user that they can't view the video (see attachment)

Yeah, that has always been the case, but we can probably fix that. I'll file separately.

> 
> I'm currently worried this experience is actually worse than the full screen
> version.
> 
> Thoughts?

Why do you think it's worse? The fullscreen one feels like you've been kicked into a totally different app unexpectedly to me. At least with this one you can tell you're still in Firefox, and you can see the web page coming through the overlay.
tracking-fennec: ? → 50+
Created attachment 8783688 [details] [diff] [review]
Don't use fullscreen mode for the HLS video view on Android
Attachment #8783688 - Flags: review+
Attachment #8780708 - Attachment is obsolete: true

Comment 9

2 years ago
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6e4605e1217
Don't use fullscreen mode for the HLS video view on Android r=sebastian

Comment 10

2 years ago
Backout by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a924e27a4666
Backout because I accidentally pushed the wrong patch r=me
Created attachment 8785465 [details] [diff] [review]
Don't use fullscreen mode for the HLS video view on Android
Attachment #8785465 - Flags: review?(s.kaspari)
Attachment #8783688 - Attachment is obsolete: true
Comment on attachment 8785465 [details] [diff] [review]
Don't use fullscreen mode for the HLS video view on Android

Review of attachment 8785465 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/java/org/mozilla/gecko/media/VideoPlayer.java
@@ +1,1 @@
> +package org.mozilla.gecko.media;

nit: License header.

@@ +25,5 @@
> +
> +public class VideoPlayer extends FrameLayout {
> +    private VideoView mVideo;
> +    private FullScreenMediaController mController;
> +    private FullScreenListener mFullScreenListener;

nit: For new Java code we stopped using the 'm' prefix.

@@ +180,5 @@
> +            // http://androidxref.com/6.0.1_r10/xref/frameworks/base/core/java/android/widget/MediaController.java#239
> +            //
> +            // The media buttons are in a horizontal linear layout which is itself packed into
> +            // a vertical layout. The vertical layout is the only child of the FrameLayout which
> +            // MediaController inherits from.

Isn't this a bit fragile? I guess this will need some testing on various Android versions and devices?

@@ +182,5 @@
> +            // The media buttons are in a horizontal linear layout which is itself packed into
> +            // a vertical layout. The vertical layout is the only child of the FrameLayout which
> +            // MediaController inherits from.
> +            LinearLayout child = (LinearLayout)getChildAt(0);
> +            LinearLayout buttons = (LinearLayout)child.getChildAt(0);

nit: space after (LinearLayout) - Did you run checkstyle yet?
Attachment #8785465 - Flags: review?(s.kaspari) → review+
(In reply to Sebastian Kaspari (:sebastian) from comment #12)
> Comment on attachment 8785465 [details] [diff] [review]
> Don't use fullscreen mode for the HLS video view on Android
> 
> Review of attachment 8785465 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/java/org/mozilla/gecko/media/VideoPlayer.java
> @@ +1,1 @@
> > +package org.mozilla.gecko.media;
> 
> nit: License header.
> 
> @@ +25,5 @@
> > +
> > +public class VideoPlayer extends FrameLayout {
> > +    private VideoView mVideo;
> > +    private FullScreenMediaController mController;
> > +    private FullScreenListener mFullScreenListener;
> 
> nit: For new Java code we stopped using the 'm' prefix.
> 
> @@ +180,5 @@
> > +            // http://androidxref.com/6.0.1_r10/xref/frameworks/base/core/java/android/widget/MediaController.java#239
> > +            //
> > +            // The media buttons are in a horizontal linear layout which is itself packed into
> > +            // a vertical layout. The vertical layout is the only child of the FrameLayout which
> > +            // MediaController inherits from.
> 
> Isn't this a bit fragile? I guess this will need some testing on various
> Android versions and devices?

Maybe, but nothing has really ever changed there since it was written: http://androidxref.com/7.0.0_r1/xref/frameworks/base/core/res/res/layout/media_controller.xml

> 
> @@ +182,5 @@
> > +            // The media buttons are in a horizontal linear layout which is itself packed into
> > +            // a vertical layout. The vertical layout is the only child of the FrameLayout which
> > +            // MediaController inherits from.
> > +            LinearLayout child = (LinearLayout)getChildAt(0);
> > +            LinearLayout buttons = (LinearLayout)child.getChildAt(0);
> 
> nit: space after (LinearLayout) - Did you run checkstyle yet?

I didn't run checkstyle, I'll definitely do that.

Comment 14

2 years ago
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f617fcd2cd2
Don't use fullscreen mode for the HLS video view on Android r=sebastian
Priority: -- → P2

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3f617fcd2cd2
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51

Updated

2 years ago
QA Contact: mihai.ninu
Comment on attachment 8785465 [details] [diff] [review]
Don't use fullscreen mode for the HLS video view on Android

Approval Request Comment
[Feature/regressing bug #]: 1286133
[User impact if declined]: Bad HLS experience
[Describe test coverage new/current, TreeHerder]: Aurora, Nightly
[Risks and why]: Low
[String/UUID change made/needed]: None
Attachment #8785465 - Flags: approval-mozilla-beta?

Updated

2 years ago
status-firefox50: --- → affected
Flags: qe-verify+
Comment on attachment 8785465 [details] [diff] [review]
Don't use fullscreen mode for the HLS video view on Android

HLS video is currently planned for Fx50, let's uplift this to Beta.
Attachment #8785465 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This patch didn't apply cleanly to beta, could we get a rebased patch?
Flags: needinfo?(snorp)

Updated

2 years ago
status-firefox50: affected → fixed

Comment 20

2 years ago
Verified as fixed on both Aurora (51.0a2 - 2016-10-06) and Beta 50.0b4 builds, on a Samsung Galaxy S6 EDGE (android 6.0) and on a Nexus 7 (Android 5.1)

Updated

2 years ago
status-firefox50: fixed → verified
status-firefox51: fixed → verified
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.