We need a pseudo-class for elements which are ancestors of the DOM full-screen element.

If you track the current full-screen element for the document (if any) via the ESM, then you could model this other pseudo-class on the way we do :hover and :active (except without the label bit), right?

* Implements -moz-full-screen-ancestor, to apply to all ancestors of the full-screen element, except containing frames in parent documents - these are the full-screen element in their own documents, so their ancestors have the class applied however. * I implemented the pseudoclass using the event state manager. The full-screen element is managed in the nsDocument though, as we seem to be re-creating some of the pres contexts (and thus the event state managers) when we transition to full-screen mode, which would wipe any state we stored in the event state manager. * Add a style rule using this pseudoclass to hide scroll bars of full-screen documents.

So the full-screen element for a document can be not in the document tree?

Correct. In that case the browser goes full-screen, but no element will have the -moz-full-screen pseudoclass applied. This should be identical to F11 full-screen mode.

OK, what elemens should :-moz-full-screen-ancestor match in that situation, if any?

It shouldn't match any elements in that situation. It won't with this patch, because in nsDocument::RequestFullScreen() we don't call nsDocument::SetFullScreenElement() if the element isn't in the document. But I *should* be calling SetFullScreenElement() if the element's not in the doc (but not setting the style state I guess), otherwise if ane element requests full-screen while not in the doc and is subsequently added to the document, it won't be detected as the full-screen element in nsGenericHTMLElement::BindToTree() and won't get the styles applied... I'll update the patch to handle that... Anything else?

Thanks for the explanation! Now that I know what the goal is, I'll review this tomorro morning.

OK, so I still don't quite understand why we need to keep track of the full screen element on the document if it's not actually in the document tree. Is this just so we can do the thing in nsGenericHTMLElement::BindToTree? But can't we just check the :-moz-full-screen flag on |this| in that method instead of comparing to aDocument->GetFullScreenElement? Or if that flag is not set at that point, make whatever code _does_ end up setting it handle the parent thing?

We track the full-screen element on the document so that if it is removed from the document and then re-added somewhere else in the same document, it will continue to be the full-screen element. This is so you can move elements around inside a document without them losing full-screen status. We could check the :-moz-full-screen flag in nsGenericHTMLElement::BindToTree() instead. We'd need to reset that flag in nsDocument::AdoptNode() as well then (I assume AdoptNode() is called before BindToTree() when adopting), as conceptually transplanting the full-screen element should cause it to lose its full-screen status. Damn, what I'm doing in nsGenericHTMLElement::BindToTree() won't work properly. When the actual full-screen element is in a subdocument, calling nsDocument::RequestFullScreen() again on a containing frame would cause the full-screen element in the subdocument tree (and any containing frames who also have full-screen status) to lose their full-screen status. I'll fix this in the next revision...

> so that if it is removed from the document and then re-added somewhere else in the same > document, it will continue to be the full-screen element. That was the case before this path too, though, right? > I assume AdoptNode() is called before BindToTree() when adopting Yes. Presumably we need to update :-moz-full-screen no matter what when adopting, right? Still wrapping my head around the setup here; sorry it's taking so long...

