Closed Bug 1126230 (top-layer) Opened 10 years ago Closed 9 years ago

Use top layer for Fullscreen API

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla44
Tracking Flags:
Tracking Status
firefox44 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: dev-doc-needed)

Attachments

(10 files, 12 obsolete files)

MozReview Request: Bug 1126230 part 1 - Use delegated constructor to simplify constructor of nsFrameConstructorState. r=bz
40 bytes, text/x-review-board-request
bzbarsky
: review+
Details
MozReview Request: Bug 1126230 part 2 - Refactor part of nsFrameConstructorState::AddChild. r=bz
40 bytes, text/x-review-board-request
bzbarsky
: review+
Details
MozReview Request: Bug 1126230 part 4 - Add -moz-top-layer internal CSS property and set it for fullscreen elements.
40 bytes, text/x-review-board-request
dbaron
: review+
Details
MozReview Request: Bug 1126230 part 5 - Give proper geometric parent for top layer frames. r=bz,dbaron
40 bytes, text/x-review-board-request
dbaron
: review+
bzbarsky
: review+
Details
MozReview Request: Bug 1126230 part 6 - Add nsIDocument::GetFullscreenStack() method. r=smaug
40 bytes, text/x-review-board-request
Details
MozReview Request: Bug 1126230 part 7 - Add static method nsDisplayListBuilder::GetOutOfFlowData(). r=roc
40 bytes, text/x-review-board-request
Details
MozReview Request: Bug 1126230 part 8 - Implement painting part for the top layer.
40 bytes, text/x-review-board-request
roc
: review+
Details
MozReview Request: Bug 1126230 part 9 - Remove fullscreen override and related test. r=dbaron
40 bytes, text/x-review-board-request
dbaron
: review+
Details
MozReview Request: Bug 1126230 part 10 - Add test for fullscreen top layer. r=dbaron
40 bytes, text/x-review-board-request
Details
MozReview Request: Bug 1126230 part 3 - Add :-moz-browser-frame pseudo class for HTML browser frame elements.
40 bytes, text/x-review-board-request
dbaron
: review+
Details
The Fullscreen API Standard defines top layer. We should implement this concept to match the spec.
Alias: top-layer
Blocks: 888979
Note that we would have problem with dialogs (opened by window.alert() or window.prompt()) and fullscreen warning box when we implement the top layer. Dialogs are probably fine as they blocks the whole document like what other things in the top layer do. However, the fullscreen warning box generally doesn't block anything. We'll probably need to add some mechanism to XUL so that we can put something on top of the top layer... I guess it probably also makes sense to allow this kind of usecase in HTML. Anne, Do you think we could have something done for this on the spec side?
Flags: needinfo?(annevk)
We should maybe add some note that UI-dialogs still need to be rendered on top? With some examples. File an issue if you think that'd help.
Flags: needinfo?(annevk)
(In reply to Anne (:annevk) from comment #2) > We should maybe add some note that UI-dialogs still need to be rendered on > top? With some examples. File an issue if you think that'd help. I mean, the parent document sometimes may also want to put things on top of its subdocument without blocking it even if the subdocument is in fullscreen. Just like what we need to do for displaying the fullscreen warning box.
Yeah that makes sense. But UI is not really considered as part of CSS' rendering model. That is, the "UI layer" isn't really a thing, but I guess we could name it in a note of sorts.
Probably we can just avoid apply top layer to XUL or chrome.
Blocks: 1199519
Blocks: ::backdrop
Bug 1126230 - Use top layer concept for rendering fullscreen elements.
Attachment #8655746 - Flags: review?(roc)
Attachment #8655746 - Flags: review?(bugs)
This patch is much much shorter than I initially thought.
Blocks: 1200896
Comment on attachment 8655746 [details] MozReview Request: Bug 1126230 part 1 - Use deletgated constructor to simplify constructor of nsFrameConstructorState. https://reviewboard.mozilla.org/r/18027/#review16135 ::: layout/generic/nsFrame.cpp:2286 (Diff revision 1) > + elem->State().HasState(NS_EVENT_STATE_FULL_SCREEN) && Put the HasState check before IsHTMLOrXHTML since HasState will almost always be false and IsHTMLOrXHTML will almost always be true. Actually, why is the IsHTMLOrXHTML check there at all??? ::: layout/generic/nsViewportFrame.cpp:82 (Diff revision 1) > + if (!doc->IsHTMLOrXHTML() || !doc->IsFullScreenDoc()) { Why do we need to check the doc IsHTMLOrXHTML here? Do we need to check doc->IsFullScreenDoc? Seems like we could just not check that, because the fullscreen stack will be empty.
Attachment #8655746 - Flags: review?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8) > Comment on attachment 8655746 [details] > MozReview Request: Bug 1126230 - Use top layer concept for rendering > fullscreen elements. > > https://reviewboard.mozilla.org/r/18027/#review16135 > > ::: layout/generic/nsFrame.cpp:2286 > (Diff revision 1) > > + elem->State().HasState(NS_EVENT_STATE_FULL_SCREEN) && > > Put the HasState check before IsHTMLOrXHTML since HasState will almost > always be false and IsHTMLOrXHTML will almost always be true. > > Actually, why is the IsHTMLOrXHTML check there at all??? Because we don't want to apply that on XUL for the sake of the fullscreen warning box, see comment 1. Also I don't see any reason we should apply top layer concept to SVG... So I add IsHTMLOrXHTML there. Probably we could just use IsHTMLDocument instead?
Comment on attachment 8655746 [details] MozReview Request: Bug 1126230 part 1 - Use deletgated constructor to simplify constructor of nsFrameConstructorState. Bug 1126230 - Use top layer concept for rendering fullscreen elements.
Attachment #8655746 - Flags: review?(roc)
You should use a check that excludes XUL. There's no reason for this not to work for SVG documents.
Considering FxOS and possibly browser.html in the future, we probably shouldn't simply exclude XUL. I don't have a clear idea what should we do here.
What is the case with FxOS here? But for browser.html, doesn't that still use chrome privileges (like also the very top level document in FxOS, IIRC)? So, could we just use nsContentUtils::IsChromeDoc here? Since if it is not a chrome document, it sure should work per the spec. Or perhaps !nsContentUtils::IsChromeDoc(doc) && !doc->IsXULDocument(); to be safer. update IID if you add some virtual method to nsIDocument.
Comment on attachment 8655746 [details] MozReview Request: Bug 1126230 part 1 - Use deletgated constructor to simplify constructor of nsFrameConstructorState. But I can't really review display list changes, and GetFullscreenStack is rather trivial, so I guess my review isn't needed here.
Attachment #8655746 - Flags: review?(bugs)
Well, actually excluding XUL is unnecessary here, because xul:browser is placed in a xul:stack, which stops its children from being out-of-flow. As the code here skips frames whose parent isn't the viewport, we always skip xul:browser. But that's still an issue for browser.html anyway. I think we have two options: 1. skip top layer rendering for all chrome documents; 2. define a new mechanism to render elements in a even later phase than the top layer. The first option could be unfortunate because it seems that, as of FxOS, not only the browser and the system, but also certificated third-party applications would have chrome privilege. Having a deteriorated behavior when they get chrome privilege doesn't sound good. For this reason, I'd like to explore the second option to add a new layer on top of the top layer.
(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #13) > Or perhaps !nsContentUtils::IsChromeDoc(doc) && !doc->IsXULDocument(); to be > safer. Actually PresContext()->IsChrome() should just work in frames. The problem is whether we should do that or not. roc hopes this works even on chrome, and I think that does make sense. (In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #14) > so I guess my review isn't needed here. OK, I'll try not to add your review load for this bug. Well, but roc sometimes may forget to remind me things like: > update IID if you add some virtual method to nsIDocument. :)
Blocks: 1201078
Blocks: 1201798
(In reply to Olli Pettay [:smaug] (reviewing overload. not reviewing for couple of days. If you want a review from me, please send email) from comment #14) > But I can't really review display list changes, and GetFullscreenStack is > rather trivial, so I guess my review isn't needed here. I've separated the GetFullscreenStack part as an independent patch. May I consider this comment as an r+ for that part with updating the IID?
Flags: needinfo?(bugs)
sure, for the dom part
Flags: needinfo?(bugs)
Comment on attachment 8655746 [details] MozReview Request: Bug 1126230 part 1 - Use deletgated constructor to simplify constructor of nsFrameConstructorState. Bug 1126230 part 1 - Use deletgated constructor to simplify constructor of nsFrameConstructorState.
Attachment #8655746 - Attachment description: MozReview Request: Bug 1126230 - Use top layer concept for rendering fullscreen elements. → MozReview Request: Bug 1126230 part 1 - Use deletgated constructor to simplify constructor of nsFrameConstructorState.
Attachment #8655746 - Flags: review?(roc) → review?(bzbarsky)
Bug 1126230 part 2 - Refactor part of nsFrameConstructorState::AddChild.
Attachment #8657977 - Flags: review?(bzbarsky)
Bug 1126230 part 3 - Add top layer flag to nsStyleContext and recompute position value per spec.
Attachment #8657978 - Flags: review?(dbaron)
Bug 1126230 part 4 - Give proper geometric parent for top layer frames.
Attachment #8657979 - Flags: review?(bzbarsky)
Bug 1126230 part 5 - Add chrome-only position value -moz-top-layer for putting element on the top layer.
Attachment #8657980 - Flags: review?(dbaron)
Bug 1126230 part 6 - Put fullscreen elements to the top layer.
Attachment #8657981 - Flags: review?(dbaron)
Bug 1126230 part 7 - Add nsIDocument::GetFullscreenStack() method. r=smaug
Attachment #8657982 - Flags: review?(bugs)
Bug 1126230 part 8 - Add static method nsDisplayListBuilder::GetOutOfFlowData().
Attachment #8657983 - Flags: review?(roc)
Bug 1126230 part 9 - Implement painting part for the top layer.
Attachment #8657984 - Flags: review?(roc)
Bug 1126230 part 10 - Remove fullscreen override and related test.
Attachment #8657985 - Flags: review?(dbaron)
Bug 1126230 part 11 - Add test for fullscreen top layer.
Attachment #8657986 - Flags: review?(dbaron)
Assignee: nobody → quanxunzhen
Blocks: 743198
Comment on attachment 8657983 [details] MozReview Request: Bug 1126230 part 8 - Add static method nsDisplayListBuilder::GetOutOfFlowData(). r=roc https://reviewboard.mozilla.org/r/18447/#review16533
Attachment #8657983 - Flags: review?(roc) → review+
Comment on attachment 8657984 [details] MozReview Request: Bug 1126230 part 9 - Implement painting part for the top layer. https://reviewboard.mozilla.org/r/18449/#review16535 ::: layout/generic/nsViewportFrame.h:115 (Diff revision 1) > + nsTArray<nsIFrame*> mTopLayerFrames; I would prefer not to cache these frames. It looks like it should not be a performance issue to find them on every paint. Currently I think this code is dangerous because frame reconstruction could cause mTopLayerFrames to contain dangling frame pointers, and if any path is able to build a display list without flushing reflow, we'd crash.
Attachment #8657984 - Flags: review?(roc)
Comment on attachment 8657978 [details] MozReview Request: Bug 1126230 part 3 - Add top layer flag to nsStyleContext and recompute position value per spec. This patch has some issues.
Attachment #8657978 - Flags: review?(dbaron)
Comment on attachment 8655746 [details] MozReview Request: Bug 1126230 part 1 - Use deletgated constructor to simplify constructor of nsFrameConstructorState. Bug 1126230 part 1 - Use deletgated constructor to simplify constructor of nsFrameConstructorState.
Comment on attachment 8657977 [details] MozReview Request: Bug 1126230 part 2 - Refactor part of nsFrameConstructorState::AddChild. Bug 1126230 part 2 - Refactor part of nsFrameConstructorState::AddChild.
Comment on attachment 8657978 [details] MozReview Request: Bug 1126230 part 3 - Add top layer flag to nsStyleContext and recompute position value per spec. Bug 1126230 part 3 - Add top layer flag to nsStyleContext and recompute position value per spec.
Attachment #8657978 - Flags: review?(dbaron)
Comment on attachment 8657979 [details] MozReview Request: Bug 1126230 part 4 - Give proper geometric parent for top layer frames. Bug 1126230 part 4 - Give proper geometric parent for top layer frames.
Comment on attachment 8657980 [details] MozReview Request: Bug 1126230 part 5 - Add chrome-only position value -moz-top-layer for putting element on the top layer. Bug 1126230 part 5 - Add chrome-only position value -moz-top-layer for putting element on the top layer.
Comment on attachment 8657981 [details] MozReview Request: Bug 1126230 part 6 - Put fullscreen elements to the top layer. Bug 1126230 part 6 - Put fullscreen elements to the top layer.
Comment on attachment 8657982 [details] MozReview Request: Bug 1126230 part 7 - Add nsIDocument::GetFullscreenStack() method. r=smaug Bug 1126230 part 7 - Add nsIDocument::GetFullscreenStack() method. r=smaug
Comment on attachment 8657983 [details] MozReview Request: Bug 1126230 part 8 - Add static method nsDisplayListBuilder::GetOutOfFlowData(). r=roc Bug 1126230 part 8 - Add static method nsDisplayListBuilder::GetOutOfFlowData(). r=roc
Attachment #8657983 - Attachment description: MozReview Request: Bug 1126230 part 8 - Add static method nsDisplayListBuilder::GetOutOfFlowData(). → MozReview Request: Bug 1126230 part 8 - Add static method nsDisplayListBuilder::GetOutOfFlowData(). r=roc
Comment on attachment 8657984 [details] MozReview Request: Bug 1126230 part 9 - Implement painting part for the top layer. Bug 1126230 part 9 - Implement painting part for the top layer.
Attachment #8657984 - Flags: review?(roc)
Comment on attachment 8657985 [details] MozReview Request: Bug 1126230 part 10 - Remove fullscreen override and related test. Bug 1126230 part 10 - Remove fullscreen override and related test.
Comment on attachment 8657986 [details] MozReview Request: Bug 1126230 part 11 - Add test for fullscreen top layer. Bug 1126230 part 11 - Add test for fullscreen top layer.
Attachment #8657982 - Flags: review?(bugs)
Comment on attachment 8657982 [details] MozReview Request: Bug 1126230 part 7 - Add nsIDocument::GetFullscreenStack() method. r=smaug Bug 1126230 part 7 - Add nsIDocument::GetFullscreenStack() method. r=smaug
Comment on attachment 8657984 [details] MozReview Request: Bug 1126230 part 9 - Implement painting part for the top layer. https://reviewboard.mozilla.org/r/18449/#review16619 ::: layout/generic/nsViewportFrame.cpp:108 (Diff revision 2) > + MOZ_ASSERT(PresContext()->IsChrome(), "Non-chrome document " I don't think this assertion is correct for <iframe mozbrowser>. Just remove it. ::: layout/generic/nsViewportFrame.cpp:118 (Diff revision 2) > + if (PresContext()->IsChrome()) { Likewise I don't think we should have this guard.
Attachment #8657984 - Flags: review?(roc) → review+
Comment on attachment 8657978 [details] MozReview Request: Bug 1126230 part 3 - Add top layer flag to nsStyleContext and recompute position value per spec. https://reviewboard.mozilla.org/r/18437/#review16623 ::: layout/style/nsRuleNode.cpp:5478 (Diff revision 2) > + display->mPosition = NS_STYLE_POSITION_ABSOLUTE; This is making style data conditional on something that's not in the style data. This means there are two problems: (1) The data from this computation might (since you don't call conditions.SetUncacheable()) be used for an element for which aContext->IsInTopLayer() is not true (2) The reverse might happen -- cached data from an element for which IsInTopLayer is false might get reused for this element. In order to avoid both of these problems, you need to do this work in nsStyleContext::ApplyStyleFixups (after calling GetUniqueStyleData if IsInTopLayer is true) instead of in ComputeDisplayData.
https://reviewboard.mozilla.org/r/18437/#review16627 ::: layout/style/nsRuleNode.cpp:5479 (Diff revision 2) > + } And, actually, one other thought here: it seems much more natural to have an internal-only event state selector to do this. If you added an event-state-based pseudo-class to nsCSSPseudoClassList.h, it seems like you'd get most of what's in patch 3 and patch 6 for free (along with a single rule in ua.css). Is there a reason not to take that approach?
Comment on attachment 8657980 [details] MozReview Request: Bug 1126230 part 5 - Add chrome-only position value -moz-top-layer for putting element on the top layer. https://reviewboard.mozilla.org/r/18441/#review16629 ::: browser/base/content/browser.css:667 (Diff revision 2) > - position: fixed; > + position: -moz-top-layer; This seems like a lot of work for a single use in chrome. Why can't chrome use the fullscreen API the same way other content does? (And, for that matter, what does this change have to do with the rest of this bug?)
Attachment #8657980 - Flags: review?(dbaron)
Comment on attachment 8657981 [details] MozReview Request: Bug 1126230 part 6 - Put fullscreen elements to the top layer. https://reviewboard.mozilla.org/r/18443/#review16631 ::: layout/style/nsStyleSet.cpp:1380 (Diff revision 2) > + if (aElement->State().HasState(NS_EVENT_STATE_FULL_SCREEN)) { See comments on patch 3; almost all of this could be avoided if this were driven off of an event-state-based pseudo-class.
Attachment #8657981 - Flags: review?(dbaron)
Comment on attachment 8657985 [details] MozReview Request: Bug 1126230 part 10 - Remove fullscreen override and related test. https://reviewboard.mozilla.org/r/18451/#review16633 ::: layout/style/nsLayoutStylesheetCache.cpp (Diff revision 2) > - mFullScreenOverrideSheet, true); This patch should also remove the mFullScreenOverrideSheet member variable (and any getters that use it). (Can we remove the :-moz-full-screen-ancestor pseudo-class? If we're not ready to do that yet, could you file a followup bug on removing it? Or do we want to keep it exposed to the Web?)
(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #47) > In order to avoid both of these problems, you need to do this work in > nsStyleContext::ApplyStyleFixups (after calling GetUniqueStyleData if > IsInTopLayer is true) instead of in ComputeDisplayData. My concern about doing so in ComputeDisplayData is that, some other properties (e.g. float, display) in StyleDisplay depends on the value of position. Recomputing the position value would need to duplicate such work in ApplyStyleFixups as well. But that's probably fine.
Comment on attachment 8657986 [details] MozReview Request: Bug 1126230 part 11 - Add test for fullscreen top layer. https://reviewboard.mozilla.org/r/18453/#review16637 ::: dom/html/test/file_fullscreen-top-layer.html:32 (Diff revision 2) > + .fixed #fullscreen { Using class="fixed" is a little misleading given that it's really really styled. Maybe class="styled" or even just class="two" (since it's the second test). ::: dom/html/test/file_fullscreen-top-layer.html:39 (Diff revision 2) > + background: black; It might be nice to use a color other than black here (say, green), since black could result from some error conditions. ::: dom/html/test/file_fullscreen-top-layer.html:129 (Diff revision 2) > + checkWindowPureColor(gBack.style.backgroundColor); Maybe better to hard-code the color here, so that you're testing better that we're *not* messing with style. ::: dom/html/test/file_fullscreen-top-layer.html:136 (Diff revision 2) > + addEventListener("click", firstClick); Here, and in the other addEventListener and removeEventListener calls, it's much clearer to explicitly write window.addEventListener, since the window is the event target you're adding a listen ::: dom/html/test/file_fullscreen-top-layer.html:150 (Diff revision 2) > + checkWindowPureColor(gFullscreen.style.backgroundColor); Again, maybe better to hard-code the color here. ::: dom/html/test/file_fullscreen-top-layer.html:155 (Diff revision 2) > + `Computed style ${prop} of #parent should not be changed`); The MozReview syntax highlighter seems to think a # inside of a \`template string\` is interesting for some reason, although I'm not sure why. Maybe it's just a bug in MozReview?
(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #49) > Comment on attachment 8657980 [details] > MozReview Request: Bug 1126230 part 5 - Add chrome-only position value > -moz-top-layer for putting element on the top layer. > > https://reviewboard.mozilla.org/r/18441/#review16629 > > ::: browser/base/content/browser.css:667 > (Diff revision 2) > > - position: fixed; > > + position: -moz-top-layer; > > This seems like a lot of work for a single use in chrome. Why can't chrome > use the fullscreen API the same way other content does? (And, for that > matter, what does this change have to do with the rest of this bug?) Actually it is not even necessary for this bug at all, because XUL browser cannot be out-of-flow and thus the fullscreen-warning won't be covered by the browser even if it keeps using "fixed" without any z-index. The main concern is Firefox OS and browser.html, where we don't use XUL browser. This is merely a warning for fullscreen. We don't want the warning box to be fullscreened. It seems to me that it also requires a lot of work to add another API for putting a UI element to the top layer stack (maintaining another list in the document, maintaining the element state in the chrome js code, etc.) Also, I think it would be good to make CSS be able to use the top layer as well.
(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #51) > Comment on attachment 8657985 [details] > MozReview Request: Bug 1126230 part 10 - Remove fullscreen override and > related test. > > https://reviewboard.mozilla.org/r/18451/#review16633 > > ::: layout/style/nsLayoutStylesheetCache.cpp > (Diff revision 2) > > - mFullScreenOverrideSheet, true); > > This patch should also remove the mFullScreenOverrideSheet member variable > (and any getters that use it). Oh, okay. > (Can we remove the :-moz-full-screen-ancestor pseudo-class? If we're not > ready to do that yet, could you file a followup bug on removing it? Or do > we want to keep it exposed to the Web?) It is bug 1199529.
Hmm, commenting in MozReview doesn't work in the way as I expected. (In reply to David Baron [:dbaron] ⌚UTC-7 from comment #53) > Comment on attachment 8657986 [details] > MozReview Request: Bug 1126230 part 11 - Add test for fullscreen top layer. > > ::: dom/html/test/file_fullscreen-top-layer.html:39 > (Diff revision 2) > > + background: black; > > It might be nice to use a color other than black here (say, green), since > black could result from some error conditions. I should just remove this line. Background color of this element is specified in the inline style attribute. > ::: dom/html/test/file_fullscreen-top-layer.html:155 > (Diff revision 2) > > + `Computed style ${prop} of #parent should not be changed`); > > The MozReview syntax highlighter seems to think a # inside of a \`template > string\` is interesting for some reason, although I'm not sure why. Maybe > it's just a bug in MozReview? It is bug 1159730.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #46) > Comment on attachment 8657984 [details] > MozReview Request: Bug 1126230 part 9 - Implement painting part for the top > layer. > > https://reviewboard.mozilla.org/r/18449/#review16619 > > ::: layout/generic/nsViewportFrame.cpp:108 > (Diff revision 2) > > + MOZ_ASSERT(PresContext()->IsChrome(), "Non-chrome document " > > I don't think this assertion is correct for <iframe mozbrowser>. Just remove > it. > > ::: layout/generic/nsViewportFrame.cpp:118 > (Diff revision 2) > > + if (PresContext()->IsChrome()) { > > Likewise I don't think we should have this guard. Those depend on how patch 3 would be changed. So the owner of <iframe mozbrowser> may not have chrome privilege? We absolutely don't want to expose this value to the web at least now. We need to decide what extent do we want to expose it.
(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #52) > My concern about doing so in ComputeDisplayData is that, some other > properties (e.g. float, display) in StyleDisplay depends on the value of > position. Recomputing the position value would need to duplicate such work > in ApplyStyleFixups as well. But that's probably fine. If you take the pseudo-class approach you won't need to worry about this at all. (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #54) > (In reply to David Baron [:dbaron] ⌚UTC-7 from comment #49) > > This seems like a lot of work for a single use in chrome. Why can't chrome > > use the fullscreen API the same way other content does? (And, for that > > matter, what does this change have to do with the rest of this bug?) > > Actually it is not even necessary for this bug at all, because XUL browser > cannot be out-of-flow and thus the fullscreen-warning won't be covered by > the browser even if it keeps using "fixed" without any z-index. > > The main concern is Firefox OS and browser.html, where we don't use XUL > browser. > > This is merely a warning for fullscreen. We don't want the warning box to be > fullscreened. It seems to me that it also requires a lot of work to add > another API for putting a UI element to the top layer stack (maintaining > another list in the document, maintaining the element state in the chrome js > code, etc.) > > Also, I think it would be good to make CSS be able to use the top layer as > well. Please put this patch in a separate bug, then, where we can discuss it on its own merits.
Blocks: 1202925
Attachment #8655746 - Attachment is obsolete: true
Attachment #8655746 - Flags: review?(bzbarsky)
Attachment #8657977 - Attachment is obsolete: true
Attachment #8657977 - Flags: review?(bzbarsky)
Attachment #8657978 - Attachment is obsolete: true
Attachment #8657979 - Attachment is obsolete: true
Attachment #8657979 - Flags: review?(bzbarsky)
Attachment #8657980 - Attachment is obsolete: true
Attachment #8657981 - Attachment is obsolete: true
Attachment #8657982 - Attachment is obsolete: true
Attachment #8657983 - Attachment is obsolete: true
Attachment #8657984 - Attachment is obsolete: true
Attachment #8657985 - Attachment is obsolete: true
Attachment #8657986 - Attachment is obsolete: true
Bug 1126230 part 1 - Use deletgated constructor to simplify constructor of nsFrameConstructorState.
Attachment #8658539 - Flags: review?(bzbarsky)
Bug 1126230 part 2 - Refactor part of nsFrameConstructorState::AddChild.
Attachment #8658540 - Flags: review?(bzbarsky)
Bug 1126230 part 4 - Give proper geometric parent for top layer frames.
Attachment #8658542 - Flags: review?(dbaron)
Attachment #8658542 - Flags: review?(bzbarsky)
Bug 1126230 part 5 - Put fullscreen elements to the top layer.
Attachment #8658543 - Flags: review?(dbaron)
Bug 1126230 part 6 - Add nsIDocument::GetFullscreenStack() method. r=smaug
Bug 1126230 part 7 - Add static method nsDisplayListBuilder::GetOutOfFlowData(). r=roc
Bug 1126230 part 8 - Implement painting part for the top layer.
Attachment #8658546 - Flags: review?(roc)
Bug 1126230 part 9 - Remove fullscreen override and related test.
Attachment #8658547 - Flags: review?(dbaron)
I should enable evolve before I do the big rebase today... I wouldn't need to discard the old reviews and create new ones if I did so... Sorry for spamming...
Comment on attachment 8658548 [details] MozReview Request: Bug 1126230 part 10 - Add test for fullscreen top layer. r=dbaron Bug 1126230 part 10 - Add test for fullscreen top layer. r=dbaron
Attachment #8658548 - Attachment description: MozReview Request: Bug 1126230 part 10 - Add test for fullscreen top layer. → MozReview Request: Bug 1126230 part 10 - Add test for fullscreen top layer. r=dbaron
(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #58) > So the owner of <iframe mozbrowser> may not have chrome privilege? Correct, AFAIK. > We > absolutely don't want to expose this value to the web at least now. We need > to decide what extent do we want to expose it. Maybe we can restrict it to certified apps for now? I'm not sure who's in charge of the FxOS browser ... Kan-Ru would know.
(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #59) > (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #52) > > My concern about doing so in ComputeDisplayData is that, some other > > properties (e.g. float, display) in StyleDisplay depends on the value of > > position. Recomputing the position value would need to duplicate such work > > in ApplyStyleFixups as well. But that's probably fine. > > If you take the pseudo-class approach you won't need to worry about this at > all. Hmmm, I don't think that works. We do not want to compute 'fixed' to 'absolute'. We only want to compute values 'static', 'relative' (and probably also 'sticky') to 'absolute'. I don't think that is something doable via adding a pseudo-class.
Attachment #8658539 - Flags: review?(bzbarsky) → review+
Comment on attachment 8658539 [details] MozReview Request: Bug 1126230 part 1 - Use delegated constructor to simplify constructor of nsFrameConstructorState. r=bz https://reviewboard.mozilla.org/r/18623/#review16697 r=me, though those do_AddRef are rather ugly.
Oh, and also "delegated" in the commit message for part 1, please.
Attachment #8658540 - Flags: review?(bzbarsky) → review+
Comment on attachment 8658540 [details] MozReview Request: Bug 1126230 part 2 - Refactor part of nsFrameConstructorState::AddChild. r=bz https://reviewboard.mozilla.org/r/18625/#review16699 ::: layout/base/nsCSSFrameConstructor.cpp:922 (Diff revision 1) > + * placeholder is also returned via parameter if the method doesn't "also returned via the aPlaceholderType parameter", perhaps? ::: layout/base/nsCSSFrameConstructor.cpp:924 (Diff revision 1) > + * really has containing block. "really has a containing block" r=me with those nits fixed.
Comment on attachment 8658542 [details] MozReview Request: Bug 1126230 part 5 - Give proper geometric parent for top layer frames. r=bz,dbaron https://reviewboard.mozilla.org/r/18629/#review16703 ::: layout/base/nsCSSFrameConstructor.cpp:1001 (Diff revision 1) > + // Items in the top layer are fixed positioned children of the viewport frame. If we're going to put them into kFixedList, why aren't we just putting them into mFixedItems? This needs comments at the very least.
Attachment #8658542 - Flags: review?(bzbarsky)
I have a question when I'm learning the code in nsCSSFrameConstructor: is it important to ensure the out-of-flow frames are stored in the tree order in their corresponding frame list? If yes, it seems currently we may have failed to maintain that when we have a fixed-pos containing block, and we insert a series of interleaved fixed-pos children and abs-pos children to the block. (My patch may make it even worse because it is likely that children in mTopLayerItems and mFixedItems will be inserted into the same child list while they grow independently.) If no, the method nsFrameConstructorState::ProcessFrameInsertions() looks unnecessarily complicated and potentially slow.
Flags: needinfo?(bzbarsky)
I would think it's important, yes. Certainly for floats its critical, because their order determines the layout. It's _possible_ that for abspos/fixedpos it's less important because for those all it affects is z-indexing, I believe, and it's possible that we do the z-index sort on the DOM tree, not the frame tree. I assume the failure mode you're talking about is something like a transformed frame, which puts fixed and absolute stuff all in the same list?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #80) > I would think it's important, yes. Certainly for floats its critical, > because their order determines the layout. It's _possible_ that for > abspos/fixedpos it's less important because for those all it affects is > z-indexing, I believe, and it's possible that we do the z-index sort on the > DOM tree, not the frame tree. We do that in the frame tree, but in the order of placeholder frames, not the order in the abs/fixed child list. > I assume the failure mode you're talking about is something like a > transformed frame, which puts fixed and absolute stuff all in the same list? Yes.
Depends on: 1203405
Comment on attachment 8658539 [details] MozReview Request: Bug 1126230 part 1 - Use delegated constructor to simplify constructor of nsFrameConstructorState. r=bz Bug 1126230 part 1 - Use delegated constructor to simplify constructor of nsFrameConstructorState. r=bz
Attachment #8658539 - Attachment description: MozReview Request: Bug 1126230 part 1 - Use deletgated constructor to simplify constructor of nsFrameConstructorState. → MozReview Request: Bug 1126230 part 1 - Use delegated constructor to simplify constructor of nsFrameConstructorState. r=bz
Comment on attachment 8658540 [details] MozReview Request: Bug 1126230 part 2 - Refactor part of nsFrameConstructorState::AddChild. r=bz Bug 1126230 part 2 - Refactor part of nsFrameConstructorState::AddChild. r=bz
Attachment #8658540 - Attachment description: MozReview Request: Bug 1126230 part 2 - Refactor part of nsFrameConstructorState::AddChild. → MozReview Request: Bug 1126230 part 2 - Refactor part of nsFrameConstructorState::AddChild. r=bz
Comment on attachment 8658541 [details] MozReview Request: Bug 1126230 part 4 - Add -moz-top-layer internal CSS property and set it for fullscreen elements. Bug 1126230 part 3 - Add top layer flag to nsStyleContext.
Attachment #8658542 - Flags: review?(bzbarsky)
Comment on attachment 8658542 [details] MozReview Request: Bug 1126230 part 5 - Give proper geometric parent for top layer frames. r=bz,dbaron Bug 1126230 part 4 - Give proper geometric parent for top layer frames.
Comment on attachment 8658543 [details] MozReview Request: Bug 1126230 part 5 - Put fullscreen elements to the top layer. Bug 1126230 part 5 - Put fullscreen elements to the top layer.
Comment on attachment 8658544 [details] MozReview Request: Bug 1126230 part 6 - Add nsIDocument::GetFullscreenStack() method. r=smaug Bug 1126230 part 6 - Add nsIDocument::GetFullscreenStack() method. r=smaug
Attachment #8658544 - Flags: review?(bugs)
Comment on attachment 8658545 [details] MozReview Request: Bug 1126230 part 7 - Add static method nsDisplayListBuilder::GetOutOfFlowData(). r=roc Bug 1126230 part 7 - Add static method nsDisplayListBuilder::GetOutOfFlowData(). r=roc
Attachment #8658545 - Flags: review?(roc)
Comment on attachment 8658546 [details] MozReview Request: Bug 1126230 part 8 - Implement painting part for the top layer. Bug 1126230 part 8 - Implement painting part for the top layer.
Comment on attachment 8658547 [details] MozReview Request: Bug 1126230 part 9 - Remove fullscreen override and related test. r=dbaron Bug 1126230 part 9 - Remove fullscreen override and related test.
Comment on attachment 8658548 [details] MozReview Request: Bug 1126230 part 10 - Add test for fullscreen top layer. r=dbaron Bug 1126230 part 10 - Add test for fullscreen top layer. r=dbaron
Attachment #8658548 - Flags: review?(dbaron)
Attachment #8658548 - Flags: review?(dbaron) → review+
Attachment #8658545 - Flags: review?(roc) → review+
Comment on attachment 8658540 [details] MozReview Request: Bug 1126230 part 2 - Refactor part of nsFrameConstructorState::AddChild. r=bz Bug 1126230 part 2 - Refactor part of nsFrameConstructorState::AddChild. r=bz
Comment on attachment 8658544 [details] MozReview Request: Bug 1126230 part 6 - Add nsIDocument::GetFullscreenStack() method. r=smaug Bug 1126230 part 6 - Add nsIDocument::GetFullscreenStack() method. r=smaug
Attachment #8658544 - Flags: review?(bugs)
Comment on attachment 8658545 [details] MozReview Request: Bug 1126230 part 7 - Add static method nsDisplayListBuilder::GetOutOfFlowData(). r=roc Bug 1126230 part 7 - Add static method nsDisplayListBuilder::GetOutOfFlowData(). r=roc
Attachment #8658545 - Flags: review+
Comment on attachment 8658546 [details] MozReview Request: Bug 1126230 part 8 - Implement painting part for the top layer. Bug 1126230 part 8 - Implement painting part for the top layer.
Comment on attachment 8658548 [details] MozReview Request: Bug 1126230 part 10 - Add test for fullscreen top layer. r=dbaron Bug 1126230 part 10 - Add test for fullscreen top layer. r=dbaron
Attachment #8658548 - Flags: review+
Comment on attachment 8658540 [details] MozReview Request: Bug 1126230 part 2 - Refactor part of nsFrameConstructorState::AddChild. r=bz Bug 1126230 part 2 - Refactor part of nsFrameConstructorState::AddChild. r=bz
Comment on attachment 8658544 [details] MozReview Request: Bug 1126230 part 6 - Add nsIDocument::GetFullscreenStack() method. r=smaug Bug 1126230 part 6 - Add nsIDocument::GetFullscreenStack() method. r=smaug
Comment on attachment 8658545 [details] MozReview Request: Bug 1126230 part 7 - Add static method nsDisplayListBuilder::GetOutOfFlowData(). r=roc Bug 1126230 part 7 - Add static method nsDisplayListBuilder::GetOutOfFlowData(). r=roc
Comment on attachment 8658546 [details] MozReview Request: Bug 1126230 part 8 - Implement painting part for the top layer. Bug 1126230 part 8 - Implement painting part for the top layer.
Comment on attachment 8658548 [details] MozReview Request: Bug 1126230 part 10 - Add test for fullscreen top layer. r=dbaron Bug 1126230 part 10 - Add test for fullscreen top layer. r=dbaron
Comment on attachment 8658542 [details] MozReview Request: Bug 1126230 part 5 - Give proper geometric parent for top layer frames. r=bz,dbaron https://reviewboard.mozilla.org/r/18629/#review16821 r=me, assuming we want to have this separate lop-layer list at all. ;)
Attachment #8658542 - Flags: review?(bzbarsky) → review+
(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #74) > (In reply to David Baron [:dbaron] ⌚UTC-7 from comment #59) > > (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #52) > > > My concern about doing so in ComputeDisplayData is that, some other > > > properties (e.g. float, display) in StyleDisplay depends on the value of > > > position. Recomputing the position value would need to duplicate such work > > > in ApplyStyleFixups as well. But that's probably fine. > > > > If you take the pseudo-class approach you won't need to worry about this at > > all. > > Hmmm, I don't think that works. We do not want to compute 'fixed' to > 'absolute'. We only want to compute values 'static', 'relative' (and > probably also 'sticky') to 'absolute'. I don't think that is something > doable via adding a pseudo-class. Why should these things compute to 'absolute' rather than 'fixed'? https://fullscreen.spec.whatwg.org/#user-agent-level-style-sheet-defaults says that fullscreen elements should be 'fixed'. Why do you not just want to implement the :fullscreen pseudo-class that is described in https://fullscreen.spec.whatwg.org/#:fullscreen-pseudo-class and then use the styles in https://fullscreen.spec.whatwg.org/#user-agent-level-style-sheet-defaults ? That seems to be a much smaller amount of code and seems to match the spec.
Flags: needinfo?(quanxunzhen)
The fullscreen part has been implemented via an important rule in ua.css. Computing to 'absolute' is a behavior described in section 6.1, which I believe is for <dialog>, so it's not important at present, and thus I removed that code in the patch. But we may have to face it again when we are going to implement <dialog>.
Flags: needinfo?(quanxunzhen)
patches 3 and 5 still seem like an awful lot of complexity just to ensure that authors who change <dialog> away from its default of 'position:absolute' that's specified in https://html.spec.whatwg.org/multipage/rendering.html#flow-content-3 , to something other than 'fixed', still get absolute positioning for it. Other than that requirement it seems like they could be replaced by a single line in nsCSSPseudoClassList.h: CSS_STATE_PSEUDO_CLASS(fullscreen, ":fullscreen", 0, "", NS_EVENT_STATE_FULLSCREEN) (assuming that dynamic change notifications for the fullscreen state are currently sent correctly) and the inclusion of the specified default styles from https://fullscreen.spec.whatwg.org/#user-agent-level-style-sheet-defaults in ua.css and from https://html.spec.whatwg.org/multipage/rendering.html#flow-content-3 in html.css .
Depends on: 1069192
Blocks: 1203891
No longer blocks: 1201798
Comment on attachment 8658539 [details] MozReview Request: Bug 1126230 part 1 - Use delegated constructor to simplify constructor of nsFrameConstructorState. r=bz Bug 1126230 part 1 - Use delegated constructor to simplify constructor of nsFrameConstructorState. r=bz
Comment on attachment 8658540 [details] MozReview Request: Bug 1126230 part 2 - Refactor part of nsFrameConstructorState::AddChild. r=bz Bug 1126230 part 2 - Refactor part of nsFrameConstructorState::AddChild. r=bz
Comment on attachment 8658541 [details] MozReview Request: Bug 1126230 part 4 - Add -moz-top-layer internal CSS property and set it for fullscreen elements. Bug 1126230 part 3 - Add -moz-top-layer internal CSS property and set it for fullscreen elements.
Attachment #8658541 - Attachment description: MozReview Request: Bug 1126230 part 3 - Add top layer flag to nsStyleContext. → MozReview Request: Bug 1126230 part 3 - Add -moz-top-layer internal CSS property and set it for fullscreen elements.
Comment on attachment 8658542 [details] MozReview Request: Bug 1126230 part 5 - Give proper geometric parent for top layer frames. r=bz,dbaron Bug 1126230 part 4 - Give proper geometric parent for top layer frames.
Comment on attachment 8658544 [details] MozReview Request: Bug 1126230 part 6 - Add nsIDocument::GetFullscreenStack() method. r=smaug Bug 1126230 part 5 - Add nsIDocument::GetFullscreenStack() method. r=smaug
Attachment #8658544 - Attachment description: MozReview Request: Bug 1126230 part 6 - Add nsIDocument::GetFullscreenStack() method. r=smaug → MozReview Request: Bug 1126230 part 5 - Add nsIDocument::GetFullscreenStack() method. r=smaug
Comment on attachment 8658545 [details] MozReview Request: Bug 1126230 part 7 - Add static method nsDisplayListBuilder::GetOutOfFlowData(). r=roc Bug 1126230 part 6 - Add static method nsDisplayListBuilder::GetOutOfFlowData(). r=roc
Attachment #8658545 - Attachment description: MozReview Request: Bug 1126230 part 7 - Add static method nsDisplayListBuilder::GetOutOfFlowData(). r=roc → MozReview Request: Bug 1126230 part 6 - Add static method nsDisplayListBuilder::GetOutOfFlowData(). r=roc
Comment on attachment 8658546 [details] MozReview Request: Bug 1126230 part 8 - Implement painting part for the top layer. Bug 1126230 part 7 - Implement painting part for the top layer.
Attachment #8658546 - Attachment description: MozReview Request: Bug 1126230 part 8 - Implement painting part for the top layer. → MozReview Request: Bug 1126230 part 7 - Implement painting part for the top layer.
Comment on attachment 8658547 [details] MozReview Request: Bug 1126230 part 9 - Remove fullscreen override and related test. r=dbaron Bug 1126230 part 8 - Remove fullscreen override and related test.
Attachment #8658547 - Attachment description: MozReview Request: Bug 1126230 part 9 - Remove fullscreen override and related test. → MozReview Request: Bug 1126230 part 8 - Remove fullscreen override and related test.
Comment on attachment 8658548 [details] MozReview Request: Bug 1126230 part 10 - Add test for fullscreen top layer. r=dbaron Bug 1126230 part 9 - Add test for fullscreen top layer. r=dbaron
Attachment #8658548 - Attachment description: MozReview Request: Bug 1126230 part 10 - Add test for fullscreen top layer. r=dbaron → MozReview Request: Bug 1126230 part 9 - Add test for fullscreen top layer. r=dbaron
Attachment #8658543 - Attachment is obsolete: true
Attachment #8658543 - Flags: review?(dbaron)
dbaron, I've updated the patches to use an internal property for top layer. Could you review the new patches here? (As MozReview doesn't renew the r? requests, I'm concerned that you may miss the update, so ni? you here.) BTW, could you review the slightly updated patch in bug 1069192 first so that I can land it?
Flags: needinfo?(dbaron)
Comment on attachment 8658546 [details] MozReview Request: Bug 1126230 part 8 - Implement painting part for the top layer. Changed a little. May need a review again.
Attachment #8658546 - Flags: review+ → review?(roc)
Comment on attachment 8658546 [details] MozReview Request: Bug 1126230 part 8 - Implement painting part for the top layer. https://reviewboard.mozilla.org/r/18637/#review18151
Attachment #8658546 - Flags: review?(roc) → review+
Blocks: 1203089
Comment on attachment 8658541 [details] MozReview Request: Bug 1126230 part 4 - Add -moz-top-layer internal CSS property and set it for fullscreen elements. https://reviewboard.mozilla.org/r/18627/#review18897 ::: layout/style/nsCSSProps.cpp:1839 (Diff revision 3) > + eCSSKeyword_none, NS_STYLE_TOP_LAYER_NONE, > + eCSSKeyword_auto, NS_STYLE_TOP_LAYER_AUTO, I'm not happy wth the names 'none' and 'auto' here. I guess 'none' is barely ok, but 'auto' seems pretty bad for "force this to be in the top layer". Maybe "force" would be better? ::: layout/style/nsRuleNode.cpp:5481 (Diff revision 3) > + // If an element is put in the top layer, while it is not absolutely > + // positioned, the position value should be computed to 'absolute' per > + // the Fullscreen API spec. > + if (display->mTopLayer != NS_STYLE_TOP_LAYER_NONE && > + !display->IsAbsolutelyPositionedStyle()) { > + display->mPosition = NS_STYLE_POSITION_ABSOLUTE; > + } Given the way the rule tree works, this code is technically wrong. However, given the restricted way this property is used, it might not matter . If that's the case, then it's probably ok to leave the incorrect code as long as it has a big comment explaining how it's incorrect. The reason it's wrong is that one of the optimizations we make in the rule tree is that we start from a cached struct on a higher node in the rule tree. For example, given: div.myclass \{ -moz-top-layer: force \} div#myid \{ -moz-top-layer: none \} and the markup: <div class="myclass"></div> <\!-- first\! --> <div id="myid" class="myclass"></div> we'll first compute style for the first div, and potentially cache a style struct in the rule tree. If you suppose the second div matches the same rules as the first div except for the one rule added on at the end, we'll then compute its display struct using the struct we computed for the first div as aStartStruct. With the code as you've written it here, the fixup to display->mPosition would incorrectly apply to the second div, when it actually should not do so. Then again, given that it's easy to fix, by adding a call to: conditions.SetUncacheable(); inside the if. You should add a comment explaining that you're doing the SetUncacheable in order to to prevent the struct being used as an aStartStruct (as opposed to the normal reason for calling it, which is to avoid caching data that depends on something in the style context tree). (At least, I think this makes it ok\!) ::: layout/style/nsStyleConsts.h:808 (Diff revision 3) > +#define NS_STYLE_TOP_LAYER_AUTO 1 // in the top layer for DOM not sure what "for DOM" means, or what it adds (see also previous comment about names) r=dbaron with those things fixed
Attachment #8658541 - Flags: review?(dbaron) → review+
Comment on attachment 8658542 [details] MozReview Request: Bug 1126230 part 5 - Give proper geometric parent for top layer frames. r=bz,dbaron https://reviewboard.mozilla.org/r/18629/#review18899 ::: layout/base/nsCSSFrameConstructor.cpp:1113 (Diff revision 3) > + if (aStyleDisplay->mTopLayer != NS_STYLE_TOP_LAYER_NONE) { You probably want to assert that the value is actually NS_STYLE_TOP_LAYER_FORCE (or AUTO, or whatever). ::: layout/base/nsCSSFrameConstructor.cpp:1152 (Diff revision 3) > if (aCanBePositioned) { > const nsStyleDisplay* disp = aNewFrame->StyleDisplay(); > + if (disp->mTopLayer != NS_STYLE_TOP_LAYER_NONE) { > + *aPlaceholderType = PLACEHOLDER_FOR_FIXEDPOS; > + return &mTopLayerItems; > + } Is it ok that this is conditional on aCanBePositioned?
Attachment #8658542 - Flags: review?(dbaron) → review+
Comment on attachment 8658547 [details] MozReview Request: Bug 1126230 part 9 - Remove fullscreen override and related test. r=dbaron https://reviewboard.mozilla.org/r/18639/#review18901 Looks good. ... except for the fact that MozReview failed to connect this to https://reviewboard.mozilla.org/r/18451/ , which this request really should have been a part of.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #115) > dbaron, I've updated the patches to use an internal property for top layer. > Could you review the new patches here? (As MozReview doesn't renew the r? > requests, I'm concerned that you may miss the update, so ni? you here.) Yes, I did indeed miss the update because of that. Thanks for the ni? > BTW, could you review the slightly updated patch in bug 1069192 first so > that I can land it? Done.
Flags: needinfo?(dbaron)
(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #119) > ::: layout/base/nsCSSFrameConstructor.cpp:1152 > (Diff revision 3) > > if (aCanBePositioned) { > > const nsStyleDisplay* disp = aNewFrame->StyleDisplay(); > > + if (disp->mTopLayer != NS_STYLE_TOP_LAYER_NONE) { > > + *aPlaceholderType = PLACEHOLDER_FOR_FIXEDPOS; > > + return &mTopLayerItems; > > + } > > Is it ok that this is conditional on aCanBePositioned? I think yes. After some investigation, I think aCanBePositioned is false only when the frame is a child of certain XUL frames, or it is an inner SVG frame. I suspect frames inside MathML behave alike, but am not completely sure. I don't think those cases are important, and thus I don't want to risk allowing them.
Blocks: 1210628
(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #120) > Comment on attachment 8658547 [details] > MozReview Request: Bug 1126230 part 8 - Remove fullscreen override and > related test. > > https://reviewboard.mozilla.org/r/18639/#review18901 > > Looks good. > > ... except for the fact that MozReview failed to connect this to > https://reviewboard.mozilla.org/r/18451/ , which this request really should > have been a part of. It is my fault that I didn't set up obsolescence when I did the first submission in this bug, which made MozReview fail to maintain the relationship of patches between pushes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/590404aac3573ac8def5d8caf2af61f013b79338 Bug 1126230 part 1 - Use delegated constructor to simplify constructor of nsFrameConstructorState. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/8bf708acbad4bebe9fab68958da532296eb7b07d Bug 1126230 part 2 - Refactor part of nsFrameConstructorState::AddChild. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/7c303cc4c30baa310569bf266b4c9e40eab4b7fd Bug 1126230 part 3 - Add -moz-top-layer internal CSS property and set it for fullscreen elements. r=dbaron https://hg.mozilla.org/integration/mozilla-inbound/rev/745d659bef49d125d32ef0c340e979bd17162490 Bug 1126230 part 4 - Give proper geometric parent for top layer frames. r=bz,dbaron https://hg.mozilla.org/integration/mozilla-inbound/rev/ada76e419aac5fc15f3ba022a729e30c26d6d98c Bug 1126230 part 5 - Add nsIDocument::GetFullscreenStack() method. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/b55511536c65e45bea5d3769c9abf8f8fda1b06c Bug 1126230 part 6 - Add static method nsDisplayListBuilder::GetOutOfFlowData(). r=roc https://hg.mozilla.org/integration/mozilla-inbound/rev/7bfa2a2d4e29989b9057afbc01b79979662a1fa1 Bug 1126230 part 7 - Implement painting part for the top layer. r=roc https://hg.mozilla.org/integration/mozilla-inbound/rev/441b55f015c2b6bdd90736a1a4c92d623502a594 Bug 1126230 part 8 - Remove fullscreen override and related test. r=dbaron https://hg.mozilla.org/integration/mozilla-inbound/rev/8a9d8a55618352a0c4327bf32b72eca24f0cafe7 Bug 1126230 part 9 - Add test for fullscreen top layer. r=dbaron
Attachment #8658542 - Attachment description: MozReview Request: Bug 1126230 part 4 - Give proper geometric parent for top layer frames. → MozReview Request: Bug 1126230 part 4 - Give proper geometric parent for top layer frames. r=bz,dbaron
Attachment #8658546 - Attachment description: MozReview Request: Bug 1126230 part 7 - Implement painting part for the top layer. → MozReview Request: Bug 1126230 part 7 - Implement painting part for the top layer. r=roc
Bug 1126230 part 3 - Add :-moz-browser-frame pseudo class for HTML browser frame elements.
Attachment #8670106 - Flags: review?(dbaron)
Comment on attachment 8658541 [details] MozReview Request: Bug 1126230 part 4 - Add -moz-top-layer internal CSS property and set it for fullscreen elements. Bug 1126230 part 4 - Add -moz-top-layer internal CSS property and set it for fullscreen elements.
Attachment #8658541 - Attachment description: MozReview Request: Bug 1126230 part 3 - Add -moz-top-layer internal CSS property and set it for fullscreen elements. r=dbaron → MozReview Request: Bug 1126230 part 4 - Add -moz-top-layer internal CSS property and set it for fullscreen elements.
Attachment #8658542 - Attachment description: MozReview Request: Bug 1126230 part 4 - Give proper geometric parent for top layer frames. r=bz,dbaron → MozReview Request: Bug 1126230 part 5 - Give proper geometric parent for top layer frames. r=bz,dbaron
Comment on attachment 8658542 [details] MozReview Request: Bug 1126230 part 5 - Give proper geometric parent for top layer frames. r=bz,dbaron Bug 1126230 part 5 - Give proper geometric parent for top layer frames. r=bz,dbaron
Comment on attachment 8658544 [details] MozReview Request: Bug 1126230 part 6 - Add nsIDocument::GetFullscreenStack() method. r=smaug Bug 1126230 part 6 - Add nsIDocument::GetFullscreenStack() method. r=smaug
Attachment #8658544 - Attachment description: MozReview Request: Bug 1126230 part 5 - Add nsIDocument::GetFullscreenStack() method. r=smaug → MozReview Request: Bug 1126230 part 6 - Add nsIDocument::GetFullscreenStack() method. r=smaug
Comment on attachment 8658545 [details] MozReview Request: Bug 1126230 part 7 - Add static method nsDisplayListBuilder::GetOutOfFlowData(). r=roc Bug 1126230 part 7 - Add static method nsDisplayListBuilder::GetOutOfFlowData(). r=roc
Attachment #8658545 - Attachment description: MozReview Request: Bug 1126230 part 6 - Add static method nsDisplayListBuilder::GetOutOfFlowData(). r=roc → MozReview Request: Bug 1126230 part 7 - Add static method nsDisplayListBuilder::GetOutOfFlowData(). r=roc
Attachment #8658546 - Attachment description: MozReview Request: Bug 1126230 part 7 - Implement painting part for the top layer. r=roc → MozReview Request: Bug 1126230 part 8 - Implement painting part for the top layer.
Comment on attachment 8658546 [details] MozReview Request: Bug 1126230 part 8 - Implement painting part for the top layer. Bug 1126230 part 8 - Implement painting part for the top layer.
Comment on attachment 8658547 [details] MozReview Request: Bug 1126230 part 9 - Remove fullscreen override and related test. r=dbaron Bug 1126230 part 9 - Remove fullscreen override and related test. r=dbaron
Attachment #8658547 - Attachment description: MozReview Request: Bug 1126230 part 8 - Remove fullscreen override and related test. r=dbaron → MozReview Request: Bug 1126230 part 9 - Remove fullscreen override and related test. r=dbaron
Comment on attachment 8658548 [details] MozReview Request: Bug 1126230 part 10 - Add test for fullscreen top layer. r=dbaron Bug 1126230 part 10 - Add test for fullscreen top layer. r=dbaron
Attachment #8658548 - Attachment description: MozReview Request: Bug 1126230 part 9 - Add test for fullscreen top layer. r=dbaron → MozReview Request: Bug 1126230 part 10 - Add test for fullscreen top layer. r=dbaron
Comment on attachment 8658541 [details] MozReview Request: Bug 1126230 part 4 - Add -moz-top-layer internal CSS property and set it for fullscreen elements. Changes since last review: https://reviewboard.mozilla.org/r/18627/diff/3-5/
Attachment #8658541 - Flags: review+ → review?(dbaron)
Comment on attachment 8658546 [details] MozReview Request: Bug 1126230 part 8 - Implement painting part for the top layer. I should request re-review on this patch after patch 3 and 4 are r+.
Flags: needinfo?(quanxunzhen)
Attachment #8658546 - Flags: review+
Cleared my ni? for comment 145 by mistake.
Flags: needinfo?(quanxunzhen)
Comment on attachment 8658541 [details] MozReview Request: Bug 1126230 part 4 - Add -moz-top-layer internal CSS property and set it for fullscreen elements. https://reviewboard.mozilla.org/r/18627/#review19263
Attachment #8658541 - Flags: review?(dbaron) → review+
Comment on attachment 8670106 [details] MozReview Request: Bug 1126230 part 3 - Add :-moz-browser-frame pseudo class for HTML browser frame elements. https://reviewboard.mozilla.org/r/21331/#review19265 ::: layout/style/nsCSSPseudoClassList.h:125 (Diff revision 1) > +// Matches HTML frame elements whose reallyIsBrowserOrApp is true. > +CSS_PSEUDO_CLASS(mozBrowserFrame, ":-moz-browser-frame", 0, "") Could you explain why you need this? Could you also explain in this comment what the selector actually does. I don't know what "reallyIsBrowserOrApp" means. Finally, perhaps this should be CSS_PSEUDO_CLASS_UA_SHEET_ONLY ?
Comment on attachment 8670106 [details] MozReview Request: Bug 1126230 part 3 - Add :-moz-browser-frame pseudo class for HTML browser frame elements. (In reply to David Baron [:dbaron] ⌚UTC-7 from comment #149) > Comment on attachment 8670106 [details] > MozReview Request: Bug 1126230 part 3 - Add :-moz-browser-frame pseudo class > for HTML browser frame elements. > > https://reviewboard.mozilla.org/r/21331/#review19265 > > ::: layout/style/nsCSSPseudoClassList.h:125 > (Diff revision 1) > > +// Matches HTML frame elements whose reallyIsBrowserOrApp is true. > > +CSS_PSEUDO_CLASS(mozBrowserFrame, ":-moz-browser-frame", 0, "") > > Could you explain why you need this? Because we need a way to stop browser element from being the topmost and covering the fullscreen request / notification. This doesn't happen to the desktop browser because XUL <browser> cannot be out-of-flow, and thus it cannot be put in the top layer. However, this could happen for browser.html and Firefox OS because in those environment, the browser element is just an HTML <iframe mozbrowser>, which would be put in the top layer when its inner document enters fullscreen. This is why the Gij test failed for the last landing. Alternatively, we can make the browser element still in the top layer, and provide a mechanism for the owner of the browser element to manually put something on top of the top layer. But that would be more complicated for the current case. We'd probably discuss that in bug 1210628. Also doing that would need some change in gaia first, otherwise we would still be blocked by the Gij test. > Could you also explain in this comment what the selector actually does. I > don't know what "reallyIsBrowserOrApp" means. That means it is an HTML frame/iframe element (probably only iframe) which has attribute "mozbrowser" and its owning document has the permission to own a browser element. reallyIsBrowserOrApp is finally called into this function: https://dxr.mozilla.org/mozilla-central/rev/89732fcdb0baca70e8b7a25a2725117113f0db80/dom/html/nsGenericHTMLFrameElement.cpp#440 > Finally, perhaps this should be CSS_PSEUDO_CLASS_UA_SHEET_ONLY ? Probably. But given <iframe mozbrowser> is only usable in privileged apps, normal content won't get any benefit from using this pseudo-class, so I won't worry too much about exposing it.
Attachment #8670106 - Flags: review?(dbaron)
(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #148) > Comment on attachment 8658541 [details] > MozReview Request: Bug 1126230 part 4 - Add -moz-top-layer internal CSS > property and set it for fullscreen elements. > > https://reviewboard.mozilla.org/r/18627/#review19263 Also, for this patch, I wonder that the order of the pseudo-classes could potentially affect the efficiency. It seems to me that we check the pseudo-class in the declared order, so I guess a pseudo-class which filters out greater number of element should be put in front. In both of the fullscreen rules, it seems to me we should put :-moz-full-screen at the very beginning. Should I do that?
Flags: needinfo?(dbaron)
https://reviewboard.mozilla.org/r/21331/#review19273 ::: layout/style/nsCSSPseudoClassList.h:125 (Diff revision 1) > +// Matches HTML frame elements whose reallyIsBrowserOrApp is true. > +CSS_PSEUDO_CLASS(mozBrowserFrame, ":-moz-browser-frame", 0, "") OK, then could you make the comment here say "is a mozbrowser or mozapp" or something like that.
Comment on attachment 8658546 [details] MozReview Request: Bug 1126230 part 8 - Implement painting part for the top layer. Changes since last review: https://reviewboard.mozilla.org/r/18637/diff/3-5/
Flags: needinfo?(quanxunzhen)
Attachment #8658546 - Flags: review?(roc)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #151) > Also, for this patch, I wonder that the order of the pseudo-classes could > potentially affect the efficiency. It seems to me that we check the > pseudo-class in the declared order, so I guess a pseudo-class which filters > out greater number of element should be put in front. > > In both of the fullscreen rules, it seems to me we should put > :-moz-full-screen at the very beginning. Should I do that? I think it makes sense to do that logically. I was even thinking of suggesting it for performance when I was doing the review, but then I realized that the negations get checked after the pseudo-classes so it doesn't actually matter here. So you may as well do it anyway.
Flags: needinfo?(dbaron)
Comment on attachment 8658546 [details] MozReview Request: Bug 1126230 part 8 - Implement painting part for the top layer. https://reviewboard.mozilla.org/r/18637/#review19287
Attachment #8658546 - Flags: review?(roc) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b31ef5cb622c6a4df9350b25490f64329f6d655b Bug 1126230 part 1 - Use delegated constructor to simplify constructor of nsFrameConstructorState. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/9ff1ca679580aa852749d3c5ed0dfe08ece33769 Bug 1126230 part 2 - Refactor part of nsFrameConstructorState::AddChild. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/a350be6992deadb83362110790b908f8e4f63cd6 Bug 1126230 part 3 - Add :-moz-browser-frame pseudo class for HTML browser frame elements. r=dbaron https://hg.mozilla.org/integration/mozilla-inbound/rev/ddf4bf202aa267bff6ecb1bac6a3ed28f57fe0bf Bug 1126230 part 4 - Add -moz-top-layer internal CSS property and set it for fullscreen elements. r=dbaron https://hg.mozilla.org/integration/mozilla-inbound/rev/071ee11270ddb222246aad58874642619dbef08a Bug 1126230 part 5 - Give proper geometric parent for top layer frames. r=bz,dbaron https://hg.mozilla.org/integration/mozilla-inbound/rev/1951399af17f9fe43a8b465059667e4873f84de3 Bug 1126230 part 6 - Add nsIDocument::GetFullscreenStack() method. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/4ebcc266fc3543ea447c53de29d85e1a83234077 Bug 1126230 part 7 - Add static method nsDisplayListBuilder::GetOutOfFlowData(). r=roc https://hg.mozilla.org/integration/mozilla-inbound/rev/e6bc3e58c8acdafab75f38ddd2b2bc992bb01895 Bug 1126230 part 8 - Implement painting part for the top layer. r=roc https://hg.mozilla.org/integration/mozilla-inbound/rev/74f41e0d78e4e1ff1156a830ab8db65ff6a65ef9 Bug 1126230 part 9 - Remove fullscreen override and related test. r=dbaron https://hg.mozilla.org/integration/mozilla-inbound/rev/b45bd6bd7d574f633a67b3fdf0256e6ae7b57d36 Bug 1126230 part 10 - Add test for fullscreen top layer. r=dbaron
(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #118) > > Then again, given that it's easy to fix, by adding a call to: > conditions.SetUncacheable(); > inside the if. You should add a comment explaining that you're doing the > SetUncacheable in order to to prevent the struct being used as an > aStartStruct (as opposed to the normal reason for calling it, which is to > avoid caching data that depends on something in the style context tree). > > (At least, I think this makes it ok\!) So we also recompute `display` and `float` properties in ComputeDisplayData, but we backup their original value in mOriginal{Display,Float} and restore them at the beginning of the function to avoid this issue. Since top-layer elements are rare, it isn't worth adding another original field for `position`.
Blocks: 1218605
Depends on: 1230508
No longer depends on: 1230508
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: