Closed Bug 842782 Opened 10 years ago Closed 6 years ago

Fullscreen video should lock screen orientation to video's orientation

Categories

(Firefox for Android Graveyard :: Audio/Video, defect, P3)

Product:
Firefox for Android Graveyard
Component:
Audio/Video
Platform:
Unspecified
Android
Type:
defect

Tracking

(firefox57 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- verified

People

(Reporter: wesj, Assigned: jhlin)

References

Details

Attachments

(4 files, 2 obsolete files)

Bug 842782 - p1: add a pref for fullscreen video orientation lock.
59 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 842782 - p2: introduce attributes for orientation lock to video element.
59 bytes, text/x-review-board-request
smaug
: review+
jwwang
: review+
Details
Bug 842782 - p3: lock fullscreen video orientation.
59 bytes, text/x-review-board-request
ralin
: review+
Details
Bug 842782 - p4: enable fullscreen video orientation lock for Fennec nightly.
59 bytes, text/x-review-board-request
jwwang
: review+
Details 
So much video is in landscape, but I always hold my phone in portrait. It always feels weird to me to have fullscreen video not take up the whole screen. I think maybe we should just lock the screen orientation when we go into fullscreen mode for video.
Component: General → Audio/Video
OS: Linux → Android
Hardware: x86 → Unspecified
Version: unspecified → Trunk
Kevin/Roland, have we ever had any other complaints or similar requests made by users after this bug was filed? Trying to prioritize
Flags: needinfo?(rtanglao)
Flags: needinfo?(kbrosnan)
I don't know that users would think about this. It is one of those things that just makes sense to do.
Flags: needinfo?(kbrosnan)
yes i agree with kevin, this does make sense to do
Flags: needinfo?(rtanglao)
It would be great if you can implement this, after entering fullscreen it should force video orientation into landscape and when you exit restore to portrait. Most of google media apps have this feature even well known video players, please make this happen :)
Joe, 
This is an old bug. What do you think? Should we do that?
Flags: needinfo?(jcheng)
Jack,
What do you think?
Flags: needinfo?(jalin)
(In reply to John from comment #4)
> It would be great if you can implement this, after entering fullscreen it
> should force video orientation into landscape and when you exit restore to
> portrait. Most of google media apps have this feature even well known video
> players, please make this happen :)

Agree

I suggest we lock screen orientation and use landscape when users watch video in full screen mode (after users press extension icon on the bottom right of the video).
Flags: needinfo?(jalin)
Xidorn, 
Can you shed some light here to guide us how to set fullscreen in landscape mode?
Use this one[1]?

[1]https://dxr.mozilla.org/mozilla-central/source/dom/base/nsIDocument.h?q=%2Bfunction%3A%22nsIDocument%3A%3ASetCurrentOrientation%28mozilla%3A%3Adom%3A%3AOrientationType%2C+uint16_t%29%22&redirect_type=single#1512
Flags: needinfo?(jcheng) → needinfo?(xidorn+moz)
I'm not familiar with Screen Orientation API. I guess it's worth checking what is screen.lockOrientation() doing.

I guess there are two approaches to implement this:

