I've landed bug 545812 (preffed off) which adds API to enable content to request that an arbitrary element be made the full-screen element. This puts the browser window into full-screen mode (using the same mechanism as browser F11 full-screen mode) and adds style rules to make the requesting full-screen element cover the entire window area. To make this secure in the face of phishing attacks, we need UI to make it obvious to the user that they've entered DOM full-screen mode. We should provide a dismissible warning saying something to the effect of "press escape to exit full-screen mode". This warning should auto-hide after some time period. Note that we're taking a similar permission model to flash's full-screen API, in that we don't require the user to approve requests for full-screen, we automatically grant requests for full-screen when they're in user generated event handlers, and pop up a warning to ensure they're aware they've entered full-screen mode. I imagine this warning could be implemented in browser.js in the window's "fullscreen" event handler. The document.mozFullscreen property (yes, on the chrome document) can be used to distinguish between DOM and browser full-screen modes. This will still work in e10s (once bug 684620 lands). So in summary: when the browser enters DOM full-screen mode, display/pop down a dismissible warning (which dismisses itself after some time period) to warn the user that they've entered full-screen mode. We need this before we can enable the DOM full-screen API on release branches. We'd like to target Fx10.

This warning message needs to display during the transition which stretches the full-screen element to encompass the entire viewable area (which we'll add in bug 684628). If we have the warning display in the same place as the pop-up blocker's pop-down warning, maybe that would accomplish that??

* Display a warning "Press F11 to leave full-screen" for 4s when entering DOM full-screen mode. * Obscure the browser (opacity:0.3) for 2s when entering DOM full-screen mode. This ensures that the warning is noticed on busy pages. * Warning is not dismiss-able, but durations can be configured by prefs.

Screenshot showing warning display, with browser obscured.

FYI screenshot is of my test page at http://pearce.org.nz/full-screen/ with a video as the full-screen element.

Updated with some suggestions from shorlander: * Centred warning message on screen. * Used san-serif content font. * Kept colours and box shadow the same; this needs to be high contrast so that it's very noticeable.

Screen shot showing "you've entered full-screen" warning. The <browser> is also faded out to make the warning more obvious. For comparison, this screenshot is of the demo at http://pearce.org.nz/full-screen/ with the video element full-screen and seeked to 26.0 seconds. Jesse: How do you think this looks?

Here's a screenshot of the same frame without the warning shown, for comparison of the effect of fading out the background.

Why F11 and not Esc? F11 has the disadvantage of being different from Flash, different from what users try naturally, and going *into* full-screen mode if you time it wrong.

It might be best to show the warning at the top of the screen, so it appears where there would be chrome rather than where there would be content.

I used F11 because Esc is mapped to cancel all loads in the doc's load group. So if you've got a <video> playing which is not yet fully downloaded and you press escape, the video's load will cancel and you'll get a grey error cross over the <video>. Chrome's full-screen API warning also suggests F11 to exit.

(In reply to Jesse Ruderman from comment #9) > It might be best to show the warning at the top of the screen, so it appears > where there would be chrome rather than where there would be content. I agree with you, but I believe Stephen Horlander disagreed.

(In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #10) > I used F11 because Esc is mapped to cancel all loads in the doc's load > group. So if you've got a <video> playing which is not yet fully downloaded > and you press escape, the video's load will cancel and you'll get a grey > error cross over the <video>. Chrome's full-screen API warning also suggests > F11 to exit. We can just eat the Esc key and exit full-screen without doing the "stop" command. I agree with Jesse; Esc is the way you cancel dialogs etc. It's the right key to use here.

* Display warning at top again, as suggested by Jesse. * Change warning to specify Escape as exit key. * Don't call run BrowserStop() command when in full-screen mode, so that full-screen <video> downloads aren't stopped when escape is pressed to exit full-screen.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11) > (In reply to Jesse Ruderman from comment #9) > > It might be best to show the warning at the top of the screen, so it appears > > where there would be chrome rather than where there would be content. > > I agree with you, but I believe Stephen Horlander disagreed. I don't think placing it where the chrome would be creates any real benefit unless it also looks like an established bit of notification UI. If the goal of the notification is both to inform and to warn then it will likely be more visible and noticeable centered. If it is activated from content that is also where your focus will be. Putting on at the top puts in your peripheral vision. Which might actually be good if we are going to show this every time something goes fullscreen :)

I'm optimizing for attention in the attack case: most of the screen has been made to look like web browser chrome, and the content area has been made confusing. Peripheral vision is fine -- the point is that something is changing on a part of the screen that wouldn't normally change, and that tips you off to the attack.

We should also show the domain name or page url in the warning message.

So, if we show the host name (or page URL) in the warning message, how should we display it? "$hostname entered full-screen\n Press ESC to leave full-screen" ? If we show the entire host name, it could be too big to display on one line in the warning message and cause the message to look bad, and hide the "entered full-screen" text. Roc also worries that the bad guys could use the text in their hostname to influence users' perceptions and actions. If we limit the displayed hostname to only show the first N characters of the domain, the bad guys could make their hostname secure.paypal.com.evilguy.com, where ".evilguy.com" appears after the N character cut off. Perhaps we should limit the hostname to the last M characters and first N characters?

* Updated to include domain name in warning text. * Domain names longer than 40 characters are shorted to $first12Characters+"..."+$last25Characters. * Hide the browser toolbar instantly when entering full-screen mode, and prevent mousehover at top of screen from showing the toolbar again (unlike regular browser full-screen mode, where the toolbar slides-down on mousehover at top of screen).

Jesse, how does this look for the entered-full-screen warning message?

What's the rationale for including the hostname in this message? Grabbing the host from browser.currentURI seems wrong to me. If you're in the middle of a navigation, the wrong hostname could be shown. The "..." should be "…". The slicing assumes the string contains only BMP characters. Have you tested with wide chars (all W's) and RTL chars? Don't we have centralized code somewhere for showing hostnames and domains? Or at least for middle-truncation? Can you post a video showing the full animation into full-screen mode?

(In reply to Jesse Ruderman from comment #22) > What's the rationale for including the hostname in this message? Someone on the security review requested showing the domain name or URL. This shows the domain name. I guess they want to make spoofing attakcs more obvious. > Grabbing the host from browser.currentURI seems wrong to me. If you're in > the middle of a navigation, the wrong hostname could be shown. We're planning to exit full-screen mode when navigation occurs. Will this still be a problem then? Should I get the URI from somewhere else? > The "..." should be "…". The slicing assumes the string contains only BMP > characters. Have you tested with wide chars (all W's) and RTL chars? OMG. Are JS strings not wide chars? RTL, urgh... Sounds like it would be easier to not bother truncating the hostname. > Don't we have centralized code somewhere for showing hostnames and domains? > Or at least for middle-truncation? Maybe. Hopefully. > Can you post a video showing the full animation into full-screen mode? I'll see what I can do...

> > Grabbing the host from browser.currentURI seems wrong to me. If you're in > > the middle of a navigation, the wrong hostname could be shown. > > We're planning to exit full-screen mode when navigation occurs. Will this > still be a problem then? I think so, because in this case the navigation happened before going into full-screen. > Should I get the URI from somewhere else? Do you have the caller's principal? > Sounds like it would be easier to not bother truncating the hostname. My "all W's" and RTL concerns don't depend on truncation...

The win of having the hostname shown seems small. It's important that users see the "Press Esc..." part before the message disappears, so I want the message to be short. Maybe we should only show the hostname if the principal that requested full-screen differs from the main page (e.g. if it was triggered by a youtube iframe from a blog)? I think we do that for some other security UI.

Uploaded video of warning in action at Jesse's request.

XUL <label>s support ellipsizing in the middle; using that here might avoid problems with RTL and Unicode.

If we display the host I think it should only be effective TLD + 1. I don't think we need the host, though...

I can show the "$host entered full-screen" line of the warning inside a description with cropping enabled. Aesthetically, the least jarring option is to use crop="left". On long domain names crop="center" can crop some of "entered full-screen" text, and crop="right" obviously does too. If I put the "entered full-screen" text in another element beside the hostname on the same line, I can't have them appear centered if I'm cropping, because a width must be specified on a description to enforce cropping, which throws out the alignment. Putting "entered full-screen" on a line by itself looks weird. So crop="left" works the best I think. I think I can get hold of the appropriate principals etc so that we only show the hostname when principals are not equal. We'll just show "Press ESC to leave full-screen" when the principals are equal. Here's a screen shot of the warning showing on a long host name.

By using crop="left", you're relying on the hostname being the first word in the message? That might not be the most natural word order in all languages.

Very good point Jesse. I'll leave the hostname out at the moment and look at adding it in a follow up bug. That way I can make progress on all the other dependencies of bug 545812.

This looks great — if you wanted UI feedback. :)

Patch v5, requesting review from dolske, and shorlander asked for UI review a week or so back.

Comment on attachment 566681 [details] [diff] [review] Patch v4 Review of attachment 566681 [details] [diff] [review]: ----------------------------------------------------------------- Initial comments from a quick skim... ::: browser/base/content/browser.js @@ +1674,4 @@ > if (window.fullScreen) > onFullScreen(); > + if (document.mozFullScreen) > + onMozFullScreenChange(); Did you test the various combinations of entering+leaving both full-screen modes? (EG F11-fs + DOM-fs - F11-fs - DOM-fs). We should ponder what the UX should be -- 2 full screen modes are going to confused users. :) [Can be addressed in a followup, doesn't need to block this from landing.] @@ +4063,5 @@ > + startWarningFadeIn: function() { > + FullScreen.warningFadeOutTimer = null; > + FullScreen.warningBox.setAttribute("hidden", "false"); > + FullScreen.warningBox.clientTop; > + FullScreen.warningBox.removeAttribute("fadeout"); startWarningFadeIn() doesn't seem to be called from anywhere? Deadwood? Also, these functions keep referring to |FullScreen.foo| over and over, which is kind of sucky. |this.foo| would be preferable, with some Function.bind() magic if needed to make |this| the object and not an event... @@ +4071,5 @@ > + // out after a few seconds. > + showWarning: function() { > + if (FullScreen.warningBox) { > + // We're already showing a warning. Don't show another. > + return; Hmm. I sorta wonder if this should extend any existing warning. But I guess since the warning just stays for a fixed timespan, that there's no harm in changing what's FS during that interval, or ability for content to sneak one FS element through and cancel a previous FS request before the time is up. @@ +4081,5 @@ > + > + // Partially fade out the browser element for the first few seconds while > + // displaying the entered-full-screen warning. This helps ensure the > + // warning is noticed on busy pages. > + gBrowser.mCurrentBrowser.setAttribute("class", "showing-full-screen-warning"); Hmm, I think this would all be simpler by simply having the warning message have a full-browser semi-transparent background, then there's just 1 thing to fade out. Or am I missing some bit of cleverness? Ah, see you've different timeouts for both. So the message appears with a subdued browser, then the content fades back to normal, then the message fades away. I think you could still achieve the same effect with a single warningbox + background color/texture. Probably with just one attribute (or zero?) and CSS with -moz-transition-delay. ::: browser/base/content/tabbrowser.xml @@ +71,5 @@ > flex="1" eventnode="document" xbl:inherits="handleCtrlPageUpDown" > onselect="if (event.target.localName == 'tabpanels') this.parentNode.updateCurrentBrowser();"> > <xul:tabpanels flex="1" class="plain" selectedIndex="0" anonid="panelcontainer"> > <xul:notificationbox flex="1"> > + <xul:stack flex="1" anonid="browserStack" align="start"> What's the align=start for? @@ +351,5 @@ > + <body> > + <![CDATA[ > + let browser = (aBrowser || this.mCurrentBrowser); > + if (browser.parentNode.childNodes.length > 1) { > + return browser.parentNode.childNodes[1]; This looks wrong. There could be multiple arbitrary thing in <stack> (browser.parentNode). You need to do something like iterate over all the children, and return the first one that's a warning box. @@ +356,5 @@ > + } > + > + /* > + The full-screen warning container does not yet exist, create a new one. > + The full-screen warning container has the following structure: I want to suggest XBL here, but then I'd be suggesting XBL. :| @@ +395,5 @@ > + let host = browser.currentURI.host ? browser.currentURI.host : null; > + if (host) { > + if (host.length > 40) { > + // Long hostname, limit to first 12 and last 25 characters. > + host = host.slice(0, 12) + "..." + host.slice(host.length - 25); Instead of this, may just want to use eTLD+1. @@ +411,5 @@ > + container.appendChild(hbox); > + container.appendChild(document.createElementNS(xulNS, "spacer")); > + browser.parentNode.appendChild(container); > + > + return browser.parentNode.childNodes[1]; Just "return container;"? ::: modules/libpref/src/init/all.js @@ +3365,5 @@ > pref("full-screen-api.allow-trusted-requests-only", true); > pref("full-screen-api.key-input-restricted", true); > +pref("full-screen-api.warn-on-enter.enabled", true); > +pref("full-screen-api.warn-on-enter.message-duration", 4000); > +pref("full-screen-api.warn-on-enter.browser-fade-duration", 2000); The duration prefs are good for getting a feel or what works and what doesn't, but I'd propose that we remove them before landing. Or figure out what the right ratio is between then, and just have a single pref that we derive both actual timeouts from.

(In reply to Justin Dolske [:Dolske] from comment #35) > We should ponder what the UX should be -- 2 full screen modes are going to > confused users. :) [Can be addressed in a followup, doesn't need to block > this from landing.] Users shouldn't have to be aware there are two full-screen modes. F11 exits DOM-triggered full-screen. Perhaps we should make ESC exit F11-triggered full-screen too.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36) > (In reply to Justin Dolske [:Dolske] from comment #35) > > We should ponder what the UX should be -- 2 full screen modes are going to > > confused users. :) [Can be addressed in a followup, doesn't need to block > > this from landing.] > > Users shouldn't have to be aware there are two full-screen modes. > > F11 exits DOM-triggered full-screen. Perhaps we should make ESC exit > F11-triggered full-screen too. That's the intent. There's a bug filed on it, but I can't find it right now. :(

Comment on attachment 567618 [details] [diff] [review] Patch v5 >--- a/browser/base/content/tabbrowser.xml >+++ b/browser/base/content/tabbrowser.xml >+ <method name="getFullScreenWarningBox"> >+ <parameter name="aBrowser"/> >+ <body> >+ <![CDATA[ >+ let browser = (aBrowser || this.mCurrentBrowser); >+ if (browser.parentNode.childNodes.length > 1) { >+ return browser.parentNode.childNodes[1]; >+ } >+ >+ /* >+ The full-screen warning container does not yet exist, create a new one. >+ The full-screen warning container has the following structure: >+ <vbox mousethrough="always" hidden="true" id="full-screen-warning-container" fadeout="true"> >+ <hbox id="full-screen-warning-message" mousethrough="never"> >+ <description>Press F11 to leave full-screen mode</description> >+ </hbox> >+ </vbox> >+ */ >+ >+ // Create the full-screen warning box. >+ const xulNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"; >+ let container = document.createElementNS(xulNS, "vbox"); >+ container.setAttribute("mousethrough", "always"); >+ container.setAttribute("id", "full-screen-warning-container"); >+ container.setAttribute("align", "center"); >+ >+ let warningBox = document.createElementNS(xulNS, "hbox"); >+ warningBox.setAttribute("id","full-screen-warning-message"); >+ warningBox.setAttribute("mousethrough", "never"); >+ >+ let msg = document.createElementNS(xulNS, "description"); >+ let warningText = this.mStringBundle.getString("full-screen-api.warning.text"); >+ msg.setAttribute("value", warningText); >+ warningBox.setAttribute("align", "center"); >+ warningBox.appendChild(msg); >+ >+ container.appendChild(warningBox); >+ browser.parentNode.appendChild(container); >+ >+ return browser.parentNode.childNodes[1]; >+ ]]> >+ </body> >+ </method> This is pretty ugly. Do we need one full-screen warning box per browser? Why?

The warning box lives in the browserStack, which is where I was told things that want to appear on top of the <browser> should go. Everything from the notificationbox down is recreated every time we add a new tab, so if this lives in the browserStack we need code to dynamically create the warning box anyway. I could add the warning box declaritively further up the tree (say as a sibling to tabbox), but I'd also need to add another xul:stack so that the warning appears on top of the browser and other content - and we already have the browserStack for this purpose. Adding another stack would break anywhere that tabbrowser and elsewhere relies on walking up the tree using parentNode. I think there's only one place where this happens, but I could easily have missed other places, and this could therefore introduce regressions.

Thanks for the quick review. (In reply to Justin Dolske [:Dolske] from comment #35) > Did you test the various combinations of entering+leaving both full-screen > modes? (EG F11-fs + DOM-fs - F11-fs - DOM-fs). Yup, we're all good here. > ::: browser/base/content/tabbrowser.xml > > + <xul:stack flex="1" anonid="browserStack" align="start"> > > What's the align=start for? Ah yes, that's deadwood. Thanks!

(In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #39) > I could add the warning box declaritively further up the tree (say as a > sibling to tabbox), but I'd also need to add another xul:stack so that the > warning appears on top of the browser and other content Or use a negative margin to move the element over the browser, or use a xul:panel...

(In reply to Justin Dolske [:Dolske] from comment #35) > @@ +4081,5 @@ > > + > > + // Partially fade out the browser element for the first few seconds while > > + // displaying the entered-full-screen warning. This helps ensure the > > + // warning is noticed on busy pages. > > + gBrowser.mCurrentBrowser.setAttribute("class", "showing-full-screen-warning"); > > Hmm, I think this would all be simpler by simply having the warning message > have a full-browser semi-transparent background, then there's just 1 thing > to fade out. Or am I missing some bit of cleverness? > > Ah, see you've different timeouts for both. So the message appears with a > subdued browser, then the content fades back to normal, then the message > fades away. I think you could still achieve the same effect with a single > warningbox + background color/texture. > > Probably with just one attribute (or zero?) and CSS with > -moz-transition-delay. We do want to be able to cancel the transitions, redisplay the warning, and restart the timeouts when a restricted keypress happens in full-screen mode, and using timeouts is more reliable way to do that I think.

* Using a xul:panel to contain the warning instead of creating one xul:box per tab. * Added code to reset timeouts and redisplay the warning message (but not obscure the <browser> element) on "showfullscreenwarning" event, which we dispatch on restricted keypress (bug 691583). (This required changes to the code here, so I figured I should pull it into this patch to reduce review overhead.) * Use |this.| in preference to than |FullScreen.|.

Comment on attachment 568246 [details] [diff] [review] Patch v6 >--- a/browser/base/content/tabbrowser.css >+++ b/browser/base/content/tabbrowser.css >+#full-screen-warning-message { >+ background-color: hsl(0,0%,15%); >+ color: white; >+ font-size: 32px; >+ font-family: sans-serif; /* use content font not system UI font */ What's the rationale behind this? This is chrome, not content. If the idea is that this message should somehow integrate with the page, then this won't work anyway, since content can use random non-default fonts. >+ border-radius: 8px; >+ margin-top: 30px; >+ padding-top: 30px; >+ padding-bottom: 30px; >+ padding-left: 50px; >+ padding-right: 50px; >+ box-shadow: 0 0 2px white; >+ text-align: center; >+} >+ >+#full-screen-warning-container { >+ border: 0; >+ padding: 20px; >+} >+ >+#full-screen-warning-container[obscure-browser] { >+ background-color: rgba(0,0,0,0.75); >+} >+ >+#full-screen-warning-container:not([obscure-browser]) { >+ background-color: rgba(0,0,0,0); >+} >+ >+#full-screen-warning-container[stop-obscuring-browser] { >+ -moz-transition-property: background-color; >+ -moz-transition-duration: 500ms; >+ background-color: rgba(0,0,0,0); >+} >+ >+#full-screen-warning-container[fade-warning-out] { >+ -moz-transition-property: opacity; >+ -moz-transition-duration: 500ms; >+ opacity: 0; >+} All of this belongs in browser/themes/*/browser/browser.css >--- a/browser/base/content/tabbrowser.xml >+++ b/browser/base/content/tabbrowser.xml > </xul:notificationbox> >+ <xul:panel mousethrough="always" id="full-screen-warning-container" ignorekeys="true" noautohide="true" noautofocus="true" level="top"> >+ <xul:hbox> >+ <xul:spacer flex="1"/> >+ <xul:hbox id="full-screen-warning-message" mousethrough="never"> >+ <xul:description id="full-screen-warning-text"></xul:description> >+ </xul:hbox> >+ <xul:spacer flex="1"/> >+ </xul:hbox> >+ </xul:panel> > </xul:tabpanels> > </xul:tabbox> Put this in browser.xul (mainPopupSet). >+ <method name="getFullScreenWarningBox"> >+ <parameter name="aBrowser"/> >+ <body> >+ <![CDATA[ >+ return document.getElementById("full-screen-warning-container"); >+ ]]> >+ </body> >+ </method> Tabbrowser doesn't need to implement this. Just call document.getElementById("full-screen-warning-container") in browser.js.

Thanks for your feedback Dão. I've removed the font-face style (purely my ignorance that we didn't set font styles on UI) and addressed other issues you raised.

Comment on attachment 569194 [details] [diff] [review] Patch v6 I'm the cancelling review request, I just noticed there's a slice down the right hand side where the transparent warning panel isn't obscuring the browser.

Comment on attachment 569194 [details] [diff] [review] Patch v6 >+ window.addEventListener("mozfullscreenchange", onMozFullScreenChange, true); >+ window.addEventListener("MozShowFullScreenWarning", onShowFullScreenWarning, true); Should MozShowFullScreenWarning be all-lowercase as well? >+ toggleDomFullScreen : function(event) { >+ if (!document.mozFullScreen) { >+ return; >+ } >+ this.showWarning(true); >+ -----^ trailing spaces >+ // If there's a full-screen toggler, remove its listeners, so that mouseover >+ // the top of the screen will not cause the toolbar to re-appear. >+ let fullScrToggler = document.getElementById("fullscr-toggler"); >+ if (fullScrToggler) { >+ fullScrToggler.removeEventListener("mouseover", this._expandCallback, false); >+ fullScrToggler.removeEventListener("dragenter", this._expandCallback, false); >+ } ----------^ ditto >+ }, >+ ---^ ditto >+ this.warningBox.addEventListener("transitionend", this.onWarningHidden, false); What happens when a theme doesn't set the transition? >+ <panel mousethrough="always" id="full-screen-warning-container" ignorekeys="true" noautohide="true" noautofocus="true" level="top"> >+ <hbox> >+ <spacer flex="1"/> >+ <hbox id="full-screen-warning-message" mousethrough="never"> >+ <description id="full-screen-warning-text"></description> >+ </hbox> >+ <spacer flex="1"/> >+ </hbox> >+ </panel> -----------------^ trailing spaces The warning text should be defined in browser.dtd and be used as an entity directly in browser.xul. The tabbrowser.xml part can go away.

(In reply to Dão Gottwald [:dao] from comment #47) > >+ window.addEventListener("MozShowFullScreenWarning", onShowFullScreenWarning, true); > > Should MozShowFullScreenWarning be all-lowercase as well? No, that's the name of the event suggested by smaug in bug 691583. > >+ this.warningBox.addEventListener("transitionend", this.onWarningHidden, false); > > What happens when a theme doesn't set the transition? What do you suggest we do? Just rely on a timer here instead of a transition? > >+ <panel mousethrough="always" id="full-screen-warning-container" ignorekeys="true" noautohide="true" noautofocus="true" level="top"> It also looks like mousethrough="always" isn't working on the panel. I will investigate further, and I may need to change to another approach here.

(In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #48) > (In reply to Dão Gottwald [:dao] from comment #47) > > >+ window.addEventListener("MozShowFullScreenWarning", onShowFullScreenWarning, true); > > > > Should MozShowFullScreenWarning be all-lowercase as well? > > No, that's the name of the event suggested by smaug in bug 691583. Right, so I'm questioning that suggestion. Is there and if so what is the rule for using camel case rather than lowercase? > > >+ this.warningBox.addEventListener("transitionend", this.onWarningHidden, false); > > > > What happens when a theme doesn't set the transition? > > What do you suggest we do? Just rely on a timer here instead of a transition? You could set the transition in browser/base/content/browser.css.

It seems camel case is the platform convention for internal events.

* Switched to using a box instead of a panel, as the panel wasn't quite taking up width 100%. * Use "pointer-events:none" for warning background to allow page to be interacted with when warning is shown. Warning message itself has "pointer-events: auto", so it blocks mouse interaction with stuff beneath it. * Other comments addressed.

Comment on attachment 569541 [details] [diff] [review] Patch v7 >--- a/browser/base/content/browser.css >+++ b/browser/base/content/browser.css >+#full-screen-warning-container[obscure-browser] { >+ background-color: rgba(0,0,0,0.75); >+} >+ >+#full-screen-warning-container:not([obscure-browser]) { >+ background-color: rgba(0,0,0,0); >+} The second rule looks like a no-op. >+#full-screen-warning-container[stop-obscuring-browser] { >+ -moz-transition-property: background-color; >+ -moz-transition-duration: 500ms; >+ background-color: rgba(0,0,0,0); >+} The browser code only cares about the opacity transition, so this can move to the theme files, right? >--- a/browser/base/content/browser.js >+++ b/browser/base/content/browser.js >+ // Cancel any "hide the toolbar" animation which is in progress, and make >+ // the toolbar hide immediately. >+ clearInterval(this._animationInterval); >+ clearTimeout(this._animationTimeout); >+ this._isAnimating = false; >+ this._shouldAnimate = false; >+ this.mouseoverToggle(false); -------------------------------------^ trailing spaces >+ this.onWarningHidden = >+ function(event) { >+ if (event.propertyName != "opacity") >+ return; nit: too much indentation on the last line >--- a/browser/base/content/browser.xul >+++ b/browser/base/content/browser.xul >+ <hbox id="full-screen-warning-container" hidden="true" fadeout="true"> >+ <hbox style="min-width: 100%;" pack="center"> Presumably the inner hbox is needed because of bug 579776 -- can you add a comment (<!-- -->) for this? >+<!ENTITY domFullScreenWarning.label "Press ESC to leave full-screen"> "leave full-screen" sounds broken to me. Is this proper English? >--- a/browser/locales/en-US/chrome/browser/tabbrowser.properties >+++ b/browser/locales/en-US/chrome/browser/tabbrowser.properties >@@ -15,8 +15,10 @@ tabs.downloading=Downloading⦠> > tabs.emptyTabTitle=New Tab > tabs.closeTab=Close Tab > tabs.close=Close > tabs.closeWarningTitle=Confirm close > tabs.closeWarningMultipleTabs=You are about to close %S tabs. Are you sure you want to continue? > tabs.closeButtonMultiple=Close tabs > tabs.closeWarningPromptMe=Warn me when I attempt to close multiple tabs >+ >+full-screen-api.warning.text=Press ESC to leave full-screen leftover from previous patch >--- a/browser/themes/gnomestripe/browser/browser.css >+++ b/browser/themes/gnomestripe/browser/browser.css >+ padding-top: 30px; >+ padding-bottom: 30px; >+ padding-left: 50px; >+ padding-right: 50px; padding: 30px 50px;

(In reply to Dão Gottwald [:dao] from comment #52) > >+<!ENTITY domFullScreenWarning.label "Press ESC to leave full-screen"> > > "leave full-screen" sounds broken to me. Is this proper English? This is deliberate. I don't want to use the word "exit" for fear of people thinking they'll be exiting the browser completely.

(In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #53) > (In reply to Dão Gottwald [:dao] from comment #52) > > >+<!ENTITY domFullScreenWarning.label "Press ESC to leave full-screen"> > > > > "leave full-screen" sounds broken to me. Is this proper English? > > This is deliberate. I don't want to use the word "exit" for fear of people > thinking they'll be exiting the browser completely. This wasn't my point. I understand "full-screen" as an adjective, as in "full-screen mode". http://en.wiktionary.org/wiki/full_screen seems to confirm this.

Additional review comments addressed. Changed warning to "Press ESC to leave full-screen mode".

Comment on attachment 569875 [details] [diff] [review] Patch v8 >diff --git a/browser/themes/winstripe/browser/browser.css b/browser/themes/winstripe/browser/browser.css >--- a/browser/themes/winstripe/browser/browser.css >+++ b/browser/themes/winstripe/browser/browser.css >+#full-screen-warning-container[fade-warning-out] { >+ -moz-transition-property: opacity; >+ -moz-transition-duration: 500ms; >+ opacity: 0; >+} You seem to have misunderstood me. The *opacity* transition needs to remain in browser/base/content/browser.css, as browser.js depends on it.

Move #full-screen-warning-container[fade-warning-out] style to /browser/base/content/browser.css. I have to make the transition rules !important otherwise the transition doesn't happen.

Comment on attachment 569918 [details] [diff] [review] Patch v9 >+function onMozFullScreenChange(event) { >+ FullScreen.toggleDomFullScreen(event); >+} s/toggleDomFullScreen/enterDomFullScreen/? As I understand it, this method isn't for _leaving_ DOM FS mode. >+ <hbox id="full-screen-warning-container" hidden="true" fadeout="true"> >+ <hbox style="min-width: 100%;" pack="center"> >+ <hbox id="full-screen-warning-message"> <!-- Inner hbox needed due to bug 579776. --> Looks like the comment needs to move one line up.

Thanks Dão. Landed with your comments addressed: https://hg.mozilla.org/integration/mozilla-inbound/rev/64e10f2dfc60