(In reply to Boris Zbarsky (:bz) from comment #10) > > so that if it is removed from the document and then re-added somewhere else in the same > > document, it will continue to be the full-screen element. > > That was the case before this path too, though, right? Yes, the full-screen element was still tracked on the document before this patch. > Presumably we need to update :-moz-full-screen no matter what when adopting, > right? Yeah, when adopting we need to reset -moz-full-screen and -moz-full-screen-ancestor state.

* Add event state NS_EVENT_STATE_FULL_SCREEN_ANCESTOR to denote -moz-full-screen-ancestor. * Use NS_EVENT_STATE_FULL_SCREEN_ANCESTOR and NS_EVENT_STATE_FULL_SCREEN to denote whether an element is the full-screen element or an ancestor of the full-screen element, rather than checking an element is a descendent of its document's mFullScreenElement element. * Added nsIDocument::NotifyFullScreenAncestorRemoved() and NotifyFullScreenElementAdded() to handle case where full-screen element is bound/unbound. We detect these cases by checking the state bits when adopting/binding elements. Added tests for these cases. * I abandoned using nsEventStateMananger::SetContentState() to track the -moz-full-screen-ancestor state, as it can't handle the case where we remove an element from a document but we require the state bits to remain in the removed subtree but not in the document. It's much simpler to just manage this states in nsDocument, when we manage the -moz-full-screen state bit. Unfortunately handling adding/removing/adopting the full-screen element required refactoring how we handle full-screen requests: * Requests for full-screen can come from any document not cross-origin to the focused document in the document tree. Previously I was resetting every document in the doc tree's full-screen state, and then re-enabling full-screen state in documents which were ancestors of the full-screen element. This meant that documents which were already in full-screen state and which were supposed to remain in full-screen state would have their full-screen state toggle off and on again, and dispatch a mozfullscreenchange event, which was misleading. I fixed this by keeping a list of the documents which were supposed to remain on, and resetting everthing else (in nsDocument::RequestFullScreen()). * Added tests for the edge cases when adding/removing/out-of-doc full-screen elements.

Comment on attachment 566365 [details] [diff] [review] Patch v2 Ah bugger, this patch brokes the :-moz-full-screen pseudo class. I'll need to re-work this again. Sorry to bother you bz.

No worries. I was lame enough that I hadn't started looking at it yet. ;)

All comments I made about patch v2 apply to this patch as well. The difference is that now requesting full-screen on an element not in a document makes that document full-screen (not the top-level document as previous).

Comment on attachment 567572 [details] [diff] [review] Patch v3 >+++ b/content/base/public/nsContentUtils.h >+ static bool IsFullScreenElement(nsIContent* aElement); >+ static bool IsFullScreenAncestor(nsIContent* aElement); I think these should take Element* or just be member methods on Element. Looking at how these are used, would it make sense to have IsFullScreenAncestor return true if the element is either the full-screen element or any of its ancestors? Or put another way, these checks are _always_ done together, right? Seems like we should have one method that does both checks. Though more on this below, actually. >+++ b/content/base/public/nsIDocument.h >+ * this document, but must be *owned* by this document. The behaviour differs I would phrase that middle part as "but must have this document as its ownerDocument". >+ * Called when this document's full-screen element, or an ancestor of the >+ * full-screen element has been removed. This is so we can keep the This needs a comma before "has been removed". Also, I would say "has been removed from its parent", because we do this even when removing from a parent in a disconnected subtree, which is rather important.. >+ * Called when this document's full-screen element, or an ancestor of the >+ * full-screen element is re-added to the document after being removed. This part I'm not sure of. In particular, there are the following issues: 1) If |el| is the full-screen element and it's not in the document right now, and I do: var div = document.createElement("div"); div.appendChild(el); should this cause |div| to match the full-screen-ancestor style? Note that this is detectable via matchesSelector and the like. 2) BindToTree is called down the tree recursively. So notifying on addition of full-screen ancestors makes the whole thing O(N^2) in tree depth, right? It seems like it should be enough to notify when binding the full-screen element. There is no particular reason for these methods to be on the document, except that SetFullScreenAncestorStyle also needs to be callable from nsDocument::SetFullScreenState, right? How about just putting SetFullScreenAncestorStyle somewhere where both nsDocument and the current callers of these methods can call it (e.g. on nsEventStateManager, so push the loop down into SetFullScreenAncestorState), and not add this API to nsIDocument at all? >+++ b/content/base/src/nsContentUtils.cpp >+bool >+nsContentUtils::IsFullScreenElement(nsIContent* aElement) >+ return (state & NS_EVENT_STATE_FULL_SCREEN) == NS_EVENT_STATE_FULL_SCREEN; The return value is a bool; no need for that == business. RIP PRBool. ;) >+bool >+nsContentUtils::IsFullScreenAncestor(nsIContent* aElement) >+ return (state & NS_EVENT_STATE_FULL_SCREEN_ANCESTOR) == NS_EVENT_STATE_FULL_SCREEN_ANCESTOR; Same here. >+++ b/content/base/src/nsDocument.cpp > nsDocument::AdoptNode(nsIDOMNode *aAdoptedNode, nsIDOMNode **aResult) >+ // Detect if we're adopting some other document's full-screen element, >+ // and update full-screen styles as appropriate if so. >+ nsCOMPtr<nsIContent> content = do_QueryInterface(aAdoptedNode); If you put this slightly lower, after the QI to nsINode, you could use adoptedNode->IsElement() to check for elementness, etc. And it would save a QI. >+ static_cast<nsDocument*>(oldOwner)-> >+ SetFullScreenState(nsnull, true, true); Why not forward-declare the four-arg version of SetFullScreenState and use it here? >+class nsDispatchFullScreenChange : public nsRunnable >+ // The full-screen element was null or not in its document when >+ // dispatched, or the element has been transplanted into another >+ // document before the event had a chance to run. Dispatch the >+ // event to the full-screen document instead. This probably needs to be pushed back up into the spec, right? Why do we check IsInDoc() when initially setting mElement but check the owner doc against mDoc here? That gives a bizarre behavior where if you send an element not in the DOM tree into fullscreen the event will fire on the document, but if you do it to an element in the DOM tree and remove it before the event fires, the event will fire on the element. I think we should pick a consistent behavior and stick with it. Should the constructor assert that !aElement || aElement->OwnerDoc() == aDoc? >+SetFullScreenAncestorStyle(nsIContent* aElement, bool aEnableStyles) This should take an Element* as its first argument, imo. >+ nsIContent* ancestor = aElement; >+ while (ancestor = ancestor->GetParent()) { Please put parens around the assignment to make it clear that this is not meant to be a == (and silence the compiler warning about this). >+ NS_ASSERTION(ancestor->IsElement(), "Only call on elements"); Actually, this assert would fail if aElement is a descendant of a DocumentFragment. You need to actually check ancestor->IsElement().... >+SetFullScreenStyleState(nsIContent* aElement, That argument should be Element* >+ if (!aElement) { None of the callers can pass in null. Make this a precondition instead? >+ NS_ASSERTION(aElement->IsElement(), "Only call on elements"); The compiler would enforce this for you with the right argument type. ;) >+nsDocument::SetFullScreenState(Element* aElement, >+ bool aIsFullScreen, >+ bool aApplyStyles) All callers pass true for aApplyStyles. Why is that argument needed? > nsDocument::RequestFullScreen(Element* aElement) >+ // Requests for full-screen can come from any document in the document tree >+ // which is same-origin to the focused document, including sibling/cousin >+ // branches. Or totally separate windows that are same-origin, etc... I'd just drop the "in the document tree" and the part after the ',' there. Also, where does "focused" come in here? >+ Only documents which are a direct ancestor of the requesting >+ // full-screen element should have their full-screen state set after a >+ // request though. So to ensure we turn off the full-screen state in all >+ // non-ancestor documents, we keep a list of the ancestor documents (the >+ // "full-screen documents") and reset the full-screen state of all the other >+ // documents in this document tree. What about documents not in this document tree, per above? What's this code really trying to accomplish? >+ // Set the full-screen element. This sets the full-screen style on the >+ // element, and the full-screen-ancestor styles on ancestors of the element >+ // in this document. Note if the element is not in its document, we still >+ // apply these styles to the ancestors in its subtree so that we can tell >+ // that we need to apply the appropriate styles to the ancestors if this >+ // element is added to its document in future. Per above, we don't need it for that, but do need it for correctly handling matchesSelector, right? Should we be asserting somewhere that aElement->OwnerDoc() == this? >+ while (parent = child->GetParentDocument()) { Again, parens around the assignment. > nsDocument::GetMozFullScreenElement(nsIDOMHTMLElement **aFullScreenElement) >+ if (element && element->IsInDoc()) { >+ nsCOMPtr<nsIDOMHTMLElement> htmlElement(do_QueryInterface(element)); >+ NS_IF_ADDREF(*aFullScreenElement = htmlElement); Just replace those two lines with: CallQueryInterface(element, aFullScreenElement); >+ } else need to set *aFullScreenElement to null, right? > nsDocument::GetFullScreenElement() >+ if (!nsContentUtils::IsFullScreenApiEnabled()) { Why do we need this check? Can mFullScreenElement be non-null if the API is disabled? >+++ b/content/base/src/nsDocument.h >+++ b/content/html/content/src/nsGenericHTMLElement.cpp I assume this lives here because non-HTML elements can't be full-screen? Is that planned to stay so, or should this code just proactively live in nsGenericElement? >@@ -1064,16 +1064,23 @@ nsGenericHTMLElement::BindToTree(nsIDocu >+ if (nsContentUtils::IsFullScreenAncestor(content) || As I said above, I don't think we need this check. I didn't review the test very carefully; please let me know if you'd like me to. >+++ b/layout/style/ua.css >+:root:-moz-full-screen-ancestor { *|* at the beginning there, please, to make that clear. >+:-moz-full-screen-ancestor { And here. >+ /* Ancestors of a full-screen element should not induce stacking contexts >+ that would prevent the full-screen element from being on top. */ >+ z-index:auto; >+ /* Ancestors of a full-screen element should not be partially transparent, >+ since that would apply to the full-screen element and make the page visible >+ behind it. It would also create a pseudo-stacking-context that would let content >+ draw on top of the full-screen element. */ >+ opacity:1; >+ /* Ancestors of a full-screen element should not apply SVG masking, clipping, or >+ filtering, since that would affect the full-screen element and create a pseudo- >+ stacking context. */ >+ mask:none; Should those be !important? As things stand now page styles would override these, and since those are the default values to start with, I assume the only reason we have this rule is to override the page styles, no? Do the tests test this? r-, but this is really close! I'm sorry about the review lag; I should be able to do the next iteration much faster now that I have this all in my head.

(In reply to Boris Zbarsky (:bz) from comment #16) > Comment on attachment 567572 [details] [diff] [review] [diff] [details] [review] > Patch v3 > Thanks for the review! > >+++ b/content/base/public/nsIDocument.h > >+ * Called when this document's full-screen element, or an ancestor of the > >+ * full-screen element is re-added to the document after being removed. The new WhatWG draft of the full-screen spec denies full-screen requests from elements which are not in the document, and exits full-screen when the full-screen element is removed. Doing this makes much of this code simpler, so we're going to do that. > >+++ b/content/base/public/nsContentUtils.h > >+ static bool IsFullScreenElement(nsIContent* aElement); > >+ static bool IsFullScreenAncestor(nsIContent* aElement); > > I think these should take Element* or just be member methods on Element. Removing the ability for the full-screen element to not be in the document means we only need IsFullScreenAncestor() in once place, so I'll make it a simple static function. > >+++ b/content/base/src/nsContentUtils.cpp > >+bool > >+nsContentUtils::IsFullScreenElement(nsIContent* aElement) > >+ return (state & NS_EVENT_STATE_FULL_SCREEN) == NS_EVENT_STATE_FULL_SCREEN; > > The return value is a bool; no need for that == business. RIP PRBool. ;) If I merge nsContentUtils::IsFullScreenElement and nsContentUtils::IsFullScreenAncestor, then ((state & NS_EVENT_STATE_FULL_SCREEN) || (state & NS_EVENT_STATE_FULL_SCREEN_ANCESTOR)) gives an error with MSVC. But I can use state.HasAtLeastOneOfStates(NS_EVENT_STATE_FULL_SCREEN | NS_EVENT_STATE_FULL_SCREEN_ANCESTOR) is neater. > >+class nsDispatchFullScreenChange : public nsRunnable > > Why do we check IsInDoc() when initially setting mElement but check the > owner doc against mDoc here? That gives a bizarre behavior... Right. Removing the full-screen element from its document will now cause us to exit full-screen. Not sure what the best target for the mozfullscreenchange event is if the full-screen element is removed from the document before the event is dispatched. Maybe we should just leave the event dispatching to the element in this case. > > nsDocument::RequestFullScreen(Element* aElement) > >+ // Requests for full-screen can come from any document in the document tree > >+ // which is same-origin to the focused document, including sibling/cousin > >+ // branches. > > Or totally separate windows that are same-origin, etc... I'd just drop the > "in the document tree" and the part after the ',' there. > > Also, where does "focused" come in here? Requests are only granted in user-generated input handlers... But I guess the handler could just call focus() on something else and then I'd be lying... > >+ Only documents which are a direct ancestor of the requesting > >+ // full-screen element should have their full-screen state set after a > >+ // request though. So to ensure we turn off the full-screen state in all > >+ // non-ancestor documents, we keep a list of the ancestor documents (the > >+ // "full-screen documents") and reset the full-screen state of all the other > >+ // documents in this document tree. > > What about documents not in this document tree, per above? What's this code > really trying to accomplish? This code is trying ensure the full-screen state is turned off in all documents except the ancestors of the full-screen element/document during a request for full-screen. Previously I was disabling full-screen state in *all* documents in the doc tree, and then re-enabling full-screen state in the full-screen ancestor documents. But for documents that were already in full-screen state and that needed to remain in full-screen state after the request, this was dispatching a mozfullscreenchange events when turning off full-screen and again when turning-back-on full-screen in those docs, which I am worried would confuse script. So... Do I need to traverse the window tree as well? Or do something else to catch documents in other doc trees? > Per above, we don't need it for that, but do need it for correctly handling > matchesSelector, right? We shouldn't (now) since we don't allow full-screen elements not in their document. > >+++ b/content/html/content/src/nsGenericHTMLElement.cpp > > I assume this lives here because non-HTML elements can't be full-screen? Is > that planned to stay so, or should this code just proactively live in > nsGenericElement? We haven't consciously decided either way. This can live in nsGenericElement. > >+++ b/layout/style/ua.css > >+:-moz-full-screen-ancestor { > Do the tests test this? No, I can't set prefs in reftests to enable the api or disable the enter-full-screen transition. Ah, but I can use matchesSelector in mochitests!

> means we only need IsFullScreenAncestor() in once place, Good . Just make it take Element*. ;) > But I can use state.HasAtLeastOneOfStates Yes, please. Support for the bitwise operators was just there to ease porting to the event state class; it used to be a plain PRUInt32 and lots of code treated it as such. > Not sure what the best target for the mozfullscreenchange event is if the full-screen > element is removed from the document before the event is dispatched. I'm not sure either. We can just pick something and make sure to bring this up in the working group. > So... Do I need to traverse the window tree as well? Or do something else to > catch documents in other doc trees? I think we should probably handle this like the focus manager does: store a pointer to the current full-screen document somewhere in a global, and if some other document requests fullscreen toggle the state from the old fullscreen doc up to their nearest common ancestor or whatnot (which might be null). This should work across windows, etc. Making it work in e10s-world is a separate kettle of fish. Followup for that.

* Review comments addressed. * Don't grant review requests for elements not in their document, and exit full-screen upon removing full-screen element from document. * Dispatch fullscreenchange event to element if it's removed from its document. * Use script runners to call nsGlobalWindow::SetFullScreen(). This function dispatches a synchronous event to chrome, and if this is called in UnbindFromTree() (to exit full-screen) a nasty looking assertion about being unsafe fires. * Refactor the tests to make it easier to add more full-screen tests.

I didn't realise I needed to annul the static nsDocument::sFullScreenDoc nsWeakPtr upon full-screen cancel to prevent it reporting as leaking. Fixed that.

Comment on attachment 570645 [details] [diff] [review] Patch v5 >+++ b/content/base/src/nsDocument.cpp >+ nsDispatchFullScreenChange(nsIDocument *aDoc, Element* aElement) >+ : mDoc(aDoc), >+ mTarget(aElement ? (static_cast<nsISupports*>(aElement)) : aDoc) {} I guess you need the cast because otherwise you get complaints about incompatible types in ?: or something? I'm sort of torn between leaving this as is or changing aElement to be an nsINode*, which should avoid the need for a cast here (since nsIDocument inherits from nsINode). You no longer need anything out of aElement other than it being an nsISupports, right? >+GetCommonAncestor(nsIDocument* aDoc1, nsIDocument* aDoc2) >+ nsCOMPtr<nsIDocument> doc1(aDoc1); These don't need to be nsCOMPtrs, and then you don't need the parentdoc* temporaries either, right? > nsDocument::RequestFullScreen(Element* aElement) >+ if (!commonAncestor && fullScreenDoc) { >+ // Other doc is in another browser window. Move it out of full-screen. >+ SetWindowFullScreen(fullScreenDoc, false); >+ } I'm not sure I understand this. Is the idea that the fullscreen state on windows is not in fact per-nsIDOMWindow? If so, worth saying that explicitly here. > nsDocument::GetMozFullScreenElement(nsIDOMHTMLElement **aFullScreenElement) >+ if (IsFullScreenDoc()) { >+ CallQueryInterface(GetFullScreenElement(), aFullScreenElement); Are we guaranteed that GetFullScreenElement() is non-null if IsFullScreenDoc()? CallQI is not null-safe... r=me with those nits. Thank you for your patience, both with the slow reviews and the chameleon-like spec!

(In reply to Boris Zbarsky (:bz) from comment #21) > Comment on attachment 570645 [details] [diff] [review] [diff] [details] [review] > Patch v5 Thanks! > >+++ b/content/base/src/nsDocument.cpp > >+ nsDispatchFullScreenChange(nsIDocument *aDoc, Element* aElement) > >+ : mDoc(aDoc), > >+ mTarget(aElement ? (static_cast<nsISupports*>(aElement)) : aDoc) {} > > I guess you need the cast because otherwise you get complaints about > incompatible types in ?: or something? Precisely. I will change this to nsINode*, I don't need anything except the event target. > > nsDocument::RequestFullScreen(Element* aElement) > >+ if (!commonAncestor && fullScreenDoc) { > >+ // Other doc is in another browser window. Move it out of full-screen. > >+ SetWindowFullScreen(fullScreenDoc, false); > >+ } > > I'm not sure I understand this. Is the idea that the fullscreen state on > windows is not in fact per-nsIDOMWindow? If so, worth saying that > explicitly here. nsGlobalWindow::SetFullScreen(bool) applies to the root window, so it's not per nsIDOMWindow. I'll add a comment to say that. > Are we guaranteed that GetFullScreenElement() is non-null if > IsFullScreenDoc()? CallQI is not null-safe... We should always have a full-screen element while in full-screen mode, but I'll add a check just in case.

(In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #23) > https://hg.mozilla.org/integration/mozilla-inbound/rev/b72b2c0f996f timeless pointed out: [2011-11-15 08:31:00] <timeless> http://pastebin.mozilla.org/1382249 -- Error in parsing value for 'clip'. Declaration dropped. Source File: resource://gre-resources/ua.css 'clip' should take the value 'auto' rather than 'none'.

(In reply to David Baron [:dbaron] from comment #25) > timeless pointed out: [...] > 'clip' should take the value 'auto' rather than 'none'. I ran across this as well, and filed bug 706192 on it. Patch posted there.

Documented here: https://developer.mozilla.org/en/CSS/:-moz-full-screen-ancestor And mentioned on Firefox 10 for developers.