1. Implement in JS side, and only works if user use native video controls or context menu to fullscreen a <video>. That way, we just need to call screen.lockOrientation() before we call requestFullscreen. Websites which create their own video controls is responsible for locking the orientation themselves. (In general, websites which create their own video controls wouldn't fullscreen a <video>. Instead, they should fullscreen some wrapper element which includes their controls as well. In that case, we can do nothing anyway.)

2. Implement in C++ side, that we special case <video> element in either Element::RequestFullscreen or nsDocument::RequestFullScreen, and call corresponding method. With this, even if the content requests fullscreen on the <video> element, we can lock the orientation as well. But I suspect whether that is a common case.

I probably prefer doing this in JS side, so that we don't need to special case anything, and also don't change the semantics of the Fullscreen API (which only means putting something into fullscreen, doesn't indicate the orientation lock behavior in addition).
Flags: needinfo?(xidorn+moz)
Xidorn,
Thanks for your feedback.
For consistency, we may want to try to play all video playbacks in fullscreen in landscape mode if possible. 

John,
Can you help follow up this bug?
Flags: needinfo?(jolin)
Why do you want all video playbacks to be in landscape mode? There are videos with aspect ratios that would fit better in portrait mode. Please change it only to fit the aspect ratio better.
(In reply to Blake Wu [:bwu][:blakewu] from comment #12)
> Xidorn,
> Thanks for your feedback.
> For consistency, we may want to try to play all video playbacks in
> fullscreen in landscape mode if possible. 
> 
> John,
> Can you help follow up this bug?

 Did some quick tests on several sites with video to see what type of element they use to request fullscreen, and looks like Xidon's suspicion in 2. is valid:
 - Youtube, CNN: video
 - Vimeo, twitch.tv, a.m.o: div
 - NY Times: figure
 - CNN: html

 I also agree that changing requestFullscreen() semantics doesn't sound like a good idea.

 Will check how Chrome implements this to get more idea.
Flags: needinfo?(jolin)
My mistake. Chrome on Android doesn't lock orientation in fullscrenn at all. :$
Correction: the latest Chrome does lock orientation. Will check which release added this feature.
Tested the same sites in comment 14 with latest Chrome(59), only Youtube locked the orientation when in fullscreen.
Tried requesting desktop page for Youtube and the orientation is not locked.

It looks like Chrome only implemented orientation lock for its native controls [1]. And unlike other sites, Youtube mobile page doesn't use custom control.

Considering a. not all video sites use video element to request fullscreen, and b. lock/rotate screen when JS calls requestFullscreen() confuses web apps that wish to implement their own behavior, I also prefer Xidorn's approach 1.

[1] https://chromium.googlesource.com/chromium/src.git/+/master/third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.h#22
I don't have handy mobile right now, I'll look more and test it tomorrow.
Priority: -- → P3
Comment on attachment 8898094 [details]
Bug 842782 - p2: enable fullscreen video orientation lock only for Fennec Nightly.

https://reviewboard.mozilla.org/r/169426/#review175692
Comment on attachment 8898094 [details]
Bug 842782 - p2: enable fullscreen video orientation lock only for Fennec Nightly.

https://reviewboard.mozilla.org/r/169426/#review175694
Attachment #8898094 - Flags: review?(ralin) → review+
Comment on attachment 8898093 [details]
Bug 842782 - p1: lock fullscreen video orientation.

https://reviewboard.mozilla.org/r/169424/#review175696

Generally looks good, Thank you :D

Apart from setting lock/unlock state over fullscreen button in videocontrols binding, we should consider Android activity lifecycle as well.
A common step to produce a irreversible lock like:
1. watch a video on Youtube
2. click fullscreen button
3. press "Home" button
4. press "Menu" button and resume Fennec
We'll find the fullscreen is dismissed but the orientation is still locked, but no way to restore it.

A way worth trying is to make the unlocking happen right after we exit fullscreen while resuming the activity. We may have to look into Fennec codebase to figure out how it works, and probably need a Fennec peer to help out.
Attachment #8898093 - Flags: review?(ralin)
When we exit fullscreen, fullscreenchange event should always be triggered, which should lead to unlocking the screen... Why is it not the case here?
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #25)
> When we exit fullscreen, fullscreenchange event should always be triggered,
> which should lead to unlocking the screen... 
So we don't have to explicitly call mozUnlockOrientation when exit?

> Why is it not the case here?
Not sure why would be this case, maybe Fennec intends to restore the lock state when resume. I have to look more into it.
(In reply to Ray Lin[:ralin] from comment #26)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #25)
> > When we exit fullscreen, fullscreenchange event should always be triggered,
> > which should lead to unlocking the screen... 
> So we don't have to explicitly call mozUnlockOrientation when exit?

We have to, but the patch should have been doing so. In the patch, onFullscreenChange calls maybeLockOrientation which should unlock screen orientation if we are exiting fullscreen.

> > Why is it not the case here?
> Not sure why would be this case, maybe Fennec intends to restore the lock
> state when resume. I have to look more into it.

So there must be some issue somewhere...
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #27)
> We have to, but the patch should have been doing so. In the patch,
> onFullscreenChange calls maybeLockOrientation which should unlock screen
> orientation if we are exiting fullscreen.
I guess the callback was never called in this case. (Installing Android Studio to watch the log :P
Many thanks to Jay's help to identify the issue, it turns out that videocontrols binding will not be the same before and after activity visibility change and the 'orientation locked' state stored in the binding object won't work as expected. It seems that destructor wasn't called when that happens so unlocking before destroying doesn't help here either.

Move the 'locked' state to video element fixes this problem. Will update the patch for review later.
Comment on attachment 8898093 [details]
Bug 842782 - p1: lock fullscreen video orientation.

https://reviewboard.mozilla.org/r/169424/#review176192

Thanks, persisting the lock state over <video> seems workable for me, though I wonder if it's fine to set a content-observable property and expose it out there.

::: toolkit/content/widgets/videocontrols.xml:1243
(Diff revision 2)
>            this.fullscreenButton.removeAttribute("fullscreened");
>          }
>        },
>  
>        onFullscreenChange() {
> +        this.updateOrientationState(this.isVideoInFullScreen());

Current binding is shared between Desktop and Android, we should keep desktop away from orientation lock unless we need it too.

We can add a check like this:
if (this.videocontrols.isTouchControls) {
  this.updateOrientationState(this.isVideoInFullScreen());
}

::: toolkit/content/widgets/videocontrols.xml:1260
(Diff revision 2)
> +          if (!!this.video.mozOrientationLocked) {
> +            return;
> +          }
> +          let dimenDiff = this.video.videoWidth - this.video.videoHeight;
> +          if (dimenDiff > 0) {
> +            this.video.mozOrientationLocked = window.screen.mozLockOrientation('landscape');

nit: Please use double quote all the time in javascript in our codebase. Fix the rest of two strings as well.

::: toolkit/content/widgets/videocontrols.xml:1266
(Diff revision 2)
> +          } else if (dimenDiff < 0) {
> +            this.video.mozOrientationLocked = window.screen.mozLockOrientation('portrait');
> +          } else {
> +            this.video.mozOrientationLocked = window.screen.mozLockOrientation(window.screen.orientation);
> +          }
> +        } else if (!!this.video.mozOrientationLocked) {

nit:
Would changing to the following be more clear?

```javascript
...
else {
  if (!this.video.mozOrientationLocked) {
    return;
  }
  ...
}
```

Reading from the function name and its parameter type, I would expect the another block of if-else do the exact opposite thing when lock is false, but both are fine to me anyway.

::: toolkit/content/widgets/videocontrols.xml:1866
(Diff revision 2)
>      ]]>
>    </constructor>
>    <destructor>
>      <![CDATA[
>      this.Utils.terminateEventListeners();
> +    this.Utils.updateOrientationState(false);

Same here.

Please add a check to ensure this is conditionally called for only Android.
Attachment #8898093 - Flags: review?(ralin)
Comment on attachment 8898093 [details]
Bug 842782 - p1: lock fullscreen video orientation.

https://reviewboard.mozilla.org/r/169424/#review176192

> Current binding is shared between Desktop and Android, we should keep desktop away from orientation lock unless we need it too.
> 
> We can add a check like this:
> if (this.videocontrols.isTouchControls) {
>   this.updateOrientationState(this.isVideoInFullScreen());
> }

Property "shouldLockOrientation" is used (at the beginning of `updateOrientationState()`)  for keep desktop from locking orientation. At first I wanted to use a pref, but it seems XUL script running in content process cannot access XPCOM components/services when e10s is enabled. So I use the build-time preprocessor trick and guard it with `MOZ_FENNEC` in the other patch.

> nit: Please use double quote all the time in javascript in our codebase. Fix the rest of two strings as well.

Wil do.

> nit:
> Would changing to the following be more clear?
> 
> ```javascript
> ...
> else {
>   if (!this.video.mozOrientationLocked) {
>     return;
>   }
>   ...
> }
> ```
> 
> Reading from the function name and its parameter type, I would expect the another block of if-else do the exact opposite thing when lock is false, but both are fine to me anyway.

Will do.

> Same here.
> 
> Please add a check to ensure this is conditionally called for only Android.

As mentioned earlier, `shouldLockOrientation` takes care of it.
Attachment #8898093 - Attachment is obsolete: true
Attachment #8898094 - Attachment is obsolete: true
Comment on attachment 8901055 [details]
Bug 842782 - p3: lock fullscreen video orientation.

https://reviewboard.mozilla.org/r/172530/#review177814

LGTM, thanks
Attachment #8901055 - Flags: review?(ralin) → review+
Large part of the patch has been rewritten after discussing with Ray.
- enable/disable orientation lock by introducing an read-only attribute to video element that is visible only to chrome or XBL scope.
- move the lock state to video element, as an chrome or XBL only attribute too.
- hide both attributes behind a pref, which is enabled only in Fennec nightly for now.
Comment on attachment 8901053 [details]
Bug 842782 - p1: add a pref for fullscreen video orientation lock.

https://reviewboard.mozilla.org/r/172526/#review177816

::: dom/media/MediaPrefs.h:211
(Diff revision 1)
>                                                                MediaWarningsAsErrorsStageFrightVsRust, bool, false);
>  
>    // resume background video decoding when the cursor is hovering over the tab.
>    DECL_MEDIA_PREF("media.resume-bkgnd-video-on-tabhover",     ResumeVideoDecodingOnTabHover, bool, false);
> +
> +  DECL_MEDIA_PREF("media.videocontrols.lock-video-orientation",  AllowLockingOrientation, bool, false);

The naming convention is XXXEnabled. Please call it {Video}OrientationLockEnabled.
Attachment #8901053 - Flags: review?(jwwang) → review+
Comment on attachment 8901054 [details]
Bug 842782 - p2: introduce attributes for orientation lock to video element.

https://reviewboard.mozilla.org/r/172528/#review177818

::: dom/html/HTMLVideoElement.h:144
(Diff revision 1)
>    FrameStatistics* GetFrameStatistics();
>  
>    already_AddRefed<VideoPlaybackQuality> GetVideoPlaybackQuality();
>  
> +
> +  bool MozAllowOrientationLocking() const

Call it MozOrientationLockEnabled.
Attachment #8901054 - Flags: review?(jwwang) → review+
Comment on attachment 8901056 [details]
Bug 842782 - p4: enable fullscreen video orientation lock for Fennec nightly.

https://reviewboard.mozilla.org/r/172532/#review177820
Attachment #8901056 - Flags: review?(jwwang) → review+
Comment on attachment 8901054 [details]
Bug 842782 - p2: introduce attributes for orientation lock to video element.

https://reviewboard.mozilla.org/r/172528/#review177916
Attachment #8901054 - Flags: review?(bugs) → review+
Pushed by jolin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/92092cde72e1
p1: add a pref for fullscreen video orientation lock. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/ba812092214c
p2: introduce attributes for orientation lock to video element. r=jwwang,smaug
https://hg.mozilla.org/integration/autoland/rev/f3653a89b841
p3: lock fullscreen video orientation. r=ralin
https://hg.mozilla.org/integration/autoland/rev/b3005c73d7ad
p4: enable fullscreen video orientation lock for Fennec nightly. r=jwwang
For the eslint failure, I think we can refactor that block as mentioned in comment 32. IMO, an early return to eliminate special cases at beginning is preferable, besides, it helps our primary code piece stay at the same indent level. :-D
(In reply to Ray Lin[:ralin] from comment #54)
> For the eslint failure, I think we can refactor that block as mentioned in
> comment 32. IMO, an early return to eliminate special cases at beginning is
> preferable, besides, it helps our primary code piece stay at the same indent
> level. :-D

Hmm... this doesn't seem like a special case that justifies early return (as the `if (!enabled) ...` at the beginning) to me.

Here we want to lock or unlock according to the input argument, and the `if (lock) { ... } else { ... }` says exactly what we want. Frankly speaking,
```
  if (lock) {
    ...
    return;
  }
  // else
  ...
```
looks more like an error return to me and is worse than the original `if (lock) { ... } else if ...` version.

Anyway, is always passing ESLint checks a hard requirement? If so, I'll update the patch.

But IMHO for this case, human judgment should override the rule. :)
Flags: needinfo?(jolin) → needinfo?(ralin)
Or just `// eslint-disable-line no-lonely-if` to blind the linter here?
(In reply to John Lin [:jolin][:jhlin] from comment #55)
> Hmm... this doesn't seem like a special case that justifies early return (as
> the `if (!enabled) ...` at the beginning) to me.
> 
> Here we want to lock or unlock according to the input argument, and the `if
> (lock) { ... } else { ... }` says exactly what we want. Frankly speaking,
> ```
>   if (lock) {
>     ...
>     return;
>   }
>   // else
>   ...
> ```
> looks more like an error return to me and is worse than the original `if
> (lock) { ... } else if ...` version.
The original one is fine to me, as I said in the previous review. My initial thought was more about considering the symmetry of lock logic, and wondering whether the refactor could bring more clarity.

> Anyway, is always passing ESLint checks a hard requirement? If so, I'll
> update the patch.
Yeah, it's a hard requirement for landing. We can do ./mach eslint [path] to have a quick check locally before push.

> But IMHO for this case, human judgment should override the rule. :)
Agree, my stance is from the respective of human readability.

> Or just `// eslint-disable-line no-lonely-if` to blind the linter here?
I'm afraid that we don't usually do that unless few inevitable cases. If we can fix it in a regular way, we should avoid overriding global eslint rules.


How about revert the "else" part to `if (lock) { ... } else if ...`, and see if the eslint failure is still? Thanks.
Flags: needinfo?(ralin)
Thanks, Ray. No complaint from ESLint with `if ... else if ...`! \o/

Will update the patch and land to inbound directly.
Sorry, just realized that I completely misunderstand comment 54 and your original review comment. :$

```
} else {
  if (!this.video.mozIsOrientationLocked) {
    return;
  }
  ...
}
```
is the right way to go!
nvm. Your decision, both are good to me :D
Pushed by jolin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b248ef575cd0
p1: add a pref for fullscreen video orientation lock. r=jwwang
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dddca972f39
p2: introduce attributes for orientation lock to video element. r=jwwang,smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d8c071710d8
p3: lock fullscreen video orientation. r=ralin
https://hg.mozilla.org/integration/mozilla-inbound/rev/051e15d44fc5
p4: enable fullscreen video orientation lock for Fennec nightly. r=jwwang
Devices:
 - Nexus 5 (Android 6.0.1);
 - Nexus 6 (Android 7.1.1);
 - Huawei MediaPad M2 (Android 5.1.1).

Verified as fixed.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
What are the plans for this riding the trains and do we have a tracking bug for that?
Assignee: nobody → jolin
Flags: needinfo?(jolin)
Blocks: 1422263
(In reply to Jan Henning [:JanH] from comment #64)
> What are the plans for this riding the trains and do we have a tracking bug
> for that?

Thanks a lot for the reminder. Filed bug 1422263 to track it (target: 59).
Flags: needinfo?(jolin)
Depends on: 1433554
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.