Closed Bug 1500719 Opened Last year Closed 8 months ago

URL bar doesn't hide after entering fullscreen

Categories

(Firefox for Android :: General, defect, P3)

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 68
Tracking Status
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- verified
firefox68 --- verified

People

(Reporter: xidorn, Assigned: rbarker)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

Steps to reproduce:
1. open attachment 9018847 [details]
2. click "fullscreen" button in the page

Expected result:
There is a semi-transparent blue cover the whole screen with the text and button underneath, and all chrome UI should be hidden.

Actual result:
The url bar is still there until touching the screen again.
Tested this on a Pixel with Android P, the issue described by you seems to only affect Release, could not reproduce it on Nightly or Beta.

@Xidron, can we close this as it seems to be fixed in the other versions of FF? I am assuming the fix will be riding the trains.
Flags: needinfo?(xidorn+moz)
OS: Unspecified → Android
Hardware: Unspecified → ARM
Version: Trunk → Firefox 63
Attached file testcase
No, it doesn't seem to me this has been fixed, but it indeed seems to be less reproducible in Beta.

Here is a better testcase which enlarges the button so that it's easier to click.

Steps to reproduce:
1. load the page and click the button
2. if the url bar disappears, press back to leave fullscreen and refresh page, then try 1 again.

It happens probably 1 in 10 times...
Flags: needinfo?(xidorn+moz)
Yeah, managed to reproduce it consistently, varying from one device to another, on some it was a 1 in 3 times occurrence, others 1 in 15 for both beta and nightly.
See Also: → 1504655
Marking this as P1, since it sounds like this breaks fullscreen mode since at least 63.
Attached file random.bat
For testing, I used a batch file that entered and exited the page provided in comment 2, I left the bat running for about 5 minutes/run and then ran a second one for about one minute without any timeout between entering and exiting fullscreen.

Regression window:
Last good revision: db130923a2cdfd7872dcf4e745293ae62220ae3d
First bad revision: 4a230b07f0cbf48e87dcb4265ea2d00893bb1b62
Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=db130923a2cdfd7872dcf4e745293ae62220ae3d&tochange=4a230b07f0cbf48e87dcb4265ea2d00893bb1b62
Bug 1437064 was landed on Firefox 64, but this bug was reproducible on 63 IIRC, so that doesn't sound very likely to be the change to be blamed.
Hmm, I'll see if there is any device where this reproduces more reliably and run another regressionwindow and see if get other results. It's quite hard to find a proper regression since the issue occurs by the looks of it at random.
Assignee: nobody → andrei.a.lazar
After trying with several other devices I could not find any valid regression range. On some devices where a build was good on others it was bad and vice versa. So trying to narrow this down doesn't seem to be doable in a reliable manner.
Can't reproduce with release 64 (also on a Pixel with Android P). If this is only reproducible on particular devices maybe we don't need to treat this as a top priority. Andrei have you been able to reproduce it at all?
Flags: needinfo?(andrei.a.lazar)
Ever now and then the button is covered by the toolbar after exiting fullscreen.

As this is about entering fullscreen, would that be a different bug?
Also, the transition itself isn't very smooth.

Is that another bug as well?
Attached file random.sh
Hey Liz, unfortunately I couldn't reproduce this, I also made a .sh script (for UNIX systems) to force this scenario but with no success. Regarding the lack of smoothness, I'm pretty sure that's a gecko issue. Should I continue my investigations?
Flags: needinfo?(andrei.a.lazar) → needinfo?(lhenry)
Thank you Andrei! It sounds like it doesn't reproduce very often, so maybe we should not be treating this as a P1 issue. If we start seeing more duplicate reports we could investigate again. 

For the smoothness issue, best to file a new bug and tag it [geckoview].
Flags: needinfo?(lhenry)
Priority: P1 → P3
Botond, please see Comment 10
Flags: needinfo?(botond)

(In reply to csheany from comment #10)

Ever now and then the button is covered by the toolbar after exiting
fullscreen.

How are you exiting fullscreen? The test page does not provide a way to exit fullscreen.

Flags: needinfo?(botond)

The devices back button.

It happens when the page is zoomed in.

Should I clone to discuss?

Flags: needinfo?(botond)

(In reply to csheany from comment #16)

The devices back button.

Thanks.

It happens when the page is zoomed in.

Ah, ok. That sounds like it may be bug 1516471, then.

Should I clone to discuss?

Feel free to either mention this behaviour in a comment on bug 1516471, or file a bug that depends on bug 1516471.

Flags: needinfo?(botond)
Assignee: andrei.a.lazar → nobody

How is this performing for you in Nightly 67.0a1 (2019-03-08)?

Flags: needinfo?(xidorn+moz)

With the testcase and steps in comment 2, it's 100% reproducible on my phone.

Flags: needinfo?(xidorn+moz)

Thank you for your response.

Do you mean that it still isn't hiding properly?

Yes, it still isn't hiding properly.

Have you been keeping an eye on it.

Do you know for how long?

If you are able to test the previous Nightly is the result the same?

It used to freeze with the button and toolbar visible at the same time...

But now I am seeing the test page reach the top of the screen under the toolbar.

(In reply to csheany from comment #22)

Have you been keeping an eye on it.

Do you know for how long?

No, I don't know.

(In reply to csheany from comment #23)

If you are able to test the previous Nightly is the result the same?

I am able to do that, but that takes time. Unless you have specific change in mind which may have changed the behavior recently, I'm not going to do that.

I don't remember the last time I checked but I thought it was happening fever times but now it is more.

The reason I was asking is...

I was testing Bug 1504865 with Bug 1158392 landed and as you can imagine made that difficult.

I was surprised because I didn't remeber it being so bad.

Yes it still wasn't smooth but it was for as long.

I cleared data for a fresh install and that may have caused Comment 24 but I'm not sure.

I mean consistent.

I just tested the second attachment and Bug 1504865 doesn't occur.

I was confused for a minute but looking at the page source it has meta data.

I wonder if that is a factor and may provide a clue as to resolving.

It was thought to be fixed by Bug 1158392 but that doesn't seem the case unless it depends on additonal patches.

I don't know if you have kept up with Bug 1504865 but it may have been cause by Bug 1298218.

Do you have any thoughts about that?

You don't have to check.

I guess I couldn't believe it was still an issue as the last comment (I made) was 2 months ago.

Do you still not know what caused the regression?

Is it about the unprefixed version of the Fullscreen API?

Thanks. That all makes sense.

So, it seems to me that bug 1158392 (containerless scrolling) is actually the reason which causes the url bar to consistently stay after entering fullscreen (as described in comment 19). If you go to about:config and change layout.scroll.root-frame-containers to 1 (from 0) and refresh the page, you would see url bar disappears automatically. (Also it seems to me even after doing this, bug 1504865 still doesn't occur for me, so maybe it has been fixed by something else as well.)

I guess maybe previously some mechanism triggers a repaint or something which removes the url bar, but now that is no longer triggered immediately.

I realize my comments in between Comment 25 were long winded and I was afraid you were in the process of testing the previous Nightly before reading Comment 30.

I also didn't want you to think it was out of the blue given the time frame from Comment 17 it just seemed related.

Would you mind posting your findings in Bug 1504865?

I think Bug 1528743 is related to the about:config pref.

Oh, well, it seems after changing layout.scroll.root-frame-containers I can reproduce bug 1504865 with https://bug1493976.bmoattachments.org/attachment.cgi?id=9018847 so it wasn't fixed without containerless scrolling... I guess nothing new worth reporting there, then.

Would you mind clariying the states in which it is fixed?

ni? Xidorn for Comment #35.

Flags: needinfo?(xidorn+moz)

So, it seems that containerless scrolling makes this issue happen very consistently.

If I disable containerless scrolling (via setting layout.scroll.root-frame-containers to 1), this bug can reproduce but in a very low frequency.

Flags: needinfo?(xidorn+moz)
Attached patch Potential fix (obsolete) — Splinter Review

This patch fixes the problem for me locally. I'm not familiar enough with the dynamic toolbar code to tell if this is the right approach.

Assignee: nobody → botond
Comment on attachment 9051854 [details] [diff] [review]
Potential fix

Randall, WDYT?
Attachment #9051854 - Flags: feedback?(rbarker)
Assignee: botond → nobody

:botond I still see the issue intermittently with your fix. Do you know what could have changed? Is the page not getting composited after it goes full screen anymore? I don't see this issue in beta or release so it seems to be a recent regression.

Flags: needinfo?(botond)

The page gets composited after it goes full screen, but at that time the compositor toolbar height, as seen in LayerManagerComposite::RenderToolbar() is still nonzero.

The toolbar height gets set to zero subsequently, and no place in the toolbar animator code is requesting another composite.

I think this is a latent bug that's been around for quite some time (likely since the latest dynamic toolbar rewrite), and it's just been masked by the fact that something else happened to be requesting a composite later. (Most of the time, anyways; if you read the comments further above, the issue is still reproducible at a low frequency on beta/release.)

It seems clear that the toolbar animator code should request its own composites after changing the toolbar height, and not rely on something else to coincidentally request a composite.

The only thing I'm really unsure about, is where in the toolbar animator code we should be requesting the composite - the place where I added it in the patch, or somewhere else in UpdateAnimation(), or in UpdateCompositorToolbarHeight() perhaps?

Flags: needinfo?(botond)
Comment on attachment 9051854 [details] [diff] [review]
Potential fix

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

This fix seemed to only intermittently fix the issue for me. I played around with where the composite is requested and I think I found a location that works more consistently. Thanks for looking into this.
Attachment #9051854 - Flags: feedback?(rbarker) → feedback-
Assignee: nobody → rbarker
Pushed by rbarker@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0ebb51472ce4
Ensure toolbar is hidden when going fullscreen r=botond

Does this fullscreen issue affect Fenix or just Fennec? If it affects Fenix, we might want to uplift it to GV 67 Beta.

I believe this is Fennec only.

See also Bug 1517809

Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68

(In reply to csheany from comment #46)

I believe this is Fennec only.

Thanks. If this bug is Fennec only, then we can let the fix ride with Fennec 68 instead of rushing to uplift to Fennec 67 Beta.

Uplifting might be worth it given Bug 1158392 which made this worse.

(In reply to csheany from comment #49)

Uplifting might be worth it given Bug 1158392 which made this worse.

Botond, should we uplift this fullscreen fix to Fenenc 67 Beta? csheany points out that containerless scrolling, which made this problem worse, rode the trains to Beta with Fennec 67 (bug 1137890).

Flags: needinfo?(botond)

The relationship to containerless scrolling is incidental, but given that this is a very low risk one-line patch, I support uplifting it to 67. Randall, any objection?

Flags: needinfo?(botond) → needinfo?(rbarker)

I don't think it can do any harm in uplifting it.

Flags: needinfo?(rbarker)
Attachment #9051854 - Attachment is obsolete: true

Comment on attachment 9052552 [details]
Bug 1500719 - Ensure toolbar is hidden when going fullscreen

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: Bug 1335895
  • User impact if declined: When user enters fullscreen mode, the URL bar can remain visible until the user taps the page.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This is a very low risk patch, as all it does is request a composite at the end of a toolbar animation.
  • String changes made/needed:
Attachment #9052552 - Flags: approval-mozilla-beta?

Comment on attachment 9052552 [details]
Bug 1500719 - Ensure toolbar is hidden when going fullscreen

Low risk patch for a visible Fennec regression, uplift approved for 67 beta 7, thanks.

Attachment #9052552 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Verified as fixed in Beta 67.0b7 build 1 on Android 6.0.1.

Also verified on the latest Nightly 68.0a1 (2019-03-31) on Galaxy S6 - Android 6.0.1.

Hi!

Due to Comment 57 and Comment 58 I'll mark this issue as Verified Fixed.

Thanks.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.