Closed
Bug 545812
Opened 15 years ago
Closed 13 years ago
API for controlling fullscreen from content
Categories
(Core :: General, defect)
Core
General
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)
8.17 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
7.93 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
8.05 KB,
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
12.35 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
24.89 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
4.90 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
23.62 KB,
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
12.66 KB,
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
Comment 1•15 years ago
|
||
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
Comment 2•15 years ago
|
||
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
Updated•15 years ago
|
Whiteboard: [evang-wanted]
Comment 3•15 years ago
|
||
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).
Comment 4•15 years ago
|
||
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)
Comment 5•15 years ago
|
||
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
Updated•14 years ago
|
Keywords: dev-doc-needed
(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
Comment 10•14 years ago
|
||
(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
Comment 11•14 years ago
|
||
(In reply to comment #10)
> You want window.fullScreen = true
Oops, sorry, apparently this is read-only without chrome privileges.
Updated•14 years ago
|
Assignee: nobody → josh
Comment 12•14 years ago
|
||
Comment 13•14 years ago
|
||
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.
Comment 14•14 years ago
|
||
Comment 15•14 years ago
|
||
Comment 16•14 years ago
|
||
Comment 17•14 years ago
|
||
Updated•14 years ago
|
Attachment #495682 -
Attachment description: Part 1 → Part 1: Window state goop, content events, styling.
Updated•14 years ago
|
Attachment #495682 -
Attachment is obsolete: true
Comment 18•14 years ago
|
||
Updated•14 years ago
|
Attachment #495691 -
Attachment is obsolete: true
Comment 19•14 years ago
|
||
Updated•14 years ago
|
Attachment #495690 -
Attachment description: Part 3 → Part 3: Key disabling.
Updated•14 years ago
|
Attachment #495690 -
Attachment description: Part 3: Key disabling. → Part 3: Extend nsGlobalWindow API with key disabling.
Updated•14 years ago
|
Attachment #495689 -
Attachment description: Part 2 → Part 2: Key disabling.
Updated•14 years ago
|
Attachment #495692 -
Attachment description: Part 5 → Part 5: Frontend prototype.
Updated•14 years ago
|
Attachment #495721 -
Attachment is obsolete: true
Comment 20•14 years ago
|
||
Updated•14 years ago
|
Attachment #495722 -
Attachment is obsolete: true
Comment 21•14 years ago
|
||
Updated•14 years ago
|
Attachment #495689 -
Attachment description: Part 2: Key disabling. → Part 3: Key disabling.
Updated•14 years ago
|
Attachment #495690 -
Attachment description: Part 3: Extend nsGlobalWindow API with key disabling. → Part 4: Extend nsGlobalWindow API with key disabling.
Updated•14 years ago
|
Comment 23•14 years ago
|
||
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
Comment 26•14 years ago
|
||
(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.
Assignee | ||
Comment 27•13 years ago
|
||
I'll see if I can make some progress on this.
Assignee: nobody → chris
Comment 28•13 years ago
|
||
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.
Assignee | ||
Comment 29•13 years ago
|
||
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.
Comment 30•13 years ago
|
||
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.
Comment 31•13 years ago
|
||
Just FYI, Safari does not have a smooth transition with this API. Only with the fullscreen video.
Comment 32•13 years ago
|
||
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.
Comment 33•13 years ago
|
||
(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.
Comment 34•13 years ago
|
||
(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.
Comment 35•13 years ago
|
||
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
Assignee | ||
Comment 36•13 years ago
|
||
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)
Assignee | ||
Comment 37•13 years ago
|
||
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)
Assignee | ||
Comment 38•13 years ago
|
||
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)
Assignee | ||
Comment 39•13 years ago
|
||
Restrict HTMLElement.mozRequestFullScreen so that it's only granted if all ancestor iframes have the mozallowfullscreen attribute set.
Attachment #550855 -
Flags: review?(jst)
Assignee | ||
Comment 40•13 years ago
|
||
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)
Assignee | ||
Comment 41•13 years ago
|
||
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)
Assignee | ||
Comment 42•13 years ago
|
||
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.
Assignee | ||
Comment 43•13 years ago
|
||
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.
Assignee | ||
Comment 45•13 years ago
|
||
Also, try builds with the above patches applied are available here:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/cpearce@mozilla.com-938cd232dd3f/
Comment 46•13 years ago
|
||
(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?
Comment 47•13 years ago
|
||
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?
Comment 48•13 years ago
|
||
(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 49•13 years ago
|
||
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 51•13 years ago
|
||
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 52•13 years ago
|
||
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-
Updated•13 years ago
|
Keywords: sec-review-needed
Assignee | ||
Comment 53•13 years ago
|
||
(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.
Assignee | ||
Comment 54•13 years ago
|
||
(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()?
Comment 55•13 years ago
|
||
(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.
Assignee | ||
Comment 56•13 years ago
|
||
bz: see comment 54 for answers to your questions.
Comment 57•13 years ago
|
||
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.
Assignee | ||
Comment 58•13 years ago
|
||
(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
Assignee | ||
Updated•13 years ago
|
Comment 59•13 years ago
|
||
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
Comment 60•13 years ago
|
||
(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.
Updated•13 years ago
|
Whiteboard: [evang-wanted] → [evang-wanted][sr:curtisk]
Assignee | ||
Comment 63•13 years ago
|
||
(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.
Comment 64•13 years ago
|
||
(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.
Comment 65•13 years ago
|
||
> 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.
Comment 66•13 years ago
|
||
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.
Comment 67•13 years ago
|
||
http://mayscript.com/blog/eric/browser-ui-spoofing-attack-revisited is very relevant. I'm with Jesse: we don't want this feature.
Comment 68•13 years ago
|
||
I think a web page should never be able to pop up to full screen.
Assignee | ||
Comment 69•13 years ago
|
||
(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.
Assignee | ||
Comment 70•13 years ago
|
||
(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.
Assignee | ||
Comment 71•13 years ago
|
||
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)
Assignee | ||
Comment 72•13 years ago
|
||
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)
Assignee | ||
Comment 73•13 years ago
|
||
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+
Assignee | ||
Comment 74•13 years ago
|
||
Implement -moz-full-screen media query.
Attachment #552297 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•13 years ago
|
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
Assignee | ||
Updated•13 years ago
|
Attachment #550863 -
Attachment description: Patch 6 v1: mochitests → Patch 7 v1: mochitests
Assignee | ||
Comment 75•13 years ago
|
||
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 76•13 years ago
|
||
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+
Assignee | ||
Comment 77•13 years ago
|
||
* 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.
Comment 79•13 years ago
|
||
(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 80•13 years ago
|
||
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 81•13 years ago
|
||
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?
Assignee | ||
Comment 82•13 years ago
|
||
(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.
Assignee | ||
Comment 84•13 years ago
|
||
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 85•13 years ago
|
||
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?
Assignee | ||
Comment 86•13 years ago
|
||
Dao: Exit full-screen mode when escape is pressed in DOM full-screen mode.
Comment 87•13 years ago
|
||
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...
Updated•13 years ago
|
Attachment #550861 -
Flags: review?(dao) → review-
Assignee | ||
Comment 88•13 years ago
|
||
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+
Comment 89•13 years ago
|
||
>> * 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.
Comment 90•13 years ago
|
||
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.
Comment 91•13 years ago
|
||
(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.
Assignee | ||
Comment 92•13 years ago
|
||
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)
Assignee | ||
Comment 93•13 years ago
|
||
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)
Assignee | ||
Comment 94•13 years ago
|
||
Test key input restrictions and test that pressing escape exits full-screen mode.
Attachment #554300 -
Flags: review?(Olli.Pettay)
Comment 95•13 years ago
|
||
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 96•13 years ago
|
||
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 97•13 years ago
|
||
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.
Comment 99•13 years ago
|
||
(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.
Assignee | ||
Comment 100•13 years ago
|
||
(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.
Assignee | ||
Comment 102•13 years ago
|
||
(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...".
Assignee | ||
Comment 103•13 years ago
|
||
(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.
Updated•13 years ago
|
Whiteboard: [evang-wanted][sr:curtisk] → [evang-wanted][secr:curtisk]
Assignee | ||
Comment 105•13 years ago
|
||
* 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 106•13 years ago
|
||
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?
Assignee | ||
Comment 107•13 years ago
|
||
(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 108•13 years ago
|
||
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 109•13 years ago
|
||
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+
Updated•13 years ago
|
Attachment #552298 -
Flags: review?(jst) → review+
Assignee | ||
Comment 110•13 years ago
|
||
Patch 4 with stricter iframe element check.
Attachment #550855 -
Attachment is obsolete: true
Attachment #555000 -
Flags: review+
Comment 111•13 years ago
|
||
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.
Assignee | ||
Updated•13 years ago
|
Attachment #550861 -
Attachment is obsolete: true
Assignee | ||
Comment 112•13 years ago
|
||
With style changes and using nsIDocuments::EnumerateSubDocuments() to traverse document trees.
Attachment #552291 -
Attachment is obsolete: true
Attachment #555319 -
Flags: review?(jst)
Comment 113•13 years ago
|
||
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+
Assignee | ||
Comment 114•13 years ago
|
||
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)
Assignee | ||
Comment 115•13 years ago
|
||
(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.
Comment 116•13 years ago
|
||
> AFAICT this is equivalent
Yeah, it is. The IsHTML() version is preferred, generally.
Assignee | ||
Comment 117•13 years ago
|
||
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.
Updated•13 years ago
|
Attachment #555587 -
Flags: review?(jst) → review+
Assignee | ||
Comment 118•13 years ago
|
||
Landed on m-i, preffed off:
http://hg.mozilla.org/integration/mozilla-inbound/rev/f212867dce42
http://hg.mozilla.org/integration/mozilla-inbound/rev/8794e3692ee8
http://hg.mozilla.org/integration/mozilla-inbound/rev/be0876c4aa2b
http://hg.mozilla.org/integration/mozilla-inbound/rev/26247cdde2bc
http://hg.mozilla.org/integration/mozilla-inbound/rev/4a40fbf67f12
http://hg.mozilla.org/integration/mozilla-inbound/rev/4c972377ec27
http://hg.mozilla.org/integration/mozilla-inbound/rev/d7cd9750988a
I had forgotten to update the IID in nsIDocument.h in two patches when I was changing the interface, so I took the liberty of fixing that before landing.
I will file follow up bugs for the remaining work to be completed.
http://hg.mozilla.org/mozilla-central/rev/f212867dce42
http://hg.mozilla.org/mozilla-central/rev/8794e3692ee8
http://hg.mozilla.org/mozilla-central/rev/be0876c4aa2b
http://hg.mozilla.org/mozilla-central/rev/26247cdde2bc
http://hg.mozilla.org/mozilla-central/rev/4a40fbf67f12
http://hg.mozilla.org/mozilla-central/rev/4c972377ec27
http://hg.mozilla.org/mozilla-central/rev/d7cd9750988a
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite?
Keywords: student-project
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Assignee | ||
Updated•13 years ago
|
Flags: in-testsuite? → in-testsuite+
Assignee | ||
Comment 120•13 years ago
|
||
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
Comment 121•13 years ago
|
||
For documentation reference: http://blog.pearce.org.nz/2011/09/mozilla-full-screen-api-progress-update.html
Link to review notes https://wiki.mozilla.org/Security/Reviews/Firefox10/CodeEditor/FullScreenAPI
Keywords: sec-review-needed → sec-review-complete
Assignee | ||
Updated•13 years ago
|
Comment 123•13 years ago
|
||
Documented:
https://developer.mozilla.org/en/DOM/Using_full-screen_mode
https://developer.mozilla.org/en/DOM/Element.mozRequestFullScreen
https://developer.mozilla.org/en/DOM/document.mozCancelFullScreen
https://developer.mozilla.org/en/DOM/document.mozFullScreen
https://developer.mozilla.org/en/DOM/document.mozFullScreenElement
https://developer.mozilla.org/en/DOM/document.mozFullScreenEnabled
https://developer.mozilla.org/en/CSS/%3A-moz-full-screen
https://developer.mozilla.org/en/DOM/DOM_event_reference
https://developer.mozilla.org/en/HTML/Element/iframe
https://developer.mozilla.org/en/DOM/HTMLIFrameElement
https://developer.mozilla.org/en/DOM/Document
https://developer.mozilla.org/en/CSS/CSS_Reference/Mozilla_Extensions#Pseudo-elements_and_pseudo-classes
Listed on Firefox 9 for developers.
Comment 124•13 years ago
|
||
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
Comment 125•13 years ago
|
||
It's in 9 but preffed off, which is why the doc explains how to enable it if you want to experiment with it.
Comment 126•13 years ago
|
||
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. :)
Comment 127•13 years ago
|
||
(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?
Comment 128•13 years ago
|
||
Done!
Updated•13 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Assignee | ||
Comment 129•13 years ago
|
||
Thanks Sheppy. I've gone through the documentation and made a few tweaks, but overall it looks great!
Comment 130•13 years ago
|
||
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.
Assignee | ||
Comment 131•13 years ago
|
||
(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.
Comment 132•13 years ago
|
||
(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.
Assignee | ||
Comment 133•13 years ago
|
||
Requesting full-screen while in full-screen mode should not exit full-screen, even if the new request is denied.
Comment 134•13 years ago
|
||
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
Updated•13 years ago
|
Updated•13 years ago
|
Blocks: gecko-games
Updated•13 years ago
|
Depends on: CVE-2012-0460
Updated•12 years ago
|
Flags: sec-review+
You need to log in
before you can comment on or make changes to this bug.
Description
•