Closed Bug 955886 Opened 7 years ago Closed 7 years ago

Title bar is visible in fullscreen with "Hide title bar when scrolling" unchecked when using document.mozFullScreen

Categories

(Firefox for Android :: General, defect)

28 Branch
ARM
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 29
Tracking Status
firefox26 --- wontfix
firefox27 --- wontfix
firefox28 + verified
firefox29 + verified
fennec 28+ ---

People

(Reporter: jhnpwa, Assigned: jdover)

References

Details

(Keywords: reproducible, testcase)

Attachments

(4 files, 1 obsolete file)

Attached file testcase
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131206145143

Steps to reproduce:

1) Open the Settings
2) In the "Display" section, uncheck "Hide title bar when scrolling"
3) Navigate to the testcase attached
4) Enter fullscreen


Actual results:

Only the Android system status bar is hidden. The Firefox title bar is cropped and non-functional.


Expected results:

Both the Android system status bar and the Firefox title bar should be hidden.
Attached image screenshot
Re-summarizing to be more specific to your attached use-case. That looks bad in the screen-shot, maybe we should track?
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
Ever confirmed: true
OS: Linux → Android
Hardware: x86_64 → ARM
Summary: Title bar is visible in fullscreen with "Hide title bar when scrolling" unchecked → Title bar is visible in fullscreen with "Hide title bar when scrolling" unchecked when using document.mozFullScreen
Is this a regression? If so, can we get a regression window?

I've also been seeing some strange behaviour with the title bar where if I scroll down on a page such that the title bar is hidden, but then long-press at the top of the page (where the title bar would have been) the context menu I get is for the title bar, not for the content I long-pressed upon. This only started happening recently so maybe there was some recent change that broke both of these things.
Works for me on a Nexus 5. What device/Android version?
The bug is reproducible in Firefox 23 by manually setting "browser.chrome.dynamictoolbar" to false in "about:config". It is not reproducible in Firefox 22. So I guess the regression occurred between Firefox 22 and 23.
Sounds like this was caused by the initial dynamic toolbar implementation. Might be worth tracking for Firefox 28 as we added UI to the settings for this option.
For comment 4
Flags: needinfo?(jhnpwa)
It affect both of my devices listed below.
1) Samsung GALAXY Tab 2 10.1 running CyanogenMod 10.2 Nightly (Android 4.3.1)
2) Sony Xperia Z running CyanogenMod 10.2.0 Stable (Android 4.3.1)
Flags: needinfo?(jhnpwa)
Yes I can reproduce now that I set the pref. Thank you for looking into the affected versions.
I am able to reproduce this too (was just double-checking), Nightly (01/05, 29.0).
Assignee: nobody → jdover
tracking-fennec: ? → 28+
Attachment #8358532 - Attachment is obsolete: true
Attachment #8358533 - Flags: review?(wjohnston)
Status: NEW → ASSIGNED
Comment on attachment 8358533 [details] [diff] [review]
Remove toolbar margin when entering fullscreen without dynamic toolbar.

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

Do you need to call refreshToolbarHeight() when coming out of Fullscreen too?
(In reply to Wesley Johnston (:wesj) from comment #14)
> Comment on attachment 8358533 [details] [diff] [review]
> Remove toolbar margin when entering fullscreen without dynamic toolbar.
> 
> Review of attachment 8358533 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do you need to call refreshToolbarHeight() when coming out of Fullscreen too?

refreshToolbarHeight() is being called when returning from fullscreen. You could set the margin, but it has no effect on the behavior.
Comment on attachment 8358533 [details] [diff] [review]
Remove toolbar margin when entering fullscreen without dynamic toolbar.

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

Great. thanks!
Attachment #8358533 - Flags: review?(wjohnston) → review+
Keywords: checkin-needed
NI on :JOsh/wes to see if he can help with approval request on aurora/beta in parallel to m-c that will happen over the weekend.

I want to try and get this in Monday's beta rather than wonfixing this or taking it on last beta.
Flags: needinfo?(wjohnston)
Flags: needinfo?(jdover)
Comment on attachment 8358533 [details] [diff] [review]
Remove toolbar margin when entering fullscreen without dynamic toolbar.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Dynamic toolbar
User impact if declined: If you flip off the toolbar (easier now with bug 944925), there's a strange blank area when you enter fullscreen. Us not drawing there means you'll just get whatever is in the buffer.
Testing completed (on m-c, etc.): Landed on mc today. Seems to be fine.

Risk to taking this patch (and alternatives if risky): This code is a little fragile, but this seems sound to me. Given that the pref isn't exposed until 28 (bug 944925), I think maybe this is better to just uplift to Aurora, but requesting both so the the drivers can explicitly decide.

String or IDL/UUID changes made by this patch: None.

Also, pinging cwiiis since I think he knows this a bit better than me.
Attachment #8358533 - Flags: feedback?(chrislord.net)
Attachment #8358533 - Flags: approval-mozilla-beta?
Attachment #8358533 - Flags: approval-mozilla-aurora?
Flags: needinfo?(wjohnston)
(In reply to Wesley Johnston (:wesj) from comment #19)
> Comment on attachment 8358533 [details] [diff] [review]
> Remove toolbar margin when entering fullscreen without dynamic toolbar.
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): Dynamic toolbar
> User impact if declined: If you flip off the toolbar (easier now with bug
> 944925), there's a strange blank area when you enter fullscreen. Us not
> drawing there means you'll just get whatever is in the buffer.
> Testing completed (on m-c, etc.): Landed on mc today. Seems to be fine.
> 
> Risk to taking this patch (and alternatives if risky): This code is a little
> fragile, but this seems sound to me. Given that the pref isn't exposed until
> 28 (bug 944925), I think maybe this is better to just uplift to Aurora, but
> requesting both so the the drivers can explicitly decide.

Hmm, I agree. Re-reading the bug comments here this should have been tracked only for Fx28 and up. For beta I don't think we need this on beta given the feature us behind a pref until Fx28.
> 
> String or IDL/UUID changes made by this patch: None.
> 
> Also, pinging cwiiis since I think he knows this a bit better than me.
Attachment #8358533 - Flags: approval-mozilla-beta?
(In reply to bhavana bajaj [:bajaj] from comment #20)
> (In reply to Wesley Johnston (:wesj) from comment #19)
> > Comment on attachment 8358533 [details] [diff] [review]
> > Remove toolbar margin when entering fullscreen without dynamic toolbar.
> > 
> > [Approval Request Comment]
> > Bug caused by (feature/regressing bug #): Dynamic toolbar
> > User impact if declined: If you flip off the toolbar (easier now with bug
> > 944925), there's a strange blank area when you enter fullscreen. Us not
> > drawing there means you'll just get whatever is in the buffer.
> > Testing completed (on m-c, etc.): Landed on mc today. Seems to be fine.
> > 
> > Risk to taking this patch (and alternatives if risky): This code is a little
> > fragile, but this seems sound to me. Given that the pref isn't exposed until
> > 28 (bug 944925), I think maybe this is better to just uplift to Aurora, but
> > requesting both so the the drivers can explicitly decide.
> 
> Hmm, I agree. Re-reading the bug comments here this should have been tracked
really meant "should not" sigh ~~
> only for Fx28 and up. For beta I don't think we need this on beta given the
> feature us behind a pref until Fx28.
> > 
> > String or IDL/UUID changes made by this patch: None.
> > 
> > Also, pinging cwiiis since I think he knows this a bit better than me.
I opened the testcase using today's Aurora build. The title bar is definitely working now. But it seems that exiting from full screen will cause the page to be partially covered by the title bar.
https://hg.mozilla.org/mozilla-central/rev/9fdbea1cc0ea
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Comment on attachment 8358533 [details] [diff] [review]
Remove toolbar margin when entering fullscreen without dynamic toolbar.

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

f+ with the assumption that this actually works :) I'd test by entering fullscreen, then perhaps rotating and exiting, rotating twice, then exiting, going to the home-screen then back into Firefox via the switcher/launcher, and assuming all of those cases work, I guess it's fine?

::: mobile/android/base/BrowserApp.java
@@ +2054,5 @@
>                      mViewFlipper.setVisibility(View.VISIBLE);
>                      if (isDynamicToolbarEnabled()) {
>                          mLayerView.getLayerMarginsAnimator().showMargins(true);
>                          mLayerView.getLayerMarginsAnimator().setMaxMargins(0, mToolbarHeight, 0, 0);
>                      }

I was wondering if this should come with an accompanying 'else refreshToolbarHeight();' in the !fullscreen case, but I assume in testing that this works, so I guess that must be done by some other callback somewhere...
Attachment #8358533 - Flags: feedback?(chrislord.net) → feedback+
I tried it once more. First, I updated both Aurora and Nightly to the newest version on 3 of my devices with the self-updater just an hour ago. Then, on each of the devices I tested both Aurora and Nightly with the following procedure:
1) Open either Aurora or Nightly
2) Make sure "Hide title bar when scrolling" is unchecked
3) Navigate to the testcase
4) Zoom in all the way and scroll back to the top left corner
5) Enter fullscreen by tapping the button on the page
6) Press the back button on the Android navigation bar or the button on the page

