BTW, what should the UI be if the document in fullscreen is on protocols other than http/https? e.g. file, about, chrome. (I guess we probably do not need to display anything for about and chrome, but not sure)

Verdi, could you provide more details about the warning box, mainly the value of paddings, the distance to the top edge.

Verdi, also the distance between three elements (the icon, the text and the button). Those information could be directly updated on your UX design page.

This is a html version impl of the warning box I wrote according to the design. Verdi, could you check this file, and modify it if necessary?

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #1) > BTW, what should the UI be if the document in fullscreen is on protocols > other than http/https? e.g. file, about, chrome. (I guess we probably do not > need to display anything for about and chrome, but not sure) If we allow this API from ftp:// that should prompt. We should allow this from data:text/html pages, but there's no "host" in the data: url. Most of the time the document has a usable http: origin on its principal (we inherit) and we should use that. But sometimes you'll have a "unique origin"--whether because it's a data: bookmarklet or because it's an http://example.com page in a sandboxed iframe!--and we need to do something sensible there. Prompt for sure, but there's no domain we can give. Generic text? We certainly don't want to prompt for "browser full screen" done by chrome code since that's a menu item (and hot key). And since that already doesn't go through this API and has never prompted that's not likely to change anyway. UN-privileged pages, whether file://, about: (some) or resource:// that don't have a host should also prompt, but like iframe-sandbox they won't have a "host" to display. Generic text, again? For the generic text "_This_document_ is now fullscreen" would be misleading, because it might be a frame or an element in a frame from a different domain altogether. Maybe simply "$ShortBrand is now fullscreen [ESC to exit]" is the best we can do -- it's accurate if not very informative, but at least it's not misleading. Perhaps an algorithm like this: if (chrome privileged) // not "chrome://" url, the privilege level skip prompt; else if (principal's URL has a host) use the prompt in this bug; else use the generic text with no lock icon;

