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

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

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)]

Attachment: 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]

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]

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?)]

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)]

(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?)]

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)]

Attachment: patch v2

This version separates toggling of browser chrome from entering full screen mode.

Comment 8

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

(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)]

(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 10

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9effde68bac5

Comment 11

[Matt Brubeck (:mbrubeck)]

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]]

https://hg.mozilla.org/mozilla-central/rev/9effde68bac5

Comment 13

[Alex Keybl [:akeybl]]

Comment on attachment 591302 [details] [diff] [review]
patch v2

[Triage Comment]
Mobile only - approved for Aurora.

Comment 14

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

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

Comment 15

[Cristian Nicolae (:xti)]

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