Closed Bug 704039 Opened 8 years ago Closed 8 years ago
.moz Cancel Full Screen restore previous full-screen state
Patch 1 v1: Rename nsDocument::SetFullScreenState(Element*,bool) to nsDocument::SetFullScreenElement(Element*)
8.96 KB, patch
|Details | Diff | Splinter Review|
65.42 KB, patch
|Details | Diff | Splinter Review|
The W3C editor's draft of the full-screen spec  now includes a stack of full-screen elements, so that Document.mozCancelFullScreen() "rolls-back" to restore the previously full-screen element to full-screen state. We should implement this, targeting Firefox 11.  http://dvcs.w3.org/hg/fullscreen/raw-file/tip/Overview.html
Whenever we call nsDocument::SetFullScreenState(Element*,bool) with a null element, the bool argument is false, and whenever the Element is non-null, the bool is true, so we can rename that SetFullScreenElement() and remove the bool argument.
Attachment #578145 - Flags: review?(bzbarsky)
* Change full-screen model to match the W3 spec of having a stack of full-screen elements, so that every time you request full-screen the existing full-screen element (if any) is pushed onto a stack, and when Document.mozCancelFullScreen() is called we rollback to having the previous full-screen element as the full-screen element. See http://dvcs.w3.org/hg/fullscreen/raw-file/tip/Overview.html for spec. * Each document maintains its own stack of full-screen elements, and we still push the containing elements onto the stacks in ancestor documents too. The popping algorithm will ensure that "containing full-screen elements" won't get set to full-screen state upon calling mozCancelFullScreen(), only elements which directly requested full-screen should return to full-screen state. * Calling Document.mozCancelFullScreen() automatically clears the stacks of every descendent document, and sets the full-screen element to the previous non-container full-screen element in this or ancestor documents. * Add a static function, nsIDocument::ExitFullScreen() to "fully exit" full-screen. This clears all full-screen stacks of all full-screen documents. * Deny requests for full-screen in a document which has full-screen subdocuments, and when requesting full-screen on an element not descendent from the current full-screen element in the requesting document (as required by the spec).
Attachment #578148 - Flags: review?(bzbarsky)
Comment on attachment 578145 [details] [diff] [review] Patch 1 v1: Rename nsDocument::SetFullScreenState(Element*,bool) to nsDocument::SetFullScreenElement(Element*) r=me
Attachment #578145 - Flags: review?(bzbarsky) → review+
A question about patch 2. DispatchFullScreenChange seems like it can trigger arbitrary script including more fullscreen requests or cancellations. Does the code actually handle that sort of thing properly? It's not clear to me that it does...
Comment on attachment 578148 [details] [diff] [review] Patch 2 v1: Implement full-screen element stacking >+ static bool IsFullScreenAncestor(Element* aElement); Why not just make this a member method on Element? >+++ b/content/base/public/nsIDocument.h You need an IID change here. Well, maybe not strictly need, but I think it would be a good idea. More after the answer to my question about reentry.
(In reply to Boris Zbarsky (:bz) from comment #4) > A question about patch 2. DispatchFullScreenChange seems like it can > trigger arbitrary script [...] DispatchFullScreenChange() uses nsPLDOMEvent::PostDOMEvent(), which is supposed to dispatch asynchronously, so all state changes in the doctree should be complete before any of the event handlers run. Unless I'm missing something? Callers of DispatchFullScreenChange() assume that no event handlers will run while they're making state changes to the doctree.
> DispatchFullScreenChange() uses nsPLDOMEvent::PostDOMEvent() Ah, ok. Perfect.
Comment on attachment 578148 [details] [diff] [review] Patch 2 v1: Implement full-screen element stacking Is there a reason that ClearFullScreenStack() uses GetFullScreenElement() and not FullScreenStackTop()? r=me with that explained or fixed and the other comments addressed.
Attachment #578148 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #8) > Is there a reason that ClearFullScreenStack() uses GetFullScreenElement() > and not FullScreenStackTop()? Nope. I'll change make ClearFullScreenStack() to use FullScreenStackTop(). Thanks for review!
Target Milestone: --- → mozilla11
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Doc updated: https://developer.mozilla.org/en/DOM/document.mozCancelFullScreen And mentioned on Firefox 11 for developers.
You need to log in before you can comment on or make changes to this bug.