(In reply to Daniel Veditz [:dveditz] from comment #5) > (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #1) > > BTW, what should the UI be if the document in fullscreen is on protocols > > other than http/https? e.g. file, about, chrome. (I guess we probably do not > > need to display anything for about and chrome, but not sure) > > If we allow this API from ftp:// that should prompt. Do we allow arbitrary JavaScript to run on that scheme? > We certainly don't want to prompt for "browser full screen" done by chrome > code since that's a menu item (and hot key). And since that already doesn't > go through this API and has never prompted that's not likely to change > anyway. Yes, but other privileged code, like addons or privileged apps, may also use the Fullscreen API, which I guess we do not need to show the prompt either. > For the generic text "_This_document_ is now fullscreen" would be > misleading, because it might be a frame or an element in a frame from a > different domain altogether. Maybe simply "$ShortBrand is now fullscreen > [ESC to exit]" is the best we can do -- it's accurate if not very > informative, but at least it's not misleading. I'm OK either way. But please notice that, the outer document is always able to detect any fullscreen change inside it, and is always able to display something on the top without triggering an origin-change notification (hence no further prompt is shown). For impl without top layer (our current behavior), the outer document can just put another fixed element with z-order 2147483647 at the end of the document. For impl with top layer and <dialog> (in the future), it can show a modal <dialog> element covers the whole viewport. > Perhaps an algorithm like this: > if (chrome privileged) // not "chrome://" url, the privilege level > skip prompt; > else if (principal's URL has a host) > use the prompt in this bug; > else > use the generic text with no lock icon; BTW, what's lock icon do you think should be shown in different cases? It seems our design here have a slightly different icon set with the control center lock icons. (Lock with a slash in control center is only for content with mixed content blocker disabled, but it seems we are going to use similar icon here for insecure connections.)

Hi Xidorn, The HTML version looks pretty great. I'm attaching a better visual guide. I think you have everything but blurring the background of the underlying layer plus hover and pressed states for the button. Also I think we should remove the spread-radius from the drop shadow.

(In reply to Verdi [:verdi] from comment #7) > Created attachment 8641949 [details] > fullscreen-video-spec.jpg > > Hi Xidorn, > The HTML version looks pretty great. I'm attaching a better visual guide. I > think you have everything but blurring the background of the underlying > layer plus hover and pressed states for the button. Also I think we should > remove the spread-radius from the drop shadow. Blurring the underlaying layer is not feasible at present. We need the CSS property backdrop-filter, which hasn't been implemented yet. I'd prefer not wait for that at present.

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #8) > > Blurring the underlaying layer is not feasible at present. We need the CSS > property backdrop-filter, which hasn't been implemented yet. I'd prefer not > wait for that at present. Got it. Agreed - let's not wait for that.

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #8) > Blurring the underlaying layer is not feasible at present. We need the CSS > property backdrop-filter, which hasn't been implemented yet. I'd prefer not > wait for that at present. If/when we look at this again, we'll want to be mindful of performance. That was one of the reasons for removing the blur from tab-modal prompts in bug 613714.

(In reply to Justin Dolske [:Dolske] from comment #10) > (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #8) > > > Blurring the underlaying layer is not feasible at present. We need the CSS > > property backdrop-filter, which hasn't been implemented yet. I'd prefer not > > wait for that at present. > > If/when we look at this again, we'll want to be mindful of performance. That > was one of the reasons for removing the blur from tab-modal prompts in bug > 613714. Yes... but it seems to me the performance issue there was mainly because of applying blur to the whole viewport. Only applying to a small area should be fine, I guess. But yes, we definitely should mind the negative effect to the performance.

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #6) > > We certainly don't want to prompt for "browser full screen" done by chrome > > code since that's a menu item (and hot key). And since that already doesn't > > go through this API and has never prompted that's not likely to change > > anyway. > > Yes, but other privileged code, like addons or privileged apps, may also use > the Fullscreen API, which I guess we do not need to show the prompt either. > Would a check for system principal work for this? That would probably include chrome:// pages too though, which Dan called out earlier. Dan, is that okay? We control chrome: pages. > > For the generic text "_This_document_ is now fullscreen" would be > > misleading, because it might be a frame or an element in a frame from a > > different domain altogether. Maybe simply "$ShortBrand is now fullscreen > > [ESC to exit]" is the best we can do -- it's accurate if not very > > informative, but at least it's not misleading. > > I'm OK either way. But please notice that, the outer document is always able > to detect any fullscreen change inside it, and is always able to display > something on the top without triggering an origin-change notification (hence > no further prompt is shown). > But the outer document probably isn't checking. Can we tell the user which inner frame is going full screen? Is generic text being proposed because we can't get the actual hostname that's going full screen?

(In reply to Tanvi Vyas [:tanvi] from comment #12) > Would a check for system principal work for this? That would probably > include chrome:// pages too though, which Dan called out earlier. Dan, is > that okay? We control chrome: pages. We currently only passed the origin to the UI code. But I think yes we should be able to provide any information if necessary, as far as the information is accessible from script code. (I believe most of the principal attributes are accessible there) > But the outer document probably isn't checking. Can we tell the user which > inner frame is going full screen? Is generic text being proposed because we > can't get the actual hostname that's going full screen? Accurately, we tell the user which origin requests this fullscreen change. However, if the document requests it is an inner document, the outer document is able to detect and cover it anyway, which I believe is an existing vulnerability for us. (Not sure how other impls handle this) Since the outer document is able to: 1. control whether an subdocument can request fullscreen (via allowfullscreen attribute on iframe); 2. use its own content to cover the subdocument; 3. exit any fullscreen requested by its subdocument. I propose that we probably should always show the origin and icon according to the outmost document, instead of the document which requests the fullscreen change (which we currently do).

(In reply to Tanvi Vyas [:tanvi] from comment #12) > But the outer document probably isn't checking. Can we tell the user which > inner frame is going full screen? Is generic text being proposed because we > can't get the actual hostname that's going full screen? Note that, the inner document is allowed to enter fullscreen only if the outer document explicitly set "allowfullscreen" attribute on it before loading the inner document. Thus I think we can show "_This_document_ is now fullscreen" as the generic text. It is not that misleading. (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #13) > Accurately, we tell the user which origin requests this fullscreen change. > However, if the document requests it is an inner document, the outer > document is able to detect and cover it anyway, which I believe is an > existing vulnerability for us. (Not sure how other impls handle this) As far as I check, all impls currently have similar behavior on the prompt which shows the origin which requests the fullscreen change. But IE/Edge is not vulnerable because they implemented top layer without having <dialog>, so that outer document is not able to use its own content to cover the subdocument in fullscreen.

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #14) > As far as I check, all impls currently have similar behavior on the prompt > which shows the origin which requests the fullscreen change. But IE/Edge is > not vulnerable because they implemented top layer without having <dialog>, > so that outer document is not able to use its own content to cover the > subdocument in fullscreen. Probably it is not a real vulnerability... We just want the user to know that someone has controlled the whole screen. It doesn't matter who it actually is.

It seems we are currently using green lock everywhere now, no gray lock anymore. So I guess that we just use white lock here, with and without the slash for http and https respectively?

(In reply to Verdi [:verdi] from comment #7) > The HTML version looks pretty great. I'm attaching a better visual guide. I > think you have everything but blurring the background of the underlying > layer plus hover and pressed states for the button. Also I think we should > remove the spread-radius from the drop shadow. It seems that the XUL button has some builtin effect on hover and pressed state, which I fail to override. So let's leave it as-is.

Thanks for the clarifications and your work on this Xidorn! (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #16) > It seems we are currently using green lock everywhere now, no gray lock > anymore. > > So I guess that we just use white lock here, with and without the slash for > http and https respectively? Yes, just the two states sound good. The generic text when an iframe is going into fullscreen mode also sounds good. I would show the lock only when both the outer document and inner document are https. If any document in the chain is http, we should show the lock with the strikethrough. You may have to go all the way up the document chain to determine this since there may be multiple levels of framing.

(In reply to Tanvi Vyas [:tanvi] from comment #18) > The generic text when an iframe is going into fullscreen mode also sounds > good. I would show the lock only when both the outer document and inner > document are https. If any document in the chain is http, we should show > the lock with the strikethrough. You may have to go all the way up the > document chain to determine this since there may be multiple levels of > framing. Climbing the document chain is probably non-trivial. I guess I could just steal the state from the control center. I believe if any level in the chain is http, the control center should either show the whole document as "insecure" or "mixed-content-blocker disabled". It could be false negative when the insecure warning is caused by a different subdocument, but I guess it's fine. Agree?

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #19) > (In reply to Tanvi Vyas [:tanvi] from comment #18) > > The generic text when an iframe is going into fullscreen mode also sounds > > good. I would show the lock only when both the outer document and inner > > document are https. If any document in the chain is http, we should show > > the lock with the strikethrough. You may have to go all the way up the > > document chain to determine this since there may be multiple levels of > > framing. > > Climbing the document chain is probably non-trivial. I guess I could just > steal the state from the control center. > > I believe if any level in the chain is http, the control center should > either show the whole document as "insecure" or "mixed-content-blocker > disabled". You are right that the control center should mostly have the right information. So you can use the security state set by the control center to determine what icon to show here. * If the top level is http, security state of the top level document will be insecure regardless of whether an https iframe exists. * If the top level is https and any http iframe has loaded, the security state will be broken (nsIWebProgressListener::STATE_IS_BROKEN) regardless of whether an https iframe exists. > It could be false negative when the insecure warning is caused by > a different subdocument, but I guess it's fine. Agree? If another resource caused the insecure warning, the top level page is potentially compromised anyway, so we should just treat it as insecure. Thanks!

Bug 1160023 part 1 - Put fullscreen warning box related logic into a nested object.

Bug 1160023 part 2 - Add Timeout object to simplify timeout managing code.

Bug 1160023 part 3 - Merge fullscreen warning box theme style and resource into the shared directory.

Bug 1160023 part 4 - Some code movement and style fix in fullscreen warning code.

Bug 1160023 part 5 - Implement the new fullscreen warning box.

Comment on attachment 8642805 [details] MozReview Request: Bug 1160023 part 2 - Add Timeout object to simplify timeout managing code. I'm not sure if this is an API we'd want to add to the global scope. Do you think it's going to be handy for a good number of consumers? In order to not get sidetracked it might make sense to just drop this change from this bug for the time being.

(In reply to Dão Gottwald [:dao] from comment #26) > Comment on attachment 8642805 [details] > MozReview Request: Bug 1160023 part 2 - Add Timeout object to simplify > timeout managing code. > > I'm not sure if this is an API we'd want to add to the global scope. Do you > think it's going to be handy for a good number of consumers? In order to not > get sidetracked it might make sense to just drop this change from this bug > for the time being. It could be useful. Have a quick search at "clearTimeout" in browser/ directory, it seems to me a bunch of those usecase can be simplified with the Timeout object, like [1], [2], and [3] (some of them may need additional method to the object, though) I find it makes things clearer and less error-prone. If you are not comfortable with adding it to global scope, I can move it into FullScreen, though. [1] https://dxr.allizom.org/mozilla-central/source/browser/base/content/browser-feeds.js?offset=3600#122-123,166-168 [2] https://dxr.allizom.org/mozilla-central/source/browser/base/content/browser-ctrlTab.js?offset=400#291-295,354-357 [3] https://dxr.allizom.org/mozilla-central/source/browser/base/content/tabbrowser.xml#3087,3311,3395-3396

I'm not against it per se, but I think it would make sense to move that to a separate bug where you can also demonstrate the usefulness better by updating some of the other cases you found to use that API.

(In reply to Dão Gottwald [:dao] from comment #28) > I'm not against it per se, but I think it would make sense to move that to a > separate bug where you can also demonstrate the usefulness better by > updating some of the other cases you found to use that API. OK, make sense. Filed bug 1191193.

(In reply to Verdi [:verdi] from comment #9) > (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #8) > > > > Blurring the underlaying layer is not feasible at present. We need the CSS > > property backdrop-filter, which hasn't been implemented yet. I'd prefer not > > wait for that at present. > > Got it. Agreed - let's not wait for that. Hi Xidorn - I just realized that if we don't blur the background the notification will need to be less transparent. .9 will work better than the original .5

Probably not so much useful for other cases in existing code. I'm going to make it not block at that bug, and just move that object into FullScreen for now.

(In reply to Verdi [:verdi] from comment #30) > Hi Xidorn - I just realized that if we don't blur the background the > notification will need to be less transparent. .9 will work better than the > original .5 OK, got it.

Comment on attachment 8642804 [details] MozReview Request: Bug 1160023 part 1 - Put fullscreen warning box related logic into a nested object. r=dao Bug 1160023 part 1 - Put fullscreen warning box related logic into a nested object. r=dao

Comment on attachment 8642805 [details] MozReview Request: Bug 1160023 part 2 - Add Timeout object to simplify timeout managing code. Bug 1160023 part 2 - Add Timeout object to simplify timeout managing code.

Comment on attachment 8642806 [details] MozReview Request: Bug 1160023 part 3 - Merge fullscreen warning box theme style and resource into the shared directory. r=dao Bug 1160023 part 3 - Merge fullscreen warning box theme style and resource into the shared directory. r=dao

Comment on attachment 8642807 [details] MozReview Request: Bug 1160023 part 4 - Some code movement and style fix in fullscreen warning code. r=dao Bug 1160023 part 4 - Some code movement and style fix in fullscreen warning code.

Comment on attachment 8642808 [details] MozReview Request: Bug 1160023 part 5 - Implement the new fullscreen warning box. Bug 1160023 part 5 - Implement the new fullscreen warning box.

:dao, sorry about restore the review requests on all patches. I think it'd probably better to use the "Ship it" button in the reviewboard instead of marking r+ in bugzilla.

I didn't have a good experience with reviewboard so far, it decreased my productivity rather than helping, so I'm not going to use it at this time.

RE patch 2, are you sure we can use ES6 Classes in the browser code? I think we should be able to, but I didn't see any usage of that in our codebase currently except the js engine tests.

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #40) > RE patch 2, are you sure we can use ES6 Classes in the browser code? I think > we should be able to, but I didn't see any usage of that in our codebase > currently except the js engine tests. I don't know, I assumed you tested it and it worked. It might also be a good idea to look if this feature is behind a pref and only enabled in pre-release builds.

(In reply to Dão Gottwald [:dao] from comment #41) > I don't know, I assumed you tested it and it worked. It might also be a good > idea to look if this feature is behind a pref and only enabled in > pre-release builds. I tested, and it works. I check the code and didn't find pref about that. Hence I guess it is okay to use. I asked on IRC but no one answered whether I can use... Probably should ask people working on this whether this part is stable enough when it works. Jason: do you think our implementation of ES6 classes is currently stable enough to be used in the browser code? Is this feature behind any pref which only enabled in pre-release builds? It seems to work fine, but I don't see we are using it anywhere else.

Can somebody sum up the reasoning behind having an "Exit full screen" button in this popup? I'm not sure what significant value it adds. And it seems like a somewhat problematic UI; if the user wants to interact with the page, we might falsely interpret this as trying to reach the button, thus showing the warning longer than wanted, directly sabotaging the user's intent.

(In reply to Dão Gottwald [:dao] from comment #43) > Can somebody sum up the reasoning behind having an "Exit full screen" button > in this popup? I'm not sure what significant value it adds. And it seems > like a somewhat problematic UI; if the user wants to interact with the page, > we might falsely interpret this as trying to reach the button, thus showing > the warning longer than wanted, directly sabotaging the user's intent. It makes it possible to exit fullscreen without a keyboard. (See bug 1140819)

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #44) > (In reply to Dão Gottwald [:dao] from comment #43) > > Can somebody sum up the reasoning behind having an "Exit full screen" button > > in this popup? I'm not sure what significant value it adds. And it seems > > like a somewhat problematic UI; if the user wants to interact with the page, > > we might falsely interpret this as trying to reach the button, thus showing > > the warning longer than wanted, directly sabotaging the user's intent. > > It makes it possible to exit fullscreen without a keyboard. (See bug 1140819) It doesn't seem actually help with bug 1140819 comment 0, since that is about exiting fullscreen after you've been there for a while rather than from the warning box. Also, on tablets your mouse movement detection isn't going to work well, is it? So a user thinking about exiting from the popup would race with the timeout.

I'm also a bit skeptical about the lock icon. In order to build trust, I would generally expect this kind of indicator to be clickable or at the very least have a tooltip, but this would again be problematic given the transient nature of the popup.

(In reply to Dão Gottwald [:dao] from comment #45) > It doesn't seem actually help with bug 1140819 comment 0, since that is > about exiting fullscreen after you've been there for a while rather than > from the warning box. Also, on tablets your mouse movement detection isn't > going to work well, is it? So a user thinking about exiting from the popup > would race with the timeout. They can make it shown again via moving the pointer to the top. (The design is, to detect the swipe down gesture and display the box on the top [1], but I'm not quite sure whether it could work because I don't have a tablet myself to test... I have filed bug 1190737 for that.) Also I believe the button is a more straightforward way for user to exit fullscreen. [1] http://people.mozilla.org/~mverdi/projects/fullscreen/#accessible-status-and-exit

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #47) > (In reply to Dão Gottwald [:dao] from comment #45) > > It doesn't seem actually help with bug 1140819 comment 0, since that is > > about exiting fullscreen after you've been there for a while rather than > > from the warning box. Also, on tablets your mouse movement detection isn't > > going to work well, is it? So a user thinking about exiting from the popup > > would race with the timeout. > > They can make it shown again via moving the pointer to the top. Does this work for sites capturing the mouse such as games? If it does, it seems annoying for these kind of use cases. If it doesn't, it's not a complete solution here. > (The design > is, to detect the swipe down gesture and display the box on the top [1], but > I'm not quite sure whether it could work because I don't have a tablet > myself to test... I have filed bug 1190737 for that.) Maybe we should only display a button when actually detecting such a gesture?

(In reply to Dão Gottwald [:dao] from comment #48) > (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #47) > > (In reply to Dão Gottwald [:dao] from comment #45) > > > It doesn't seem actually help with bug 1140819 comment 0, since that is > > > about exiting fullscreen after you've been there for a while rather than > > > from the warning box. Also, on tablets your mouse movement detection isn't > > > going to work well, is it? So a user thinking about exiting from the popup > > > would race with the timeout. > > > > They can make it shown again via moving the pointer to the top. > > Does this work for sites capturing the mouse such as games? If it does, it > seems annoying for these kind of use cases. If it doesn't, it's not a > complete solution here. No, currently it doesn't. This is probably an issue... > > (The design > > is, to detect the swipe down gesture and display the box on the top [1], but > > I'm not quite sure whether it could work because I don't have a tablet > > myself to test... I have filed bug 1190737 for that.) > > Maybe we should only display a button when actually detecting such a gesture? If we display a button there, we should always do so... Sometimes having button but sometimes not seems to be confusing...

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #49) > > > (The design > > > is, to detect the swipe down gesture and display the box on the top [1], but > > > I'm not quite sure whether it could work because I don't have a tablet > > > myself to test... I have filed bug 1190737 for that.) > > > > Maybe we should only display a button when actually detecting such a gesture? > > If we display a button there, we should always do so... Sometimes having > button but sometimes not seems to be confusing... If we take that route I wouldn't conflate the close button with the warning at all. Sliding in a close button in an upper corner should be sufficient.

I don't want to discuss about the UI design anymore here... I'm just implementing the design from Verdi [1], and I think this design is fine. If you have any better idea, could you discuss that with him? [1] http://people.mozilla.org/~mverdi/projects/fullscreen/#proposal

For whoever interested in the new popup, here's the try build: Linux 32bit: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/xquan@mozilla.com-c06b6abf8461/try-linux/ Linux 64bit: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/xquan@mozilla.com-c06b6abf8461/try-linux64/ OS X: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/xquan@mozilla.com-c06b6abf8461/try-macosx64/ Windows 32bit: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/xquan@mozilla.com-c06b6abf8461/try-win32/ Windows 64bit: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/xquan@mozilla.com-c06b6abf8461/try-win64/ Verdi, could you verify this is what you designed? Tanvi, could you confirm that you and the security team are happy with this?

I tested the Mac build posted and it appears to match the design from UX as approved by Sec Team: http://people.mozilla.org/~mverdi/projects/fullscreen/#proposal Nice job! As we discussed at length during the design phase, we agreed to get the proposed design implemented and in Nightly before we tweak the settings further. I now think that the fade-in/out transition can be quicker, but I will hold my tongue until after this gets into Nightly :)

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #53) > > Verdi, could you verify this is what you designed? > I got this to work on a Windows 8 VM and I think it's right (I had a really low frame rate for some reason so, hard to tell). I'll try again at home on a real machine. Strangely I still get the old notification on my Mac.

(In reply to Jet Villegas (:jet) from comment #54) > As we discussed at length during the design phase, we agreed to get the > proposed design implemented and in Nightly before we tweak the settings > further. This is awesome! I tested this on my Windows 10 machine and it works as designed. There are things that are not as smooth as I'm sure everyone would like but I bet those all fall into the category of "tweak the settings further" once it's implemented.

verdi, could you please specifically answer the questions that came up in comment 43 and below? Thanks!

(In reply to Dão Gottwald [:dao] from comment #57) > verdi, could you please specifically answer the questions that came up in > comment 43 and below? Thanks! The main security concern with full screen is a phishing attack. To combat this, we make it obvious that something is happening when the user triggers full screen (transition), we inform the user about what just happened (message about being in full screen + connection status) and we give the user an easy way to exit (close button, keyboard command, swipe down from top gesture, game controller button). So including a button is one of the ways to provide an easy exit. For example, if you are using your mouse to trigger full screen, it may be more convenient to click an another on-screen button to exit instead of reaching for the keyboard. This does not interfere with interacting with the content. Try out Xidorn’s build in comment 53. The swipe down from the top gesture (rather than the mouse to the top) is there for tablet users. The lock icon (or other status icon) is there to let the user know the status of the content going full screen. But the full screen interface is not meant to nor should it be a reimplementation of the full interface. If the user has questions about the site and the connection status they will have to exit full screen to answer them. Likewise, for granular device permissions (use this microphone not that microphone) they will have to exit full screen and use the control center (or doorhanger). More reference: https://bugzilla.mozilla.org/show_bug.cgi?id=1129061#c2 http://people.mozilla.org/~mverdi/projects/fullscreen

Looks good to me. I get the desire to make the transition faster but when I made the transition-duration prefs shorter it was more of a jarring "blink" effect than a fade. (tested on a Mac).

Comment 59 was supposed to be an OK from the security team. I think that was the point of the needinfo? to tanvi so I'm clearing that so we don't hold you up any further.

(In reply to Daniel Veditz [:dveditz] from comment #60) > Comment 59 was supposed to be an OK from the security team. I think that was > the point of the needinfo? to tanvi so I'm clearing that so we don't hold > you up any further. Oh, yes, that's right. Thanks.

Comment on attachment 8642808 [details] MozReview Request: Bug 1160023 part 5 - Implement the new fullscreen warning box. > #fullscreen-domain-text, #fullscreen-generic-text { nit: New line after the comma (here and throughout this patch) > #fullscreen-generic-text { > display: none; > } > #fullscreen-domain-text[hidden] + #fullscreen-generic-text { > display: block; > } Use this rule instead: #fullscreen-domain-text:not([hidden]) + #fullscreen-generic-text { display: none; } > }, gPrefService.getIntPref("full-screen-api.warning.timeout", 3000)); getIntPref doesn't take two parameters > window.addEventListener("mousemove", this, true); Would it make sense to use MousePosTracker instead? Seems like a fitting use case to me. > #fullscreen-exit-button { > background: #fafafa; > border: 1px solid #ccc; [...] I don't think this button styling even works, can you remove it?

(In reply to Dão Gottwald [:dao] from comment #62) > Comment on attachment 8642808 [details] > MozReview Request: Bug 1160023 part 5 - Implement the new fullscreen warning > box. > > > window.addEventListener("mousemove", this, true); > > Would it make sense to use MousePosTracker instead? Seems like a fitting use > case to me. I would say no. The main reason behind that is: if the user continues moving the mouse, we probably don't want to show or hide the notification box after the delay, which means we want the mousemove events instead of the mouseenter/leave events. In addition, I feel that listening to mousemove event ourselves here is more efficient, as there is no additional function call for the target rect, and we don't need to check the horizontal-axis positions.

Comment on attachment 8642804 [details] MozReview Request: Bug 1160023 part 1 - Put fullscreen warning box related logic into a nested object. r=dao Bug 1160023 part 1 - Put fullscreen warning box related logic into a nested object. r=dao

Comment on attachment 8642805 [details] MozReview Request: Bug 1160023 part 2 - Add Timeout object to simplify timeout managing code. Bug 1160023 part 2 - Add Timeout object to simplify timeout managing code. r=dao

Comment on attachment 8642806 [details] MozReview Request: Bug 1160023 part 3 - Merge fullscreen warning box theme style and resource into the shared directory. r=dao Bug 1160023 part 3 - Merge fullscreen warning box theme style and resource into the shared directory. r=dao

Comment on attachment 8642807 [details] MozReview Request: Bug 1160023 part 4 - Some code movement and style fix in fullscreen warning code. r=dao Bug 1160023 part 4 - Some code movement and style fix in fullscreen warning code. r=dao

Comment on attachment 8642808 [details] MozReview Request: Bug 1160023 part 5 - Implement the new fullscreen warning box. Bug 1160023 part 5 - Implement the new fullscreen warning box.

You can use https://reviewboard.mozilla.org/r/14847/diff/2-3/ to see the difference between this revision and the last one.

Eric, probably you are a better person for my question in comment 42.

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #63) > > Would it make sense to use MousePosTracker instead? Seems like a fitting use > > case to me. > > I would say no. The main reason behind that is: if the user continues moving > the mouse, we probably don't want to show or hide the notification box after > the delay, which means we want the mousemove events instead of the > mouseenter/leave events. MousePosTracker doesn't use mouseenter/leave events. It just tells you when entering or leaving a rect that you provide. > In addition, I feel that listening to mousemove event ourselves here is more > efficient, as there is no additional function call for the target rect, By that logic you shouldn't have abstracted away setTimeout and friends either. Overhead of a JS function call is close to zero; this isn't something we should optimize for. > and we don't need to check the horizontal-axis positions. Similarly, this doesn't sound like a big deal.

(In reply to Dão Gottwald [:dao] from comment #71) > (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #63) > > > Would it make sense to use MousePosTracker instead? Seems like a fitting use > > > case to me. > > > > I would say no. The main reason behind that is: if the user continues moving > > the mouse, we probably don't want to show or hide the notification box after > > the delay, which means we want the mousemove events instead of the > > mouseenter/leave events. > > MousePosTracker doesn't use mouseenter/leave events. It just tells you when > entering or leaving a rect that you provide. I meant, we want to catch mousemoves here instead of only being told when the mouse enters or leaves the area.

I don't think that basing stuff on classes is a good idea just yet. The main problem is that it's Nightly Only. There are a few blockers left, most of which we hope to land in 43. That's a vague target, though. The last piece blocking the release of classes (global level lexical scoped variable declarations) is still in development, and it's unclear if it'll make the train for 43.

I agree. There's a good amount of work left: https://bugzilla.mozilla.org/showdependencytree.cgi?id=837314&hide_resolved=1 so it is possible classes could slip past FF43. To my mind the biggest risk is this: ES6 class-bindings are let-bindings, so we cannot ship classes without first making let and const ES6-compatible (bug 950547). And that is risky, because we've been using let and const in Firefox, with pre-ES6 semantics, for years. See bug 1001090 where Shu-yu Guo already had to fix up tons of existing code to work correctly when he changed the behavior of `let` to be closer to the ES6 standard.

Comment on attachment 8642804 [details] MozReview Request: Bug 1160023 part 1 - Put fullscreen warning box related logic into a nested object. r=dao Bug 1160023 part 1 - Put fullscreen warning box related logic into a nested object. r=dao

Comment on attachment 8642805 [details] MozReview Request: Bug 1160023 part 2 - Add Timeout object to simplify timeout managing code. Bug 1160023 part 2 - Add Timeout object to simplify timeout managing code.

Comment on attachment 8642806 [details] MozReview Request: Bug 1160023 part 3 - Merge fullscreen warning box theme style and resource into the shared directory. r=dao Bug 1160023 part 3 - Merge fullscreen warning box theme style and resource into the shared directory. r=dao

Comment on attachment 8642807 [details] MozReview Request: Bug 1160023 part 4 - Some code movement and style fix in fullscreen warning code. r=dao Bug 1160023 part 4 - Some code movement and style fix in fullscreen warning code. r=dao

Comment on attachment 8642808 [details] MozReview Request: Bug 1160023 part 5 - Implement the new fullscreen warning box. Bug 1160023 part 5 - Implement the new fullscreen warning box.

Comment on attachment 8642808 [details] MozReview Request: Bug 1160023 part 5 - Implement the new fullscreen warning box. > <!-- The "escape" button on Mac keyboards is lowercase --> Please use the LOCALIZATION NOTE comment format.

url: https://hg.mozilla.org/integration/fx-team/rev/8ff2e6a39c9fd408b70d49d64fd2ffb0a2630f30 changeset: 8ff2e6a39c9fd408b70d49d64fd2ffb0a2630f30 user: Xidorn Quan <quanxunzhen@gmail.com> date: Thu Aug 13 22:42:06 2015 +1000 description: Bug 1160023 part 1 - Put fullscreen warning box related logic into a nested object. r=dao url: https://hg.mozilla.org/integration/fx-team/rev/d097272c0eed3b8852e98341b989bd2afb25fcf1 changeset: d097272c0eed3b8852e98341b989bd2afb25fcf1 user: Xidorn Quan <quanxunzhen@gmail.com> date: Thu Aug 13 22:42:06 2015 +1000 description: Bug 1160023 part 2 - Add Timeout object to simplify timeout managing code. r=dao url: https://hg.mozilla.org/integration/fx-team/rev/5f36745dc6fc49cf1bf68358fee988b77840911c changeset: 5f36745dc6fc49cf1bf68358fee988b77840911c user: Xidorn Quan <quanxunzhen@gmail.com> date: Thu Aug 13 22:42:06 2015 +1000 description: Bug 1160023 part 3 - Merge fullscreen warning box theme style and resource into the shared directory. r=dao url: https://hg.mozilla.org/integration/fx-team/rev/6ac9e967deff608212f58cea2a20668c9d78b100 changeset: 6ac9e967deff608212f58cea2a20668c9d78b100 user: Xidorn Quan <quanxunzhen@gmail.com> date: Thu Aug 13 22:42:06 2015 +1000 description: Bug 1160023 part 4 - Some code movement and style fix in fullscreen warning code. r=dao url: https://hg.mozilla.org/integration/fx-team/rev/b84298274cba324b702b7761aaaf22d334d65add changeset: b84298274cba324b702b7761aaaf22d334d65add user: Xidorn Quan <quanxunzhen@gmail.com> date: Thu Aug 13 22:42:06 2015 +1000 description: Bug 1160023 part 5 - Implement the new fullscreen warning box. r=dao

It seems this patch causes a wide range of chrome performance regression. Probably because the new box does not use "display:none" when hidden?

https://hg.mozilla.org/mozilla-central/rev/8ff2e6a39c9f https://hg.mozilla.org/mozilla-central/rev/d097272c0eed https://hg.mozilla.org/mozilla-central/rev/5f36745dc6fc https://hg.mozilla.org/mozilla-central/rev/6ac9e967deff https://hg.mozilla.org/mozilla-central/rev/b84298274cba

This should fix the performance regression by this bug. This patch returns to using "display: none" for hidden fullscreen warning box instead if "visibility: hidden". The original impl uses visibility because "display: none" would cause the transition not to happen in some cases. In this patch, nested requestAnimationFrame calls are used to workaround this issue.

Comment on attachment 8647851 [details] [diff] [review] followup patch to fix performance regression > #fullscreen-warning[hidden] { >- /* To ensure the transition always works fine, never change >- the value of display property. */ >- display: flex; >- visibility: hidden !important; >- transition: initial; >- pointer-events: none !important; >+ display: none; > } display:none is the default here. You should just remove this rule.

(In reply to Dão Gottwald [:dao] from comment #86) > Comment on attachment 8647851 [details] [diff] [review] > followup patch to fix performance regression > > > #fullscreen-warning[hidden] { > >- /* To ensure the transition always works fine, never change > >- the value of display property. */ > >- display: flex; > >- visibility: hidden !important; > >- transition: initial; > >- pointer-events: none !important; > >+ display: none; > > } > > display:none is the default here. You should just remove this rule. That won't work, we explicitly specify a display value for #fullscreen-warning, which suppress hidden attribute. [1] [1] https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/hidden

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #87) > That won't work, we explicitly specify a display value for > #fullscreen-warning, which suppress hidden attribute. [1] Can you use #fullscreen-warning:not([hidden]) there?

(In reply to Dão Gottwald [:dao] from comment #88) > (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #87) > > That won't work, we explicitly specify a display value for > > #fullscreen-warning, which suppress hidden attribute. [1] > > Can you use #fullscreen-warning:not([hidden]) there? OK, but I'm curious if there is any reason behind that suggestion?

Just that it allows us to get rid of that other rule for [hiddden].

url: https://hg.mozilla.org/integration/fx-team/rev/9fc432621b17fe4853323927d211b6928497ae97 changeset: 9fc432621b17fe4853323927d211b6928497ae97 user: Xidorn Quan <quanxunzhen@gmail.com> date: Fri Aug 14 20:23:32 2015 +1000 description: Bug 1160023 followup - Avoid constructing box for hidden fullscreen warning. r=dao

url: https://hg.mozilla.org/integration/fx-team/rev/e2ba843de4ca45289d48c5af4feab7fccd3dd131 changeset: e2ba843de4ca45289d48c5af4feab7fccd3dd131 user: Xidorn Quan <quanxunzhen@gmail.com> date: Fri Aug 14 22:39:59 2015 +1000 description: Bug 1160023 followup 2 - Fix test failure on browser_domFullscreen_fullscreenMode.js.

https://hg.mozilla.org/mozilla-central/rev/9fc432621b17 https://hg.mozilla.org/mozilla-central/rev/e2ba843de4ca

Hey Vaida Can you help me to verified the fix? From the title of the bug and a attachment, I have understand that, when I make a video full screen, the popup design is changed. Please let me know if I am wrong or right ? :) .

(In reply to Ashickur Rahman from comment #94) > Hey Vaida > > Can you help me to verified the fix? From the title of the bug and a > attachment, I have understand that, when I make a video full screen, the > popup design is changed. Please let me know if I am wrong or right ? :) . Hi Ashickur! That's right, but there's someone already working on verifying this fix :(. May I suggest you check out the following Firefox 43 query we have in place for [good first verify] bug fixes? Here's the link: https://goo.gl/RkMSb3.

We tested this across platforms (Windows 7 64-bit, Mac OS X 10.11.1 and Ubuntu 14.04 32-bit) using latest Firefox 43 beta 5 and found only one new issue (bug 1227129). I'm gonna close this bug as verified fixed based on our testing.