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

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Law, Cheuk-wing]

Attachment: 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.

Comment 1

[Law, Cheuk-wing]

Attachment: screenshot

Comment 2

[Aaron Train [:aaronmt]]

Re-summarizing to be more specific to your attached use-case. That looks bad in the screen-shot, maybe we should track?

Comment 3

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 4

[Kevin Brosnan [Ex-Mozilla]]

Works for me on a Nexus 5. What device/Android version?

Comment 5

[Kartikaya Gupta (email:[e-mail])]

(FYI I filed bug 956075 for comment 3).

Comment 6

[Law, Cheuk-wing]

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.

Comment 7

[Kevin Brosnan [Ex-Mozilla]]

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.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

For comment 4

Comment 9

[Law, Cheuk-wing]

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)

Comment 10

[Kevin Brosnan [Ex-Mozilla]]

Yes I can reproduce now that I set the pref. Thank you for looking into the affected versions.

Comment 11

[Aaron Train [:aaronmt]]

I am able to reproduce this too (was just double-checking), Nightly (01/05, 29.0).

Comment 12

[Josh Dover [:jdover]]

Attachment: Remove toolbar margin when entering fullscreen without dynamic toolbar.

Comment 13

[Josh Dover [:jdover]]

Attachment: Remove toolbar margin when entering fullscreen without dynamic toolbar.

Comment 14

[Wesley Johnston (:wesj)]

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?

Comment 15

[Josh Dover [:jdover]]

(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 16

[Wesley Johnston (:wesj)]

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!

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/9fdbea1cc0ea

Comment 18

[bhavana bajaj [:bajaj]]

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.

Comment 19

[Wesley Johnston (:wesj)]

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.

Comment 20

[bhavana bajaj [:bajaj]]

(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.

Comment 21

[bhavana bajaj [:bajaj]]

(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.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c65cec1da746

Comment 23

[Law, Cheuk-wing]

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.

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/9fdbea1cc0ea

Comment 25

[Chris Lord [:cwiiis]]

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...

Comment 26

[Law, Cheuk-wing]

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?

Comment 27

[Josh Dover [:jdover]]

Attachment: 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.

Comment 28

[Law, Cheuk-wing]

(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 29

[Wesley Johnston (:wesj)]

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.

Comment 30

[Chris Lord [:cwiiis]]

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.

Comment 31

[Lukas Blakk [:lsblakk] use ?needinfo]

removing the esr tracking flag as it appears to have been in error.

Comment 32

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 33

[Cristina Madaras, QA [:CristinaM]]

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).

Comment 34

[Cristina Madaras, QA [:CristinaM]]

(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.