"Full Screen" add-on (window.fullScreen) has problems in native Fennec

VERIFIED FIXED in Firefox 11

Status

()

Firefox for Android
General
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: mbrubeck, Assigned: mbrubeck)

Tracking

Trunk
Firefox 12
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 fixed, firefox12 fixed, firefox13 verified, fennec11+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
My "full screen" extension adds a menu option that toggles the window.fullScreen property of Fennec's main chrome window:

http://hg.mozilla.org/users/mbrubeck_mozilla.com/fullscreen/
http://limpet.net/mbrubeck/temp/fullscreen.xpi

This has some problems in native Fennec:
* Fennec displays an alert "Press BACK to exit full screen" even though this doesn't work.
* The back button is broken while full screen mode is active.

This happens because the Java front-end sends FullScreen:Exit whenever it is in full screen mode.  This causes browser.js to call browser.contentDocument.mozCancelFullScreen, but this does not work if full screen was not set by mozRequestFullscreen.

When full screen is set directly from chrome, we should *not* override the back button.
(Assignee)

Comment 1

6 years ago
Created attachment 590012 [details] [diff] [review]
patch

This fixes the problems listed above.  We now activate the special back-button logic only for DOM Full Screen mode, not for other causes of full screen.  But we still want to toggle the chrome toolbars whenever full screen mode changes.
Attachment #590012 - Flags: review?(margaret.leibovic)

Comment 2

6 years ago
Have you looked at bug 698836 comment 5? I remember when writing that patch I was confused about the distinction between times when we wanted to hide the system bar as opposed to the browser chrome. I assumed they were the same, but Brad wanted me to treat them separately. I'm still not completely clear on why we did that, so maybe Brad can chime in here.

Comment 3

6 years ago
Comment on attachment 590012 [details] [diff] [review]
patch

I'm passing this off to Brad because I don't feel like I understand this well enough to do a good review.
Attachment #590012 - Flags: review?(margaret.leibovic) → review?(blassey.bugs)
Comment on attachment 590012 [details] [diff] [review]
patch

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

I've been ruminating on this for a while because it is a change to how we've done things. Full screen means two different things (hiding OS chrome vs. hiding browser chrome) depending on what part of the code you're in. Since this java code has to respond to calls from both I feel as though we need to handle them separately.

So, r- because after a couple days of thinking about it I think we should continue to handle them as separate things and it shouldn't overly complicate this patch
Attachment #590012 - Flags: review?(blassey.bugs) → review-
(Assignee)

Comment 5

6 years ago
(In reply to Brad Lassey [:blassey] from comment #4)
> I've been ruminating on this for a while because it is a change to how we've
> done things. Full screen means two different things (hiding OS chrome vs.
> hiding browser chrome) depending on what part of the code you're in. Since
> this java code has to respond to calls from both I feel as though we need to
> handle them separately.

But it's the current code that treats them the same -- currently, window.fullScreen and mozRequestFullscreen both get caught by the same "fullscreen" event handler, and send the same message to the Java layer.

With my patch, the message is sent only for mozRequestFullscreen, so that the Java code can do different things for DOM fullscreen and chrome fullscreen.

In desktop Firefox, window.fullScreen and mozRequestFullscreen *both* hide both the OS chrome and the browser chrome, so I think it's correct to do that in both cases in fennec.  Where they should differ is in handling the "escape" button, which is what my patch fixes.
that's sorta my point, in the chrome layers we do both. At the widget layer, we only hide OS chrome (http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/nsBaseWidget.h#127). The java code is called by both layers so needs to handle both conditions separately.
(Assignee)

Comment 7

6 years ago
Created attachment 591302 [details] [diff] [review]
patch v2

This version separates toggling of browser chrome from entering full screen mode.
Attachment #590012 - Attachment is obsolete: true
Attachment #591302 - Flags: review?(blassey.bugs)
(In reply to Matt Brubeck (:mbrubeck) from comment #7)
> Created attachment 591302 [details] [diff] [review]
> patch v2
> 
> This version separates toggling of browser chrome from entering full screen
> mode.

wrong patch? interdiff (yea, I still use interdiff) shows no changes from the previous patch
(Assignee)

Comment 9

6 years ago
(In reply to Brad Lassey [:blassey] from comment #8)
> wrong patch? interdiff (yea, I still use interdiff) shows no changes from
> the previous patch

Bugzilla interdiff seems to be broken.  This is definitely a new patch.
Attachment #591302 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 10

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9effde68bac5
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 12
(Assignee)

Comment 11

6 years ago
Comment on attachment 591302 [details] [diff] [review]
patch v2

[Approval Request Comment]
User impact if declined: "Full Screen" add-on will not work correctly.

Testing completed (on m-c, etc.): Pushed to inbound on 1/26.

Risk to taking this patch (and alternatives if risky):
This is a low-risk mobile-only patch; it touches code that is used only by the experimental DOM full-screen API, which is not widely used on the web.

We don't really need this patch in order to ship Fennec; the bug is only exposed by my add-on.  The main reason I'm requesting approval is in case it helps us avoid merge conflicts in other patches.
Attachment #591302 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/9effde68bac5
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Comment on attachment 591302 [details] [diff] [review]
patch v2

[Triage Comment]
Mobile only - approved for Aurora.
Attachment #591302 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
tracking-fennec: --- → 11+
https://hg.mozilla.org/releases/mozilla-aurora/rev/ad6066f0d40e
status-firefox11: --- → fixed
status-firefox12: --- → fixed
(Assignee)

Updated

5 years ago
Depends on: 726873
Verified fixed on:

Firefox 13.0a1 (2012-03-06)
20120306031101
http://hg.mozilla.org/mozilla-central/rev/7d0d1108a14e

--
Device: HTC Desire
OS: Android 2.2
Status: RESOLVED → VERIFIED
status-firefox13: --- → verified
You need to log in before you can comment on or make changes to this bug.