853986 - Empty space at the top of fullscreen HTML5 video with dynamic toolbar enabled

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Aaron Train [:aaronmt]]

See screenshot

STR:

i) http://people.mozilla.org/~atrain/mobile/tests/media.html; play any video, long-tap and full-screen it

--
Samsung Galaxy Note (Android 4.1.2)
Nightly (03/22)

Comment 1

[Aaron Train [:aaronmt]]

Attachment: Nightly (03/22) - Screenshot

Comment 2

[Chris Peterson [:cpeterson]]

Aaron, is this a recent regression? Was the Galaxy Note one of our blacklisted devices?

Comment 3

[Aaron Train [:aaronmt]]

This is also reproducible on an assortment of devices, such as the LGE Nexus 4, and Samsung Galaxy Nexus and this Alcatel One Touch EVO 7" Tablet; this seems like a recent regression and not specific to a device.

Comment 5

[Aaron Train [:aaronmt]]

Cristian, can you help find a regression range?

Comment 6

[Teodora Vermesan (:TeoVermesan)]

The regression window for this issue is:
good build: 07.03.2013  
bad build:  08.03.2013 

pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ee4879719f78&tochange=cb432984d5ce

Comment 7

[Teodora Vermesan (:TeoVermesan)]

The tinderbox inbound builds are:
Good build: 1362588481
Bad build: 1362589064
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=42845185ca07&tochange=5948f1c95fbe

Comment 9

[Chris Lord [:cwiiis]]

From bug 855240:

"A further note, been trying to debug this - the bug occurs regardless of orientation, and when the dynamic toolbar is disabled too. It seems to oscillate between the window size and some larger size, for which I've not yet figured out the relation."

Bug 859946 likely has the same root cause.

Comment 10

[Chris Lord [:cwiiis]]

I have a fix (at least for bug 855240) that isn't too horrible :) Will verify full-screen video playback and fix that too, if it doesn't fix that (which it may not).

Comment 11

[Chris Lord [:cwiiis]]

On second thoughts, it isn't going to help me to conflate these two separate issues, will reopen 855240.

Comment 13

[Chris Lord [:cwiiis]]

Could someone from front-end take this?

I think all it needs is to call hideMargins() when full-screening a video (and maybe call showMargins() if they were visible pre-fullscreen?) and it'd be a nice introduction to the code for someone.

Comment 14

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

Sending to Wes

Comment 15

[Erin Lancaster [:elan]]

P1 since it is a top issue per QA.

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

we're disabling dynamic toolbar for 22, bumping to 23

Comment 18

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2

I can't really reproduce this (partly because none of the videos on Aaron's test page work for me?) I think this does what you describe chris? This doesn't maintain state either (i.e. the urlbar will show whenever you come out of fullscreen). I think that might actually be nice?

Comment 19

[Aaron Train [:aaronmt]]

(In reply to Wesley Johnston (:wesj) from comment #18)
> Created attachment 746088 [details] [diff] [review]
> Patch 1/2
> 
> I can't really reproduce this (partly because none of the videos on Aaron's
> test page work for me?)

More block-listings recently landed; maybe you're hitting one on your device. SII? How about http://cd.pn/b/

Comment 20

[Chris Lord [:cwiiis]]

Comment on attachment 746088 [details] [diff] [review]
Patch 1/2

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

This does what I suggested, but there are problems;

1- There's no checking or flag or anything to stop the margins being shown elsewhere
2- We don't tell Gecko that the viewport margins have disappeared, so the page may not be sized full-screen

I think we should fix both of these things. Also, sorry this review took so long, I missed the mail somehow :/

