Closed Bug 876562 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox for Android :: Toolbar, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 24
Tracking Status
firefox22 + verified
firefox23 + verified
firefox24 --- verified
fennec 22+ ---

People

(Reporter: cpearce, Assigned: tnikkel)

References

()

Details

(Keywords: regression)

Attachments

(3 files)

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
Photo showing fullscreen element not properly covering entire screen, leaving white content underneath visible. On my Nexus 7.
full-screen mode is more front-end than gfx/panzoom, cc'ing :mfinkle.
Margaret - Can you start the investigation on this?
Assignee: nobody → margaret.leibovic
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.
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.
(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
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.
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.
this is the underlying platform bug for bug 876562, which is tracking-firefox22+
tracking-fennec: --- → 22+
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.
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.
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.
Keywords: regression
I'm guessing this is Chris'.
Assignee: margaret.leibovic → chrislord.net
(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.
Good idea, maybe tn can help.
Assignee: chrislord.net → tnikkel
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.
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.
Status: NEW → ASSIGNED
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.
Attached patch patchSplinter Review
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.
Attachment #762302 - Flags: review?(bugmail.mozilla)
Comment on attachment 762302 [details] [diff] [review]
patch

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

Makes perfect sense. Nice catch!
Attachment #762302 - Flags: review?(bugmail.mozilla) → review+
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.
FWIW I would support uplifting this considering how fullscreen is horribly broken without it. :tn, what do you think?
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.
I also support the uplift
Recommend this for uplift too
The original bug was seen in B2G, so given the fix is only Android, I'm confused.
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.
https://hg.mozilla.org/mozilla-central/rev/e2374b14da71
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Status: RESOLVED → VERIFIED
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
Attachment #762302 - Flags: approval-mozilla-beta?
Attachment #762302 - Flags: approval-mozilla-aurora?
Attachment #762302 - Flags: approval-mozilla-beta?
Attachment #762302 - Flags: approval-mozilla-beta+
Attachment #762302 - Flags: approval-mozilla-aurora?
Attachment #762302 - Flags: approval-mozilla-aurora+
Verified fixed on:
Build: Firefox for Android 23.0b10(2013-07-29)
Device: Asus Transformer TF
OS: Android 4.0.3
You need to log in before you can comment on or make changes to this bug.