If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Use "immersive" fullscreen mode

RESOLVED DUPLICATE of bug 1031519

Status

()

Firefox for Android
General
RESOLVED DUPLICATE of bug 1031519
3 years ago
a year ago

People

(Reporter: snorp, Assigned: ronak khandelwal, Mentored)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec+)

Details

(Whiteboard: [lang=java])

Attachments

(1 attachment, 6 obsolete attachments)

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

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 936099
Assignee: nobody → lucasr.at.mozilla
Status: RESOLVED → REOPENED
tracking-fennec: --- → 36+
Resolution: DUPLICATE → ---
Bug 936099 is too large of a scope. Let's keep this one and only focus on fullscreen
Blocks: 936099
Created attachment 8505265 [details] [diff] [review]
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).
Attachment #8505265 - Flags: review?(lucasr.at.mozilla)
Created attachment 8505266 [details] [diff] [review]
Patch

Whoops. Removed some cruft. Improved some error logging.
Attachment #8505266 - Flags: review?(lucasr.at.mozilla)
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?
Attachment #8505266 - Flags: review?(lucasr.at.mozilla)

Updated

3 years ago
Attachment #8505265 - Flags: review?(lucasr.at.mozilla)
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 :)

Updated

3 years ago
Duplicate of this bug: 1105815
There is a request from bug 110518 that full-screen actually hide the status-bar.

Updated

3 years ago
Assignee: lucasr.at.mozilla → nobody
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.
Mentor: wjohnston@mozilla.com, michael.l.comella@gmail.com
tracking-fennec: 36+ → +
Whiteboard: [lang=java]
(Assignee)

Comment 10

3 years ago
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
Flags: needinfo?(wjohnston)
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.
Flags: needinfo?(wjohnston)
(Assignee)

Comment 12

3 years ago
Created attachment 8560885 [details] [diff] [review]
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
Attachment #8560885 - Flags: feedback?(wjohnston)
(Assignee)

Comment 13

3 years ago
Anyone????
Flags: needinfo?(michael.l.comella)
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 {
Attachment #8560885 - Flags: feedback?(wjohnston) → feedback+
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).
Assignee: nobody → ronak9896
Flags: needinfo?(michael.l.comella)
(Assignee)

Comment 16

3 years ago
Created attachment 8564482 [details] [diff] [review]
patch1076966

pending functionality review.

Thanks,
Attachment #8564482 - Flags: review?(snorp)
Attachment #8564482 - Flags: review?(nalexander)
Attachment #8564482 - Flags: feedback?
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
Attachment #8564482 - Flags: review?(nalexander) → review+
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.
Attachment #8564482 - Flags: review?(snorp) → review+
Ronak, Nick or I will check in the final patch when you post it. Thanks!
(Assignee)

Comment 20

3 years ago
Created attachment 8567455 [details] [diff] [review]
patch.diff

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

Thank you,
Attachment #8567455 - Flags: review?(snorp)
Attachment #8567455 - Flags: review?(nalexander)
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!)
Attachment #8567455 - Flags: review?(snorp) → review+
Keywords: checkin-needed
Attachment #8505265 - Attachment is obsolete: true
Attachment #8505266 - Attachment is obsolete: true
Attachment #8560885 - Attachment is obsolete: true
Attachment #8564482 - Attachment is obsolete: true
Attachment #8564482 - Flags: feedback?
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 :)
Keywords: checkin-needed
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.
Flags: needinfo?(ronak9896)
(Assignee)

Comment 24

3 years ago
Created attachment 8569875 [details] [diff] [review]
patch1076966.diff

Can you check it again.
Thank you
Attachment #8567455 - Attachment is obsolete: true
Attachment #8567455 - Flags: review?(nalexander)
Flags: needinfo?(ronak9896)
Attachment #8569875 - Flags: review?(snorp)
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.
Attachment #8569875 - Flags: review?(snorp) → review?(michael.l.comella)
(Assignee)

Updated

3 years ago
Flags: needinfo?(michael.l.comella)
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
Attachment #8569875 - Flags: review?(michael.l.comella) → review-
(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?
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
Flags: needinfo?(michael.l.comella)
(Assignee)

Comment 29

3 years ago
Created attachment 8571338 [details] [diff] [review]
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 :)
Attachment #8571338 - Flags: review?(michael.l.comella)
(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.
Flags: needinfo?(ronak9896)
(Assignee)

Comment 31

3 years ago
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.
Flags: needinfo?(ronak9896)
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?
Attachment #8571338 - Flags: review?(michael.l.comella) → review-
Attachment #8569875 - Attachment is obsolete: true
(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! :)
Depends on: 1139013
(Assignee)

Comment 34

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

Thank you,
Flags: needinfo?(michael.l.comella)
(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?
Flags: needinfo?(michael.l.comella)
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1031519
You need to log in before you can comment on or make changes to this bug.