Closed Bug 719557 Opened 12 years ago Closed 12 years ago
"Full Screen" add-on (window
.full Screen) has problems in native Fennec
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.
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)
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 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-
(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.
This version separates toggling of browser chrome from entering full screen mode.
(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
(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+
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 12
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?
Status: ASSIGNED → RESOLVED
Closed: 12 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+
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
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.