We need a better way to allow key input in DOM full-screen mode. We currently display a warning "Press ESC to exit full-screen mode" when non-whitelisted key input occurs (see bug 696918). We need to determine a way to allow enough key input to be useful for HTML5 games, while still minimizing the security risk.

Talking to cpearce and Steven, I think this bug is not a great fit for a student bug until more of the unknowns about how we want to do it are sorted out. That said, when those are clear, we could revisit.

In Chrome Canary (18.0.1010.0), when calling Element.webkitRequestFullScreen(Element.ALLOW_KEYBOARD_INPUT) all key input is allowed. I'm not sure what restrictions are put on that mode to preve phishing etc. When calling webkitRequestFullScreen() key input is allowed, except key events for the following keys are blocked: a,e,i,g,f,h,d,c,b,1,2,3,4,5,6,7,8,9,0.

Weird. I'm not happy with just allowing keyboard input willy-nilly.

So the plan so far is to implement Element.mozRequestFullScreenWithKeys() such that when called the browser enters fullscreen mode the same as withoutKeys, except that the "Press ESC to exit full-screen mode" warning includes a UI feature to make the warning stop appearing on key input. Perhaps it should also stay visible for longer (say 10s instead of 2s) so that it's easier to use the UI to stop the warning. My current idea is to have a checked checkbox with a label something like "Show this warning on key press". The user can then uncheck the checkbox to have the warning not appear on key input for this page during this session. We'll also have UI somewhere so that the user can add exceptions to grant key input to pages forever. Perhaps that would be a configure button in the warning box (maybe with a spanner or cog icon, or just text "Configure") which opens up UI with similar functionality to the pop-up blocker whitelist UI. We'd need to ensure that the mouse cursor was always visible while this warning/checkbox was shown, and that the warning doesn't hide when the mouse is hovering over it. The UI/js can set the permissions using nsIPermissionManager, and on the C++ side of things we can conveniently use nsContentUtils::IsSitePermAllow() to check whether a site is allowed key input in nsPresShell::HandleEventInternal() before dispatching the event which triggers the warning UI on key input.

Screenshot of showing WIP warning that will display upon entering fullscreen with keys and also upon keypress if a focused frame's domain has not been authorized for key input in fullscreen mode.

Just a progress report on the work here... I have a patch basically working, but I'm finding that the UI I've added sometimes has problems with hiding/showing upon mouseover, I think being caused by bug 708553. So I need to fix bug 708553 before I can make more progress here.

This might be a stupid question, but how will this work with mouselock? If you enter fullscreen-mode-with-keys with mouselock, I think you won't be able to uncheck the box for showing the UI warning. Unless mouselock is disabled while the warning is shown perhaps?

That's a good question. See bug 633602 comment 79. Basically we'll need add a way to toggle off pointer lock while the user uses the UI to grant key access to the page.

Thanks for the link, and very happy to hear this has already been thought of.