In all 6 cases, there are evidences that the toolbar is covering the page after step 6:
- Text get cut off at the top, zooming out too much can make the page area complete blank.
- The vertical scrollbar is clipped at the top.
- The end of page indicator at the top (blue on Jelly Bean, gray on KitKat) is obviously covered
- Rotating the screen, switching to other apps, navigating to other pages or switching to other tabs won't "repair" it. (Except "about:home")

Merely switching to an already opened "about:home" tab will fix it for all tabs.

Here are the devices I've used to conduct this test:
- Samsung GALAXY Tab 2 10.1 running CyanogenMod 11 Nightly Android 4.4.2
- Sony Xperia Z running CyanogenMod 10.2.0 Stable Android 4.3.1
- Sony Xperia ZR running unmodified Sony ROM version 10.4.B.0.569 Android 4.3

I can provide screenshots if needed. I have even tried restoring to defaults by going to Android Settings -> Apps -> Downloaded -> Aurora/Nightly -> Clear data but it did no work. Should I open a new bug for it?
I'm not sure how I was so blind. The test case I was using only had the header of the webpage hidden and it was just the right size so that I didn't notice. This new patch fixes covering of the page by the toolbar.
Attachment #8363116 - Flags: review?(wjohnston)
Attachment #8363116 - Flags: feedback?(jhnpwa)
(In reply to Josh Dover from comment #27)
> Created attachment 8363116 [details] [diff] [review]
> Fix hidden page content after leaving fullscreen
> 
> I'm not sure how I was so blind. The test case I was using only had the
> header of the webpage hidden and it was just the right size so that I didn't
> notice. This new patch fixes covering of the page by the toolbar.

Thank you for the quick response. I compiled mozilla-central myself with the patch applied and it indeed works. At least I can no longer spot any irregularities from the testcase and some real life web pages.
Comment on attachment 8363116 [details] [diff] [review]
Fix hidden page content after leaving fullscreen

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

Chris should check this.
Attachment #8363116 - Flags: review?(wjohnston) → review?(chrislord.net)
Comment on attachment 8363116 [details] [diff] [review]
Fix hidden page content after leaving fullscreen

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

I'd like this to use refreshToolbarHeight rather than calling setToolbarMargin manually.

::: mobile/android/base/BrowserApp.java
@@ +2061,5 @@
>                      if (isDynamicToolbarEnabled()) {
>                          mLayerView.getLayerMarginsAnimator().showMargins(true);
>                          mLayerView.getLayerMarginsAnimator().setMaxMargins(0, mToolbarHeight, 0, 0);
> +                    } else {
> +                        setToolbarMargin(mViewFlipper.getHeight());

Please use refreshToolbarHeight for this, and modify that code if necessary. We shouldn't have this randomly littered about the file (my bad for making setToolbarMargin available I suppose).

It'd be good to modify the code above that does setToolbarMargin(0) too and have that just call refreshToolbarHeight. Otherwise this code is liable to break if refreshToolbarHeight is called elsewhere while in fullscreen.
Attachment #8363116 - Flags: review?(chrislord.net) → review-
removing the esr tracking flag as it appears to have been in error.
Comment on attachment 8358533 [details] [diff] [review]
Remove toolbar margin when entering fullscreen without dynamic toolbar.

this already landed on aurora, confirming the uplift approval for consistency.
Attachment #8358533 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I'm not able to reproduce this anymore on Nightly (2014-04-23), Aurora (2014-04-23) and Beta 29.0b10 using Nexus 5 (Android 4.4.2), Samsung Galaxy S3 (Android 4.3) and Galaxy Nexus (Android 4.2).
(In reply to cristina.madaras from comment #33)
> I'm not able to reproduce this anymore on Nightly (2014-04-23), Aurora
> (2014-04-23) and Beta 29.0b10 using Nexus 5 (Android 4.4.2), Samsung Galaxy
> S3 (Android 4.3) and Galaxy Nexus (Android 4.2).

Verified as fixed in builds: 
28.0
29.0
using Motorola Razr (Android 4.0.4). Also based on comment 33 I will mark this bug as verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.