Last Comment Bug 704039 - Make document.mozCancelFullScreen restore previous full-screen state
: Make document.mozCancelFullScreen restore previous full-screen state
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Chris Pearce (:cpearce)
:
Mentors:
Depends on:
Blocks: 545812
  Show dependency treegraph
 
Reported: 2011-11-20 19:11 PST by Chris Pearce (:cpearce)
Modified: 2012-02-14 11:34 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch 1 v1: Rename nsDocument::SetFullScreenState(Element*,bool) to nsDocument::SetFullScreenElement(Element*) (8.96 KB, patch)
2011-11-30 17:40 PST, Chris Pearce (:cpearce)
bzbarsky: review+
Details | Diff | Splinter Review
Patch 2 v1: Implement full-screen element stacking (65.42 KB, patch)
2011-11-30 17:56 PST, Chris Pearce (:cpearce)
bzbarsky: review+
Details | Diff | Splinter Review

Description Chris Pearce (:cpearce) 2011-11-20 19:11:57 PST
The W3C editor's draft of the full-screen spec [1] 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.

[1] http://dvcs.w3.org/hg/fullscreen/raw-file/tip/Overview.html
Comment 1 Chris Pearce (:cpearce) 2011-11-30 17:40:31 PST
Created attachment 578145 [details] [diff] [review]
Patch 1 v1: Rename nsDocument::SetFullScreenState(Element*,bool) to nsDocument::SetFullScreenElement(Element*)

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.
Comment 2 Chris Pearce (:cpearce) 2011-11-30 17:56:06 PST
Created attachment 578148 [details] [diff] [review]
Patch 2 v1: Implement full-screen element stacking

* 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).
Comment 3 Boris Zbarsky [:bz] 2011-12-01 13:10:42 PST
Comment on attachment 578145 [details] [diff] [review]
Patch 1 v1: Rename nsDocument::SetFullScreenState(Element*,bool) to nsDocument::SetFullScreenElement(Element*)

r=me
Comment 4 Boris Zbarsky [:bz] 2011-12-01 13:52:49 PST
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 5 Boris Zbarsky [:bz] 2011-12-01 13:53:49 PST
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.
Comment 6 Chris Pearce (:cpearce) 2011-12-04 13:40:42 PST
(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.
Comment 7 Boris Zbarsky [:bz] 2011-12-05 20:26:51 PST
> DispatchFullScreenChange() uses nsPLDOMEvent::PostDOMEvent()

Ah, ok.  Perfect.
Comment 8 Boris Zbarsky [:bz] 2011-12-05 20:55:04 PST
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.
Comment 9 Chris Pearce (:cpearce) 2011-12-06 00:25:39 PST
(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!
Comment 12 Eric Shepherd [:sheppy] 2012-02-14 11:34:53 PST
Doc updated:

https://developer.mozilla.org/en/DOM/document.mozCancelFullScreen

And mentioned on Firefox 11 for developers.

Note You need to log in before you can comment on or make changes to this bug.