I have a implemented Element.mozRequestFullScreenWithKeys(), but I haven't settled on the UI/usage pattern for entering fullscreen with keys support. Here's what I propose to implement, UI/usage wise (which is different to what I outlined in comment 4): * Element.mozRequestFullScreen() - enters fullscreen immediately, displaying a warning "$domain.com entered fullscreen\nPress ESC to exit fullscreen" for a few seconds, and obscuring the website behind for a shorter period, in order to limit the effectiveness of websites throwing up lots of look-alike warnings to confuse people. Keypresses except of the whitelisted keys (basically those needed for the video controls) cause a warning to appear "$domain.com is fullscreen\nPress ESC to exit fullscreen". This is basically the same behaviour as we have now, except I'm proposing we add the domain to the warning. * Element.mozRequestFullScreenWithKeys() - enters fullscreen immediately, but displays a warning box "$domain.com entered fullscreen with key input\nPress ESC to exit fullscreen\n[Allow][Exit]" where [Allow][Exit] are buttons. The warning box does not auto hide. When [Allow] is clicked, the domain is granted key input permanently. [Exit] just exits fullscreen. When a domain that has already been granted fullscreenWithKeys (via the [Allow] button) requests fullscreenWithKeys, the warning box "$domain.com entered fullscreen with key input\nPress ESC to exit fullscreen\n" is displayed, but it auto hides. When entering fullscreenWithKeys, we obscure the background just as in the without keys case. We'll temporarily disable pointer-lock while this UI is showing. I was considering auto hiding the [Allow][Exit] warning box and showing it again on key press, except I think that making the warning/granting UI not auto-hide is better because: 1. It means the user is consciously aware that they're going into fullscreen mode, and that key input is allowed. The warning doesn't go away, they can't forget they're in fullscreen (at least on the first entry). 2. (I think) this is a better user experience for HTML5 games than the approach where we auto-hide the approval UI and show it again on keypress. When a user enters fullscreen for a game, they are likely to click around with the mouse to start a game, and only use the keys once the game begins, whereupon they'll have to stop playing to approve key input, which is a disruption to their flow. Better to do it all upfront with a non-auto-hiding UI. Plus the warning is only non-auto-hiding the first time they try to enter fullscreen-with-keys for a given domain (provided they [Allow] it). 3. Having a non-auto-hiding approval UI matches Chrome's behaviour (though they always require approval for both fullscreen with and without keys, and in either mode key input is allowed, whereas I'm only proposing the approval UI for fullscreenWithKeys), making it more consistent for authors across browsers. Any body from the UI/Security/whatever teams got an opinion on this? Feedback welcome.

Comment on attachment 592940 [details] Screenshot of key input warning This screenshot is obsolete, and doesn't reflect the current proposal. I'll whip one up later today...

(In reply to Chris Pearce (:cpearce) from comment #10) > > Any body from the UI/Security/whatever teams got an opinion on this? > Feedback welcome. FWIW : * I like adding the domain to the full screen warning * The idea of needing the user to explicitly allow access with keys seems like a very good approach. Is there a way to revoke the decision once it's been made ? (you said permanently, wondering just how permanent that is). I'm not sure about only needing the explicit opt in the first time. I'm not sure 'with key input' will mean much to users, but this is where UX can step in :) * I agree that not auto-hiding the allow/exit box seems better - I agree with the list of reasons. Seems like this has already been flagged for UI and security review as well :)

(In reply to Ian Melven :imelven from comment #12) > (In reply to Chris Pearce (:cpearce) from comment #10) [...] > * The idea of needing the user to explicitly allow access with keys seems > like a very good approach. Is there a way to revoke the decision once it's > been made ? (you said permanently, wondering just how permanent that is). Yep, I'm going to add a section to about:permissions to enable users to revoke fullscreen with keys permission on a site-by-site basis. By "permanent", I meant once granted the permission sticks until revoked (using about:permissions). > I'm not sure about only needing the explicit opt in the first time. I'm not > sure 'with key input' will mean much to users, but this is where UX can step > in :) Yeah, I wondered if 'with key input' is the best turn of phrase. ;) I do think it's important to distinguish between the with and without keys case however; it'd be confusing to just ask users to grant fullscreen access (without mentioning key input) as they may then wonder why they don't get asked for permission when entering fullscreen without key input. Thanks for your input.

We'd also need to display the auth UI when key input occurs in a subdocument of a fullscreenWithKeys document, thus requiring each domain to be granted key access individually.

This bug is affecting me at https://github.com/grantgalitz/GameBoy-Online/issues/17#issuecomment-4895787 If we could force firefox to always box the fullscreen area with a small frame to show the user it's a boxed webpage and not something else, I'd be fine with that. That's better than making the fullscreen api useless to use when having heavily interactive keyboard-based web apps.

Screenshot of http://pearce.org.nz/full-screen/ displaying the fullscreen key access authorization UI. This UI shows upon calling Element.mozRequestWithKeys() until the user clicks either of the "Allow" or "Exit fullscreen" buttons.

Screenshot of entered fullscreen warning. This show upon entering fullscreen (when keyboard access isn't requested) or when the user enters fullscreen with keyoard access at a domain which has has previously been granted keyboard access by clicking on the "allow" button on the authorization UI. This warning auto hides. The behaviour for this fullscreen warning is the same as what we have currently (for the without key access case) except that the domain name now appears in the warning message.

To make things clearer, I have created a screencast of my proposed fullscreen key access authorization UI in action. You can view it here: http://pearce.org.nz/full-screen/fullscreen_with_keys_ui_proposal.webm This screencast shows me running my build with my fullscreenWithKeys patches on http://pearce.org.nz/full-screen .

(In reply to Chris Pearce (:cpearce) from comment #16) > Created attachment 612042 [details] > Screenshot of key access authorization UI > > Screenshot of http://pearce.org.nz/full-screen/ displaying the fullscreen > key access authorization UI. This UI shows upon calling > Element.mozRequestWithKeys() until the user clicks either of the "Allow" or > "Exit fullscreen" buttons. I find "pearce.org.nz is fullscreen" kind of weird. "pearce.org.nz is now fullscreen" would be better in that it implies a mode change, although it still feels a little bit odd to me. I also find the three different text sizes weird. Last but not least, there's no "Disallow" option in response to "Allow keyboard access?". Of course technically it doesn't make sense to have such an option; it rather seems that the question should be put differently.

Thanks for your feedback Dão. (In reply to Dão Gottwald [:dao] from comment #19) > (In reply to Chris Pearce (:cpearce) from comment #16) > > Created attachment 612042 [details] > > Screenshot of key access authorization UI > > > > Screenshot of http://pearce.org.nz/full-screen/ displaying the fullscreen > > key access authorization UI. This UI shows upon calling > > Element.mozRequestWithKeys() until the user clicks either of the "Allow" or > > "Exit fullscreen" buttons. > > I find "pearce.org.nz is fullscreen" kind of weird. "pearce.org.nz is now > fullscreen" would be better in that it implies a mode change, although it > still feels a little bit odd to me. OK, but I don't think it makes sense to display "pearce.org.nz is now fullscreen" when non-whitelisted key input occurs in fullscreen mode without key access requested, i.e. when we've not just made the transition into fullscreen. I think displaying "pearce.org.nz is fullscreen" makes sense in that case, and displaying "pearce.org.nz is now fullscreen" upon entering fullscreen is a good change to make. > I also find the three different text sizes weird. The idea is to draw the eye to the more important stuff, the domain name, and the "Allow key access?" question... > Last but not least, there's no "Disallow" option in response to "Allow > keyboard access?". Of course technically it doesn't make sense to have such > an option; it rather seems that the question should be put differently. I'm not sure how we could pose the question differently... What would you suggest? We could just remove the question text and have "Allow key access" and "Exit fullscreen" buttons, but then the buttons' labels start to get wide, which looks weird, and there's no prompt to action for the user. We could always have a "Not now" button which just hides the key-access-auth box instead of (or as well as) an "Exit fullscreen" button, and unhide the key-access-auth box every time there's key input for a not yet authorized document. Another option to consider is whether to allow keys to be granted on a session basis instead of only permanently. For example we could have "Allow", "Always Allow", and "Exit Fullscreen" options, where "Allow" only grants permission for the current session, and "Always Allow" permissions never expire.

(In reply to Chris Pearce (:cpearce) from comment #20) > I'm not sure how we could pose the question differently... What would you > suggest? We could just remove the question text and have "Allow key access" > and "Exit fullscreen" buttons, but then the buttons' labels start to get > wide, which looks weird, and there's no prompt to action for the user. "Allow key access" vs. "Exit fullscreen" is also weird in that it doesn't sound like a binary choice. How about: Allow fullscreen with key access? [Yes] [No]

I prefer verb button labels, especially for security choices. Changing it to "Yes"/"No" doesn't really make it more binary.

Lets schedule a review discussion on this, I know we have chatted a few times about this. But we have new team members and we would like to bring this to a resolution. Please work with me to find a slot/time that works for you.

(In reply to Jesse Ruderman from comment #22) > I prefer verb button labels, especially for security choices. Changing it > to "Yes"/"No" doesn't really make it more binary. You missed the point of what I said. It works just as well this way: Allow fullscreen with key access? [Allow] [Disallow]

I have reworked the UI, taking Dão's feedback on board. How does this look?

Comment on attachment 613472 [details] Video showing fullscreen authorization UI Dão: What do you think about the fullscreen key input authorization UI shown in this video?

Comment on attachment 613472 [details] Video showing fullscreen authorization UI Looks OK to me. You should probably end those sentences with a full stop.

We need this fix as soon as possible. We are building a bigger web application for internal use. Every employee has to install firefox and did work most time of the day with it. This application requires to write long texts. FullScreen mode would be necessary but didn´t work, because of this annoying warning message. If it didn´t get fixed till september, we have to go to Chrome Browser. Fullscreen mode in chrome did work for us. But this will be a major choice, we can´t switch back the next years.

Part 1: IDL changes required to add mozRequestFullScreenWithKeys() to Element. Just stubs out mozRequestFullScreenWithKeys() to return NS_OK, implemented in next patch.

* Implements nsGenericElement::mzoRequestFullScreenWithKeys(). * Wraps the fullscreen element stack into its own class so that it can store/restore withKeys status. * nsPresShell::HandleEventInternal now checks if the *root* document is fullscreenWithKeys, and if so dispatches a "MozShowFullScreenWarning" to chrome as before, except it doesn't dispatch if the site has a "fullscreenKeys" permission of "Allow", as reported by the permission manager. I add UI to chrome in the next patch to grant "Allow fullscreenKeys" permission to a domain.

Comment on attachment 613918 [details] [diff] [review] Patch 2 v1: nsDocument/nsPresShell implementation of requestFullScreenWithKeys Olli maybe you'd like to look over the changes to nsPresShell, since you reviewed my original fullscreen nsPresShell changes? Feel free to comment on other changes of course! ;)

Comment on attachment 613916 [details] [diff] [review] Patch 1 v1: Element.mozRequestFullScreenWithKeys() IDL changes Review of attachment 613916 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the two comments addressed. I'm assuming you are using a script to tell you which idl files have to be updated so I didn't check if the list is complete. ::: content/base/src/nsGenericElement.cpp @@ +6500,5 @@ > > return NS_OK; > } > + > +nsresult nsGenericElement::MozRequestFullScreenWithKeys() Please follow the coding style: ReturnValue Class::Method(aParam) { return something; } ::: content/html/content/src/nsGenericHTMLElement.h @@ +578,5 @@ > > + /** > + * Helper to implement mozRequestFullScreen[withKeys]. > + */ > + nsresult RequestFullScreen(bool aWithKeys); Maybe that could be moved to part2? There is no need to add a method that would create a link error if used.

Add "full-screen-api.withKeys.enabled" pref, disabled by default, to toggle fullScreenWithKeys. If the pref is false, Element.mozRequestFullScreenWithKeys() will cause a mozfullscreenerror event to be fired at the document, and a message logged to the webconsole. I'll enable this feature once we're past security review.

Adds UI to grant fullscreenKeys permission to a domain. Behaviour matches that shown in my video in attachment 613472 [details] (except I added the full stops as requested).

Add tests. This is just a copy of dom/tests/mochitest/chrome/test_dom_fullscreen_warning.xul except that we request with keys instead of without, and we don't expect the warning event to fire when key input occurs.

Add fullscreen with keyboard support permissions to about:permissions. Behaviour matches that shown in attachment 613472 [details].

Try builds are available to play with Element.mozRequestFullScreenWithKeys() here: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/cpearce@mozilla.com-9fcba7a221f2/ Test page: http://pearce.org.nz/full-screen/ Make sure you set the full-screen-api.withKeys.enabled pref to true, else they won't work! Tests are greenish on Try: https://tbpl.mozilla.org/?tree=Try&rev=9fcba7a221f2

(In reply to Chris Pearce (:cpearce) from comment #36) > Created attachment 613925 [details] [diff] [review] > Patch 6 v1: Add fullscreen with key support to about:permissions > > Add fullscreen with keyboard support permissions to about:permissions. > Behaviour matches that shown in attachment 613472 [details]. about:permissions isn't fully supported yet. We will need these changes to also be made to the Page Info > Permissions pane.

With Mounir's two changes made. I used http://people.mozilla.com/~sfink/uploads/update-uuids to update the uuids.

As patch v1, but made the brace style conform, and brought forward declaration of nsGenericElement::RequestFullScreen(bool aWithKeys) as Mounir suggested.

Comment on attachment 613925 [details] [diff] [review] Patch 6 v1: Add fullscreen with key support to about:permissions Permissions are handled in Page Info > Permissions. about:permissions as a work in progress is still hidden from the user.

Bitrot fix caused by indentation change in previous patch revision.

Comment on attachment 613923 [details] [diff] [review] Patch 4 v1: Add authorization UI to browser >+ // Returns the URI of the fullscreen document. This is either the URI of the focused >+ // document, or if the focused document is the chrome document, this returns the >+ // URI of the content document. >+ getCurrentDocUri: function() { >+ let focusManager = Cc["@mozilla.org/focus-manager;1"].getService(Ci.nsIFocusManager); >+ if (focusManager.focusedWindow.document != document) { >+ return focusManager.focusedWindow.document.documentURIObject; >+ } else { >+ return gBrowser.selectedBrowser.contentWindow.document.documentURIObject; >+ } >+ }, Why does this care about what's focused? We should have a better way to get the document entering full-screen mode. >+ let [displayHost, fullHost] = DownloadUtils.getURIHost(uri.spec); Adding DownloadUtils to the global scope only for this doesn't make much sense. >+ let bundleService = Cc["@mozilla.org/intl/stringbundle;1"]. >+ getService(Ci.nsIStringBundleService); >+ let pbBundle = bundleService.createBundle("chrome://browser/locale/browser.properties"); Services.strings >+fullscreen.is=%S is fullscreen >+fullscreen.entered=%S is now fullscreen Needs more meaningful entity names and/or l10n comments. >+#full-screen-warning-message button { >+ font-size: 12pt; >+} Use child selectors or give these buttons a common class. >+#full-screen-keys-are-enabled-text, #full-screen-keys-request-text, #full-screen-escape-hint { Break the line after each comma. >+ font-size: 14pt; >+} >+ >+#full-screen-domain-text { >+ font-size: 32px; >+} Why are you switching back and forth between px and pt?

(In reply to Dão Gottwald [:dao] from comment #43) > Comment on attachment 613923 [details] [diff] [review] > Patch 4 v1: Add authorization UI to browser Thanks for your fast feedback. > >+ // Returns the URI of the fullscreen document. This is either the URI of the focused > >+ // document, or if the focused document is the chrome document, this returns the > >+ // URI of the content document. > >+ getCurrentDocUri: function() { > >+ let focusManager = Cc["@mozilla.org/focus-manager;1"].getService(Ci.nsIFocusManager); > >+ if (focusManager.focusedWindow.document != document) { > >+ return focusManager.focusedWindow.document.documentURIObject; > >+ } else { > >+ return gBrowser.selectedBrowser.contentWindow.document.documentURIObject; > >+ } > >+ }, > > Why does this care about what's focused? Because that's where keyboard input is most likely to go to when the user types. When key input occurs on a document whose domain doesn't have fullscreenKeys-allow permission, we pop up the keys auth UI again anyway, so this hopes to reduce the number of times the user must authorize key input. > We should have a better way to get the document entering full-screen mode. Alternatives include either dispatching another event from content/C++ to only the document which requested fullscreen, or doing a depth first traversal of the document tree to find the bottom-most fullscreen document in the mozfullscreenchanged handler. > >+ let [displayHost, fullHost] = DownloadUtils.getURIHost(uri.spec); > > Adding DownloadUtils to the global scope only for this doesn't make much > sense. Ok, what do you suggest? Pull out the functionality shared in DownloadUtils.getURIHost into another file, and have DownloadUtils and browser.js both include that?

(In reply to Chris Pearce (:cpearce) from comment #44) > > >+ // Returns the URI of the fullscreen document. This is either the URI of the focused > > >+ // document, or if the focused document is the chrome document, this returns the > > >+ // URI of the content document. > > >+ getCurrentDocUri: function() { > > >+ let focusManager = Cc["@mozilla.org/focus-manager;1"].getService(Ci.nsIFocusManager); > > >+ if (focusManager.focusedWindow.document != document) { > > >+ return focusManager.focusedWindow.document.documentURIObject; > > >+ } else { > > >+ return gBrowser.selectedBrowser.contentWindow.document.documentURIObject; > > >+ } > > >+ }, > > > > Why does this care about what's focused? > > Because that's where keyboard input is most likely to go to when the user > types. What guarantees that focusManager.focusedWindow doesn't belong to a different browser window? > > >+ let [displayHost, fullHost] = DownloadUtils.getURIHost(uri.spec); > > > > Adding DownloadUtils to the global scope only for this doesn't make much > > sense. > > Ok, what do you suggest? Pull out the functionality shared in > DownloadUtils.getURIHost into another file, and have DownloadUtils and > browser.js both include that? For now you can just import DownloadUtils into a local, temporary scope where you need it.

Comment on attachment 613935 [details] [diff] [review] Patch 2 v2: nsDocument/nsPresShell implementation of requestFullScreenWithKeys >+void >+nsFullScreenStack::Clear() >+{ >+ while (!IsEmpty()) { >+ Pop(); >+ } >+} Wouldn't mStack.Clear() work? >+bool >+nsFullScreenStack::GetTopWithKeys() const >+{ >+ if (mStack.IsEmpty()) { >+ return false; >+ } >+ return mStack[mStack.Length() - 1]->mWithKeys; >+} Perhaps return !mStack.IsEmpty() && mStack[mStack.Length() - 1]->mWithKeys; >+class nsFullScreenStack { { should be in the next line >+ nsFullScreenStack() { { should be in the next line >+ ~nsFullScreenStack() { { should be in the next line >+ bool IsEmpty() { { should be in the next line >+ class Entry { { should be in the next line. >diff --git a/layout/base/nsPresShell.cpp b/layout/base/nsPresShell.cpp >--- a/layout/base/nsPresShell.cpp >+++ b/layout/base/nsPresShell.cpp >@@ -6299,18 +6299,17 @@ IsFullScreenAndRestrictedKeyEvent(nsICon > // Bail out if the event is not a key event, or the target's document is > // not in DOM full screen mode, or full-screen key input is not restricted. You should update the comment. >@@ -6383,40 +6382,54 @@ PresShell::HandleEventInternal(nsEvent* > // XXX How about IME events and input events for plugins? > if (NS_IS_TRUSTED_EVENT(aEvent)) { > switch (aEvent->message) { > case NS_KEY_PRESS: > case NS_KEY_DOWN: > case NS_KEY_UP: { > nsIDocument *doc = mCurrentEventContent ? > mCurrentEventContent->OwnerDoc() : nsnull; >- nsIDocument* root = nsnull; >+ nsIDocument* root = nsContentUtils::GetRootDocument(doc); So, um, for every key event we search the root document. Could you get root document only when needed.

Comment on attachment 613935 [details] [diff] [review] Patch 2 v2: nsDocument/nsPresShell implementation of requestFullScreenWithKeys Why do we want to use nsAutoPtr<Entry> as opposed to Entry as the element type in mStack? Using IsSitePermAllow on a documentURI is broken. It needs to be passed a principal codebase at the very least. Better yet would be to have a sane API there that takes origins, not URIs. :(

(In reply to Boris Zbarsky (:bz) from comment #47) > Comment on attachment 613935 [details] [diff] [review] > Patch 2 v2: nsDocument/nsPresShell implementation of > requestFullScreenWithKeys > > Why do we want to use nsAutoPtr<Entry> as opposed to Entry as the element > type in mStack? Because MOZ_COUNT_DTOR was reporting leaks... but that's because I didn't have a copy constructor defined! I'll do that.

(In reply to Boris Zbarsky (:bz) from comment #47) > Comment on attachment 613935 [details] [diff] [review] > [...] > Using IsSitePermAllow on a documentURI is broken. It needs to be passed a > principal codebase at the very least. Better yet would be to have a sane > API there that takes origins, not URIs. :( Do you mean something like: nsContentUtils::IsSitePermAllow(fullscreenDoc->NodePrincipal()->GetURI(),"fullscreenKeys") (modulo getter_AddRefs on the URI etc)?

And dealing sensibly with a null URI (means system principal), yes.

Comment on attachment 613925 [details] [diff] [review] Patch 6 v1: Add fullscreen with key support to about:permissions This looks good to me, but as mentioned in other comments, about:permissions isn't fully supported yet, so unfortunately you'll have to add this new permission to the Page Info UI as well :/ >diff --git a/browser/components/preferences/aboutPermissions.js b/browser/components/preferences/aboutPermissions.js >@@ -348,17 +348,26 @@ let PermissionDefaults = { >+ // For fullscreen API key input permissions, we by default show a warning on >+ // fullscreen key input, and we don't allow this default to be overridden, >+ // the warning can only be turned off on a site-by-site basis. >+ get fullscreenKeys() { >+ return this.UNKNOWN; >+ }, >+ set fullscreenKeys(aValue) { >+ }, Since you can only change the behavior on a per-site basis, perhaps we should hide/disable this item in the "All Sites" pane. Adding "fullscreenKeys" to _noGlobalAllow hides the "Allow" menuitem in the menulist, and it seems weird to have a menulist with only one item ("Always Ask") in it. I don't think it's a huge deal, but maybe we should ask a UX person about this (Boriss was the UX person responsible for about:permissions). >@@ -472,16 +486,18 @@ let AboutPermissions = { > case "nsPref:changed": > this._supportedPermissions.forEach(function(aType){ > this.updatePermission(aType); > }, this); >+ document.getElementById("fullscreenKeys-pref-item").hidden = >+ !Services.prefs.getBoolPref("full-screen-api.withKeys.enabled"); It would be better to check exactly which pref changed in here, but the existing code already fails to do that, so I don't think that it matters much.

Boriss: Could you please comment Margaret's question in comment 51, regarding behaviour of fullscreenKeys permissions in about:permissions? For context, see attachment 613472 [details] for a video showing about:permissions and script requesting fullscreen with keyboard access in action. Thanks!

(In reply to Margaret Leibovic [:margaret] from comment #51) > Since you can only change the behavior on a per-site basis, perhaps we > should hide/disable this item in the "All Sites" pane. Adding > "fullscreenKeys" to _noGlobalAllow hides the "Allow" menuitem in the > menulist, and it seems weird to have a menulist with only one item ("Always > Ask") in it. I don't think it's a huge deal, but maybe we should ask a UX > person about this (Boriss was the UX person responsible for > about:permissions). This should just follow the same paradigm that geolocation uses, since that can't be enabled for all sites. Geolocation has a "Always Ask" and a "Block" option.

(In reply to Jared Wein [:jaws] from comment #53) > This should just follow the same paradigm that geolocation uses, since that > can't be enabled for all sites. Geolocation has a "Always Ask" and a "Block" > option. But we don't have a "block" option. "Always Ask" is the block/not-allowed option. We don't block on a per-site basis.

I've been going back and forward on this for a while, and I think the best solution is to not add a new Element.mozRequestFullScreenWithKeys() method, but to instead just have Element.mozRequestFullScreen() show the non-auto-hiding UI (similar to that shown in attachment 613472 [details]) which authorizes entering fullscreen, and allow full keyboard access (except for the ESC key, which still functions as a bail out). Basically, I don't see the point in having two paths for fullscreen, one with different authorization requirements; it'll just confuse users. This behaviour matches the current draft fullscreen spec, and it's also the same behaviour as Chrome's implementation.

Comment on attachment 613924 [details] [diff] [review] Patch 5 v1: Add tests for mozRequestFullscreenWithKeys() Based on that, these tests will need some updates.

> Basically, I don't see the point in having two paths for fullscreen, one with > different authorization requirements; it'll just confuse users. I thought the reason for having separate modes was to allow full-screen video (1) with one click (2) without granting the site inordinate privileges (which has drawbacks even in the non-attack case: users have to read the hostname carefully; we have to trade off privacy vs convenience when users clear their browsing history; a MITM attacker could later take advantage of the privilege to spoof an https site) Keeping the common case simple is probably worth the potential confusion for users who encounter the rare case.

(In reply to Jesse Ruderman from comment #57) > > Basically, I don't see the point in having two paths for fullscreen, one with > > different authorization requirements; it'll just confuse users. > > I thought the reason for having separate modes was to allow full-screen video > > (1) with one click It's only two clicks the first time, thereafter we only show the warning message when entering fullscreen, and it autohides after a few seconds, so it's one extra click, and thereafter one click to enter fullscreen on the approved domain. > (2) without granting the site inordinate privileges (which has drawbacks > even in the non-attack case: > users have to read the hostname carefully; The approval UI doesn't auto-hide, so that will encourage and increase the likelihood that the user reads the domain name, thus increasing the probability the user comprehending they've entered fullscreen mode. Because there's no separate fullscreen-with-keys mode, the approval UI will always shows when entering fullscreen. This makes the use cases covered by the old fullscreen-without-keys-mode more secure, since we now show the domain name and require explicit approval the first time we enter fullscreen. For example some banking websites require entering a password using the mouse(!!), so with the new approval UI this makes such sites harder spoof. > we > have to trade off privacy vs convenience when users clear their browsing > history; a MITM attacker could later take advantage of the privilege to > spoof an https site) This attack can still work if the user isn't in fullscreen, and clearing browser history right before a MITM attack seems like a very rare case. > Keeping the common case simple is probably worth the potential confusion for > users who encounter the rare case. I'd say the common case is FaceBook's photo viewer, followed by YouTube. Adding 1 authorization click over the lifetime of using fullscreen on a domain seems like minimal overhead for enhanced awareness and security.

Here's a screencast of the approval UI I'm proposing for fullscreen (with unrestricted key access) in fullscreen mode.

Try builds of the build used to take the screencast in attachment 616431 [details] are available here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/cpearce@mozilla.com-bfca77b9bff6 The patches are visible as changesets in this try run: https://tbpl.mozilla.org/?tree=Try&rev=bfca77b9bff6 I'll wait for the security review on Monday Pacific Time before uploading and requesting review on the patches.

The outcome of the security review today was to go ahead with enabling keys in fullscreen mode, and show the approval UI upon entering fullscreen, but tone back the whitelisting. The approval UI must change to include a "remember my decision" checkbox. When this checkbox is unchecked, which it is by default, pressing "allow" only allows fullscreen this time, and deny exits. When the "remember my decision" checkbox is checked, the allow button grants permission permanently, and the deny button blocks future fullscreen requests (they automatically fail).

Sound´s really good to me. will this be in firefox 13, 14, ..?

Probably 15; 14 gets uplifted onto Aurora tomorrow.

Screencast of latest UI, after security review complete.

Add per-domain approval/permission to go fullscreen to Page Info. (see attachment 618490 [details] to see it in action).

Remove full-screen-api.key-input-restricted keys pref, since keys aren't restricted in fullscreen mode per se anymore.

Don't dispatch 'MozShowFullScreenWarning' event upon keypress in DOM fullscreen mode, since we're not warning on keypress anymore.

Chrome code needs to know which document specifically requested fullscreen, so that we know the domain to show in the fullscreen approval UI when entering fullscreen. We can't rely on "mozfullscreenchange" events, since they're dispatched in root-to-leaf order, so we dispatch "MozEnteredDomFullScreen" to only the document which requested fullscreen. This is easier than figuring out which document requested fullscreen in JS by traversing the tree or somesuch. We also dispatch "MozEnteredDomFullScreen" when we call mozCancelFullScreen, and we rollback to the previous fullscreen element if that element's domain hasn't been approved for fullscreen. i.e, if you do the following: 1. request fullscreen in parent doc, don't approve or deny domain for fullscreen. 2. request fullscreen in child doc (approval ui should update to reflect child's domain). 3. cancel fullscreen in child, fullscreen will rollback to having parent fullscreen, and approval ui should update to reflect parent's (unapproved for fullscreen) domain. If we don't dispatch "MozEnteredDomFullScreen" when we rollback fullscreen, we wouldn't update the approval UI in step 3.

Basically the same as before, but with "fullscreen" rather than "fullscreenKeys", and added block and global block options.

Add UI to approve entering fullscreen. Changes since last patch: * We now listen to "MozEnteredDomFullscreen" in order to determine the document that entered fullscreen. * Added a "Remember decision for $domain" checkbox. If this is checked, the we add a permission to permanently allow or deny fullscreen, otherwise the UI pops up on every entrance to fullscreen. * Made the <browser> dimming permanent while the approval UI is showing, but made it not as dim. We don't dim the <browser> while entering a fullscreen on a previously approved document (i.e. one which was approved with "Remember decision for $domain" checked).

The existing fullscreen chrome-mochitests were testing that we dispatched a MozShowFullScreenWarning event upon restricted keypress. Since we don't do that anymore, there's not a lot of point having this test. We're already testing in content/html/content/file_fullscreen-api-keys.html that pressing ESC and F11 exits fullscreen, so that should be sufficient I think.

Comment on attachment 618493 [details] [diff] [review] Patch 3 v1: Don't dispatch 'MozShowFullScreenWarning' event upon keypress in DOM fullscreen mode >+bool >+nsContentUtils::IsSitePermAllow(nsIURI* aURI, const char* aType) >+{ >+ return TestSitePerm(aURI, aType, nsIPermissionManager::ALLOW_ACTION); >+} >+ >+bool >+nsContentUtils::IsSitePermDeny(nsIURI* aURI, const char* aType) >+{ >+ return TestSitePerm(aURI, aType, nsIPermissionManager::DENY_ACTION); > } Because this API is horribly error prone, please change the nsIURI parameter to nsIPrincipal for both these methods. I'd like to see a new patch with that change.

Comment on attachment 618508 [details] [diff] [review] Patch 4 v1: Dispatch "MozEnteredDomFullScreen" on fullscreen enter ># HG changeset patch ># Parent 8ab666b0b4b52f78ce1919622eb95751c2f34e59 >Bug 716107 part 4 - Dispatch MozEnteredDomFullScreen to document which requests fullscreen. r=? > >diff --git a/content/base/src/nsDocument.cpp b/content/base/src/nsDocument.cpp >--- a/content/base/src/nsDocument.cpp >+++ b/content/base/src/nsDocument.cpp >@@ -8749,16 +8749,33 @@ nsDocument::RestorePreviousFullScreenSta > if (static_cast<nsDocument*>(doc)->mFullScreenStack.IsEmpty()) { > // Full-screen stack in document is empty. Go back up to the parent > // document. We'll pop the containing element off its stack, and use > // its next full-screen element as the full-screen element. > doc = doc->GetParentDocument(); > } else { > // Else we popped the top of the stack, and there's still another > // element in there, so that will become the full-screen element. >+ if (fullScreenDoc != doc) { >+ // We've popped so enough off the stack that we've rolled back to >+ // a fullscreen element in a parent document. If this document isn't >+ // authorized for fullscreen, dispatch an event to chrome so it >+ // knows to show the authorization UI. >+ nsCOMPtr<nsIURI> uri; >+ if (NS_SUCCEEDED(doc->NodePrincipal()->GetURI(getter_AddRefs(uri))) && >+ !nsContentUtils::IsSitePermAllow(uri, "fullscreen")) { >+ nsRefPtr<nsAsyncDOMEvent> e = >+ new nsAsyncDOMEvent(doc, >+ NS_LITERAL_STRING("MozEnteredDomFullScreen"), >+ true, >+ false); Shouldn't we dispatch the event only to chrome? so the last parameter should be true. Needs tests. > >+ nsRefPtr<nsAsyncDOMEvent> e = >+ new nsAsyncDOMEvent(this, >+ NS_LITERAL_STRING("MozEnteredDomFullScreen"), >+ true, >+ false); Ditto

Comment on attachment 618491 [details] [diff] [review] Patch 1 v1: Add "fullscreen" permissions to page info > <!ENTITY permImage "Load Images"> > <!ENTITY permPopup "Open Pop-up Windows"> > <!ENTITY permCookie "Set Cookies"> > <!ENTITY permInstall "Install Extensions or Themes"> > <!ENTITY permGeo "Share Location"> > <!ENTITY permPlugins "Activate Plugins"> >+<!ENTITY permFullscreen "Fullscreen"> All other permission labels describe actions using a verb. Can you make this consistent with them, i.e. write "Go fullscreen" or something like this?

SecReview Notes: https://wiki.mozilla.org/Security/AppsProject/Element.mozRequestFullscreenWithKeys

How about "Enter Fullscreen"?

Using nsIPrincipal instead of nsIURIs for nsContentUtils::IsSitePerm{Allow,Deny}.

With test and dispatching to MozEnteredDomFullscreen to chrome instead of content.

When I updated patch 4 I changed the "MozEnteredDomFullScreen" event to be "MozEnteredDomFullscreen", so I need to update this patch to reflect at.

Latest patches greenish on Try: https://tbpl.mozilla.org/?tree=Try&rev=e3e9ecd406cb Try builds are available here if anyone wants to play with them: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/cpearce@mozilla.com-e3e9ecd406cb/

Comment on attachment 618914 [details] [diff] [review] Patch 3 v2: Don't dispatch 'MozShowFullScreenWarning' event upon keypress in DOM fullscreen mode >+TestSitePerm(nsIPrincipal* aPrincipal, const char* aType, PRUint32 aPerm) >+{ >+ if (!aPrincipal || nsContentUtils::IsSystemPrincipal(aPrincipal)) { >+ return false; >+ } Shouldn't you return true if aPerm is nsIPermissionManager::ALLOW_ACTION and principal is system and false is aPerm is nsIPermissionManager::DENY_ACTION and principal is system. >+ >+ nsCOMPtr<nsIURI> uri; >+ if (NS_FAILED(aPrincipal->GetURI(getter_AddRefs(uri))) || !uri) { >+ return false; >+ } Also, here, the return value should depend on the aPerm. We should probably deny the permission if we don't know the uri So, for example nsCOMPtr<nsIURI> uri; if (NS_FAILED(aPrincipal->GetURI(getter_AddRefs(uri))) || !uri) { return aPerm != nsIPermissionManager::ALLOW_ACTION; }

Comment on attachment 618915 [details] [diff] [review] Patch 4 v2: Dispatch "MozEnteredDomFullScreen" on fullscreen enter ># HG changeset patch ># Parent 56a6a0b55e20d33e1be41f591193bc1f67323161 >Bug 716107 part 4 - Dispatch MozEnteredDomFullscreen to document which requests fullscreen. r= > >diff --git a/content/base/src/nsDocument.cpp b/content/base/src/nsDocument.cpp >--- a/content/base/src/nsDocument.cpp >+++ b/content/base/src/nsDocument.cpp >@@ -8748,16 +8748,31 @@ nsDocument::RestorePreviousFullScreenSta > if (static_cast<nsDocument*>(doc)->mFullScreenStack.IsEmpty()) { > // Full-screen stack in document is empty. Go back up to the parent > // document. We'll pop the containing element off its stack, and use > // its next full-screen element as the full-screen element. > doc = doc->GetParentDocument(); > } else { > // Else we popped the top of the stack, and there's still another > // element in there, so that will become the full-screen element. >+ if (fullScreenDoc != doc) { >+ // We've popped so enough off the stack that we've rolled back to >+ // a fullscreen element in a parent document. If this document isn't >+ // authorized for fullscreen, dispatch an event to chrome so it >+ // knows to show the authorization UI. >+ if (!nsContentUtils::IsSitePermAllow(doc->NodePrincipal(), "fullscreen")) { >+ nsRefPtr<nsAsyncDOMEvent> e = >+ new nsAsyncDOMEvent(doc, >+ NS_LITERAL_STRING("MozEnteredDomFullscreen"), >+ true, >+ true); >+ e->PostDOMEvent(); >+ } >+ >+ } > sFullScreenDoc = do_GetWeakReference(doc); > break; > } > } > > if (doc == nsnull) { > // We moved all documents out of full-screen mode, reset global full-screen > // state and move the top-level window out of full-screen mode. >@@ -9066,16 +9081,23 @@ nsDocument::RequestFullScreen(Element* a > > // Dispatch "mozfullscreenchange" events. Note this loop is in reverse > // order so that the events for the root document arrives before the leaf > // document, as required by the spec. > for (PRUint32 i = 0; i < changed.Length(); ++i) { > DispatchFullScreenChange(changed[changed.Length() - i - 1]); > } > >+ nsRefPtr<nsAsyncDOMEvent> e = >+ new nsAsyncDOMEvent(this, >+ NS_LITERAL_STRING("MozEnteredDomFullscreen"), >+ true, >+ true); >+ e->PostDOMEvent(); >+ > // Remember this is the requesting full-screen document. > sFullScreenDoc = do_GetWeakReference(static_cast<nsIDocument*>(this)); > > #ifdef DEBUG > // Note assertions must run before SetWindowFullScreen() as that does > // synchronous event dispatch which can run script which exits full-screen! > NS_ASSERTION(GetFullScreenElement() == aElement, > "Full-screen element should be the requested element!"); >diff --git a/dom/tests/mochitest/chrome/Makefile.in b/dom/tests/mochitest/chrome/Makefile.in >--- a/dom/tests/mochitest/chrome/Makefile.in >+++ b/dom/tests/mochitest/chrome/Makefile.in >@@ -40,16 +40,19 @@ topsrcdir = @top_srcdir@ > srcdir = @srcdir@ > VPATH = @srcdir@ > relativesrcdir = dom/tests/mochitest/chrome > > include $(DEPTH)/config/autoconf.mk > include $(topsrcdir)/config/rules.mk > > _TEST_FILES = \ >+ test_MozEnteredDomFullscreen_event.xul \ >+ MozEnteredDomFullscreen_chrome.xul \ >+ MozEnteredDomFullscreen_content.html \ > test_dom_fullscreen_warning.xul \ > dom_fullscreen_warning.xul \ > test_fullscreen.xul \ > fullscreen.xul \ > test_fullscreen_preventdefault.xul \ > fullscreen_preventdefault.xul \ > focus_window2.xul \ > focus_frameset.html \ >diff --git a/dom/tests/mochitest/chrome/MozEnteredDomFullscreen_chrome.xul b/dom/tests/mochitest/chrome/MozEnteredDomFullscreen_chrome.xul >new file mode 100644 >--- /dev/null >+++ b/dom/tests/mochitest/chrome/MozEnteredDomFullscreen_chrome.xul >@@ -0,0 +1,83 @@ >+<?xml version="1.0"?> >+<?xml-stylesheet href="chrome://global/skin" type="text/css"?> >+<?xml-stylesheet href="chrome://mochikit/content/tests/SimpleTest/test.css" type="text/css"?> >+<!-- >+ Test that "MozEnteredFullscreen" is dispatched to chrome on documents that enter fullscreen. >+ >+ Test Description: >+ >+ This chrome window has a browser. The browser's contentDocument (the "outer document") >+ in turn has an iframe (the "inner document"). >+ >+ We request fullscreen in the outer document, and check that MozEnteredDomFullscreen is >+ dispatched to chrome, targeted at the outer document. >+ >+ Then we request fullscreen in the inner document, and check that MozEnteredDomFullscreen >+ is dispatched to chrome, targeted at the inner document. >+ >+ Then we cancel fullscreen in the inner document, and check that MozEnteredDomFullscreen is >+ dispatched again to chrome, targeted at the outer document. This still happens, since the >+ outer document's domain was never approved for fullscreen. >+--> >+<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" onload="start();"> >+ >+<script type="application/javascript" src="chrome://mochikit/content/chrome-harness.js"></script> >+<script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script> >+<script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script> >+<script type="application/javascript"><![CDATA[ >+ >+function ok(condition, msg) { >+ window.opener.wrappedJSObject.ok(condition, msg); >+} >+ >+function is(a, b, msg) { >+ window.opener.wrappedJSObject.is(a, b, msg); >+} >+ >+var gBrowser = null; >+var gOuterDoc = null; >+var gInnerDoc = null; >+ >+function firstEntry(event) { >+ is(event.target, gOuterDoc, "First MozEnteredDomFullscreen should be targeted at outer doc"); >+ window.removeEventListener("MozEnteredDomFullscreen", firstEntry, false); >+ ok(gOuterDoc.mozFullScreenElement != null, "Outer doc should be in fullscreen"); >+ gInnerDoc = gOuterDoc.getElementById("innerFrame").contentDocument; >+ window.addEventListener("MozEnteredDomFullscreen", secondEntry, false); >+ gInnerDoc.defaultView.focus(); >+ gInnerDoc.body.mozRequestFullScreen(); >+} >+ >+function secondEntry(event) { >+ is(event.target, gInnerDoc, "Second MozEnteredDomFullscreen should be targeted at inner doc"); >+ ok(gInnerDoc.mozFullScreenElement != null, "Inner doc should be in fullscreen"); >+ window.removeEventListener("MozEnteredDomFullscreen", secondEntry, false); >+ window.addEventListener("MozEnteredDomFullscreen", thirdEntry, false); >+ gInnerDoc.mozCancelFullScreen(); >+} >+ >+function thirdEntry(event) { >+ is(event.target, gOuterDoc, "Third MozEnteredDomFullscreen should be targeted at outer doc"); >+ ok(gOuterDoc.mozFullScreenElement != null, "Outer doc return to fullscreen after cancel fullscreen in inner doc"); >+ window.removeEventListener("MozEnteredDomFullscreen", thirdEntry, false); >+ gOuterDoc.mozCancelFullScreen(); >+ window.opener.wrappedJSObject.done(); >+} >+ >+function start() { >+ SimpleTest.waitForFocus( >+ function() { >+ gBrowser = document.getElementById("browser"); >+ gOuterDoc = gBrowser.contentDocument; >+ gBrowser.contentWindow.focus(); >+ window.addEventListener("MozEnteredDomFullscreen", firstEntry, false); >+ gOuterDoc.body.mozRequestFullScreen(); >+ }); >+} >+ >+]]> >+</script> >+ >+<browser type="content" id="browser" width="400" height="400" src="MozEnteredDomFullscreen_content.html"/> >+ >+</window> >diff --git a/dom/tests/mochitest/chrome/MozEnteredDomFullscreen_content.html b/dom/tests/mochitest/chrome/MozEnteredDomFullscreen_content.html >new file mode 100644 >--- /dev/null >+++ b/dom/tests/mochitest/chrome/MozEnteredDomFullscreen_content.html >@@ -0,0 +1,8 @@ >+<html> >+<head> >+</head> >+<body style="background-color: blue;"> >+<p>Outer doc</p> >+<iframe id="innerFrame" src="data:text/html,<html><body style='background-color: red;'><p>Inner doc</p></body></html>"></iframe> >+</body> >+</html> >\ No newline at end of file >diff --git a/dom/tests/mochitest/chrome/test_MozEnteredDomFullscreen_event.xul b/dom/tests/mochitest/chrome/test_MozEnteredDomFullscreen_event.xul >new file mode 100644 >--- /dev/null >+++ b/dom/tests/mochitest/chrome/test_MozEnteredDomFullscreen_event.xul >@@ -0,0 +1,36 @@ >+<?xml version="1.0"?> >+<?xml-stylesheet href="chrome://global/skin" type="text/css"?> >+<?xml-stylesheet href="chrome://mochikit/content/tests/SimpleTest/test.css" type="text/css"?> >+<!-- >+ Test that "MozShowFullScreenWarning" is dispatched to chrome on restricted keypress. >+ --> >+<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" width="400" height="400"> >+ >+ <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script> >+ >+<script> >+SimpleTest.waitForExplicitFinish(); >+ >+// Ensure the full-screen api is enabled, and will be disabled on test exit. >+var gPrevEnabled = SpecialPowers.getBoolPref("full-screen-api.enabled"); >+SpecialPowers.setBoolPref("full-screen-api.enabled", true); >+ >+var gPrevTrusted = SpecialPowers.getBoolPref("full-screen-api.allow-trusted-requests-only"); >+SpecialPowers.setBoolPref("full-screen-api.allow-trusted-requests-only", false); >+ >+ >+newwindow = window.open("MozEnteredDomFullscreen_chrome.xul", "_blank","chrome,resizable=yes,width=400,height=400"); >+ >+function done() >+{ >+ newwindow.close(); >+ SpecialPowers.setBoolPref("full-screen-api.enabled", gPrevEnabled); >+ SpecialPowers.setBoolPref("full-screen-api.allow-trusted-requests-only", gPrevTrusted); >+ SimpleTest.finish(); >+} >+ >+</script> >+ >+<body xmlns="http://www.w3.org/1999/xhtml" style="height: 300px; overflow: auto;"/> >+ >+</window>

Updated with smaug's review comments. Carrying forward r=smaug.

I had to update the MozEnteredFullscreen chrome test to serve the test HTML file with a domain, because the test was failing after the changes I made to Patch 3. The test document was being served with a system pricipal (and null URI strangely), so after my last changes the document was automatically being allowed fullscreen permission and so we weren't dispatching MozEnteredFullscreen when we were supposed to. This patch updates the test to serve the inner test page as a content document, so permissions work as expected on the test's domain. Carrying forward r=smaug.

Comment on attachment 619775 [details] [diff] [review] Patch 4 v3: Dispatch "MozEnteredDomFullScreen" on fullscreen enter Review of attachment 619775 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/tests/mochitest/chrome/test_MozEnteredDomFullscreen_event.xul @@ +29,5 @@ > + > +// Ensure "fullscreen" permissions are not present on the test URI. > +var pm = Cc["@mozilla.org/permissionmanager;1"].getService(Ci.nsIPermissionManager); > +var uri = make_uri("http://mochi.test:8888"); > +pm.remove(uri, "fullscreen"); This needs to be: pm.remove(uri.host, "fullscreen"); I'll fix it before I land.

Comment on attachment 618509 [details] [diff] [review] Patch 5 v2: Add "fullscreen" to about permissions Looks good, and this avoids the odd UX problem that came up in the last patch.

Comment on attachment 618929 [details] [diff] [review] Patch 6 v3: Add UI to approve fullscreen >+++ b/browser/base/content/browser.css > #full-screen-warning-container[fade-warning-out] { >- -moz-transition-property: opacity !important; >+ -moz-transition-property: opacity, background-color !important; > -moz-transition-duration: 500ms !important; > opacity: 0.0; >+ background-color: rgba(0,0,0,0); > } I don't understand this change. Leaving the background-color alone and reducing the opacity should fade the whole thing including the background. >+++ b/browser/base/content/browser.js >+ showWarning: function(targetDoc) { >+ if (!document.mozFullScreen || >+ !gPrefService.getBoolPref("full-screen-api.approval-required")) { > return; > } >+++ b/modules/libpref/src/init/all.js >+pref("full-screen-api.approval-required", true); Is this pref used outside of browser/? If not, it belongs in browser/app/profile/firefox.js.

Yeah, the full-screen-api.approval-required pref is only used by Firefox, and we don't need the background CSS transition. I've removed the background CSS transition, and moved the pref to firefox.js. Thanks!

Comment on attachment 620087 [details] [diff] [review] Patch 6 v4: Add authorization UI to browser >+ // Ensure focus switches away from the (now hidden) warning box. If the user >+ // clicked buttons in the fullscreen key authorization UI, it would have been >+ // focused, and any key events would be directed at the (now hidden) chrome >+ // document instead of the target document. >+ content.focus(); make this gBrowser.selectedBrowser.focus() >+ if (event.propertyName == "opacity") { >+ this.cancelWarning(); >+ } >+ if (this.warningBox) { >+ this.warningBox.setAttribute("fade-warning-out", "true"); >+ } >+ if (!isApproved) >+ document.mozCancelFullScreen(); >+ if (!document.mozFullScreen || >+ !gPrefService.getBoolPref("full-screen-api.approval-required")) { > return; > } remove braces >+ <vbox id="full-screen-warning-message" align="center"> >+ <hbox> >+ <description id="full-screen-domain-text"></description> >+ </hbox> What's the point of this hbox? What happens with very long domain names? >+ <description class="full-screen-description" value="&fullscreenExitHint.value;"></description> >+ <vbox id="full-screen-approval-pane" align="center"> >+ <description class="full-screen-description" value="&fullscreenApproval.value;"></description> >+ <hbox> >+ <button label="&fullscreenAllowButton.label;" oncommand="FullScreen.setFullscreenAllowed(true);"/> >+ <button label="&fullscreenExitButton.label;" oncommand="FullScreen.setFullscreenAllowed(false);"/> >+ </hbox> >+ <checkbox id="full-screen-remember-decision"/> >+ </vbox> >+ </vbox> replace ></description> with /> >+#full-screen-warning-message button { >+ font-size: 120%; >+} >+ >+#full-screen-warning-message checkbox { >+ font-size: 120%; >+} #full-screen-approval-pane > button, #full-screen-approval-pane > checkbox { font-size: 120%; } Please remove your trailing spaces throughout this patch.

I've made the descriptions and checkbox labels that display the domain names use word-wrap:break-word so that long domain names are line broken and displayed on multiple lines. I finally figured out how to make my text editor remove trailing whitespace, so this updated patch removes some other trailing whitespace in the files I touched. I've removed braces around single line if blocks in the code I touched.

Screenshot showing the fullscreen approval UI rendering a fake long domain name.

Comment on attachment 620180 [details] [diff] [review] Patch 6 v5: Add authorization UI to browser >+ <vbox id="full-screen-warning-message" align="center"> >+ <description id="full-screen-domain-text"></description> >+ <description class="full-screen-description" value="&fullscreenExitHint.value;"></description> >+ <vbox id="full-screen-approval-pane" align="center"> >+ <description class="full-screen-description" value="&fullscreenApproval.value;"></description> >+ <hbox> >+ <button label="&fullscreenAllowButton.label;" oncommand="FullScreen.setFullscreenAllowed(true);"/> >+ <button label="&fullscreenExitButton.label;" oncommand="FullScreen.setFullscreenAllowed(false);"/> >+ </hbox> >+ <checkbox id="full-screen-remember-decision"/> >+ </vbox> >+ </vbox> replace ></description> with /> >+#full-screen-warning-message button { See my previous comment about changing this selector. >+#full-screen-domain-text { >+ font-size: 300%; >+ word-wrap: break-word; >+ /* We must specify a min-width, otherwise word-wrap:break-word doesn't work. Bug 630864. */ >+ min-width: 1px; >+} Put this in browser/base/content/browser.css (except the font-size) >+#full-screen-remember-decision label { Avoid the descendant selector here as well, it's slow (we have many labels in browser.xul).

Review comments addressed.

Comment on attachment 620244 [details] [diff] [review] Patch 6 v6: Add authorization UI to browser >+#full-screen-domain-text { >+ word-wrap: break-word; >+ /* We must specify a min-width, otherwise word-wrap:break-word doesn't work. Bug 630864. */ >+ min-width: 1px; >+} >+ >+#full-screen-remember-decision > .checkbox-label-box > .checkbox-label { >+ word-wrap: break-word; >+ /* We must specify a min-width, otherwise word-wrap:break-word doesn't work. Bug 630864. */ >+ min-width: 1px; > } merge these two rules >+.full-screen-approval-button { >+ font-size: 120%; >+} >+#full-screen-remember-decision { >+ font-size: 120%; > } ditto

https://hg.mozilla.org/integration/mozilla-inbound/rev/75495aac50c6 https://hg.mozilla.org/integration/mozilla-inbound/rev/bb2e382a70b3 https://hg.mozilla.org/integration/mozilla-inbound/rev/a6a5f79e630d https://hg.mozilla.org/integration/mozilla-inbound/rev/f3210a94c24f https://hg.mozilla.org/integration/mozilla-inbound/rev/15b2ca578fff https://hg.mozilla.org/integration/mozilla-inbound/rev/9399756013e3 https://hg.mozilla.org/integration/mozilla-inbound/rev/b1ca2802b98e

https://hg.mozilla.org/mozilla-central/rev/75495aac50c6 https://hg.mozilla.org/mozilla-central/rev/bb2e382a70b3 https://hg.mozilla.org/mozilla-central/rev/a6a5f79e630d https://hg.mozilla.org/mozilla-central/rev/f3210a94c24f https://hg.mozilla.org/mozilla-central/rev/15b2ca578fff https://hg.mozilla.org/mozilla-central/rev/9399756013e3 https://hg.mozilla.org/mozilla-central/rev/b1ca2802b98e

Before opening a new bug that could not be needed or useful, is there a reason behind the choice of Allow/Deny? I could be wrong but usually Firefox uses Allow/Don't allow for all permission dialogs .