1064843 - (::backdrop) Add support for the ::backdrop pseudo-element

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[rctgamer3]

There is no way of styling the backdrop when an element is displayed in full-screen mode (as of today, 2014-09-09)

Comment 1

[Eric Shepherd [:sheppy]]

The documentation for this property is here: https://developer.mozilla.org/en-US/docs/Web/CSS/::backdrop

Comment 2

[Jean-Yves Perrier [:teoli]]

IE11 supports ::-ms-backdrop  [ http://msdn.microsoft.com/en-us/library/ie/dn312072%28v=vs.85%29.aspx ]

Comment 3

[rctgamer3]

Chrome 37 supports ::backdrop unprefixed (because of <dialog>) [ http://www.chromestatus.com/features/5770237022568448 ]

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

This would require updating our fullscreen implementation to match the current spec. We should do that. However I am not available to work on fullscreen, I have too many other things to work on. I've been trying to find someone to take over maintenance of the fullscreen API, but have been unsuccessful so far.

Comment 5

[Stuart P. Bentley]

FWIW, this is useful for more than just fullscreen - assuming I'm reading the definition right and the element doesn't only apply in fullscreen, it can be used normally in a document for pure-CSS, wrapperless parallax scrolling backgrounds (by giving the ::backdrop an image and a Z-transform).

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

::backdrop only exists for element in top layer. Currently only fullscreen element and dialog can be put in the top layer, hence this pseudo element is not usable to other cases.

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

It seems this pseudo-element doesn't fit well with our current frame ownership model, and thus may need a lot of hacks.

Since any element can enter fullscreen, it is possible, and actually usually, that the primary frame in fullscreen is not a container frame, which means we are not able to put a backdrop frame as a child of the corresponding fullscreen frame. So we may only put it as a sibling.

The pseudo-element not being a child of its related frame means we need to handle it specially in the element restyler, otherwise we would resolve to the style of the pseudo-element of the frame's parent.

If we put it as a sibling, another issue arises: since the backdrop frame is also an out-of-flow frame, we may need to create a placeholder frame for it, because many places in our code expect that. However, adding a placeholder frame could make lifetime management even harder.

Because the backdrop frame and the fullscreen frame are likely in the same frame list, it is risky to recursively destroy one when we are destroying the other one, doing that could break the frame list when they are neighbor. This also applies to the placeholders.

Also note that currently I'm going to only support them to share the same geometry parent, but in theory, they may have different parents: one is the viewport, and the other is the initial containing block (depending on the position property).

I figured out a solution which looks feasible but risky:
* put the backdrop frame and the fullscreen frame as siblings, and share the same placeholder
* the placeholder is responsible to remove both frames when the destroy root is not an ancestor of them
* change the element restyler to handle backdrop frame specially
* add code to nsFrameManager::GetPlaceholderFrameFor to return the placeholder of the fullscreen frame for the backdrop frame

bz, could you make some suggestion whether this is worth to try, or probably there is some smarter solution I'm not aware of?

Comment 8

[Boris Zbarsky [:bzbarsky]]

I can't think of any better ideas for the element restyler.

But for the rest, could we perhaps use a mechanism similar to the existing nsCanvasFrame::mCustomContentContainer setup as a place to put the backdrop (and in particular to make it always a child of the canvasframe?

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Boris Zbarsky [:bz] from comment #8)
> I can't think of any better ideas for the element restyler.
> 
> But for the rest, could we perhaps use a mechanism similar to the existing
> nsCanvasFrame::mCustomContentContainer setup as a place to put the backdrop
> (and in particular to make it always a child of the canvasframe?

Do you suggest that we should create anonymous content nodes for backdrop pseudo-elements? It probably works, and we could manage them in the document as part of the fullscreen stack. With that, we decouple the lifetime between the frames, and we have a placeholder for each out-of-flow frame. And we don't need specific frame for backdrop, just use whatever it would create from the style.

It sounds promising. I wonder it could make the element restyler more complicated, though. I'll need some investigation on the management of anonymous content nodes then.

Please correct me if there's anything I misunderstood.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Do you suggest that we should create anonymous content nodes for backdrop
> pseudo-elements? 

Yes, like we do for ::before/::after.

I agree that the hard part here would be the element restyler.

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

After thinking about this for several days, I still have no clear idea how this could be done... I probably should pick something else to do first and back here later...

What we would need is:
1. access the backdrop frame from the top layer frame efficiently (for element restyler)
2. the backdrop content node should be child of the top layer element (for restyler to handle undisplayed content)
  * even if it is not necessary, we still need to access the top layer element from the backdrop node for style context to create the frame

It seems both element and frame can store arbitrary pointer in their property, but using that may complicate the lifetime management because of potential dangling pointer.

FWIW,
> Because the backdrop frame and the fullscreen frame are likely in the same
> frame list, it is risky to recursively destroy one when we are destroying
> the other one, doing that could break the frame list when they are neighbor.
> This also applies to the placeholders.
this is wrong. The frame list won't be broken because of recursively removing sibling frames, because frames are unlinked from the list before they are destroyed.

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

I discussed this with dbaron during the TPAC, and he said we may probaby put the placeholder of the backdrop frame as a child of the top layer frame in a special child list. I think that should be the best way in theory, as we handle most of pseudo-elements that way. However, the difficulty is that a parent frame is required to be a container frame in our code, while a frame in top layer may not be a container frame (e.g. video frame, image frame).

dbaron said it should be a quite recent change that we have such requirement. It seems this change was done in bug 508665, which was fixed in May 2014. But it seems to me this requirement actually exists far before that change, and that change was just to enable compiler to force it so that we wouldn't do something crazy but keep static_casting the parent frame to the container frame.

After some investigation, it seems to me it is pretty hard to allow arbitrary frame as a parent. dbaron thought "being a parent" could be a different concept with "having principal children", but at least text frame and bidi pres utils both assume that, parents have principal childlist.

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ea3deddc0b9

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f758d85a203e

Comment 15

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fb573b6a99c

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

Attachment: part 1 - make nsImageFrame inherit nsContainerFrame

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

Attachment: part 2 - make nsSubdocumentFrame inherit nsContainerFrame

Comment 18

[Xidorn Quan [:xidorn] UTC+11]

Attachment: part 3 - make nsFormControlFrame inherit nsContainerFrame

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

Attachment: part 4 - add placeholder type for top layer

Comment 20

[Xidorn Quan [:xidorn] UTC+11]

Attachment: part 5 - add backdrop frame list

Comment 21

[Xidorn Quan [:xidorn] UTC+11]

Attachment: part 6 - add ::backdrop pseudo-element

Comment 22

[Xidorn Quan [:xidorn] UTC+11]

Attachment: part 7 - add frame class for backdrop

Comment 23

[Xidorn Quan [:xidorn] UTC+11]

Attachment: part 8 - change placeholder creating method to accept parent style context

Comment 24

[Xidorn Quan [:xidorn] UTC+11]

Attachment: part 9 - create and render backdrop frame

Comment 25

[Xidorn Quan [:xidorn] UTC+11]

Attachment: part 10 - move a helper function to WindowSnapshot.js

Comment 26

[Xidorn Quan [:xidorn] UTC+11]

Attachment: part 11 - add test for ::backdrop

Comment 27

[Daniel Holbert [:dholbert]]

Comment on attachment 8701257 [details] [diff] [review]
part 1 - make nsImageFrame inherit nsContainerFrame

Review of attachment 8701257 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comments fixed as noted below. (If you like, I'm happy to sanity-check updated comment text; feel free to just carry forward r+, too, though.)

::: layout/generic/nsAtomicContainerFrame.h
@@ +1,5 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* This Source Code is subject to the terms of the Mozilla Public License
> + * version 2.0 (the "License"). You can obtain a copy of the License at
> + * http://mozilla.org/MPL/2.0/. */

Please replace this license boilerplate with what we've got here:
 https://www.mozilla.org/en-US/MPL/headers/
(and also here:
 https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Mode_Line )

I think this version that you used is an earlier version of our recommended-boilerplate (e.g. I see that we've got that same text in nsFlexContainerFrame.h and various ruby source files).  But, it's best to use the latest recommended boilerplate for new files.

@@ +3,5 @@
> +/* This Source Code is subject to the terms of the Mozilla Public License
> + * version 2.0 (the "License"). You can obtain a copy of the License at
> + * http://mozilla.org/MPL/2.0/. */
> +
> +/* base class for rendering objects need child lists but behave like leaf */

Two things:

(1) Grammar nit: s/need/that need/

(2) This comment is a bit enigmatic. Please expand on it, either here or in a new code-comment above the "class" declaration.  The inevitable question that anyone looking at this file will wonder: why do these frames need child lists, if they behave like leaves? Please add an explanation to answer this, in a code-comment. (possibly mentioning ::backdrop as a concrete use-case)

@@ +16,5 @@
> +public:
> +  NS_DECL_ABSTRACT_FRAME(nsAtomicContainerFrame)
> +
> +  virtual bool IsLeaf() const override { return true; }
> +  virtual FrameSearchResult PeekOffsetNoAmount(bool aForward,

Please add a comment above these overrides to explain why they're here.

Sample comment, which you could use here if it's accurate:
  // Bypass the nsContainerFrame impl of several methods,
  // so we behave like a leaf frame:

Comment 28

[Daniel Holbert [:dholbert]]

Comment on attachment 8701257 [details] [diff] [review]
part 1 - make nsImageFrame inherit nsContainerFrame

Review of attachment 8701257 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsAtomicContainerFrame.h
@@ +10,5 @@
> +#define nsAtomicContainerFrame_h___
> +
> +#include "nsContainerFrame.h"
> +
> +class nsAtomicContainerFrame : public nsContainerFrame

One more thing on "part 1" - an actual code change.

Please do the following:
(1) Give nsAtomicContainerFrame a "GetSplittableType" override, and return NS_FRAME_NOT_SPLITTABLE.
(Otherwise, its subclasses will inherit nsSplittableFrame's impl [nsSplittableFrame is nsContainerFrame's superclass], and that impl says they're splittable. And I think most of the leaf-frame subclasses do *not* actually have proper support for being split across pages right now. So we should make them continue to return NS_FRAME_NOT_SPLITTABLE -- and that seems like a reasonable default behavior for nsAtomicContainerFrame to subsume.)

(2) Give nsImageFrame its own "GetSplittableType" override, and return NS_FRAME_SPLITTABLE from it. (which is what nsImageFrame currently returns from this method, by virtue of already descending from nsSplittableFrame)  So, nsImageFrame is a special-case among nsAtomicContainerFrame's descendants w.r.t. this method.

Comment 29

[Daniel Holbert [:dholbert]]

Comment on attachment 8701258 [details] [diff] [review]
part 2 - make nsSubdocumentFrame inherit nsContainerFrame

Review of attachment 8701258 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on part 2, nits below.

::: layout/generic/nsLeafFrame.cpp
@@ -106,5 @@
>    return 0;
>  }
>  
> -// XXX how should border&padding effect baseline alignment?
> -// => descent = borderPadding.bottom for example

This XXX comment (above AddBordersAndPadding) is probably still valid (or at least, its validity isn't impacted by this bug).

Instead of removing this comment, could you move it to live next to your aMetrics.SetSize(...) function-call? (where AddBordersAndPadding was called previously)

::: layout/generic/nsSubDocumentFrame.cpp
@@ +760,2 @@
>                                    aCBSize, aAvailableISize,
>                                    aMargin, aBorder, aPadding, aFlags);

Please reindent/rewrap the args here, so that they're to the right of the open-paren.

@@ +775,5 @@
>        aReflowState.AvailableWidth(), aReflowState.AvailableHeight()));
>  
> +  NS_ASSERTION(aReflowState.ComputedWidth() != NS_UNCONSTRAINEDSIZE,
> +               "Shouldn't have unconstrained stuff here "
> +               "Thanks to the rules of reflow");

s/Thanks/thanks/

(I see this is copypasted from an existing assertion in nsLeafFrame -- maybe make the same capitalization fix there.)

(Also, if we ever fix bug 765861, these assertions should be contingent on that bug's flag, since this can be made to trivially fail with an extremely-large "width" property that happens to produce nscoord_MAX.)

Comment 30

[Daniel Holbert [:dholbert]]

Comment on attachment 8701259 [details] [diff] [review]
part 3 - make nsFormControlFrame inherit nsContainerFrame

Review of attachment 8701259 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on part 3; nit below.

::: layout/forms/nsFormControlFrame.cpp
@@ +49,5 @@
> +{
> +  nscoord result;
> +  DISPLAY_MIN_WIDTH(this, result);
> +
> +  result = GetIntrinsicISize();

Drop the blank line here, I think. (You removed the blank line in nsLeafFrame [where this is copypasted from] elsewhere in this patch -- but you left the blank line in this copypasted version.  And there's no blank line in GetPrefISize, below, so the removal seems reasonable.)

Comment 31

[Daniel Holbert [:dholbert]]

Comment on attachment 8701263 [details] [diff] [review]
part 5 - add backdrop frame list

Review of attachment 8701263 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsContainerFrame.h
@@ +91,5 @@
>                                     nsFrameList& aChildList);
>  
>    /**
> +   * This method sets up the backdrop frame list and store the given
> +   * placeholder frame into it.

If SetInitialChildList stays (see below "Do we need..."):
s/and store/and inserts/ here.

@@ +93,5 @@
>    /**
> +   * This method sets up the backdrop frame list and store the given
> +   * placeholder frame into it.
> +   *
> +   * This being a separate method because subclasses of nsContainerFrame

If SetInitialChildList stays (see below "Do we need..."):
s/This being/This is/

@@ +94,5 @@
> +   * This method sets up the backdrop frame list and store the given
> +   * placeholder frame into it.
> +   *
> +   * This being a separate method because subclasses of nsContainerFrame
> +   * assume the method SetInitialChildList() is called only once, and

Do we really need a separate method for this?

Observations:
 (1) nsContainerFrame's own impl doesn't actually assume it's only called once. It assumes it's only called once *for the principal child list* -- and right now, that's the only child list it handles.  If you extend it to include this new child list, it seems like it could.
 (2) The code in this new method (SetupBackdropFrameList) also assumes it's only called once (*for the backdrop child list*). So the assumptions all seem to be consistent -- only call once for a particular child list.
 (3) Subclasses' implementations that are principal-child-list-specific could also just check the aChildList arg, and do their principal-list stuff *only* if it's kPrincipalList. (They probably *should* be doing that check already. To the extent that they don't, it's just because they haven't had to until now.)

I'd expect that nsContainerFrame would look like:
  if (aChildList == kPrincipalList) {
    // kPrincipalList-related stuff
  } else if (aChildList == kBackdropList) {
    // kBackdropList-related stuff
  } else {
    MOZ_NOTREACHED("Unrecognized list");
  }
...and the child impls should check the list ID, and their "default" code should only run if aListID is kPrincipalList.  And if they don't recognize the list ID, they should invoke their superclass's method.

(The changes to the SetInitialChildList impls here -- checking aListID for kPrincipalList more aggressively -- might want to live in its own patch, for regression-tracking purposes, since it's not expected to change behavior but might have some unexpected fallout.  And then this patch here could simply add the " }else if (aChildList == kBackdropList){" in my code-sample above.)

Comment 32

[Daniel Holbert [:dholbert]]

Comment on attachment 8701271 [details] [diff] [review]
part 11 - add test for ::backdrop

Review of attachment 8701271 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the test, just a few nits:

::: dom/html/test/file_fullscreen-backdrop.html
@@ +38,5 @@
> +
> +const gFullscreenElementBackground = getComputedStyle(gFullscreen).background;
> +
> +function begin() {
> +  assertWindowPureColor(window, "white");

Looks like all of your assertWindowPureColor() calls have a helpful info() call just before them, except for this first one.

Consider adding an info() call here, too, so that it'll be easier to figure out what's going on & where things are failing, if/when this first assertWindowPureColor() call fails someday.

@@ +50,5 @@
> +
> +function enterFullscreen() {
> +  is(getComputedStyle(gFullscreen).background, gFullscreenElementBackground,
> +     "Computed background of #fullscreen shouldn't be changed");
> +  info("Content should be black after enter fullscreen");

Probably worth mentioning (either in this assertion message or in a comment) that black is the *default* :backdrop color. Otherwise, it's unclear where "black" comes from here.  (since you don't explicitly style anything as black in this testcase)

@@ +63,5 @@
> +  assertWindowPureColor(window, "blue");
> +
> +  gFullscreen.style.background = "";
> +  setBackdropStyle("display: none");
> +  info("Content should back to white because we hide the backdrop");

"should back to white" doesn't quite make sense.

Possible replacement (also explaining where the white comes from here, I think? correct if my sourcing is wrong):
  info("Content should be white (from <body>) when backdrop is hidden");

Comment 33

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1027653e7bfe

Comment 34

[Xidorn Quan [:xidorn] UTC+11]

Attachment: part 1 v2 - make nsImageFrame inherit nsContainerFrame

Comment 35

[Xidorn Quan [:xidorn] UTC+11]

Attachment: part 5.1 - correct SetInitialChildList behavior for unknown child list

Comment 36

[Xidorn Quan [:xidorn] UTC+11]

Attachment: part 5.2 - add backdrop frame list

Comment 37

[Xidorn Quan [:xidorn] UTC+11]

Attachment: part 9 - create and render backdrop frame

Comment 38

[Xidorn Quan [:xidorn] UTC+11]

Attachment: part 11 v2 - add test for ::backdrop

Update text and add one more test item.

Comment 39

[Daniel Holbert [:dholbert]]

Comment on attachment 8702525 [details] [diff] [review]
part 1 v2 - make nsImageFrame inherit nsContainerFrame

Review of attachment 8702525 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good -- just 2 comment nits and one code-nit on the imageframe GetSplittableType impl:

::: layout/generic/nsAtomicContainerFrame.h
@@ +13,5 @@
> +
> +/**
> + * This class is for frames which need child lists but act like a leaf
> + * frame. In general, all frames of elements laid out according to the
> + * CSS box model would need child list for ::backdrop in case being put

s/in case being put in/in case they are viewed as/, I think

@@ +35,5 @@
> +    return nsFrame::PeekOffsetCharacter(aForward, aOffset, aRespectClusters);
> +  }
> +  nsSplittableType GetSplittableType() const override
> +  {
> +    return nsFrame::GetSplittableType();

Unlike with nsImageFrame below, I'm OK with keeping this the way you've got it (invoking an ancestor impl instead of returning an enum), since this is in a cluster of other function-impls that directly invoke a nsFrame:: impl, and the "Bypass" comment explains things nicely.

*However* -- one nit here -- please update the comment above this block ("Bypass the nsContainerFrame impl ") to say "nsContainerFrame/nsSplittableFrame impl", because here you're not actually bypassing a nsContainerFrame impl of GetSplittableType.  The impl that you're bypassing lives on nsSplittableFrame.

::: layout/generic/nsImageFrame.h
@@ +122,5 @@
>  #endif
>  
> +  nsSplittableType GetSplittableType() const override
> +  {
> +    return nsSplittableFrame::GetSplittableType();

This seems a bit too abstract; I'd rather we just directly returned the enum value here:
  return NS_FRAME_SPLITTABLE;
...i.e. we're explicitly declaring that nsImageFrame is splittable.

(This is what we do in all other GetSplittableType impls -- we just directly return a value, rather than deferring to some other class's impl.)

(If we were doing something more complex, or if there were more enum values and we were using a specially-chosen one that might change en-masse, I could see this making more sense from a code-sharing & abstracting-away-complexity perspective. But the way it stands now, it seems simpler to just directly return the enum value declaring that we're splittable.)

Comment 40

[Daniel Holbert [:dholbert]]

Comment on attachment 8702526 [details] [diff] [review]
part 5.1 - correct SetInitialChildList behavior for unknown child list

Review of attachment 8702526 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/forms/nsFieldSetFrame.cpp
@@ +663,5 @@
>  nsFieldSetFrame::SetInitialChildList(ChildListID    aListID,
>                                       nsFrameList&   aChildList)
>  {
> +  nsContainerFrame::SetInitialChildList(aListID, aChildList);
> +  MOZ_ASSERT(aListID != kPrincipalList || GetInner());

This assertion is becoming more complex in this patch, so please give it an assertion-message. (Ideally, all non-obvious and/or multi-part assertions should have explanatory messages.)

I think (?) what we're meaning to assert here is: "setting the principal child list should populate our inner frame".  Something like that would make this easier to read/grok.

::: layout/forms/nsTextControlFrame.cpp
@@ +1230,5 @@
>  
>    // Mark the scroll frame as being a reflow root. This will allow
>    // incremental reflows to be initiated at the scroll frame, rather
>    // than descending from the root frame of the frame hierarchy.
> +  if (nsIFrame* first = GetFirstPrincipalChild()) {

I'd rather not do assignment inside of an "if" condition -- it's uncommon enough that it's easy to mistake for an "==" check.  (And it's confusing enough in "while()" conditions that compilers suggest an extra layer of parens as a hint to disambiguate, which suggests that it's easy for humans to misread or to mistake for a typo).

Let's just keep the old code unchanged here (with "first" declared/assigned outside of the if condition), and just insert your early-return right before that assignment.

::: layout/generic/nsFirstLetterFrame.cpp
@@ +80,5 @@
>  nsFirstLetterFrame::SetInitialChildList(ChildListID  aListID,
>                                          nsFrameList& aChildList)
>  {
> +  MOZ_ASSERT(aListID == kPrincipalList, "First letter frame should "
> +             "only set principal child list via SetInitialChildList");

This current message-text is ambiguous -- it can be interpreted as saying
"The principal list should *only* be set via  SetInitialChildList (not via any other method)".

That's what the interpretation I came away with after my initial reading [ignoring the code around it], and I'm pretty sure that's *not* actually the meaning you're going for.

SO: please at least reword to remove the ambiguity noted above, like:
"Principal child list is the only list that nsFirstLetterFrame should set via this function".

BETTER: if you can, please reword to explain the reason for this expectation. (I'm not sure what the reason is, offhand. For example: "nsFirstLetterFrame doesn't expect children in any non-principal child lists" -- if that's true; I'm not sure it is, but it seems like something that might be true).

If you're not sure, though, then please at least reword to clear up the ambiguous meaning.

::: layout/tables/nsTableOuterFrame.cpp
@@ +105,1 @@
>                 "expected a single table frame");

Since you're changing the assertion to check the list ID, please change the message accordingly. e.g.:
 "expected a single table frame in principal child list"

Comment 41

[Daniel Holbert [:dholbert]]

Comment on attachment 8702527 [details] [diff] [review]
part 5.2 - add backdrop frame list

Review of attachment 8702527 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, assuming the OUT_OF_FLOW check noted below is sane (e.g. just an optimization)

Commit message nit:
>Bug 1064843 part 6 - Add backdrop frame list.

Looks like "part 6" might be a typo here, unless you're planning on renumbering patches after this one.  (It's currently labeled as "part 5.2" on this bug, and there's another patch labeled as "part 6".)

::: layout/generic/nsContainerFrame.cpp
@@ +254,5 @@
>        return list ? *list : nsFrameList::EmptyList();
>      }
> +    case kBackdropList: {
> +      nsFrameList* list = GetPropTableFrames(BackdropProperty());
> +      return list ? *list : nsFrameList::EmptyList();

There's a comment at the top of this function (nsContainerFrame::GetChildList) which needs updating for this change:
  222   // We only know about the principal child list and the overflow lists.
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsContainerFrame.cpp?rev=e36c0185f1fd&mark=222-222#220

@@ +288,5 @@
>      ::AppendIfNonempty(this, propTable, ExcessOverflowContainersProperty(),
>                         aLists, kExcessOverflowContainersList);
>    }
> +  if (GetStateBits() & NS_FRAME_OUT_OF_FLOW) {
> +    ::AppendIfNonempty(this, propTable, BackdropProperty(),

Why the OUT_OF_FLOW check here? Is that bit set if we're fullscreen, & this is just an optimization to avoid a property hashtable operation in the common case of in-flow content?  (This check will pass for floated/abspos frames, of course; but maybe that's not a problem if this is just an optimization.)

Might be worth a brief explanatory comment, since there's no obvious relationship between NS_OUT_OF_FLOW & BackdropProperty.  For example, if this is correct:
 // (Bypass BackdropProperty hashtable-lookup for in-flow frames,
 // which are definitely not fullscreen & hence definitely have no backdrop.)

Comment 42

[Daniel Holbert [:dholbert]]

Comment on attachment 8702529 [details] [diff] [review]
part 11 v2 - add test for ::backdrop

Review of attachment 8702529 [details] [diff] [review]:
-----------------------------------------------------------------

Test looks good -- thanks for clarifying the messages. Just one nit:

::: dom/html/test/file_fullscreen-backdrop.html
@@ +8,5 @@
> +  <script type="text/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
> +  <script type="text/javascript" src="/tests/SimpleTest/WindowSnapshot.js"></script>
> +  <script type="text/javascript" src="file_fullscreen-utils.js"></script>
> +</head>
> +<body style="background: white">

Does the style="background: white" actually matter here? Won't the page-background be white anyway, without this?  Or is there some fullscreen weirdness that puts something non-white behind the <body> which shines through?

* If this styling *doesn't* affect anything, then we should drop it for simplicity's sake.
* If this styling *does* matter (i.e. the test fails without it), it seems like it might be worth testing both cases -- e.g. have <body> unstyled by default, and then in whichever part of the test where it makes a difference, you'd first test the unstyled-<body> behavior, and then give the <body> a background-color and test again.

So either way, I think this wants to just be an unstyled <body> in the HTML.

Comment 43

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Daniel Holbert [:dholbert] from comment #40)
> Comment on attachment 8702526 [details] [diff] [review]
> part 5.1 - correct SetInitialChildList behavior for unknown child list
> 
> Review of attachment 8702526 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/forms/nsTextControlFrame.cpp
> @@ +1230,5 @@
> >  
> >    // Mark the scroll frame as being a reflow root. This will allow
> >    // incremental reflows to be initiated at the scroll frame, rather
> >    // than descending from the root frame of the frame hierarchy.
> > +  if (nsIFrame* first = GetFirstPrincipalChild()) {
> 
> I'd rather not do assignment inside of an "if" condition -- it's uncommon
> enough that it's easy to mistake for an "==" check.  (And it's confusing
> enough in "while()" conditions that compilers suggest an extra layer of
> parens as a hint to disambiguate, which suggests that it's easy for humans
> to misread or to mistake for a typo).
> 
> Let's just keep the old code unchanged here (with "first" declared/assigned
> outside of the if condition), and just insert your early-return right before
> that assignment.

This is actually not an "assignment", but a variable declaration in "if". I consider this as a good practice because it clearly limits the scope of the variable. It is much like Swift/Rust's "if let var = somefunction() { }".

It shouldn't mistake for an "==" check because there is a type name before, and thus compilers wouldn't suggest an extra layer of parens for this (and actually adding parens would not work at all).

I believe this syntax is something we should promote for pointer variables, especially smart pointers. I think it being uncommon is simply because people are unaware of this kind of syntax, which we should change.

Comment 44

[Daniel Holbert [:dholbert]]

OK, that sounds good to me. (I agree that the typename makes it not as likely to be mistaken for a "==" check, which reduces the concern -- and given the scoping benefits, it sounds like it adds value, so I'm ok with keeping it.)

Comment 45

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Daniel Holbert [:dholbert] from comment #41)
> Comment on attachment 8702527 [details] [diff] [review]
> part 5.2 - add backdrop frame list
> 
> Review of attachment 8702527 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, assuming the OUT_OF_FLOW check noted below is sane (e.g. just an
> optimization)
> 
> Commit message nit:
> >Bug 1064843 part 6 - Add backdrop frame list.
> 
> Looks like "part 6" might be a typo here, unless you're planning on
> renumbering patches after this one.  (It's currently labeled as "part 5.2"
> on this bug, and there's another patch labeled as "part 6".)

No, it isn't a typo. I'm going to renumber all patches after this, and I actually have done locally. The new "part 9" I submitted above also has "part 10" in the commit message. I keep using the old part number in bugzilla to avoid spamming people here by silly renumbering changes.

Comment 46

[Daniel Holbert [:dholbert]]

(Good, that makes sense; just wanted to double-check, in case it was a mistake.)

Comment 47

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8701266 [details] [diff] [review]
part 7 - add frame class for backdrop

Could you add comments around the implementation of the Reflow and ComputeAutoSize methods explaining what the backdrop frame is a child of?  (This helps to understand what the methods are doing, since it explains where their input sizes come from.)

r=dbaron with that

Comment 48

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8702528 [details] [diff] [review]
part 9 - create and render backdrop frame

>+  } else if (frameType == nsGkAtoms::backdropFrame) {
>+    // Style context of backdrop frame has no parent style context, and
>+    // thus we do not need to reparent it.
>+    return NS_OK;
>   }

This is problematic.

Sometimes reparenting is done because we're trying to create a completely
new style context tree.  In this case (mInRebuildAllStyleData will be true),
we need to make new style contexts for every frame.

So if mInRebuildAllStyleData is true, you need to make a new style context
anyway.  I think that should be good enough.

(You should probably add a testcase that asserts without this fix.)



In nsContainerFrame::SetInitialChildList, please put {} around the contents
of the #ifdef DEBUG to avoid leaking variables outside the #ifdef.


Calling CreateBackdropFrameFor from inside AddChild where we add the
placeholder seems pretty odd.  Can you construct it in a more direct way?


r=dbaron with those things fixed

Comment 49

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron [:dbaron] ⌚️UTC+13 from comment #48)
> Comment on attachment 8702528 [details] [diff] [review]
> part 9 - create and render backdrop frame
> 
> Calling CreateBackdropFrameFor from inside AddChild where we add the
> placeholder seems pretty odd.  Can you construct it in a more direct way?

Discussed this with dbaron on IRC and we decided to keep this as it is:

<xidorn> dbaron: and I have no idea what is a more direct way to create backdrop frame. any suggestion? it seems to me AddChild is called in various places which could be creating a top layer frame
<dbaron> xidorn, I'm a little surprised it can be created in multiple places
<dbaron> xidorn, maybe I have the wrong mental model, though
<dbaron> xidorn, is there one backdrop for each top layer frame?
<dbaron> xidorn, if so, I guess it's fine
<xidorn> dbaron: yes
<dbaron> xidorn, ok, then maybe just paste this irc conversation into the bug
<dbaron> xidorn, sounds fine as it is

Comment 50

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f63a05272d30

Comment 51

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=84d9727e47cd

Comment 52

[Xidorn Quan [:xidorn] UTC+11]

https://hg.mozilla.org/integration/mozilla-inbound/rev/708022a7d463a876211e329507eb79795fe9a892
Bug 1064843 part 1 - Make nsImageFrame inherit nsContainerFrame. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/c18d1efe3b6336940b698373750a2d27aa77c00a
Bug 1064843 part 2 - Make nsSubDocumentFrame inherit nsContainerFrame. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c17d3fad08b13c864db7f1ced6509cbfad4745c
Bug 1064843 part 3 - Make nsFormControlFrame inherit nsContainerFrame. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/f767402068c92cc568c88e514b34def948d201d0
Bug 1064843 part 4 - Add a placeholder type for top layer. r=roc

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e8764d61e1d88bd1c486095e42f0ca83c05daf5
Bug 1064843 part 5 - Ensure frames behave properly for unknown child list id passed into SetInitialChildList. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa9137c2014540a644be0f41192e3f81bd5221f5
Bug 1064843 part 6 - Add backdrop frame list. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e0f57fab7610380df6472b513e13110278318a3
Bug 1064843 part 7 - Add backdrop pseudo-element and add related UA stylesheet. r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d48fabd4fb0f5fdedb02fb186632671c80a2087
Bug 1064843 part 8 - Add frame class for backdrop frame. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/c611f24fd6d0f5a880ef0452d75164cfa65e477d
Bug 1064843 part 9 - Change nsCSSFrameConstructor::CreatePlaceholderFrameFor to accept parent style context. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/96460bf88fb3ec6cc3a2e1bd62fff5ce32db58ac
Bug 1064843 part 10 - Create and render backdrop frame for top layer frames. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/968b6c59246546ba734f02370144ee960185b2b8
Bug 1064843 part 11 - Move checkWindowPureColor helper function from top-layer test to WindowSnapshot.js. r=roc

https://hg.mozilla.org/integration/mozilla-inbound/rev/c991d2a17bb72d0c7f8ab7b2cfe68405bbab5c83
Bug 1064843 part 12 - Add test for ::backdrop of fullscreen. r=dholbert

Comment 53

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/708022a7d463
https://hg.mozilla.org/mozilla-central/rev/c18d1efe3b63
https://hg.mozilla.org/mozilla-central/rev/1c17d3fad08b
https://hg.mozilla.org/mozilla-central/rev/f767402068c9
https://hg.mozilla.org/mozilla-central/rev/4e8764d61e1d
https://hg.mozilla.org/mozilla-central/rev/aa9137c20145
https://hg.mozilla.org/mozilla-central/rev/4e0f57fab761
https://hg.mozilla.org/mozilla-central/rev/3d48fabd4fb0
https://hg.mozilla.org/mozilla-central/rev/c611f24fd6d0
https://hg.mozilla.org/mozilla-central/rev/96460bf88fb3
https://hg.mozilla.org/mozilla-central/rev/968b6c592465
https://hg.mozilla.org/mozilla-central/rev/c991d2a17bb7

Comment 54

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/708022a7d463
https://hg.mozilla.org/mozilla-central/rev/c18d1efe3b63
https://hg.mozilla.org/mozilla-central/rev/1c17d3fad08b
https://hg.mozilla.org/mozilla-central/rev/f767402068c9
https://hg.mozilla.org/mozilla-central/rev/4e8764d61e1d
https://hg.mozilla.org/mozilla-central/rev/aa9137c20145
https://hg.mozilla.org/mozilla-central/rev/4e0f57fab761
https://hg.mozilla.org/mozilla-central/rev/3d48fabd4fb0
https://hg.mozilla.org/mozilla-central/rev/c611f24fd6d0
https://hg.mozilla.org/mozilla-central/rev/96460bf88fb3
https://hg.mozilla.org/mozilla-central/rev/968b6c592465
https://hg.mozilla.org/mozilla-central/rev/c991d2a17bb7

Comment 55

[yisibl]

Developer tools should show this pseudo element.

Comment 56

[Xidorn Quan [:xidorn] UTC+11]

(In reply to percyley from comment #55)
> Developer tools should show this pseudo element.

This is bug 1215373, and the developer tools have already been showing this pseudo element.

Comment 57

[Jean-Yves Perrier [:teoli]]

Docs:
https://developer.mozilla.org/en-US/Firefox/Releases/47#CSS
and
https://developer.mozilla.org/en-US/Firefox/Releases/47#CSS

Comment 58

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Jean-Yves Perrier [:teoli] from comment #57)
> Docs:
> https://developer.mozilla.org/en-US/Firefox/Releases/47#CSS
> and
> https://developer.mozilla.org/en-US/Firefox/Releases/47#CSS

The two URLs are identical :)

I suppose the other one is
https://developer.mozilla.org/en-US/docs/Web/CSS/::backdrop

Comment 59

[Jean-Yves Perrier [:teoli]]

Yes, of course, sorry.

Comment 60

[yisibl]

I think we should be add 'top layer' in 'Browser compatibility' table.

Comment 61

[xuyen viet]

I have applied this way to the website http://treobangron.vn/