Closed Bug 545812 Opened 15 years ago Closed 13 years ago

API for controlling fullscreen from content

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: paul, Assigned: cpearce)

References

(Depends on 14 open bugs, Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, Whiteboard: [evang-wanted][secr:curtisk])

Attachments

(8 files, 22 obsolete files)

(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
cpearce
: review+
Details | Diff | Splinter Review
(deleted), patch
jst
: review+
Details | Diff | Splinter Review
(deleted), patch
cpearce
: review+
Details | Diff | Splinter Review
(deleted), patch
smaug
: review+
Details | Diff | Splinter Review
(deleted), patch
cpearce
: review+
Details | Diff | Splinter Review
(deleted), patch
jst
: review+
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
Webkit does it. Do we want to provide the same feature? Feature requested by: http://blog.jilion.com/2010/02/11/sublimevideo-supports-firefox
It's not really alt-click to fullscreen, they're using some WebKit specific methods to support fullscreen from script (http://trac.webkit.org/changeset/50893). There's a discussion on the WhatWG mailing list about what the API should look like. The thread starts here: http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2010-January/024860.html
Summary: [video tag] Alt-click to enter fullscreen mode → API for controlling fullscreen from content
Yes, an API to make the <video> go fullscreen would be really appreciated. Robert O'Callahan wrote a proposal here: http://www.mail-archive.com/whatwg@lists.whatwg.org/msg19915.html He propose "enterFullscreen()". "enterFullScreen()" [uppercase S] might be even better to be in harmony with the existing WebKit API, but anyway... Regarding the "security" issues, the WebKit team is preventing programmatic call to that function and the API can be only triggered via a user click. At this point the transition effect to go fullscreen is secondary IMHO, one should focus on simply getting the API to execute the same action that is called when the user chooses "Full Screen" from the contextual menu (or when clicking the fullscreen button in the built-in controls). β€”Zeno Jilion.com
Whiteboard: [evang-wanted]
I am currently deciding whether to attempt to implement a slideshow application in Flash or HTML5/CSS3/Javascript. I am not a proficient programmer, so I am unsure I want to tackle drawing everything on the HTML5 canvas with Javascript. I am interested in being able to create the slideshow and enable the user to switch to true fullscreen display. I haven't decided yet whether to implement two layouts (4:3 and 16:9) and then allow those to be scaled up or down. So, will the work you are discussing simply scale whatever is on the canvas? Also, would be displaying UI controls and text boxes on top of the scaled graphical background. Would all that work? Will this be hard for a mediocre programmer to tackle (my background is software testing).
W3C does not recommend that: "User agents should not provide a public API to cause videos to be shown full-screen. A script, combined with a carefully crafted video file, could trick the user into thinking a system-modal dialog had been shown, and prompt the user for a password. There is also the danger of "mere" annoyance, with pages launching full-screen videos when links are clicked or pages navigated. Instead, user-agent-specific interface features may be provided to easily allow the user to obtain a full-screen playback mode." (http://www.w3.org/TR/html5/video.html)
Pierre, don't worry, the Mozilla folks are very aware of this... I was also mentioning it in my previous comment ("security issues"), and there're trivial solutions to this... β€”Zeno
(In reply to comment #2) > Yes, an API to make the <video> go fullscreen would be really appreciated. Zeno, can u try following at Jilion.com http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2010-June/026803.html
I thought about this a fair bit a few weeks ago, and wrote some detailed notes on how I think this should work that expand on my WHATWG proposal. https://wiki.mozilla.org/Gecko:FullScreenAPI
Blocks: 470628
(In reply to comment #3) > I am currently deciding whether to attempt to implement a slideshow application > in Flash or HTML5/CSS3/Javascript. I am not a proficient programmer, so I am > unsure I want to tackle drawing everything on the HTML5 canvas with Javascript. > I am interested in being able to create the slideshow and enable the user to > switch to true fullscreen display. I haven't decided yet whether to implement > two layouts (4:3 and 16:9) and then allow those to be scaled up or down. So, > will the work you are discussing simply scale whatever is on the canvas? Also, > would be displaying UI controls and text boxes on top of the scaled graphical > background. Would all that work? Will this be hard for a mediocre programmer > to tackle (my background is software testing). You want window.fullScreen = true
(In reply to comment #10) > You want window.fullScreen = true Oops, sorry, apparently this is read-only without chrome privileges.
Assignee: nobody → josh
Attached patch Part 1: Window state goop, content events, styling. (obsolete) (deleted) β€” β€” Splinter Review
I'm going to be dumping my rebased WIP patches from September in this bug mainly for safekeeping. They're not really worth looking at yet unless you're keenly interested in watching me fumble my way along.
Attached patch Part 3: Key disabling. (obsolete) (deleted) β€” β€” Splinter Review
Attached patch Part 4 (obsolete) (deleted) β€” β€” Splinter Review
Attached patch Part 5: Frontend prototype. (obsolete) (deleted) β€” β€” Splinter Review
Attachment #495682 - Attachment description: Part 1 → Part 1: Window state goop, content events, styling.
Attachment #495682 - Attachment is obsolete: true
Attached patch Part 1: Window state goop, content events, styling. (obsolete) (deleted) β€” β€” Splinter Review
Attachment #495691 - Attachment is obsolete: true
Attached patch Part 2: Chrome events, request API. (obsolete) (deleted) β€” β€” Splinter Review
Attachment #495690 - Attachment description: Part 3 → Part 3: Key disabling.
Attachment #495690 - Attachment description: Part 3: Key disabling. → Part 3: Extend nsGlobalWindow API with key disabling.
Attachment #495689 - Attachment description: Part 2 → Part 2: Key disabling.
Attachment #495692 - Attachment description: Part 5 → Part 5: Frontend prototype.
Attachment #495721 - Attachment is obsolete: true
Attached patch Part 1: Window state goop, content events, styling. (obsolete) (deleted) β€” β€” Splinter Review
Attachment #495722 - Attachment is obsolete: true
Attached patch Part 2: Chrome events, request API. (obsolete) (deleted) β€” β€” Splinter Review
Attachment #495689 - Attachment description: Part 2: Key disabling. → Part 3: Key disabling.
Attachment #495690 - Attachment description: Part 3: Extend nsGlobalWindow API with key disabling. → Part 4: Extend nsGlobalWindow API with key disabling.
Blocks: 631042
No longer blocks: 569993
I'm not working on this at the moment, no matter how much I wish I were.
Assignee: josh → nobody
Component: Video/Audio → General
Keywords: student-project
QA Contact: video.audio → general
(In reply to comment #4) > W3C does not recommend that: > "User agents should not provide a public API to cause videos to be shown > full-screen. A script, combined with a carefully crafted video file, could > trick the user into thinking a system-modal dialog had been shown, and > prompt the user for a password. There is also the danger of "mere" > annoyance, with pages launching full-screen videos when links are clicked or > pages navigated. Instead, user-agent-specific interface features may be > provided to easily allow the user to obtain a full-screen playback mode." > (http://www.w3.org/TR/html5/video.html) You know why each time you go full screen on flash there is a banner in the middle telling you "press esc to quit"? I think they have the same concern. Actually I find it annoying, Fx should come up with a better idea, e.g. popping up a warning when user go screen the first time on a site and user can check "never warn me again on this site", this way it only annoy you once.
I'll see if I can make some progress on this.
Assignee: nobody → chris
Has there been any progress made on this bug? I'd like to take bug 470628 (which depends on this bug), but am curious as to how far out a patch for this bug may land.
Yes. I have patches that work which are based on trunk which implements all the new methods and attributes in the spec, but I don't have any tests or UI. I also haven't addressed the finer points of the security considerations listed on the spec page, so I may require some more refactoring. Hopefully I'll have something up for review next week.
Couple of random thoughts (which probably end up being followups, but might as well ask now). 1) We've talked a bit about full-tab perhaps being desirable in addition to full-screen. Will this same API/implementation be adaptable to support full-tab as well? I'm not sure if we need to expose the distinction to content, it could just be a UA option. 2) Will this implementation be flexible enough to allow smooth transitions between fullscreen (or fulltab) and normal mode? For example, see what Safari does... Instead of blinking between the two modes, the video zooms in/out while the rest of the screen fades in/out. It's not just a cool effect, but makes it very clear what's happening.
Just FYI, Safari does not have a smooth transition with this API. Only with the fullscreen video.
Right: if the element is not a video you won't get the zoom (growing/shrinking) transition, but you'll still get the fade-in/out transition on the whole screen.
(In reply to comment #30) > 1) We've talked a bit about full-tab perhaps being desirable in addition to > full-screen. Will this same API/implementation be adaptable to support > full-tab as well? I'm not sure if we need to expose the distinction to > content, it could just be a UA option. Exposing it to content seems slightly strange, since it can already do full-tab. E.g. youtube's poor man's fullscreen mode for html5 video.
(In reply to comment #32) > Right: if the element is not a video you won't get the zoom > (growing/shrinking) transition, but you'll still get the fade-in/out > transition on the whole screen. Hopefully all the growing/shrinking fade-in/out can be turned off, it only makes the transition sluggish.
Is it ok if we add to the "URL" field of this bug the feature's wiki page? Just for better tracking purposes. Thanks. https://wiki.mozilla.org/Platform/Features/Full_Screen_APIs
Attached patch Patch 1 v1: dom/content changes for full-screen api (obsolete) (deleted) β€” β€” Splinter Review
Implement the base changes to document and element, except the "withkeys" functions, and no security constraints. The API is only enabled when the full-screen-api.enabled pref is set to true. See the spec at https://wiki.mozilla.org/index.php?title=Gecko:FullScreenAPI Special thanks to Josh Matthews whose patches formed the base of my work, and made getting started a lot easier. Question: would the reference to the full-screen element be better off as a weak reference?
Attachment #495689 - Attachment is obsolete: true
Attachment #495690 - Attachment is obsolete: true
Attachment #495692 - Attachment is obsolete: true
Attachment #495744 - Attachment is obsolete: true
Attachment #495745 - Attachment is obsolete: true
Attachment #550849 - Flags: review?(jst)
Attached patch Patch 2 v1: -moz-full-screen CSS pseudo class (obsolete) (deleted) β€” β€” Splinter Review
Style sheet changes required to make the current full-screen element have the -moz-full-screen pseudo class apply, and the stlye rules required to make the full-screen element encompass the whole screen.
Attachment #550852 - Flags: review?(bzbarsky)
Attached patch Patch 3 v1: dom fullscreenchange event (obsolete) (deleted) β€” β€” Splinter Review
Dispatch a fullscreenchange event when we go into, or leave, DOM full-screen mode. Question: This is a mozilla-specific API at this stage, should this event be called mozfullscreenchange instead?
Attachment #550854 - Flags: review?(Olli.Pettay)
Attached patch Patch 4 v1: iframe.mozallowfullscreen (obsolete) (deleted) β€” β€” Splinter Review
Restrict HTMLElement.mozRequestFullScreen so that it's only granted if all ancestor iframes have the mozallowfullscreen attribute set.
Attachment #550855 - Flags: review?(jst)
Don't animate the toolbar hide when we enter DOM full-screen mode, and exit DOM full-screen on escape key press. I think we should run with this until we implement a "you've entered full-screen mode, press ESC to exit" drop down, or something equivalent. I also want this so that when I get reftests (or reftest-like mochitests which compare screenshots) I won't need to worry about a half hidden toolbar causing screengrab comparisons in reftests to fail.
Attachment #550861 - Flags: review?(dao)
Attached patch Patch 7 v1: mochitests (obsolete) (deleted) β€” β€” Splinter Review
Mochitests for the content changes. I was trying to use WindowSnapshot.js to compare full-screen screenshots to verify that full-screen was working, but that was failing randomly I think due to timing issues, so I've removed those tests and will add them in again later when I figure out how to do them reliably.
Attachment #550863 - Flags: review?(jst)
Roc and I discussed this, and we decided we'd start out by following the Flash model of just displaying a warning when you enter full-screen mode. This way it's not so critical to implement the "withKeys" API variants, since the user should always know when they've entered full-screen. Other remaining ToDo items are roughly: * Must decide what to do when user exits browser while in dom full-screen mode. Currently the full-screened-ness is restored on restart, but the dom full-screen mode isn't. Something needs to change here. * Change-tab (including open new tab in foreground) should break out of full-screen mode. * Disable plugins in full-screen mode documents (do we still need to do this if we don't implement the "withKeys" api variants?). * Implement the "you're in full-screen mode. ok/exit" UI/popdown.
Another ToDo: * We should only allow mozRequestFullScreen() to go full-screen mode when running in a trusted event context (like a user mouse click event).
(In reply to comment #42) > * Must decide what to do when user exits browser while in dom full-screen > mode. Currently the full-screened-ness is restored on restart, but the dom > full-screen mode isn't. Something needs to change here. I'm not sure, but I think restoring user-initiated fullscreen but not DOM-initiated fullscreen would be the best. > * Disable plugins in full-screen mode documents (do we still need to do this > if we don't implement the "withKeys" api variants?). Probably not. I think for now we just shouldn't implement a withKeys method at all. Later if we implement it, we would ensure plugins are disabled when it is used. One other item is to do a smooth zoom-out transition from the fullscreen element, but that can be another bug.
(In reply to comment #42) > Roc and I discussed this, and we decided we'd start out by following the > Flash model of just displaying a warning when you enter full-screen mode. > This way it's not so critical to implement the "withKeys" API variants, > since the user should always know when they've entered full-screen. Is there a pref like "pref("full-screen.warning.enabled", false);" for experienced user to turn it off ? or allow user to set warning time?
Is it possible to specify site-specific preferences about this feature? Such as allow this site to store cookies, run Javascript, open pop-ups,... There are other features that might need website specific permissions (access to hardware such as microphone, camera). Could you integrate this in a larger permission related module, maybe in a future version?
(In reply to Chris Pearce [:cpearce] from comment #42) > Roc and I discussed this, and we decided we'd start out by following the > Flash model of just displaying a warning when you enter full-screen mode. > This way it's not so critical to implement the "withKeys" API variants, > since the user should always know when they've entered full-screen. Will the non-withKeys version still ban keys from day 1? (I scanned thought the patches and didn't notice key filtering code. It would be bad if the first release allowed keys in the non-withKeys variant preventing taking the keys away from sites later.)
Comment on attachment 550861 [details] [diff] [review] Patch 6 v1: Don't animate toolbar hide on dom full-screen >Bug 545812 - Don't animate the toolbar hide when switching to DOM API full-screen mode. r=? >--- a/browser/base/content/browser.js >+++ b/browser/base/content/browser.js > if (enterFS) { >- // Add a tiny toolbar to receive mouseover and dragenter events, and provide affordance. >- // This will help simulate the "collapse" metaphor while also requiring less code and >- // events than raw listening of mouse coords. >- let fullScrToggler = document.getElementById("fullscr-toggler"); >- if (!fullScrToggler) { >- fullScrToggler = document.createElement("hbox"); >- fullScrToggler.id = "fullscr-toggler"; >- fullScrToggler.collapsed = true; >- gNavToolbox.parentNode.insertBefore(fullScrToggler, gNavToolbox.nextSibling); >+ if (document.mozFullScreen) { >+ this._shouldAnimate = false; What exactly is document.mozFullScreen?
(In reply to Bogdan Bivolaru from comment #47) > There are other features that might need website specific permissions > (access to hardware such as microphone, camera). Could you integrate this in > a larger permission related module, maybe in a future version? I don't think that's a good idea: http://robert.ocallahan.org/2011/06/permissions-for-web-applications_30.html But we should discuss that somewhere else. (In reply to Henri Sivonen (:hsivonen) from comment #48) > (In reply to Chris Pearce [:cpearce] from comment #42) > > Roc and I discussed this, and we decided we'd start out by following the > > Flash model of just displaying a warning when you enter full-screen mode. > > This way it's not so critical to implement the "withKeys" API variants, > > since the user should always know when they've entered full-screen. > > Will the non-withKeys version still ban keys from day 1? (I scanned thought > the patches and didn't notice key filtering code. It would be bad if the > first release allowed keys in the non-withKeys variant preventing taking the > keys away from sites later.) Actually I got this wrong. what we should do is implement withKeys *only* at first. It's the non-withKeys version that we should not implement until we actually implement the key-disabling code.
Comment on attachment 550854 [details] [diff] [review] Patch 3 v1: dom fullscreenchange event ># HG changeset patch ># Parent 2b611319841e4195413aaa885a5c36718c79d60d >Bug 545812 - Dispatch fullscreenchange event when we change full-screen mode. r=? > >diff --git a/content/base/src/nsContentUtils.cpp b/content/base/src/nsContentUtils.cpp >--- a/content/base/src/nsContentUtils.cpp >+++ b/content/base/src/nsContentUtils.cpp >@@ -653,17 +653,18 @@ nsContentUtils::InitializeEventTable() { > { nsGkAtoms::ondevicemotion, NS_DEVICE_MOTION, EventNameType_None, NS_EVENT }, > { nsGkAtoms::ondeviceorientation, NS_DEVICE_ORIENTATION, EventNameType_None, NS_EVENT }, > > { nsGkAtoms::ontransitionend, NS_TRANSITION_END, EventNameType_None, NS_TRANSITION_EVENT }, > { nsGkAtoms::onanimationstart, NS_ANIMATION_START, EventNameType_None, NS_ANIMATION_EVENT }, > { nsGkAtoms::onanimationend, NS_ANIMATION_END, EventNameType_None, NS_ANIMATION_EVENT }, > { nsGkAtoms::onanimationiteration, NS_ANIMATION_ITERATION, EventNameType_None, NS_ANIMATION_EVENT }, > { nsGkAtoms::onbeforeprint, NS_BEFOREPRINT, EventNameType_HTMLXUL, NS_EVENT }, >- { nsGkAtoms::onafterprint, NS_AFTERPRINT, EventNameType_HTMLXUL, NS_EVENT } >+ { nsGkAtoms::onafterprint, NS_AFTERPRINT, EventNameType_HTMLXUL, NS_EVENT }, >+ { nsGkAtoms::onfullscreenchange, NS_FULLSCREENCHANGE, EventNameType_HTML, NS_EVENT_NULL } Yes, I think the event should have moz prefix if we don't have a spec for this. FYI, bz is changing InitializeEventTable, Bug 675405, so one of you will need to update the patch a bit. >+class nsDispatchFullScreenChange : public nsRunnable { { should be in the next line >+public: >+ nsDispatchFullScreenChange(nsIDocument *aDoc, >+ nsIDOMDocument *aDomDoc) >+ : mDoc(aDoc) >+ { >+ mTarget = aDoc->GetFullScreenElement(); >+ if (!mTarget) { >+ mTarget = aDomDoc; >+ } >+ } There shouldn't be need for aDomDoc. Just set mTarget to point aDoc. > void nsDocument::UpdateFullScreenStatus(PRBool aIsFullScreen) { >+ // Only maintain full-screen state on DOM documents. >+ nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(static_cast<nsIDocument*>(this)); >+ if (!domDoc) >+ return; So, there shouldn't be need for domDoc variable.
Attachment #550854 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 550852 [details] [diff] [review] Patch 2 v1: -moz-full-screen CSS pseudo class So this is wrong on its face (why is it adding an unused NS_EVENT_STATE_FULL_SCREEN, adding two different atoms for the :-moz-full-screen pseudoclass, etc), and I'm not sure whether it even gives correct behavior.... Specific questions: 1) can the full-screen element of a document change without the full-screen document state changing? 2) Why do we really need the full-screen document state (changes to which force a complete restyle on everything in the document)? 3) If an element is full-screen, are all its containing frames expected to match -moz-full-screen? 4) Is there a reason that we set up the bool pref var cache for IsFullScreenApiEnabled lazily? Doing that violates the constraint we're trying to enforce where selector matching doesn't mutate any state....
Attachment #550852 - Flags: review?(bzbarsky) → review-
(In reply to DΓ£o Gottwald [:dao] from comment #49) > What exactly is document.mozFullScreen? DΓ£o: It means that an element owned by the document has requested that the document go into full-screen mode, and if the element is in the document, the element will be expanded to encompass the entire screen. This enables us to distinguish between the user pressing F11 to put the window into full-screen mode from an element in the document requesting full-screen mode, so that UI can behave accordingly.
(In reply to Boris Zbarsky (:bz) from comment #52) > Comment on attachment 550852 [details] [diff] [review] [diff] [details] [review] > Patch 2 v1: -moz-full-screen CSS pseudo class Thanks for reviewing this. > So this is wrong on its face (why is it adding an unused > NS_EVENT_STATE_FULL_SCREEN, adding two different atoms for the > :-moz-full-screen pseudoclass, etc), Ah oops, detritus from a previous version. > and I'm not sure whether it even gives > correct behavior.... Behaviour appears correct based on my testing. ;) I linked to a try build in comment 45 if you or anyone wants to test themselves. Test page at: http://pearce.org.nz/full-screen/ > Specific questions: > > 1) can the full-screen element of a document change without the full-screen > document state changing? Yes. If a document is in full-screen mode, and another element requests full-screen, the other element will become the full-screen element, and have the css-pseudo class change to apply to it and not the original full-screen element. > 2) Why do we really need the full-screen document state (changes to which > force > a complete restyle on everything in the document)? I tried removing the document state, but when I do that, if an element requests full-screen while another element is already the full-screen element, the pseudo-class doesn't switch apply to the new full-screen element. Is there a way to force the pseudo-class to be re-evaluated only on the affected elements? Perhaps this is what you're getting at in question 1? > 3) If an element is full-screen, are all its containing frames expected to > match -moz-full-screen? Yes, iframes which contain the full-screen element will match. Note that nsIDocument::GetFullScreenElement() returns the full-screen element when the full-screen element is in the current document, or the element which contains the full-screen element if the full-screen element is in a child document. So the ePseudoClass_mozFullScreenElement case in SelectorMatches() will Just Work and apply the pseudo-class to containing documents as well. > 4) Is there a reason that we set up the bool pref var cache for > IsFullScreenApiEnabled lazily? Doing that violates the constraint we're > trying to enforce where selector matching doesn't mutate any state.... How about I put the pref var cache in nsContentUtils::Init()?
(In reply to Chris Pearce [:cpearce] from comment #53) > (In reply to DΓ£o Gottwald [:dao] from comment #49) > > What exactly is document.mozFullScreen? > > DΓ£o: It means that an element owned by the document has requested that the > document go into full-screen mode, and if the element is in the document, > the element will be expanded to encompass the entire screen. You're querying the chrome document here, though.
bz: see comment 54 for answers to your questions.
Sorry I wasn't cced... > Perhaps this is what you're getting at in question 1? Yes. ;) It sounds like the document state thing is just hiding the lack of dynamic change handling for the other bit by recomputing everything. I think the right way to handle this is probably to just get rid of the document state altogether (unless you need it elsewhere for some reason) and certainly to not look at that document state from the style system. Instead, just track the fullscreen state of elements. In particular, see mozilla::dom::Element::AddStates/RemoveStates. If you just add an event state bit for this (as you do), do the event state pseudoclass thing, and call AddStates/RemoveStates on elements that start/stop being a fullscreen element, the style system end should just work. Furthermore, if it's an event state pseudoclass you won't even need to add any code to the rule processor. AddStates/RemoveStates are private, so you may need to add a friend class unless one of the ESM, global window, or focus manager is doing the state-setting. Or I guess we could just make them public... I really wish C++ had a way to expose a particular method to particular classes. :( > How about I put the pref var cache in nsContentUtils::Init()? Sounds great.
(In reply to DΓ£o Gottwald [:dao] from comment #55) > (In reply to Chris Pearce [:cpearce] from comment #53) > > (In reply to DΓ£o Gottwald [:dao] from comment #49) > > > What exactly is document.mozFullScreen? > > > > DΓ£o: It means that an element owned by the document has requested that the > > document go into full-screen mode, and if the element is in the document, > > the element will be expanded to encompass the entire screen. > > You're querying the chrome document here, though. DΓ£o: When a dom document is in full-screen state, all ancestor documents are in full-screen state as well, including the ancestor chrome document. See the proposed specification at: https://wiki.mozilla.org/index.php?title=Gecko:FullScreenAPI
I am opposed to this feature. I don't think a flashed warning is sufficient even for a non-keys version. See https://wiki.mozilla.org/Gecko:FullScreenAPI#Jesse.27s_concerns
(In reply to Chris Pearce [:cpearce] from comment #58) > (In reply to DΓ£o Gottwald [:dao] from comment #55) > > (In reply to Chris Pearce [:cpearce] from comment #53) > > > (In reply to DΓ£o Gottwald [:dao] from comment #49) > > > > What exactly is document.mozFullScreen? > > > > > > DΓ£o: It means that an element owned by the document has requested that the > > > document go into full-screen mode, and if the element is in the document, > > > the element will be expanded to encompass the entire screen. > > > > You're querying the chrome document here, though. > > DΓ£o: When a dom document is in full-screen state, all ancestor documents are > in full-screen state as well, including the ancestor chrome document. And this is going to work with separate chrome/content processes?
(In reply to Jesse Ruderman from comment #59) > I am opposed to this feature. I don't think a flashed warning is sufficient > even for a non-keys version. See > https://wiki.mozilla.org/Gecko:FullScreenAPI#Jesse.27s_concerns Have there been any attacks of that nature using Flash? Can you elaborate on why a Flash-style warning fails to mitigate the spoofing risks you describe? We should minimize the risk here, but this feature is needed.
> Can you elaborate on why a Flash-style warning fails to mitigate the spoofing > risks you describe? This is an honest question, not a parry. I want to understand this to get a better idea of what might address your concerns.
Whiteboard: [evang-wanted] → [evang-wanted][sr:curtisk]
(In reply to DΓ£o Gottwald [:dao] from comment #60) > (In reply to Chris Pearce [:cpearce] from comment #58) > > (In reply to DΓ£o Gottwald [:dao] from comment #55) > > > You're querying the chrome document here, though. > > > > DΓ£o: When a dom document is in full-screen state, all ancestor documents are > > in full-screen state as well, including the ancestor chrome document. > > And this is going to work with separate chrome/content processes? Currently this won't work with separate chrome/content processes, but we'll make this work cross-process in a follow up bug. AFAICT window.fullscreen doesn't work cross-process yet either.
(In reply to Chris Pearce [:cpearce] from comment #63) > > > > You're querying the chrome document here, though. > > > > > > DΓ£o: When a dom document is in full-screen state, all ancestor documents are > > > in full-screen state as well, including the ancestor chrome document. > > > > And this is going to work with separate chrome/content processes? > > Currently this won't work with separate chrome/content processes, but we'll > make this work cross-process in a follow up bug. What exactly is that followup work going to involve? Do you have a concrete plan? > AFAICT window.fullscreen doesn't work cross-process yet either. Maybe, but window.fullScreen isn't new. We should try to make new features e10s-ready from the start and avoid adding more to the pile of things that need to be fixed.
> Can you elaborate on why a Flash-style warning fails to mitigate the spoofing > risks you describe? * User might not be looking at the screen. * Site site might add visual distractions during the transition. I saw a demo where a bunch of similarly-styled messaged appeared at once; the chance of someone reading the one saying "Press Esc to exit full screen mode" seemed low. * Site might make it seem as if the "Press Esc to exit full screen mode" is part of an image (e.g. a banner ad or instructional screenshot) rather than something the browser is saying right now. * Site might fake the opposite transition, as if the video finished and exited full screen mode, leaving the user believing they are no longer in full screen mode. * Even after seeing the "Press Esc to exit full screen mode" message, the illusion may be strong enough on its own to convince the user they are not in full screen mode. Some of these concerns can be mitigated by positioning the warning to appear near the top of the screen, rather than in the middle, so it is forced to overlap with the fake toolbars. Some of these concerns diminish once users are accustomed to watching full-screen videos, learning what the messages look like and getting used to pressing Esc after a video finishes. FWIW, even Flash, which routinely chooses to subvert browser security in order to have "unique functionality", chose not to allow keyboard input in auto-full-screen mode. > We should minimize the risk here, but this feature is needed. I'm not convinced this feature is needed. A full-screen button in the titlebar would cover all the common cases (the page has only one video element; the page has several video elements but only one is playing). And we need that button on the titlebar anyway, at least on Lion.
1. This sounds like the most invasive/annoying popup/popbehind ad ever. Once the user interacts with the page somewhere it jumps at you. 2. Jesse's points seem valid, in particular regarding esc spoofing. It's now very possible to make a convincing duplicate of the Firefox UI (even without XUL), especially as it's reduced in scope. It's even possible to detect OS and mimic that enough (really just a menubar of some sort). That would make a very convincing decoy to many users. You could likely mitigate some of that by leaving a small semi-transparent bug in the top right corner (personal pref on location) that would be non-invasive but allow for easy detection of any spoof. Another option is some border effect that takes a few pixels. But I bet more novice users would still fall for it. At a bare minimum, I'd suggest a pref to completely disable the ability for those who don't want to risk it.
http://mayscript.com/blog/eric/browser-ui-spoofing-attack-revisited is very relevant. I'm with Jesse: we don't want this feature.
I think a web page should never be able to pop up to full screen.
(In reply to Kai Engert (:kaie) from comment #68) > I think a web page should never be able to pop up to full screen. The request for full-screen only works in the event handlers for user-initiated keypress or mouse click events. So a site can't just switch to full-screen whenever it wants to. Full-screen can be disabled by a pref as well (it's off by default in the patches at the moment). (In reply to Jesse Ruderman from comment #65) > > Can you elaborate on why a Flash-style warning fails to mitigate the spoofing > > risks you describe? If a pop-down appeared to grant/deny permission before entering full-screen then your concerns should be alleviated? Similar to how a pop-down appears to grant/deny popups. Then the user will be aware they're entering full-screen mode, as they have to consciously confirm it. This pop-down can make them aware that they can exit by pressing escape. > > We should minimize the risk here, but this feature is needed. > > I'm not convinced this feature is needed. It's wanted for HTML5 games, as well as video players. However games are rather limited by not being able to receive all key input. Most modern games require a plethora of hot keys. Maybe what games are really looking for is a way to trap mouse input inside the window? It's pretty much impossible to write good FPS controls without mouse input captured.
(In reply to DΓ£o Gottwald [:dao] from comment #64) > (In reply to Chris Pearce [:cpearce] from comment #63) > > > > > You're querying the chrome document here, though. > > > > > > > > DΓ£o: When a dom document is in full-screen state, all ancestor documents are > > > > in full-screen state as well, including the ancestor chrome document. > > > > > > And this is going to work with separate chrome/content processes? > > > > Currently this won't work with separate chrome/content processes, but we'll > > make this work cross-process in a follow up bug. > > What exactly is that followup work going to involve? Do you have a concrete > plan? We'll need to proxy requests to nsGlobalWindow::SetFullScreen() from content to the chrome process using IPC. While handling the request on the chrome side we'd need to set the mozFullScreen state in the chrome document.
Attached patch Patch 1 v2: dom/content changes for full-screen api (obsolete) (deleted) β€” β€” Splinter Review
Reworked to include bz's feedback: * Moved setup of full-screen-api.enabled pref cache to nsContentUtils::Init(). * Removed document state (I'll add an event state to track the -moz-full-screen elements in patch 2).
Attachment #550849 - Attachment is obsolete: true
Attachment #552291 - Flags: review?(jst)
Attachment #550849 - Flags: review?(jst)
Changed to store -moz-full-screen state in an event state instead of relying on a document state.
Attachment #550852 - Attachment is obsolete: true
Attachment #552292 - Flags: review?(bzbarsky)
Rebased on top of bz's changes from bug 675405, and review comments addressed. We now dispatch a mozfullscreenchange event when we enter or leave DOM full-screen mode. Carrying forward r=smaug.
Attachment #550854 - Attachment is obsolete: true
Attachment #552294 - Flags: review+
Attached patch Patch 5 v1: -moz-full-screen media query (obsolete) (deleted) β€” β€” Splinter Review
Implement -moz-full-screen media query.
Attachment #552297 - Flags: review?(bzbarsky)
Attachment #550861 - Attachment description: Patch 5 v1: Don't animate toolbar hide on dom full-screen → Patch 6 v1: Don't animate toolbar hide on dom full-screen
Attachment #550863 - Attachment description: Patch 6 v1: mochitests → Patch 7 v1: mochitests
Attached patch Patch 7 v2: mochitests (deleted) β€” β€” Splinter Review
Updated to reflect fullscreenchange event being renamed to mozfullscreenchange.
Attachment #550863 - Attachment is obsolete: true
Attachment #552298 - Flags: review?(jst)
Attachment #550863 - Flags: review?(jst)
Comment on attachment 552292 [details] [diff] [review] Patch 2 v2: -moz-full-screen CSS pseudo class This looks fine with 3 nits: 1) I'd prefer "border: none" for the iframe style. 2) Probably safer to use "*|*:-moz-full-screen" for the style in ua.css 3) The ua.css style should move to before the comment about xml parse errors. r=me with those addressed.
Attachment #552292 - Flags: review?(bzbarsky) → review+
* Restrict requests to element.mozFullScreen() so that they're only granted in trusted contexts (in mouse or key event handlers). * Update tests.
Attachment #552299 - Flags: review?(Olli.Pettay)
(In reply to Chris Pearce [:cpearce] from comment #69) > However games are rather limited by not being able to receive all key input. > Most modern games require a plethora of hot keys. Maybe what games are > really looking for is a way to trap mouse input inside the window? It's > pretty much impossible to write good FPS controls without mouse input > captured. They are looking for a "mouse lock" feature as well, but it's mostly a separate issue. (In reply to Jesse Ruderman from comment #65) > * User might not be looking at the screen. The user wouldn't be sending input events without looking at the screen, surely. > * Site site might add visual distractions during the transition. I saw a > demo where a bunch of similarly-styled messaged appeared at once; the chance > of someone reading the one saying "Press Esc to exit full screen mode" > seemed low. That can be avoided by making the transition to full-screen a smooth "zoom out" so the message does not overlap the Web content until the end of the transition. > * Site might make it seem as if the "Press Esc to exit full screen mode" is > part of an image (e.g. a banner ad or instructional screenshot) rather than > something the browser is saying right now. Sounds a bit far-fetched, but we can avoid this by putting the message over the browser chrome, as you suggest. > * Site might fake the opposite transition, as if the video finished and > exited full screen mode, leaving the user believing they are no longer in > full screen mode. > * Even after seeing the "Press Esc to exit full screen mode" message, the > illusion may be strong enough on its own to convince the user they are not > in full screen mode. If a page goes full-screen then either the user didn't want it to, or they did. If they didn't, and we ensure the "Press Esc" message is visible, I'm confident they'll immediately exit full-screen and these issues will not apply. So the threat here would be some Web site that actually offers a desirable full-screen service but later tries to abuse full-screen status. But such threats are just as valid if we offer a "full-screen" button in the browser UI. Heck, it's almost as valid in Firefox today, if the Web page encourages the user to press F11 to go full-screen. I don't see any way to avoid problems like that except by showing some site identification on the screen whenever the user is providing input. Perhaps we could show a URL-bar overlay at the top of the screen for a few seconds after each input event.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #78) > The user wouldn't be sending input events without looking at the screen, surely. "Take the new blind-writing challenge! Can you write your name without looking?" ;-) But now in all seriousness, what about delaying the switch from full-screen after the input event?
Comment on attachment 552299 [details] [diff] [review] Patch 8 v1: Only grant full-screen requests in trusted contexts > NS_IMETHODIMP > nsDocument::MozCancelFullScreen() > { > if (!nsContentUtils::IsFullScreenApiEnabled() || >+ !nsContentUtils::IsRequestFullScreenAllowed() || > !IsFullScreenDoc() || > !GetWindow()) > return NS_OK; What is the reason for this restriction? Either remove this, or explain why it is needed. In latter case, add also a comment to the code. >+function setRequireTrustedContext(value) { >+ netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect"); >+ var ps = Components.classes["@mozilla.org/preferences-service;1"] >+ .getService(Components.interfaces.nsIPrefBranch); >+ // Request full-screen from a non trusted context (this script isn't a user >+ // generated event!). We should not receive a "fullscreenchange" event. >+ ps.setBoolPref("full-screen-api.allow-trusted-requests-only", value); >+} netscape.security.PrivilegeManager shouldn't be used in mochitests. Use SpecialPowers.get/setBoolPref https://developer.mozilla.org/en/SpecialPowers Those addressed, r=me
Attachment #552299 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 552297 [details] [diff] [review] Patch 5 v1: -moz-full-screen media query >+#define NS_STYLE_OFF 0 >+#define NS_STYLE_ON 1 NS_STYLE_FULLSCREEN_ON/OFF, please. Also, do you need the IsFullScreenApiEnabled() check in GetFullScreen? Seems like if that's false, IsFullScreenDoc() should always return false... But apart from all that, wouldn't using the media query mentioned in bug bug 678173 work better here than inventing a new -moz-full-screen? Or do we want this media query to mean something different from the view-mode case?
(In reply to Olli Pettay [:smaug] from comment #80) > Comment on attachment 552299 [details] [diff] [review] > > if (!nsContentUtils::IsFullScreenApiEnabled() || > >+ !nsContentUtils::IsRequestFullScreenAllowed() || > > !IsFullScreenDoc() || > > !GetWindow()) > > return NS_OK; > What is the reason for this restriction? It's to prevent the browser going full-screen from script without the user initiating it. This stops the full-screen from being used as popups used to be, and it also makes it harder for bad guys' script to go full-screen and spoof the browser window and some important login page, like paypal etc. I'll add a comment to the patch.
(In reply to Boris Zbarsky (:bz) from comment #81) > But apart from all that, wouldn't using the media query mentioned in bug bug > 678173 work better here than inventing a new -moz-full-screen? Or do we > want this media query to mean something different from the view-mode case? I think using view-mode here would make sense.
Comment on attachment 552297 [details] [diff] [review] Patch 5 v1: -moz-full-screen media query Agreed.
Attachment #552297 - Attachment is obsolete: true
Attachment #552297 - Flags: review?(bzbarsky)
Comment on attachment 550861 [details] [diff] [review] Patch 6 v1: Don't animate toolbar hide on dom full-screen > _keyToggleCallback: function(aEvent) > { > // if we can use the keyboard (eg Ctrl+L or Ctrl+E) to open the toolbars, we > // should provide a way to collapse them too. > if (aEvent.keyCode == aEvent.DOM_VK_ESCAPE) { >- FullScreen._shouldAnimate = false; >- FullScreen.mouseoverToggle(false, true); >+ if (document.mozFullScreen) { >+ document.mozCancelFullScreen(); >+ } else { >+ FullScreen._shouldAnimate = false; >+ FullScreen.mouseoverToggle(false, true); >+ } > } What's the rationale for this change?
Dao: Exit full-screen mode when escape is pressed in DOM full-screen mode.
Comment on attachment 550861 [details] [diff] [review] Patch 6 v1: Don't animate toolbar hide on dom full-screen (In reply to Chris Pearce [:cpearce] from comment #86) > Dao: Exit full-screen mode when escape is pressed in DOM full-screen mode. So this front-end code piece is the only part handling ESC? This seems too weak, since content scripts can consume the event before chrome code. At the very least, the event listener needs to be a capturing listener, although I'm not quite sure if that's sufficient and e10s-friendly...
Attachment #550861 - Flags: review?(dao) → review-
With comment added in nsGenericHTMLElement::MozRequestFullScreen() and using SpecialPowers in mochitests. Carrying forward r=smaug.
Attachment #552299 - Attachment is obsolete: true
Attachment #553046 - Flags: review+
>> * User might not be looking at the screen. > The user wouldn't be sending input events without looking at the screen, surely. It would help if we could guarantee the full-screening happens immediately after the input event. Bug 678994 is one problematic case. Maybe we should add a timer to the popup blocker heuristic. >> * Site site might add visual distractions during the transition. > That can be avoided by making the transition to full-screen a smooth "zoom out" so the message does not overlap the Web content until the end of the transition. Sure, that will help. Especially if we make sure evil scripts can't jank the animation, e.g. by temporarily hanging the browser. > So the threat here would be some Web site that actually offers a desirable full-screen service but later tries to abuse full-screen status. But such threats are just as valid if we offer a "full-screen" button in the browser UI. Heck, it's almost as valid in Firefox today, if the Web page encourages the user to press F11 to go full-screen. This is a good point. I could almost imagine arguing for a no-keyboard-input version of full screen, just so users don't get accustomed to doing something dangerous every time they watch a video.
This feature may add attack surface by turning "Firefox stops painting" bugs and "popup blocker can be bypassed" bugs into security bugs. As I said at the end of bug 545812 comment 65, I'm not convinced this feature is needed. But it sounds like we have the scariest problems under control.
(In reply to Jesse Ruderman from comment #65) > * User might not be looking at the screen. > > * Site site might add visual distractions during the transition. I saw a > demo where a bunch of similarly-styled messaged appeared at once; the chance > of someone reading the one saying "Press Esc to exit full screen mode" > seemed low. > > * Site might make it seem as if the "Press Esc to exit full screen mode" is > part of an image (e.g. a banner ad or instructional screenshot) rather than > something the browser is saying right now. > > * Site might fake the opposite transition, as if the video finished and > exited full screen mode, leaving the user believing they are no longer in > full screen mode. > > * Even after seeing the "Press Esc to exit full screen mode" message, the > illusion may be strong enough on its own to convince the user they are not > in full screen mode. How about a warning bar with buttons (leave full screen and close warning button)on it?just like the pop up "warning bar", if you don't close it manually,it will appear forever, and when you hover on the button, it changes color.
Attached patch Patch 9 v1: Restrict key input while in full-screen mode (obsolete) (deleted) β€” β€” Splinter Review
Restrict key input while in DOM full-screen mode to non-alphanumeric keys only. This is to reduce the effectiveness of full-screen apps being able to throw up a fake login page and phish logins etc, as password can't be typed in. Input into <input> and <textarea> elements is also blocked by this. Tests to follow in separate patch.
Attachment #554298 - Flags: review?(Olli.Pettay)
Attached patch Patch 10 v1: Exit full-screen mode on escape press (obsolete) (deleted) β€” β€” Splinter Review
Exit full-screen mode when escape key is pressed. Dao expressed concern in comment 87 that an escape key press listener in browser.js wouldn't be reliable, so implementing in gecko here.
Attachment #554299 - Flags: review?(Olli.Pettay)
Test key input restrictions and test that pressing escape exits full-screen mode.
Attachment #554300 - Flags: review?(Olli.Pettay)
Comment on attachment 554298 [details] [diff] [review] Patch 9 v1: Restrict key input while in full-screen mode > nsresult > nsIContent::PreHandleEvent(nsEventChainPreVisitor& aVisitor) > { > //FIXME! Document how this event retargeting works, Bug 329124. > aVisitor.mCanHandle = PR_TRUE; > aVisitor.mMayHaveListenerManager = HasFlag(NODE_HAS_LISTENERMANAGER); > >+ // In DOM full-screen mode, we restrict key input to non-alphanumeric >+ // key codes to reduce the effectiveness of phishing attacks. >+ if (IsFullScreenAndRestrictedKeyEvent(this, aVisitor.mEvent)) { >+ aVisitor.mCanHandle = PR_FALSE; >+ return NS_OK; >+ } We don't want to prevent script initiated events. So this check should probably go to PresShell::HandleEventInternal What all event handling do we want to prevent? accesskeys? "accessnumbers" (alt+<number> to activate a tab)?
Attachment #554298 - Flags: review?(Olli.Pettay) → review-
Comment on attachment 554299 [details] [diff] [review] Patch 10 v1: Exit full-screen mode on escape press > // In DOM full-screen mode, we restrict key input to non-alphanumeric > // key codes to reduce the effectiveness of phishing attacks. > if (IsFullScreenAndRestrictedKeyEvent(this, aVisitor.mEvent)) { >+ int key = static_cast<const nsKeyEvent*>(aVisitor.mEvent)->keyCode; >+ if (aVisitor.mEvent->message == NS_KEY_UP && key == NS_VK_ESCAPE) { >+ // Exit full-screen mode when the escape key is pressed and released. >+ // We exit full-screen on key-up rather than key-down or key-press as >+ // key-up is the last key event in the sequence fired for a key press. >+ // We stop blocking key events after we leave full-screen, so if we >+ // leave full-screen on key-down or key-press, we'll fire the remaining >+ // key events in the sequence, which may be confusing for content. >+ NS_DispatchToCurrentThread( >+ NS_NewRunnableMethod(GetOwnerDoc(), &nsIDocument::CancelFullScreen)); >+ } So script initiated key event could cancel fullscreen. Would it make sense to handle the esc key somewhere in chrome/ui level and let it to must call MozCancelFullScreen?
Attachment #554299 - Flags: review?(Olli.Pettay) → review-
Comment on attachment 554300 [details] [diff] [review] Patch 11 v1: Test full-screen key event restriction and escape-key-exit So the test should include testing for script initiated key events and whether those can be dispatched and handled normally.
Attachment #554300 - Flags: review?(Olli.Pettay) → review-
I think probably instead of just blocking alphanumeric input, it should exit full-screen.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #98) > I think probably instead of just blocking alphanumeric input, it should exit > full-screen. I'm with roc on this one; just bounce out of full screen.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #98) > I think probably instead of just blocking alphanumeric input, it should exit > full-screen. That sounds incredibly annoying for games; if the user presses the wrong key accidentally, they'll end up being bounced out of full-screen.
I don't think games are going to want to use the no-keys API.
(In reply to Olli Pettay [:smaug] from comment #96) > Comment on attachment 554299 [details] [diff] [review] > Patch 10 v1: Exit full-screen mode on escape press > [...] > > Would it make sense to handle the esc key somewhere in chrome/ui level and > let it to must call MozCancelFullScreen? Dao's comment 87 gave me the impression that putting the full-screen-exit key press handler in chrome/ui was insufficient: "content scripts can consume the event before chrome code. At the very least, the event listener needs to be a capturing listener, although I'm not quite sure if that's sufficient and e10s-friendly...".
(In reply to Olli Pettay [:smaug] from comment #95) > Comment on attachment 554298 [details] [diff] [review] > Patch 9 v1: Restrict key input while in full-screen mode > [...] > What all event handling do we want to prevent? We want to prevent any events which could be used in phishing attacks. Basically anything with an alphanumeric virtual key code. > accesskeys? "accessnumbers" (alt+<number> to activate a tab)? Our intention is to block any event with virtual key code in the restricted ranges, and exit full-screen mode when such key events are encountered, including accesskeys and accessnumbers. Additionally any tab-change or browser navigation will exit full-screen mode anyway, though I've not implemented this yet.
I think probably the best approach to "blocking" keyboard input is to always allow the event to be processed, but exit full-screen in cases where a forbidden key event is processed by page content. I think we should allow ctrl-F and perhaps other chrome command keys to be processed without exiting full-screen. So I think we need to instrument key event dispatch to detect when content has "processed" the event. I think we should have some global state that indicates whether content has processed the event. We would set that when any content script event listener is called. Some native key event listeners would also need to set it, e.g. text inputs would set it when they insert text on keypress. The previous paragraph could be treated as an enhancement, though.
Whiteboard: [evang-wanted][sr:curtisk] → [evang-wanted][secr:curtisk]
* Exit full-screen mode upon non-alpahnumeric input. * Don't exit full-screen mode upon script-dispatched alphanumeric key events. * Add tests for exiting full-screen and script-dispatched alphanumeric key events.
Attachment #554298 - Attachment is obsolete: true
Attachment #554299 - Attachment is obsolete: true
Attachment #554300 - Attachment is obsolete: true
Attachment #554985 - Flags: review?(Olli.Pettay)
Comment on attachment 550855 [details] [diff] [review] Patch 4 v1: iframe.mozallowfullscreen nsresult nsGenericHTMLElement::MozRequestFullScreen() { if (!nsContentUtils::IsFullScreenApiEnabled()) return NS_OK; + // Ensure that all ancestor <iframe> elements have the mozallowfullscreen + // boolean attribute set. + nsINode* node = static_cast<nsINode*>(this); + do { + nsIContent* content = static_cast<nsIContent*>(node); + if (content->Tag() == nsGkAtoms::iframe && Should we be more explicit here and check that the node is actually an HTML node too, i.e. call content->NondeInfo()->Equals(nsGkAtoms::iframe, kNameSpaceID_XHTML)? + !content->HasAttr(kNameSpaceID_None, nsGkAtoms::mozallowfullscreen)) { And is the presence of the attribute all we want to check here, should mozallowfullscreen="false" not disallow fullscreen support?
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #106) > Comment on attachment 550855 [details] [diff] [review] > Patch 4 v1: iframe.mozallowfullscreen > [...] > Should we be more explicit here and check that the node is actually an HTML > node too Sure, sounds sensible. Thanks! > + !content->HasAttr(kNameSpaceID_None, > nsGkAtoms::mozallowfullscreen)) { > > And is the presence of the attribute all we want to check here, should > mozallowfullscreen="false" not disallow fullscreen support? mozallowfullscreen is a boolean attribute, so it's considered to be "on" when present, regardless of value. I agree that mozallowfullscreen="false" turning on full-screen doesn't make sense, but that usage matches other uses of boolean attributes in HTML5. Or is the spec moving away from using boolean attributes?
Comment on attachment 552291 [details] [diff] [review] Patch 1 v2: dom/content changes for full-screen api +PRBool nsContentUtils::IsFullScreenApiEnabled() { Function opening '{' on its own line in this code, please. There's a number of these to fix here. +static void VisitDocTree(nsIDocShellTreeNode* aNode, nsDocTreeVisitor* aVisitor) Seems like this coulde be written using the existing map of sub documents (mSubDocuments) that we already have in nsDocument rather than using the docshell tree. r- only because of the above mentioned issue, other than that this looks good, but I'd like to look through the updated patch with that fixed (or have it explained why that won't work and read through this code once more).
Attachment #552291 - Flags: review?(jst) → review-
Comment on attachment 550855 [details] [diff] [review] Patch 4 v1: iframe.mozallowfullscreen r=jst with the stricter iframe element check mentioned earlier.
Attachment #550855 - Flags: review?(jst) → review+
Attachment #552298 - Flags: review?(jst) → review+
Attached patch Patch 4 v1: iframe.mozallowfullscreen (deleted) β€” β€” Splinter Review
Patch 4 with stricter iframe element check.
Attachment #550855 - Attachment is obsolete: true
Attachment #555000 - Flags: review+
Comment on attachment 555000 [details] [diff] [review] Patch 4 v1: iframe.mozallowfullscreen >+ if (content->NodeInfo()->Equals(nsGkAtoms::iframe, kNameSpaceID_XHTML) && Please change this to content->IsHTML(nsGkAtoms::iframe) >+ !content->HasAttr(kNameSpaceID_None, nsGkAtoms::mozallowfullscreen)) { And I approve of boolean attributes.
Attachment #550861 - Attachment is obsolete: true
Attached patch Patch 1 v3: dom/content changes for full-screen api (obsolete) (deleted) β€” β€” Splinter Review
With style changes and using nsIDocuments::EnumerateSubDocuments() to traverse document trees.
Attachment #552291 - Attachment is obsolete: true
Attachment #555319 - Flags: review?(jst)
Comment on attachment 554985 [details] [diff] [review] Patch 9: Break out of full-screen mode upon non-alphanumeric key input >-nsDocument::MozCancelFullScreen() >+nsDocument::MozCancelFullScreen() { { should be in its own line >+ if (!nsContentUtils::IsRequestFullScreenAllowed()) >+ return NS_OK; if (expr) { stmt; } >+ if (IsFullScreenAndRestrictedKeyEvent(mCurrentEventContent, aEvent) && >+ aEvent->message == NS_KEY_DOWN) { Maybe aEvent->message == NS_KEY_DOWN first and then IsFullScreenAndRestrictedKeyEvent Keys are pretty restricted. I wonder if we should enable more keys when a11y is active.
Attachment #554985 - Flags: review?(Olli.Pettay) → review+
Olli pointed out in another patch's review that my brace style on if statements didn't always match the local convention. I also did this in this patch, so I've fixed that and am re-requesting review.
Attachment #555319 - Attachment is obsolete: true
Attachment #555587 - Flags: review?(jst)
Attachment #555319 - Flags: review?(jst)
(In reply to Ms2ger from comment #111) > Comment on attachment 555000 [details] [diff] [review] > Patch 4 v1: iframe.mozallowfullscreen > > >+ if (content->NodeInfo()->Equals(nsGkAtoms::iframe, kNameSpaceID_XHTML) && > > Please change this to content->IsHTML(nsGkAtoms::iframe) AFAICT this is equivalent, so unless anyone objects I'll make this change when landing.
> AFAICT this is equivalent Yeah, it is. The IsHTML() version is preferred, generally.
Attached patch WIP Patch: e10s support (deleted) β€” β€” Splinter Review
This patch is a work-in-progress to add e10s support for the DOM full-screen API. It works, with the one exception that if you press F11 (which exits browser full-screen mode) while in DOM full-screen mode, the window will exit full-screen mode, but the document will remain in DOM full-screen mode, with the full-screen element taking up the entire viewable area. This is because there's no way to get access to the corresponding PBrowserParent in C++ in the chrome process when the Xul window's full-screen mode is toggled. bz and cjones have a tentative plan to sit down and figure out how to do this, once that's done we can fix this properly. I won't block on e10s support, we want to land the DOM full-screen API before e10s support will be turned on.
Attachment #555587 - Flags: review?(jst) → review+
Depends on: 684618
Depends on: 684620
Depends on: 684625
Depends on: 684627
Depends on: 684628
Flags: in-testsuite? → in-testsuite+
Additional work: * Implement the full-screen view-mode media query (bug 678173) to apply when in browser *or* DOM full-screen mode. * Add a pseudo-class for elements which are ancestors of the full-screen element. * Add a style rule to set overflow-hidden on the root element when it is an ancestor of the full-screen element. Currently there's nothing hiding scroll bars on the root full-screen element.
Depends on: 678173
Depends on: 685069
Depends on: 685402
Depends on: 685779
Depends on: 687687
Depends on: 688082
Depends on: 688648
Depends on: 689058
Depends on: 690989
Depends on: 691583
Depends on: 691947
Depends on: 692593
Depends on: 694204
Depends on: 694690
Depends on: 695158
Depends on: 696918
Depends on: 697636
Depends on: 698340
No longer blocks: 695935
Depends on: 695935
Depends on: 698977
Depends on: 699354
Depends on: 699722
Depends on: 699885
Depends on: 700151
Depends on: 701251
Depends on: 701258
Depends on: 701260
Depends on: 701442
sheppy, I think this landed for Firefox 10 (not 9). Did I miss something? http://blog.pearce.org.nz/2011/11/firefoxs-html-full-screen-api-enabled.html
It's in 9 but preffed off, which is why the doc explains how to enable it if you want to experiment with it.
That said, the target milestone on this bug says mozilla9, which is where I get my information from as to what release something will be in. :)
(In reply to Eric Shepherd [:sheppy] from comment #126) > That said, the target milestone on this bug says mozilla9, which is where I > get my information from as to what release something will be in. :) Yeah, I think the documentation is great. Sheppy, do you think we can add a part to say that it is enabled by default in Fx10?
Done!
Thanks Sheppy. I've gone through the documentation and made a few tweaks, but overall it looks great!
Depends on: 700764
Depends on: 701640
Depends on: 701992
Depends on: 702146
Depends on: 702295
Depends on: 703079
Blocks: 703881
Depends on: 703367
Depends on: 704039
Depends on: 704709
Depends on: 705363
Depends on: 705234
Depends on: 706555
Depends on: 706672
Depends on: 708174
Depends on: 708553
Hi guys. I have tested this feature on latest Aurora and I've encountered the following issues. The test page is: http://pearce.org.nz/full-screen/ The test plan along with the testcases is: https://wiki.mozilla.org/Platform/Features/Full_Screen_APIs/TestPlan The issues are, on MAC OS X 10.6: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0a2) Gecko/20111219 Firefox/10.0a2 1. When pressing "full screen (f=toggle)" button, the page in full screen starts to flicker. 2. When pressing the "create windowed plugin" button and then the "full screen button" the page goes in full screen even though it shouldn't. 3. When pressing the "create windowless plugin" button and then the "full screen button" the page goes in full screen even though it shouldn't. 4. When pressing the "full screen" button and then the "create windowed plugin" button, the page doesn't exit from full screen even though it should. 5. When pressing the "full screen" button and then the "create windowless plugin" button, the page doesn't exit from full screen even though it should. 6. The "remove full screen element from doc" button doesn't work. When in full screen, when pressing this button a poem appears. Is this intended? 7. The "request full screen and transplant" button does not work. on WINDOWS 7: Mozilla/5.0 (Windows NT 6.1; rv:10.0a2) Gecko/20111219 Firefox/10.0a2 1. When pressing the "create windowless plugin" button and then the "full screen button" the page goes in full screen even though it shouldn't. 2. When pressing the "full screen" button and then the "create windowless plugin" button, the page doesn't exit from full screen even though it should. 3. The "remove full screen element from doc" button doesn't work. When in full screen, when pressing this button a poem appears. Is this intended? 4. The "request full screen and transplant" button does not work. on WINDOWS XP Mozilla/5.0 (Windows NT 5.1; rv:10.0a2) Gecko/20111219 Firefox/10.0a2 1. When pressing the "create windowless plugin" button and then the "full screen button" the page goes in full screen even though it shouldn't. 2. When pressing the "full screen" button and then the "create windowless plugin" button, the page doesn't exit from full screen even though it should. 3. The "remove full screen element from doc" button doesn't work. When in full screen, when pressing this button a poem appears. Is this intended? 4. The "request full screen and transplant" button does not work. on Ubuntu 11.10 x64 Mozilla/5.0 (X11; Linux x86_64; rv:10.0a2) Gecko/20111220 Firefox/10.0a2 1. When pressing the "create windowless plugin" button and then the "full screen button" the page goes in full screen even though it shouldn't. 2. When pressing the "full screen" button and then the "create windowless plugin" button, the page doesn't exit from full screen even though it should. 3. The "remove full screen element from doc" button doesn't work. When in full screen, when pressing this button a poem appears. Is this intended? 4. The "request full screen and transplant" button does not work. I've conducted this tests on order to signed-off this feature on Aurora.
(In reply to Vlad [QA] from comment #130) > Hi guys. > > I have tested this feature on latest Aurora and I've encountered the > following issues. > The test page is: http://pearce.org.nz/full-screen/ > The test plan along with the testcases is: > https://wiki.mozilla.org/Platform/Features/Full_Screen_APIs/TestPlan > > The issues are, on MAC OS X 10.6: > Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0a2) Gecko/20111219 > Firefox/10.0a2 > > 1. When pressing "full screen (f=toggle)" button, the page in full screen > starts to flicker. I assume what you're referring to is covered by Bug 708174. > 2. When pressing the "create windowed plugin" button and then the "full > screen button" the page goes in full screen even though it shouldn't. > 3. When pressing the "create windowless plugin" button and then the "full > screen button" the page goes in full screen even though it shouldn't. > 4. When pressing the "full screen" button and then the "create windowed > plugin" button, the page doesn't exit from full screen even though it should. > 5. When pressing the "full screen" button and then the "create windowless > plugin" button, the page doesn't exit from full screen even though it should. Windowed plugins do not cause full-screen requests to be denied and do not cause full-screen to exit on MacOSX. > 6. The "remove full screen element from doc" button doesn't work. When in > full screen, when pressing this button a poem appears. Is this intended? When in full-screen, pressing the "remove full screen element from doc" should cause full-screen to exit and should cause the poem to appear. > 7. The "request full screen and transplant" button does not work. This is expected behaviour on all platforms. > on WINDOWS 7: > Mozilla/5.0 (Windows NT 6.1; rv:10.0a2) Gecko/20111219 Firefox/10.0a2 > > 1. When pressing the "create windowless plugin" button and then the "full > screen button" the page goes in full screen even though it shouldn't. This is expected behaviour on all platforms. > 2. When pressing the "full screen" button and then the "create windowless > plugin" button, the page doesn't exit from full screen even though it should. This is expected behaviour on all platforms. > 3. The "remove full screen element from doc" button doesn't work. When in > full screen, when pressing this button a poem appears. Is this intended? When in full-screen, pressing the "remove full screen element from doc" should cause full-screen to exit and should cause the poem to appear. This is expected behaviour on all platforms. > 4. The "request full screen and transplant" button does not work. This is expected behaviour on all platforms. > on WINDOWS XP > Mozilla/5.0 (Windows NT 5.1; rv:10.0a2) Gecko/20111219 Firefox/10.0a2 > > 1. When pressing the "create windowless plugin" button and then the "full > screen button" the page goes in full screen even though it shouldn't. > 2. When pressing the "full screen" button and then the "create windowless > plugin" button, the page doesn't exit from full screen even though it should. > 3. The "remove full screen element from doc" button doesn't work. When in > full screen, when pressing this button a poem appears. Is this intended? > 4. The "request full screen and transplant" button does not work. This is expected behaviour on all platforms. > > on Ubuntu 11.10 x64 > Mozilla/5.0 (X11; Linux x86_64; rv:10.0a2) Gecko/20111220 Firefox/10.0a2 > > 1. When pressing the "create windowless plugin" button and then the "full > screen button" the page goes in full screen even though it shouldn't. > 2. When pressing the "full screen" button and then the "create windowless > plugin" button, the page doesn't exit from full screen even though it should. > 3. The "remove full screen element from doc" button doesn't work. When in > full screen, when pressing this button a poem appears. Is this intended? > 4. The "request full screen and transplant" button does not work. This is expected behaviour on all platforms.
(In reply to Chris Pearce (:cpearce) (Away until Jan 4 NZST) from comment #131) > > 4. The "request full screen and transplant" button does not work. > > This is expected behaviour on all platforms. What if I press on "Request fullscreen" button on the red div first, and then "Request fullscreen and transplant" ? The page doesn't exit from fullscreen mode.
Requesting full-screen while in full-screen mode should not exit full-screen, even if the new request is denied.
Depends on: 716107
Hi Chris, What about if activate general full screen in Firefox (F11 key), then enter DOM full screen and then exit DOM full screen? General full screen is not active anymore. There is a different behavior if trying with youtube full screen for example instead of DOM full screen. Related issue here: https://bugzilla.mozilla.org/show_bug.cgi?id=701251#c9
Depends on: 714675
Depends on: 712181
Depends on: 721711
No longer blocks: 718107
Depends on: 718107
Depends on: 720278
Depends on: 721026
Depends on: 702000
Depends on: 725466
Depends on: 725311
Depends on: 726474
Depends on: CVE-2012-0460
Depends on: 730583
Depends on: 729872
Depends on: 731029
Depends on: 738275
Depends on: 738764
Depends on: 714809
Depends on: 743198
Depends on: 745067
Depends on: 744860
Depends on: 746437
Depends on: 748198
Depends on: 760422
Depends on: 779286
Depends on: 779324
Flags: sec-review+
Depends on: 801891
Depends on: 805301
Depends on: 743564
Depends on: 867967
Depends on: 1053783
Depends on: 968526
Depends on: 1111099
Depends on: 1114370
No longer depends on: 1114370
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: