1076966 - Use "immersive" fullscreen mode

Status: RESOLVED DUPLICATE of bug 1031519

(Firefox for Android Graveyard :: General, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

We should use the new "immersive" fullscreen mode for videos and other places. Mostly I care about videos.

https://developer.android.com/training/system-ui/immersive.html

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Bug 936099 is too large of a scope. Let's keep this one and only focus on fullscreen

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch

This sets the immersive flags whenever we go fullscreen. It also prevents our normal "Hit back to exit" message from showing (since its no longer relevant and Android shows its own "Pull down to exit" message for us).

Still a little janky, but no worse than our current situation (and fixing that jank is probably a different bug).

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch

Whoops. Removed some cruft. Improved some error logging.

Comment 5

[Lucas Rocha (:lucasr)]

Comment on attachment 8505266 [details] [diff] [review]
Patch

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

Doesn't GeckoApp already have a setFullscreen() API? How is this case different? I've recently landed an ActivityUtils class that consolidates fullscreen logic. Maybe you want to reuse that here?

Comment 6

[Wesley Johnston (:wesj)]

Ahh. It does. I missed it. This code is kinda strange. The platform calls GeckoAppShell.setFullScreen, which calls GeckoApp.setFullScreen() which calls ActivityUtils.setFullScreen().

Then Gecko fires a separate event to browser.js who fires an event to the frontend for DOMFullScreen:Start and shows "Hit back to exit" toast. GeckoApp uses that event to hide the browser chrome and notify the LayerView that we're fullscreen. I guess the LayerView wants to know if its the root document that's fullscreen or an inner element (for scrolling reasons if I remember).

I'll refactor this to use both when I've got time, but I'm busy so if some contributor wants it, take it :)

Comment 8

[Aaron Train [:aaronmt]]

There is a request from bug 110518 that full-screen actually hide the status-bar.

Comment 9

[Marat Tanalin]

With tablets equipped with OLED displays (e.g. Samsung Galaxy Tab S), true full-screen mode became especially important (probably even critical) for Firefox since OLED displays (unlike LCD) are subject to irreversible burn-in.

Note that I'm talking about _permanent_ full-screen when viewing _anything_ including webpages, not just playing videos in full screen.

An example or a real full-screen-by-default Android application is the default PDF-reader application of Galaxy Tab S tablet: even Android's black status line at the top is not visible (and shouldn't be visible in Firefox too) -- this line may cause burn-in effect on OLED displays after long-term use.

Thanks.

Comment 10

[ronak khandelwal]

Hey,

This bug seems interesting and i can learn alot while working on it. Do you guys think I can take on this one .

Thank you

p.s: I have some basic idea but can you guys tell me a bit more about as to what has to be done Thank you again

Comment 11

[Wesley Johnston (:wesj)]

This may be as simple as modifying ActivityUtils.java here:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/util/ActivityUtils.java#19

so that it includes immersive fullscreen parameters (for Android v19+). The patch in here already shows how to set those. I'd start there. Test with settings a video fullscreen, and with a page that uses the html5 fullscreen api.

Comment 12

[ronak khandelwal]

Attachment: Bug1076966.diff

hey ,

I got the build working and tried playing one or two videos from ted.com .
What i noticed was the system ui was not there instead a white patch was there a white line as thick as the notification bar and I also think that we should use "SYSTEM_UI_FLAG_IMMERSIVE_STICKY" flag instead of " SYSTEM_UI_FLAG_IMMERSIVE".


Thank you

Comment 13

[ronak khandelwal]

Anyone????

Comment 14

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8560885 [details] [diff] [review]
Bug1076966.diff

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

Hey!  I flagged some small nits and would like to see another revision.  These aren't major but we'll need them before landing.  snorp will do the functionality review.

::: mobile/android/base/util/ActivityUtils.java
@@ +23,2 @@
>          if (Versions.feature11Plus) {
> +            

nit: no newline.

@@ +30,5 @@
>              }
>  
>              window.getDecorView().setSystemUiVisibility(newVis);
> +       }
> +       

nit: cuddle else, spaces around parens like:

} else if (Versions.feature19Plus) {

@@ +32,5 @@
>              window.getDecorView().setSystemUiVisibility(newVis);
> +       }
> +       
> +       else if(Versions.feature19Plus){
> +            

nit: no new lines, spaces around parens like:

if (fullscreen) {

@@ +39,5 @@
> +                newVis=View.SYSTEM_UI_FLAG_LAYOUT_STABLE
> +                        | View.SYSTEM_UI_FLAG_LAYOUT_HIDE_NAVIGATION
> +                        | View.SYSTEM_UI_FLAG_LAYOUT_FULLSCREEN
> +                        | View.SYSTEM_UI_FLAG_HIDE_NAVIGATION
> +                        | View.SYSTEM_UI_FLAG_FULLSCREEN 

nit: no trailing whitespace, throughout.

@@ +44,5 @@
> +                        | View.SYSTEM_UI_FLAG_IMMERSIVE;
> +            }
> +            
> +            else{
> +                newVis=View.SYSTEM_UI_FLAG_VISIBLE;

newVis = View.SYSTEM_UI_FLAG_VISIBLE;

@@ +48,5 @@
> +                newVis=View.SYSTEM_UI_FLAG_VISIBLE;
> +            }
> +            
> +            window.getDecorView().setSystemUiVisibility(newVis);
> +       }

} else {

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Thanks, Nick and Ronak!

(In reply to Nick Alexander :nalexander from comment #14)
> snorp will do the functionality review.

Sounds good to me! Ronak, when you upload your next patch, add an r? with ":snorp" and ":nalexander" and select the auto-complete result (the colon helps auto-completion and is the reviewer's IRC nick).

Comment 16

[ronak khandelwal]

Attachment: patch1076966

pending functionality review.

Thanks,

Comment 17

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8564482 [details] [diff] [review]
patch1076966

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

I noted some nits and would like to kill trailing whitespace throughout that file, but otherwise over to snorp.

::: mobile/android/base/util/ActivityUtils.java
@@ +35,2 @@
>              
>              if(fullscreen){

nit: if (fullscreen)

@@ +35,3 @@
>              
>              if(fullscreen){
>                  newVis=View.SYSTEM_UI_FLAG_LAYOUT_STABLE

nit: newVis = View

@@ +46,1 @@
>                  newVis=View.SYSTEM_UI_FLAG_VISIBLE;

nit: newVis = View

Comment 18

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8564482 [details] [diff] [review]
patch1076966

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

As Nick said, please clean up the trailing whitespace, but looks fine otherwise.

Comment 19

[James Willcox (:snorp) ([e-mail]) (he/him)]

Ronak, Nick or I will check in the final patch when you post it. Thanks!

Comment 20

[ronak khandelwal]

Attachment: patch.diff

Hey,
Sorry for the white spaces gedit puts them automatically I think I had to manually remove them.

Thank you,

Comment 21

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8567455 [details] [diff] [review]
patch.diff

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

I'm sorry, we can't accept a patch that was written with gedit.

(Kidding, this looks fine!)

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Attachment 8567455 [details] [diff] doesn't apply cleanly. Please rebase. Also, please try to mark old patches as obsolete when you attach new ones. Makes things much less confusing :)

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Looks like this patch doesn't apply cleanly either, Ronak! You can follow the steps in bug 1122767 comment 18 to fix this up and upload a new patch.

Comment 24

[ronak khandelwal]

Attachment: patch1076966.diff

Can you check it again.
Thank you

Comment 25

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8569875 [details] [diff] [review]
patch1076966.diff

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

It looks ok, but :mcomella is making some other changes in this area, so let's see what he says.

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8569875 [details] [diff] [review]
patch1076966.diff

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

We should watch out about replacing "full screen" mode with "immersive" mode - in the latter (with the IMMERSIVE flag set), there is no way to navigate out of immersive mode unless there is UI for it. This is fine for our video players which have a fullscreen button, but where else might fullscreen be used now, and in the future? That being said, I *think* we're fine but prepare for potential regressions.

To mitigate this later, I filed bug 1137283.

::: mobile/android/base/util/ActivityUtils.java
@@ +32,2 @@
>  
> +        } else if (Versions.feature19Plus) {

If I'm not mistaken, this won't get run because any device that's feature19Plus will return true for feature11Plus, taking the if statement above.

Did immersive mode work in your tests?

@@ +32,5 @@
>  
> +        } else if (Versions.feature19Plus) {
> +
> +            if (fullscreen) {
> +                newVis = View.SYSTEM_UI_FLAG_LAYOUT_STABLE

I can't figure out from the docs what this does - do you know?

@@ +34,5 @@
> +
> +            if (fullscreen) {
> +                newVis = View.SYSTEM_UI_FLAG_LAYOUT_STABLE
> +                        | View.SYSTEM_UI_FLAG_LAYOUT_HIDE_NAVIGATION
> +                        | View.SYSTEM_UI_FLAG_LAYOUT_FULLSCREEN

I'm pretty sure these SYSTEM_UI_FLAG_LAYOUT_* flags should be applied to Views within the layout, not the decorview. From the docs [1]:

View would like its window to be layed out as if it has requested SYSTEM_UI_FLAG_FULLSCREEN, even if it currently hasn't. This allows it to avoid artifacts when switching in and out of that mode, at the expense that some of its user interface may be covered by screen decorations when they are shown. You can perform layout of your inner UI elements to account for non-fullscreen system UI through the fitSystemWindows(Rect) method. 

---

Have you tested without these?

[1]: https://developer.android.com/reference/android/view/View.html#SYSTEM_UI_FLAG_LAYOUT_FULLSCREEN

Comment 27

[Marat Tanalin]

(In reply to Michael Comella (:mcomella) from comment #26)
> We should watch out about replacing "full screen" mode with "immersive" mode
> - in the latter (with the IMMERSIVE flag set), there is no way to navigate
> out of immersive mode unless there is UI for it. This is fine for our video
> players which have a fullscreen button, but where else might fullscreen be
> used now, and in the future?

FWIW, I'm interested in _permanent_ full-screen mode (i.e. without Android's status line at the top at all) sothat I could use browser (regardless of kind of content: be it video or just web pages) without danger of my tablet's OLED-display burn-in. Will it be possible to turn-on such permanent full-screen mode (at least via an extension) with this implementation?

Comment 28

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

This addon [1] was used to make the browser fullscreen before we had the dynamic toolbar that swiped off-screen, but unfortunately, I don't know if it still works, and I doubt it'd hide the Android status bar and definitely does not hide the navigation bars (since this feature was added in KitKat).

(In reply to Marat Tanalin | tanalin.com from comment #27)
> Will it be possible to turn-on such permanent full-screen mode

I don't think we have any plans as such but please feel free to file and we can see what comes of it.

> (at least via an extension) with this implementation?

An extension would need hooks into Android's fullscreen APIs which means it'd be just as much work as supporting the feature directly in product.

[1]: https://addons.mozilla.org/En-us/android/addon/full-screen-252573/?src=search

Comment 29

[ronak khandelwal]

Attachment: bug1076966.diff

Hey,
Sorry for the delay . I couldn't test it due to some technical problems otherwise I think we should be fine.
Thank you :)

Comment 30

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to ronak khandelwal from comment #29)
> Sorry for the delay . I couldn't test it due to some technical problems

Are these any technical problems we can help with?

The Android fullscreen APIs are very tricky to use properly so my comments are largely speculation based on the (traditionally underwhelming) Android docs: it'd be great if you could test and answer my questions in comment 26.

Comment 31

[ronak khandelwal]

Hey Michael,

Yeah the immersive mode works even without those flags and I'd still want a immersive_sticky flag instead of immersive flag the reason being whenever for what so ever reason the user wants to see the status bar it won't go away on it own so atleast for the video player I'd suggest we change the flag. And I did something stupid with my phone(custom rom's and stuff) so sorry for the delay and thanks anyway.

Comment 32

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8571338 [details] [diff] [review]
bug1076966.diff

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

Sorry for the delay.

It looks like this patch builds on another patch that is not present here - can you fold and uplift the combination?

Comment 33

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to ronak khandelwal from comment #31)
> Yeah the immersive mode works even without those flags and I'd still want a
> immersive_sticky flag instead of immersive flag

Good research - I agree! :)

Comment 34

[ronak khandelwal]

Hey Micheal ,
So what should I do next.
change the flag and ?.

Thank you,

Comment 35

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to ronak khandelwal from comment #34)
> So what should I do next.
> change the flag and ?.

As per comment 32, this patch builds on another patch I don't have and thus is missing the appropriate context to be able to review - can you fold together the patches (see `hg help qfold`) and upload the result?