Last Comment Bug 719557 - "Full Screen" add-on (window.fullScreen) has problems in native Fennec
: "Full Screen" add-on (window.fullScreen) has problems in native Fennec
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All Android
: -- normal (vote)
: Firefox 12
Assigned To: Matt Brubeck (:mbrubeck)
:
:
Mentors:
Depends on: 726873
Blocks: 698836
  Show dependency treegraph
 
Reported: 2012-01-19 12:56 PST by Matt Brubeck (:mbrubeck)
Modified: 2012-03-06 07:49 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
verified
11+


Attachments
patch (5.28 KB, patch)
2012-01-19 15:16 PST, Matt Brubeck (:mbrubeck)
blassey.bugs: review-
Details | Diff | Splinter Review
patch v2 (5.80 KB, patch)
2012-01-24 15:49 PST, Matt Brubeck (:mbrubeck)
blassey.bugs: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Matt Brubeck (:mbrubeck) 2012-01-19 12:56:56 PST
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.
Comment 1 Matt Brubeck (:mbrubeck) 2012-01-19 15:16:16 PST
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.
Comment 2 :Margaret Leibovic 2012-01-19 17:38:02 PST
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 :Margaret Leibovic 2012-01-20 11:37:36 PST
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.
Comment 4 Brad Lassey [:blassey] (use needinfo?) 2012-01-24 14:19:08 PST
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
Comment 5 Matt Brubeck (:mbrubeck) 2012-01-24 14:34:09 PST
(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.
Comment 6 Brad Lassey [:blassey] (use needinfo?) 2012-01-24 14:42:11 PST
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.
Comment 7 Matt Brubeck (:mbrubeck) 2012-01-24 15:49:42 PST
Created attachment 591302 [details] [diff] [review]
patch v2

This version separates toggling of browser chrome from entering full screen mode.
Comment 8 Brad Lassey [:blassey] (use needinfo?) 2012-01-24 23:59:41 PST
(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
Comment 9 Matt Brubeck (:mbrubeck) 2012-01-25 09:48:31 PST
(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.
Comment 11 Matt Brubeck (:mbrubeck) 2012-01-26 09:22:10 PST
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.
Comment 12 Marco Bonardo [::mak] 2012-01-26 16:25:37 PST
https://hg.mozilla.org/mozilla-central/rev/9effde68bac5
Comment 13 Alex Keybl [:akeybl] 2012-01-27 16:19:25 PST
Comment on attachment 591302 [details] [diff] [review]
patch v2

[Triage Comment]
Mobile only - approved for Aurora.
Comment 14 Brad Lassey [:blassey] (use needinfo?) 2012-01-30 11:48:08 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/ad6066f0d40e
Comment 15 Cristian Nicolae (:xti) 2012-03-06 07:49:45 PST
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

Note You need to log in before you can comment on or make changes to this bug.