876562 - In Fennec content behind fullscreen content should not be pannable, zoomable, or visible

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

Fullscreen in webcontent in Fennec Nightly is broken.

STR:
1. Open http://pearce.org.nz/f in Fennec.
2. Touch the "toggle fullscreen" button. This requests fullscreen on the parent element of the blue div and sets the parent's element's background colour to grey.
3. Observe that the white div with the blue border and the Jaberwockey text can be seen behind the fullscreen element (the blue div) and can be panned.
4. Also observe that double tapping on the page can cause it to resize, and changes the amount of the white div underneath that's visible.

Expected results:
The entire screen should have a grey background, and the white/jaberwockey div underneath should not be visible or pannable. I'm not sure what double tapping should do, but it shouldn't make the underlying non-fullscreen content visible.

A note about fullscreen:
Fullscreen works by applying the :-moz-full-screen style [1] to the element that requested fullscreen, and to the element that contains the fullscreen element in parent documents (typically <iframe> or <browser> elements). All ancestors of fullscreen elements inside documents have the :-moz-full-screen-ancestor style [2] applied. These stlyes set position:fixed; and sets a position, so presumably the pan/zoom controller is getting confused due to these.

Pretty much the same bug exists on B2G (I filed bug 876542).

[1] http://mxr.mozilla.org/mozilla-central/source/layout/style/ua.css#234
[2] http://mxr.mozilla.org/mozilla-central/source/layout/style/full-screen-override.css#6

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Photo of fullscreen broken in fennec

Photo showing fullscreen element not properly covering entire screen, leaving white content underneath visible. On my Nexus 7.

Comment 2

[Chris Lord [:cwiiis]]

full-screen mode is more front-end than gfx/panzoom, cc'ing :mfinkle.

Comment 3

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

Margaret - Can you start the investigation on this?

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

I'm not convinced this is a front end bug, the same bug exists on B2G when content inside the browser app requests fullscreen, and seems to be related to having a fullscreen request inside content with particular positioning.

Comment 5

[:Margaret Leibovic]

I agree with Chris that this is probably not a frontend bug if it's present on B2G. It sounds like some sort of layout bug to me.

Playing around with this, I also noticed that it looks like there's a dynamic toolbar bug here. If the toolbar is visible when entering fullscreen mode, a light blue bar is visible at the top of the screen, but this doesn't happen if you enter fullscreen mode while the toolbar is scrolled off the page.

In both cases, the underlying content is visible to the right of the fullscreen content.

Comment 6

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #5)
 
> Playing around with this, I also noticed that it looks like there's a
> dynamic toolbar bug here. If the toolbar is visible when entering fullscreen
> mode, a light blue bar is visible at the top of the screen, but this doesn't
> happen if you enter fullscreen mode while the toolbar is scrolled off the
> page.

I can file this as a separate bug, but I wonder if we should update the way we hide the browser toolbar for fullscreen mode to hide it the same way we do for the dynamic toolbar. For reference, this is how we do it now:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#1455

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

I've tested http://pearce.org.nz/f on Fennec Release, Nightly, Aurora, and Beta builds, and this appears to be a regression that starts in Beta builds. 

I'm specifically double tapping on the blue div to the right of the "plugin container" box/line. When I do this on Release builds, the background content flashes visisble, but once the zoom is complete the background content is no longer visible, as expected.

So this is a regression from something in Fennec Beta.

Comment 8

[:Margaret Leibovic]

I was just pointed at bug 853986, which looks like it's conflated the layout issue with the toolbar issue.

There's a lot of discussion and work already started in that bug, so we may want to mark this as a dupe, or narrow the scope of that bug to just be about the dynamic toolbar issue.

Comment 9

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

this is the underlying platform bug for bug 876562, which is tracking-firefox22+

Comment 10

[(not currently active) Ted Mielczarek]

mfinkle had pointed me at bug 875226 (which is possibly related or a dupe), so I put my testcase there: attachment 756531 [details] is an extremely simple fullscreen testcase: tap on the black div to fullscreen it. Attachment 756534 [details] shows the result on my Nexus 4, which is pretty broken. I don't have a B2G device to test on.

Comment 11

[:Margaret Leibovic]