::: mobile/android/base/BrowserApp.java
@@ +1290,3 @@
>                      mBrowserToolbar.hide();
> +                    if (isDynamicToolbarEnabled()) {
> +                        mLayerView.getLayerMarginsAnimator().hideMargins(false);

If we're hiding the toolbar, we should probably just immediately hide the margins here.

Comment 21

[Aaron Train [:aaronmt]]

So, 22 is indeed affected here on mozilla-beta; despite having the dynamic-toolbar disabled.

Here is a screenshot from http://cd.pn/b full-screen on my Nexus 4 using Firefox Beta on Google Play.

http://cl.ly/image/2j3M2h0r3121

Comment 22

[Aaron Train [:aaronmt]]

Status of attached patch?

Comment 23

[Chris Lord [:cwiiis]]

(In reply to Aaron Train [:aaronmt] from comment #21)
> So, 22 is indeed affected here on mozilla-beta; despite having the
> dynamic-toolbar disabled.
> 
> Here is a screenshot from http://cd.pn/b full-screen on my Nexus 4 using
> Firefox Beta on Google Play.
> 
> http://cl.ly/image/2j3M2h0r3121

This screen makes it look like it isn't related to the dynamic toolbar...

Comment 24

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

Wes - Can you repro using STR in comment 21 ?

Comment 25

[Wesley Johnston (:wesj)]

I can! From some logging it looks like the video width == window.outerWidth. I'll have to keep digging to see what's wrong.

Comment 26

[:Margaret Leibovic]

(In reply to Chris Lord [:cwiiis] from comment #23)
> (In reply to Aaron Train [:aaronmt] from comment #21)
> > So, 22 is indeed affected here on mozilla-beta; despite having the
> > dynamic-toolbar disabled.
> > 
> > Here is a screenshot from http://cd.pn/b full-screen on my Nexus 4 using
> > Firefox Beta on Google Play.
> > 
> > http://cl.ly/image/2j3M2h0r3121
> 
> This screen makes it look like it isn't related to the dynamic toolbar...

In bug 876562, cpearce mentions that this issue is also present on B2G, so I think that's a separate issue from the blue bar that's appearing where the toolbar would be.

Comment 27

[:Margaret Leibovic]

Let's make this bug about the dynamic toolbar issue, bug 876562 can be about the layout issue.

Given that this is a dynamic toolbar issue, this should probably track 23, not 22.

Comment 28

[:Margaret Leibovic]

Attachment: screenshot w/ patch applied

Wes, when I applied your patch, I seemed to have the inverse problem, and a blue bar showed up at the bottom of the screen :/

I was testing with ted's simple fullscreen testcase:
http://people.mozilla.com/~tmielczarek/fullscreen.html

I kinda wonder if we'll need a fix for the content issue (bug 876562) before we can come up with a good fix for this blank toolbar space issue.

Comment 30

[:Margaret Leibovic]

Alex, I think you might have wanted to relnote bug 876562, not this one (this is only an issue with the dynamic toolbar enabled, which it's not for 22).

Comment 31

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

Comment on attachment 746088 [details] [diff] [review]
Patch 1/2

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

After tn's patch in bug 876562 is applied, this patch (or some cleaner variant) should do the trick to fix the rest of this bug.

Comment 32

[Alex Keybl [:akeybl]]

(In reply to :Margaret Leibovic from comment #30)
> Alex, I think you might have wanted to relnote bug 876562, not this one
> (this is only an issue with the dynamic toolbar enabled, which it's not for
> 22).

Thanks Margaret.

Comment 34

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #31)
> Comment on attachment 746088 [details] [diff] [review]
> Patch 1/2
> 
> Review of attachment 746088 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> After tn's patch in bug 876562 is applied, this patch (or some cleaner
> variant) should do the trick to fix the rest of this bug.

That patch is on 23 now, can we get a nom on this for uplift with a risk eval?

Comment 35

[(not currently active) Ted Mielczarek]

The bug 876562 fix made this less horrible, but it's still broken. My simple testcase:
http://people.mozilla.com/~tmielczarek/fullscreen.html

It should produce an all-black screen when you tap on the black box, but now it leaves an empty space where the toolbar was.

Comment 36

[(not currently active) Ted Mielczarek]

Attachment: current behavior of simple testcase on Nexus 4

Here's what screenshot.html looks like on my Nexus 4 after tapping the black box on a recent Nightly (2013-07-07).

Comment 37

[Wesley Johnston (:wesj)]

Attachment: Patch

Updated. And now I'm really really really late. This uses setMaxMargins as it seemed like the easiest way to accomplish both things you mentioned. i.e. it locks the toolbar offscreen and notifies gecko that the page size may need to be updated.

Comment 38

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

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

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

::: mobile/android/base/BrowserApp.java
@@ +1591,5 @@
>              @Override
>              public void run() {
> +                if (fullscreen) {
> +                    if (isDynamicToolbarEnabled()) {
> +                        mBrowserToolbar.hide();

Probably want to do mBrowserToolbar.hide() and show() even if dynamic toolbar is not enabled, right?

Comment 39

[Wesley Johnston (:wesj)]

Attachment: Patch

Grr. Yeah. I was playing with moving things around to get something prettier and apparently moved to much.

Comment 40

[Chris Lord [:cwiiis]]

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

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

LGTM, I think.

Comment 41

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a67961a377b8

Comment 42

[Wesley Johnston (:wesj)]

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

Noming this a bit early, because we need this on beta to ship dynamic toolbar.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): dynamic toolbar
User impact if declined: fullscreen isn't quite full screen at first
Testing completed (on m-c, etc.): landed on mc today
Risk to taking this patch (and alternatives if risky): This is a fairly simple change for an edge case we just missed at first. I don't think there are many alternatives beyond disabling the dynamic toolbar.
String or IDL/UUID changes made by this patch: none

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a67961a377b8

Comment 44

[(not currently active) Ted Mielczarek]

My simple fullscreen testcase does the right thing as of today's nightly. Thanks for fixing this!

Comment 45

[bhavana bajaj [:bajaj]]

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

Looks like it fixed the issue Ted was reporting.Lets take this on branches..The patch is simple and we do not want to ship dynamic toolbar with this bug.

Comment 46

[Wesley Johnston (:wesj)]

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

Comment 47

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/releases/mozilla-beta/rev/0e6f4fc762d3

Comment 48

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 24.0a2( 2013-07-16)
Device: Samsung Galaxy Nexus
OS: Android 4.1.1

Comment 49

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 23.0b6
Device: LG Nexus 4
OS: Android 4.2.2.

Comment 50

[Teodora Vermesan (:TeoVermesan)]

Verified fixed, so removing the keyword: "verifyme"