Last Comment Bug 716107 - Better key input support in DOM full-screen mode
: Better key input support in DOM full-screen mode
Status: RESOLVED FIXED
[sec-assigned:curtisk]
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Chris Pearce (:cpearce)
:
Mentors:
Depends on: 743904 746885
Blocks: 705234 gecko-games 545812
  Show dependency treegraph
 
Reported: 2012-01-06 15:14 PST by Chris Pearce (:cpearce)
Modified: 2012-08-15 08:31 PDT (History)
33 users (show)
dveditz: sec‑review+
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Screenshot of key input warning (115.48 KB, image/jpeg)
2012-01-30 17:36 PST, Chris Pearce (:cpearce)
no flags Details
Screenshot of key access authorization UI (120.05 KB, image/png)
2012-04-03 17:39 PDT, Chris Pearce (:cpearce)
no flags Details
Screenshot of entered fullscreen warning (119.04 KB, image/png)
2012-04-03 17:43 PDT, Chris Pearce (:cpearce)
no flags Details
Video showing fullscreen authorization UI (3.77 MB, video/webm)
2012-04-09 19:35 PDT, Chris Pearce (:cpearce)
dao+bmo: feedback+
Details
Patch 1 v1: Element.mozRequestFullScreenWithKeys() IDL changes (122.57 KB, patch)
2012-04-11 02:13 PDT, Chris Pearce (:cpearce)
mounir: review+
Details | Diff | Splinter Review
Patch 2 v1: nsDocument/nsPresShell implementation of requestFullScreenWithKeys (25.29 KB, patch)
2012-04-11 02:19 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 3 v1: add full-screen-api.withKeys.enabled pref, disabled by default (10.07 KB, patch)
2012-04-11 02:28 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 4 v1: Add authorization UI to browser (16.49 KB, patch)
2012-04-11 02:31 PDT, Chris Pearce (:cpearce)
dao+bmo: review-
Details | Diff | Splinter Review
Patch 5 v1: Add tests for mozRequestFullscreenWithKeys() (14.25 KB, patch)
2012-04-11 02:35 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 6 v1: Add fullscreen with key support to about:permissions (10.21 KB, patch)
2012-04-11 02:37 PDT, Chris Pearce (:cpearce)
margaret.leibovic: review+
Details | Diff | Splinter Review
Patch 1 v2: Element.mozRequestFullScreenWithKeys() IDL changes (122.58 KB, patch)
2012-04-11 03:03 PDT, Chris Pearce (:cpearce)
cpearce: review+
Details | Diff | Splinter Review
Patch 2 v2: nsDocument/nsPresShell implementation of requestFullScreenWithKeys (26.03 KB, patch)
2012-04-11 03:08 PDT, Chris Pearce (:cpearce)
bzbarsky: review-
bugs: review+
Details | Diff | Splinter Review
Patch 3 v2: add full-screen-api.withKeys.enabled pref, disabled by default (10.07 KB, patch)
2012-04-11 03:11 PDT, Chris Pearce (:cpearce)
bugs: review+
Details | Diff | Splinter Review
Video of fullscreen approval UI (proposal #2) (3.02 MB, video/webm)
2012-04-18 20:17 PDT, Chris Pearce (:cpearce)
no flags Details
Video of fullscreen approval UI (with unrestricted keys) (3.85 MB, video/webm)
2012-04-25 17:30 PDT, Chris Pearce (:cpearce)
no flags Details
Patch 1 v1: Add "fullscreen" permissions to page info (6.77 KB, patch)
2012-04-25 17:34 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 2 v1: Remove full-screen-api.key-input-restricted keys pref (6.20 KB, patch)
2012-04-25 17:35 PDT, Chris Pearce (:cpearce)
bugs: review+
Details | Diff | Splinter Review
Patch 3 v1: Don't dispatch 'MozShowFullScreenWarning' event upon keypress in DOM fullscreen mode (10.22 KB, patch)
2012-04-25 17:38 PDT, Chris Pearce (:cpearce)
bugs: review-
Details | Diff | Splinter Review
Patch 4 v1: Dispatch "MozEnteredDomFullScreen" on fullscreen enter (2.87 KB, patch)
2012-04-25 18:52 PDT, Chris Pearce (:cpearce)
bugs: review-
Details | Diff | Splinter Review
Patch 5 v2: Add "fullscreen" to about permissions (9.35 KB, patch)
2012-04-25 18:54 PDT, Chris Pearce (:cpearce)
margaret.leibovic: review+
Details | Diff | Splinter Review
Patch 6 v2: Add UI to approve fullscreen (20.95 KB, patch)
2012-04-25 19:10 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 7 v1: Remove chrome fullscreen tests (12.89 KB, patch)
2012-04-25 19:21 PDT, Chris Pearce (:cpearce)
bugs: review+
Details | Diff | Splinter Review
Patch 1 v2: Add "fullscreen" permissions to page info (6.77 KB, patch)
2012-04-26 14:18 PDT, Chris Pearce (:cpearce)
dao+bmo: review+
Details | Diff | Splinter Review
Patch 3 v2: Don't dispatch 'MozShowFullScreenWarning' event upon keypress in DOM fullscreen mode (11.54 KB, patch)
2012-04-26 20:46 PDT, Chris Pearce (:cpearce)
bugs: review+
Details | Diff | Splinter Review
Patch 4 v2: Dispatch "MozEnteredDomFullScreen" on fullscreen enter (9.40 KB, patch)
2012-04-26 20:48 PDT, Chris Pearce (:cpearce)
bugs: review+
Details | Diff | Splinter Review
Patch 6 v3: Add UI to approve fullscreen (21.03 KB, patch)
2012-04-26 22:13 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 3 v3: Don't dispatch 'MozShowFullScreenWarning' event upon keypress in DOM fullscreen mode (11.88 KB, patch)
2012-04-30 16:59 PDT, Chris Pearce (:cpearce)
cpearce: review+
Details | Diff | Splinter Review
Patch 4 v3: Dispatch "MozEnteredDomFullScreen" on fullscreen enter (10.61 KB, patch)
2012-04-30 17:05 PDT, Chris Pearce (:cpearce)
cpearce: review+
Details | Diff | Splinter Review
Patch 6 v4: Add authorization UI to browser (21.46 KB, patch)
2012-05-01 15:14 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 6 v5: Add authorization UI to browser (34.14 KB, patch)
2012-05-01 21:22 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Screenshot showing displaying (fake) long domain name (277.50 KB, image/jpeg)
2012-05-01 21:23 PDT, Chris Pearce (:cpearce)
no flags Details
Patch 6 v6: Add authorization UI to browser (34.74 KB, patch)
2012-05-02 03:29 PDT, Chris Pearce (:cpearce)
dao+bmo: review+
Details | Diff | Splinter Review

Description Chris Pearce (:cpearce) 2012-01-06 15:14:22 PST
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.
Comment 1 David Humphrey (:humph) 2012-01-13 07:14:25 PST
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.
Comment 2 Chris Pearce (:cpearce) 2012-01-17 18:44:38 PST
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.
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-18 17:53:48 PST
Weird.

I'm not happy with just allowing keyboard input willy-nilly.
Comment 4 Chris Pearce (:cpearce) 2012-01-23 12:51:49 PST
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.
Comment 5 Chris Pearce (:cpearce) 2012-01-30 17:36:47 PST
Created attachment 592940 [details]
Screenshot of key input warning

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.
Comment 6 Chris Pearce (:cpearce) 2012-02-22 17:14:07 PST
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.
Comment 7 Alon Zakai (:azakai) 2012-03-14 11:29:45 PDT
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?
Comment 8 Chris Pearce (:cpearce) 2012-03-14 12:46:36 PDT
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.
Comment 9 Alon Zakai (:azakai) 2012-03-14 13:44:38 PDT
Thanks for the link, and very happy to hear this has already been thought of.
Comment 10 Chris Pearce (:cpearce) 2012-04-02 21:56:32 PDT
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 11 Chris Pearce (:cpearce) 2012-04-03 12:06:31 PDT
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...
Comment 12 Ian Melven :imelven 2012-04-03 14:25:20 PDT
(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 :)
Comment 13 Chris Pearce (:cpearce) 2012-04-03 14:48:16 PDT
(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.
Comment 14 Chris Pearce (:cpearce) 2012-04-03 15:20:50 PDT
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.
Comment 15 Grant Galitz 2012-04-03 15:35:55 PDT
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.
Comment 16 Chris Pearce (:cpearce) 2012-04-03 17:39:37 PDT
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.
Comment 17 Chris Pearce (:cpearce) 2012-04-03 17:43:17 PDT
Created attachment 612045 [details]
Screenshot of entered fullscreen warning

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.
Comment 18 Chris Pearce (:cpearce) 2012-04-03 19:48:30 PDT
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 .
Comment 19 Dão Gottwald [:dao] 2012-04-04 01:22:37 PDT
(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.
Comment 20 Chris Pearce (:cpearce) 2012-04-04 03:19:02 PDT
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.
Comment 21 Dão Gottwald [:dao] 2012-04-04 04:29:21 PDT
(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]
Comment 22 Jesse Ruderman 2012-04-04 14:09:57 PDT
I prefer verb button labels, especially for security choices.  Changing it to "Yes"/"No" doesn't really make it more binary.
Comment 23 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2012-04-04 14:20:49 PDT
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.
Comment 24 Dão Gottwald [:dao] 2012-04-05 02:40:13 PDT
(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]
Comment 25 Chris Pearce (:cpearce) 2012-04-09 19:35:36 PDT
Created attachment 613472 [details]
Video showing fullscreen authorization UI

I have reworked the UI, taking Dão's feedback on board. How does this look?
Comment 26 Chris Pearce (:cpearce) 2012-04-09 21:26:19 PDT
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 27 Dão Gottwald [:dao] 2012-04-10 02:24:43 PDT
Comment on attachment 613472 [details]
Video showing fullscreen authorization UI

Looks OK to me. You should probably end those sentences with a full stop.
Comment 28 André 2012-04-10 23:56:50 PDT
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.
Comment 29 Chris Pearce (:cpearce) 2012-04-11 02:13:58 PDT
Created attachment 613916 [details] [diff] [review]
Patch 1 v1: Element.mozRequestFullScreenWithKeys() IDL changes

Part 1: IDL changes required to add mozRequestFullScreenWithKeys() to Element. Just stubs out mozRequestFullScreenWithKeys() to return NS_OK, implemented in next patch.
Comment 30 Chris Pearce (:cpearce) 2012-04-11 02:19:17 PDT
Created attachment 613918 [details] [diff] [review]
Patch 2 v1: nsDocument/nsPresShell implementation of requestFullScreenWithKeys

* 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 31 Chris Pearce (:cpearce) 2012-04-11 02:23:04 PDT
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 32 Mounir Lamouri (:mounir) 2012-04-11 02:25:41 PDT
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.
Comment 33 Chris Pearce (:cpearce) 2012-04-11 02:28:09 PDT
Created attachment 613921 [details] [diff] [review]
Patch 3 v1: add full-screen-api.withKeys.enabled pref, disabled by default

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.
Comment 34 Chris Pearce (:cpearce) 2012-04-11 02:31:47 PDT
Created attachment 613923 [details] [diff] [review]
Patch 4 v1: Add authorization UI to browser

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).
Comment 35 Chris Pearce (:cpearce) 2012-04-11 02:35:25 PDT
Created attachment 613924 [details] [diff] [review]
Patch 5 v1: Add tests for mozRequestFullscreenWithKeys()

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.
Comment 36 Chris Pearce (:cpearce) 2012-04-11 02:37:56 PDT
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].
Comment 37 Chris Pearce (:cpearce) 2012-04-11 02:40:21 PDT
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
Comment 38 Jared Wein [:jaws] (please needinfo? me) 2012-04-11 02:56:28 PDT
(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.
Comment 39 Chris Pearce (:cpearce) 2012-04-11 03:03:47 PDT
Created attachment 613933 [details] [diff] [review]
Patch 1 v2:  Element.mozRequestFullScreenWithKeys() IDL changes

With Mounir's two changes made. I used http://people.mozilla.com/~sfink/uploads/update-uuids to update the uuids.
Comment 40 Chris Pearce (:cpearce) 2012-04-11 03:08:58 PDT
Created attachment 613935 [details] [diff] [review]
Patch 2 v2: nsDocument/nsPresShell implementation of requestFullScreenWithKeys

As patch v1, but made the brace style conform, and brought forward declaration of nsGenericElement::RequestFullScreen(bool aWithKeys) as Mounir suggested.
Comment 41 Dão Gottwald [:dao] 2012-04-11 03:10:10 PDT
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.
Comment 42 Chris Pearce (:cpearce) 2012-04-11 03:11:51 PDT
Created attachment 613937 [details] [diff] [review]
Patch 3 v2: add full-screen-api.withKeys.enabled pref, disabled by default

Bitrot fix caused by indentation change in previous patch revision.
Comment 43 Dão Gottwald [:dao] 2012-04-11 03:19:00 PDT
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?
Comment 44 Chris Pearce (:cpearce) 2012-04-11 03:53:35 PDT
(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?
Comment 45 Dão Gottwald [:dao] 2012-04-11 07:07:14 PDT
(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 46 Olli Pettay [:smaug] (TPAC) 2012-04-11 08:02:53 PDT
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 47 Boris Zbarsky [:bz] (TPAC) 2012-04-11 11:35:21 PDT
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.  :(
Comment 48 Chris Pearce (:cpearce) 2012-04-11 17:44:21 PDT
(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.
Comment 49 Chris Pearce (:cpearce) 2012-04-11 20:12:24 PDT
(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)?
Comment 50 Boris Zbarsky [:bz] (TPAC) 2012-04-11 20:26:02 PDT
And dealing sensibly with a null URI (means system principal), yes.
Comment 51 :Margaret Leibovic 2012-04-12 14:08:29 PDT
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.
Comment 52 Chris Pearce (:cpearce) 2012-04-12 17:31:09 PDT
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!
Comment 53 Jared Wein [:jaws] (please needinfo? me) 2012-04-12 17:45:22 PDT
(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.
Comment 54 Chris Pearce (:cpearce) 2012-04-12 18:36:11 PDT
(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.
Comment 55 Chris Pearce (:cpearce) 2012-04-16 17:20:07 PDT
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 56 Olli Pettay [:smaug] (TPAC) 2012-04-17 03:30:16 PDT
Comment on attachment 613924 [details] [diff] [review]
Patch 5 v1: Add tests for mozRequestFullscreenWithKeys()

Based on that, these tests will need some updates.
Comment 57 Jesse Ruderman 2012-04-18 15:54:42 PDT
> 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.
Comment 58 Chris Pearce (:cpearce) 2012-04-18 16:27:27 PDT
(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.
Comment 59 Chris Pearce (:cpearce) 2012-04-18 20:17:32 PDT
Created attachment 616431 [details]
Video of fullscreen approval UI (proposal #2)

Here's a screencast of the approval UI I'm proposing for fullscreen (with unrestricted key access) in fullscreen mode.
Comment 60 Chris Pearce (:cpearce) 2012-04-18 20:28:28 PDT
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.
Comment 61 Chris Pearce (:cpearce) 2012-04-23 16:53:06 PDT
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).
Comment 62 André 2012-04-24 00:50:59 PDT
Sound´s really good to me. will this be in firefox 13, 14, ..?
Comment 63 Chris Pearce (:cpearce) 2012-04-24 02:12:12 PDT
Probably 15; 14 gets uplifted onto Aurora tomorrow.
Comment 64 Chris Pearce (:cpearce) 2012-04-25 17:30:13 PDT
Created attachment 618490 [details]
Video of fullscreen approval UI (with unrestricted keys)

Screencast of latest UI, after security review complete.
Comment 65 Chris Pearce (:cpearce) 2012-04-25 17:34:36 PDT
Created attachment 618491 [details] [diff] [review]
Patch 1 v1: Add "fullscreen" permissions to page info

Add per-domain approval/permission to go fullscreen to Page Info. (see attachment 618490 [details] to see it in action).
Comment 66 Chris Pearce (:cpearce) 2012-04-25 17:35:52 PDT
Created attachment 618492 [details] [diff] [review]
Patch 2 v1: Remove full-screen-api.key-input-restricted keys pref

Remove full-screen-api.key-input-restricted keys pref, since keys aren't restricted in fullscreen mode per se anymore.
Comment 67 Chris Pearce (:cpearce) 2012-04-25 17:38:35 PDT
Created attachment 618493 [details] [diff] [review]
Patch 3 v1: Don't dispatch 'MozShowFullScreenWarning' event upon keypress in DOM fullscreen mode

Don't dispatch 'MozShowFullScreenWarning' event upon keypress in DOM fullscreen mode, since we're not warning on keypress anymore.
Comment 68 Chris Pearce (:cpearce) 2012-04-25 18:52:52 PDT
Created attachment 618508 [details] [diff] [review]
Patch 4 v1: Dispatch "MozEnteredDomFullScreen" on fullscreen enter

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.
Comment 69 Chris Pearce (:cpearce) 2012-04-25 18:54:58 PDT
Created attachment 618509 [details] [diff] [review]
Patch 5 v2: Add "fullscreen" to about permissions

Basically the same as before, but with "fullscreen" rather than "fullscreenKeys", and added block and global block options.
Comment 70 Chris Pearce (:cpearce) 2012-04-25 19:10:39 PDT
Created attachment 618511 [details] [diff] [review]
Patch 6 v2: Add UI to approve fullscreen

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).
Comment 71 Chris Pearce (:cpearce) 2012-04-25 19:21:59 PDT
Created attachment 618512 [details] [diff] [review]
Patch 7 v1: Remove chrome fullscreen tests

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 72 Olli Pettay [:smaug] (TPAC) 2012-04-26 04:17:45 PDT
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 73 Olli Pettay [:smaug] (TPAC) 2012-04-26 04:20:09 PDT
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 74 Dão Gottwald [:dao] 2012-04-26 06:33:10 PDT
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?
Comment 75 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2012-04-26 14:17:59 PDT
SecReview Notes: 
https://wiki.mozilla.org/Security/AppsProject/Element.mozRequestFullscreenWithKeys
Comment 76 Chris Pearce (:cpearce) 2012-04-26 14:18:03 PDT
Created attachment 618802 [details] [diff] [review]
Patch 1 v2: Add "fullscreen" permissions to page info

How about "Enter Fullscreen"?
Comment 77 Chris Pearce (:cpearce) 2012-04-26 20:46:46 PDT
Created attachment 618914 [details] [diff] [review]
Patch 3 v2: Don't dispatch 'MozShowFullScreenWarning' event upon keypress in DOM fullscreen mode

Using nsIPrincipal instead of nsIURIs for nsContentUtils::IsSitePerm{Allow,Deny}.
Comment 78 Chris Pearce (:cpearce) 2012-04-26 20:48:08 PDT
Created attachment 618915 [details] [diff] [review]
Patch 4 v2: Dispatch "MozEnteredDomFullScreen" on fullscreen enter

With test and dispatching to MozEnteredDomFullscreen to chrome instead of content.
Comment 79 Chris Pearce (:cpearce) 2012-04-26 22:13:28 PDT
Created attachment 618929 [details] [diff] [review]
Patch 6 v3: Add UI to approve fullscreen

When I updated patch 4 I changed the "MozEnteredDomFullScreen" event to be "MozEnteredDomFullscreen", so I need to update this patch to reflect at.
Comment 80 Chris Pearce (:cpearce) 2012-04-27 01:40:15 PDT
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 81 Olli Pettay [:smaug] (TPAC) 2012-04-27 05:42:02 PDT
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 82 Olli Pettay [:smaug] (TPAC) 2012-04-27 05:51:02 PDT
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>
Comment 83 Chris Pearce (:cpearce) 2012-04-30 16:59:24 PDT
Created attachment 619773 [details] [diff] [review]
Patch 3 v3: Don't dispatch 'MozShowFullScreenWarning' event upon keypress in DOM fullscreen mode

Updated with smaug's review comments.

Carrying forward r=smaug.
Comment 84 Chris Pearce (:cpearce) 2012-04-30 17:05:27 PDT
Created attachment 619775 [details] [diff] [review]
Patch 4 v3: Dispatch "MozEnteredDomFullScreen" on fullscreen enter

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 85 Chris Pearce (:cpearce) 2012-05-01 02:30:42 PDT
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 86 :Margaret Leibovic 2012-05-01 08:15:59 PDT
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 87 Dão Gottwald [:dao] 2012-05-01 15:00:42 PDT
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.
Comment 88 Chris Pearce (:cpearce) 2012-05-01 15:14:07 PDT
Created attachment 620087 [details] [diff] [review]
Patch 6 v4: Add authorization UI to browser

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 89 Dão Gottwald [:dao] 2012-05-01 15:38:05 PDT
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.
Comment 90 Chris Pearce (:cpearce) 2012-05-01 21:22:36 PDT
Created attachment 620180 [details] [diff] [review]
Patch 6 v5: Add authorization UI to browser

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.
Comment 91 Chris Pearce (:cpearce) 2012-05-01 21:23:47 PDT
Created attachment 620181 [details]
Screenshot showing displaying (fake) long domain name

Screenshot showing the fullscreen approval UI rendering a fake long domain name.
Comment 92 Dão Gottwald [:dao] 2012-05-02 02:01:44 PDT
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).
Comment 93 Chris Pearce (:cpearce) 2012-05-02 03:29:52 PDT
Created attachment 620244 [details] [diff] [review]
Patch 6 v6: Add authorization UI to browser

Review comments addressed.
Comment 94 Dão Gottwald [:dao] 2012-05-02 05:14:54 PDT
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
Comment 97 Francesco Lodolo [:flod] 2012-05-08 22:40:30 PDT
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 .

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