Even though this bug presents itself when the dynamic toolbar is disabled, the regression range in bug 853986 indicates that this is some fallout from the set of dynamic toolbar patches that landed on March 6 (unfortunately tinderbox builds aren't still around, but I double checked this with Nightly builds).

Given davehunt's latest comment in bug 876542, I'm starting to think maybe this isn't the same issue as whatever was showing up on b2g.

I'm cc'ing kats, since it looks like he did a bunch of the reviews from bug 716403, which seems to be the suspect for causing this regression.

I think we need to look at what parts of the changes from that bug take effect regardless of whether or not the dynamic toolbar pref is enabled.

Comment 12

[:Margaret Leibovic]

Actually, looking through those changesets, there are a bunch of changes in core code that might affect both android and b2g. These are suspicious:

http://hg.mozilla.org/integration/mozilla-inbound/rev/f7f0e7e4af43
http://hg.mozilla.org/integration/mozilla-inbound/rev/f065ac54e83e
http://hg.mozilla.org/integration/mozilla-inbound/rev/5d0c3b244e89

Also cc'ing roc, since he did some of these reviews as well.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm guessing this is Chris'.

Comment 14

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> I'm guessing this is Chris'.

Normally, yes, but he is PTO for another week and we'd like to start the investigation since this is affecting Beta (Fx22).

Is there anyone (:tn maybe) that could help us start looking for the cause? Margaret is willing to do some legwork here, just needs some pointers.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Good idea, maybe tn can help.

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

Here's a reduced testcase that mimics the CSS styles applied to fullscreen and fullscreen-ancestor elements, but doesn't actually use the fullscreen API:

http://bit.ly/19ecjil

The behaviour on desktop is correct, behaviour on mobile should match desktop's behaviour, but doesn't.

Comment 17

[Timothy Nikkel (:tnikkel)]

Attachment: further reduced testcase

Thanks Chris, that was very helpful.

Here is a further reduced testcase.

It looks like we are calculating the size of the fixed position content/layer incorrectly.

Comment 18

[Timothy Nikkel (:tnikkel)]

It looks like layout is sizing the frame correctly, but somewhere in the gfx/layers/compositor code the scale is getting applied twice (ie 0.5 * 0.5 = 0.25) so it is shown too small.

Comment 19

[Timothy Nikkel (:tnikkel)]

Attachment: patch

The problem appears to be caused by part 4 from bug 716403 (http://hg.mozilla.org/mozilla-central/rev/14feb7d2c538), specifically the changes to setScrollClampingSize.

I don't think
  let scrollPortWidth = Math.min(screenWidth * factor, pageWidth * zoom);
makes sense, as screenWidth * factor is in CSS pixels and pageWidth * zoom is in device pixels. So I presume this wants to compute Math.min(screenWidth * factor, pageWidth), but that is equivalent to screenWidth * factor because factor is smaller than pageWidth / screenWidth from the previous line.

Comment 20

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

Comment on attachment 762302 [details] [diff] [review]
patch

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

Makes perfect sense. Nice catch!

Comment 21

[Timothy Nikkel (:tnikkel)]

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

Comment 22

[Alex Keybl [:akeybl]]

Feels risky to take this fix in the final beta. Please set status-firefox22 back to affected if you'd like us to reconsider this decision.

Comment 23

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

FWIW I would support uplifting this considering how fullscreen is horribly broken without it. :tn, what do you think?

Comment 24

[Timothy Nikkel (:tnikkel)]

I was going to ask for kats thoughts, but I agree. The patch replaces clearly wrong code, so presumably any problems it causes would be less bad then the problems it fixes.

Comment 25

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

Setting status back as per comment 22, then.

Comment 26

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

I also support the uplift

Comment 27

[Aaron Train [:aaronmt]]

Recommend this for uplift too

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The original bug was seen in B2G, so given the fix is only Android, I'm confused.

Comment 29

[Timothy Nikkel (:tnikkel)]

There must be two different bugs. Bug 876542 covers the b2g case.

This bug was caused by bug 716403 which landed in 22. Bug 876542, comment 3 indicates that it is present in b2g v1.0.1, which is based on 18, so it had to be caused by something else.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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

Comment 31

[Timothy Nikkel (:tnikkel)]

Comment on attachment 762302 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 716403
User impact if declined: fullscreen doesn't show fullscreen and some fixed position items are wrongly positioned/sized
Testing completed (on m-c, etc.): been on m-c for a weekend
Risk to taking this patch (and alternatives if risky): this replaces two clearly wrong lines of code with what was there before, any regressions it causes should hopefully be less problematic than the bug that it is fixing
String or IDL/UUID changes made by this patch: none

Comment 32

[Timothy Nikkel (:tnikkel)]

Thanks Alex.

https://hg.mozilla.org/releases/mozilla-aurora/rev/15c47873f860
https://hg.mozilla.org/releases/mozilla-beta/rev/5499cb854684

Comment 33

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 23.0b10(2013-07-29)
Device: Asus Transformer TF
OS: Android 4.0.3