243519 - absolute positioning doesn't work correctly if the root element has a border

Status: VERIFIED FIXED

(Core :: Layout: Positioned, defect)

Description

[Anne (:annevk)]

Personally, I consider this a "blocker", since my new weblog layout needs to use
hacks for Mozilla now (not for Opera). If this might be a simple fix, I can give
it a try.

Comment 1

[Hixie (not reading bugmail)]

The bug seems to be that we use the root element as the containing block for 
positioned elements, not the initial containing block.

This is probably a duplicate, but I don't remember seeing this bug filed before.

Opera gets this right.

Comment 2

[Boris Zbarsky [:bzbarsky]]

It's not been filed before because the root was always used but was always at
least the height of the viewport.

We do have bugs on what the ICB should be for abs pos elements, certainly.  See
bug 105286, bug 196937.

The basic problem is that we implement what CSS2 sorta said and CSS2.1 changed
it to be consistent with a different part of CSS2 that sorta said something else.

Note that mInitialContainingBlock in the frame constructor is in fact the root.
 This is used for the following cases:

1)  Absolutely positioned boxes
2)  Floating boxes with no block ancestor (eg in XUL?  Not sure how this arises,
    at this poing).

It makes sense to use the root for the latter, but for the former we should
probably use the viewport...  So the following changes probably need to happen:

a)  Have two members, not one (mRoot and mInitialContainingBlock)
b)  Audit all uses of mInitialContainingBlock and use one or the other
c)  Change the root frame construction to set those accordingly
d)  Change nsViewportFrame to have an absolute child list too, not just a fixed
    child list.  Fix its reflow method, etc, accordingly.

Comment 3

[Hixie (not reading bugmail)]

(In reply to comment #2)
> mInitialContainingBlock ... is used for the following cases:
> 
> 2)  Floating boxes with no block ancestor

The root element is forced to block-level. So the only way I can see this
happening is if the root element is floated. In which case using the root
element as the container isn't going to work... :-)

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase for dynamic change

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase for bottom/right

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Another testcase for bottom/right

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Bottom right with borders

Comment 8

[Boris Zbarsky [:bzbarsky]]

Well, I have a patch that implements my suggestion, and it's no good -- the
scrolling testcase for bottom/right fails, since the abs pos frame should move
when the viewport is scrolled and it won't if it's a child of the viewport
(assuming it's done right).

So scratch that.

Back to the issue at hand, I wonder where we handle the bottom/right positioning
bit, since the root frame is not in fact that big.  So the problem is not in
fact that we're using the root as the ICB.  Testcase coming up to demonstrate that.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase showing that the root is not the ICB here

Comment 10

[Boris Zbarsky [:bzbarsky]]

> The root element is forced to block-level

Yeah, well.  I'm not sure how this interacts with XUL, like I said.

Comment 11

[Hixie (not reading bugmail)]

Yeah, XUL might need to be fixed to handle this. Who knows.

Comment 12

[Hixie (not reading bugmail)]

In CSS terms, the ICB should be a rectangle the size of the viewport anchored at
the origin; so yeah, that's not quite the same as the viewport.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Another testcase showing that the root is sorta like the ICB

Fun stuff, eh?	;)

We may want to actually have an honest-to-God ICB frame around (that would be a
child of the canvas and a parent of the root's frame).....  That's a bloody
mess; the root code is very fragile (what with the six more-or-less separate
codepaths -- XUL, root table, root block, and all of those while printing).

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

oh no, don't add another frame to the root frame orgy

Comment 15

[Boris Zbarsky [:bzbarsky]]

Well, I don't really see how else we can implement an ICB distinct from the root
element frame....

Note that we could probably fix the original testcase in this bug by finding
whatever code screws up the cb calculation (by not taking the border/padding
into account) and fixing it.  Could be tough -- the absolute containing block
code looks pretty generic to me, so I'm not sure why it's screwing up.

Not sure about the "Another testcase showing that the root is sorta like the
ICB" testcase, though (as in, not sure whether what we'd do with the border
issue fixed is fine per the CSS spec or not).

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If anyone had asked me, I would have had CSS make the root element be the
initial containing block.

Comment 17

[Boris Zbarsky [:bzbarsky]]

That raises issues with percentage sizes on the root element (which we currently
do relative to the viewport, btw), as well as absolute positioning of the root
element (which is perfectly legal in CSS, as far as I can tell).

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes. Well in fact if anyone had asked me I would have made the root element be
given the size and position of the viewport. Make it the viewport, basically. No
absolute positioning, no sizing allowed :-). Oh well, that's not where we're at...

Currently (for HTML with no special overflow settings) we have
  viewportframe
    scrollframe
      scrollportframe
        canvasframe (mDocElementContainingBlock)
          rootelementframe (mInitialContainingBlock)

Don't we just want mInitialContainingBlock to point to canvasframe, eliminate
mDocElementContainingBlock and shift its users to mInitialContainingBlock? (It's
only used as the place for CreateDocElementFrame to attach the
rootelementframe.) We might have to audit mInitialContainingBlock users to see
if any of them really want the rootelementframe and change them if so.

Comment 19

[Hixie (not reading bugmail)]

If you can do that while still making the ICB be the height and width of the
viewport, anchored at the canvas origin...

Comment 20

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

comment 18 sounds reasonable to me.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We can customize canvasframe to have the correct ICB size and position. That
shouldn't break anything. Currently the canvasframe hacks its size to include
its overflow, but that shouldn't be necessary: overflow from the canvasframe
should be still accessible using the enclosing scrollframe.

The other question is what we need to do about XUL and about printing, if anything.

Comment 22

[Boris Zbarsky [:bzbarsky]]

> We can customize canvasframe to have the correct ICB size and position.

Only if we fix background painting accordingly.  Right now the canvas frame is
what implements the CSS rules on the canvas background being painted behind
everything.  That's why it's stretched out to cover the entire overflow area.

For printing, the ICB should probably be the size of the first page (in fact,
could _be_ the first pageframe). 

Same for print preview, I would say.

XUL... we need to solve the general problem of what to do with XUL and CSS.  :(

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> Only if we fix background painting accordingly.

Ah, right. I think we can fix background painting to just use the overflow area;
it's already so special-cased for the canvas.

> For printing, the ICB should probably be the size of the first page (in fact,
> could _be_ the first pageframe).

Yes, that's consistent.

> XUL... we need to solve the general problem of what to do with XUL and CSS.
> :(

I think we can leave XUL alone for now. I don't see why XUL can't ultimately use
the same root frame set as everything else, with judicious style rules to
disable overflow and position and size the root box to the canvas. Might require
absolute positioning on XUL, which we should do anyway :-).

Comment 24

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #23)
> Ah, right. I think we can fix background painting to just use the overflow
> area; it's already so special-cased for the canvas.

True.  Sure, let's do that.

> > For printing, the ICB should probably be the size of the first page (in 
> Yes, that's consistent.

We probably need to make pageframe have an nsAbsoluteContainingBlock member,
then, and fix its reflow methods, etc.  Same for canvas.  The viewport code can
probably be used as a basis for this... I wish we could share the code in a
useful way.  Maybe eliminate the nsFixedContainingBlock class, make the
nsAbsoluteContainingBlock constructor take the listname, and create a common
superclass for viewport/canvas/page (page already inherits from viewport, note)
that takes a listname for the abs/fixed containing block?

That should at least allow sharing most of the framelist-munging code, and the
reflow code that would be duplicated is not that much....

Comment 25

[Boris Zbarsky [:bzbarsky]]

Although, I guess with my proposal page would need to be an absolute _and_ fixed
containing block, so maybe that makes things too complicated to do the single
superclass thing.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we can just make sure that as much as possible is pushed from the host
frame to nsAbsoluteContainingBlock, so all that is duplicated in
nsBlockFrame/nsViewportFrame/nsCanvasFrame/nsPageContentFrame is some simple
forwarding logic.

Comment 27

[Boris Zbarsky [:bzbarsky]]

See ViewportFrame::SetInitialChildList, ViewportFrame::AppendFrames,
ViewportFrame::InsertFrames, etc.  We're not talking a lot of code, really; it's
as pushed as possible under the circumstances, I think....

Comment 28

[Boris Zbarsky [:bzbarsky]]

*** Bug 269746 has been marked as a duplicate of this bug. ***

Comment 29

[David Hyatt]

I'm confused by this CSS2.1 spec change.  If the ICB is just the size of the viewport, doesn't this effectively turn absolute positioned elements into fixed positioned elements when the containing block is the ICB?   This sounds like a hugely incompatible change when compared with CSS2, unless I am (hopefully) misunderstanding it.

Our corresponding bug in WebKit is:

http://bugzilla.opendarwin.org/show_bug.cgi?id=9347

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #29)
> I'm confused by this CSS2.1 spec change.  If the ICB is just the size of the
> viewport, doesn't this effectively turn absolute positioned elements into
> fixed positioned elements when the containing block is the ICB?

No, because such abs-pos elements move when you scroll the viewport, and fixed-pos elements don't. Other than that, yes they'd position the same.

Comment 31

[David Hyatt]

Is that actually compatible with the existing web?  Does this need to be quirks vs. strict, or is it safe to make this change?

Comment 32

[Anne (:annevk)]

As I already told you by e-mail... Opera is doing this and doesn't break sites to my knowledge. (We have it fixed for over a year now.)

Comment 33

[Boris Zbarsky [:bzbarsky]]

The top and left of the document element correspond with the top and left of the viewport by default, so the change doesn't really affect positioning with "top" and "left".  And since IE's support for "right" and "bottom" is pretty broken, people don't use them much.... In addition to which, "right" coincides too, so the only real behavior change for most web sites (which don't style the <html> element) is the "bottom" behavior.

So I think it should be pretty safe to make this change.  We just need to do it.  :(

Comment 34

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

So I'm trying to figure out if bug 425432 is a duplicate of this bug or not.  It's not clear to me what this bug is asking us to change.  It seems like currently we're using the correct dimensions of the containing block for absolutely positioned elements per CSS 2.1 and per compatibility with other browsers, based on my tests at http://dbaron.org/css/test/2008/abs-pos-cb/ , but we're getting the position wrong since we're offsetting by the top/left margin and border of the root element.

Is that what this bug is asking us to fix?  It's really not clear to me.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think you are correct, what we need to fix here is the positioning of abs-pos elements when the root element has a margin or border.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I have a patch that fixes this by making CanvasFrame (the child of the root scrollframe) an absolute containing block. Works great.

I just need to fix printing too. I think I'll fix printing by adding a CanvasFrame as the child of the nsPageContentFrame --- less additional code than making nsPageContentFrame be an absolute containing block, and makes the frame hierarchy more consistent between printing and screen.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

My patch works well, including printing. It also makes us use ConstructBlock to construct a root block frame, which enables absolute positioning of root blocks. During this work I simplified the "propagate root element background to viewport" code, which I've submitted in bug 438987. The changes there conflict with my patch here a little, so I'll submit my patch here when that bug is fixed.

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: WIP

This is just for backup or possibly for reference if someone wants to see how to make an element be an abs-pos container with the current setup.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Here's the complete patch.

I possibly got a little carried away with the scope here. Once the canvas frame is an abs-pos container, it's relatively easy to support absolute and fixed positioning of the root element; just switch to ConstructBlock and ConstructTableFrame in ConstructDocElementFrame. That also enables -moz-column on the root element. Removing ConstructDocElementTableFrame also makes fixed-pos content work when the root element is a table. Abs-pos content also works when the root element is a table (provided you don't explicitly make the table an abs-pos container).

The trickiest changes here are the changes to pagination so that every nsPageContentFrame has a CanvasFrame child and the CanvasFrames support continuations. Hence r? fantasai. As I mentioned above, this approach seemed simpler and leading to greater consistency compared to the alternative of making nsPageContentFrame an abs-pos container.

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

wanted1.9.1 because this fixes a visual Acid3 bug.

Comment 41

[Boris Zbarsky [:bzbarsky]]

roc, how is the case of auto-offset abs-pos root table handled?  I don't see reflow state changes here to deal with lack of an ancestor block, and CanvasFrame isn't a block, right?

Comment 42

[Boris Zbarsky [:bzbarsky]]

Or for that matter, auto-offset abs-pos root block, since we now allow positioning it.

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Good question. I'll check.

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I presume the correct behaviour is that auto is treated as 0, since the root element will be positioned at (0,0) if it's not positioned.

Comment 45

[Boris Zbarsky [:bzbarsky]]

Yeah, the right behavior is that plus not crashing.

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

The last patch had some problems with overflow-container stuff that caused reftest failures. This patch passes reftests and adds some more tests, in particular tests for abs-pos root elements with auto offsets (which worked fine --- CanvasFrame returns true for IsContainingBlock()) and printing with a page-overflowing positioned root element.

Boris, if you're looking at this and feel like reviewing it instead of David, that's fine.

Comment 47

[fantasai]

Comment on attachment 333185 [details] [diff] [review]
fix v2

Comments for things below "diff --git a/layout/generic/nsHTMLFrame.cpp b/layout/generic/nsHTMLFrame.cpp"

+CanvasFrame::SetInitialChildList(nsIAtom*        aListName,
+                                 nsIFrame*       aChildList)

Add an assertion to make sure aChildList has only one frame if aListName is not the abspos list.

+                                                  There may already be
+      // children placeholders which don't get reflowed but must not be
+      // lost until the canvas frame is destroyed.

I believe this comment is no longer accurate due to bz's fix for bug 200774.

-
+    

You have a few stray whitespace changes here and there.

+    nsRect overflowContainerBounds;
+    ReflowOverflowContainerChildren(aPresContext, aReflowState,
+                                    overflowContainerBounds, 0,
+                                    aStatus);
+    aDesiredSize.mOverflowArea.UnionRect(aDesiredSize.mOverflowArea,
+                                         overflowContainerBounds);

Just
+    ReflowOverflowContainerChildren(aPresContext, aReflowState,
+                                    aDesiredSize.mOverflowArea, 0,
+                                    aStatus);
will do. ReflowOverflowContainerChildren expands the rect, it does not replace it.
"Expands aOverflowRect as necessary to accommodate these children."

+  // A PageContentFrame must always have one child: the canvas frame.

This has nothing to do with the subsequent if-block, so I suggest moving it to right on top of
   // Resize our frame allowing it only to be as big as we are

-  return NS_NewAreaFrame(aPresShell, aContext,
+  return NS_NewBlockFrame(aPresShell, aContext,

I don't understand these changes.

Comment 48

[fantasai]

Comment on attachment 333185 [details] [diff] [review]
fix v2

+        // Root overflow containers will be normal children of
+        // the canvas frame, but that's ok because there
+        // aren't any other frames we need to isolate them from
+        // during reflow.

In order for this to work, you need to override StealFrame (like I did in nsColumnSetFrame)

virtual nsresult StealFrame(nsPresContext* aPresContext,
                            nsIFrame*      aChild,
                            PRBool         aForceNormal)
{ // CanvasFrame keeps overflow container continuations of its child frame in main child list
  return nsContainerFrame::StealFrame(aPresContext, aChild, PR_TRUE) ||
         nsContainerFrame::StealFrame(aPresContext, aChild)
}

Comment 49

[fantasai]

     // how the root frame hierarchy should look

Does this comment need to be updated to match the changes you're making in this patch?

+  nsFrameItems frameItems;
...
-  *aNewFrame = contentFrame;
+  *aNewFrame = frameItems.childList;
+  NS_ASSERTION(!frameItems.childList || !frameItems.childList->GetNextSibling(),
+               "Multiple frames created for root element?");

What you're doing here with frameItems makes absolutely no sense to me. Why are only some roots assigned to frameItems.AddChild(), but *aNewFrame is changed to use frameItems.childList.. which is null in half the cases. Then later you have a check to make sure frameItems only has one child.. Why would you even bother to use a nsFrameItems if you only need to deal with one frame?

-                    isBlockFrame);
+                    PR_FALSE);

If I'm following correct, in some cases here the aFrame argument to ProcessChildren will indeed be a block. Why is it always false for this call?

@@ -10542,19 +10555,19 @@ nsCSSFrameConstructor::ReplicateFixedFra

So.. the reason these placeholders are in the docRootFrame and not the pageContentFrame (which is where they used to be) is because of bug 200774. Given bz's description in bug 200774 comment 5, I would suspect that you're reintroducing that crash here. You should test that. If that's the case, then you need to push those placeholders back down into the docRootElement. Alternatively, maybe you can do some display list fiddling so that instead of replicating the fixed frames on each page, we just paint the pageContentFrame's FirstInFlow's frames over every page. Replicating frames like we do now is both hacky and buggy anyway.

I don't understand nsCSSFrameConstructor's state-saving stuff, so I didn't review that. Also I didn't quite understand the changes in nsFrame.cpp.

Please document somewhere what the (entire) root frame hierarchy should look like and how it maps to the various data members of nsCSSFrameConstructor, from the reflow root down to the docRootElementFrame, in screen, print, and print preview modes. I don't remember if this is written down already, but it should be. And if it is, it'll need updating. :)

Comment 50

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #47)
> -  return NS_NewAreaFrame(aPresShell, aContext,
> +  return NS_NewBlockFrame(aPresShell, aContext,
> 
> I don't understand these changes.

I'm just getting rid of the use of nsAreaFrames, which are nsBlockFrames in (almost) all but name.

(In reply to comment #48)
> In order for this to work, you need to override StealFrame (like I did in
> nsColumnSetFrame)
> 
> virtual nsresult StealFrame(nsPresContext* aPresContext,
>                             nsIFrame*      aChild,
>                             PRBool         aForceNormal)
> { // CanvasFrame keeps overflow container continuations of its child frame in
> main child list
>   return nsContainerFrame::StealFrame(aPresContext, aChild, PR_TRUE) ||
>          nsContainerFrame::StealFrame(aPresContext, aChild)
> }

Good catch. But why is the || needed? nsColumnSetFrame doesn't have it. Seems to me that just passing PR_TRUE for aForceNormal should be sufficient.

(In reply to comment #49)
>      // how the root frame hierarchy should look
> 
> Does this comment need to be updated to match the changes you're making in
> this patch?

No, because that only describes the non-printing case which is unaffected by the patch.

> +  nsFrameItems frameItems;
> ...
> -  *aNewFrame = contentFrame;
> +  *aNewFrame = frameItems.childList;
> +  NS_ASSERTION(!frameItems.childList ||
> !frameItems.childList->GetNextSibling(),
> +               "Multiple frames created for root element?");
> 
> What you're doing here with frameItems makes absolutely no sense to me. Why
> are only some roots assigned to frameItems.AddChild(), but *aNewFrame is
> changed to use frameItems.childList.. which is null in half the cases. Then
> later you have a check to make sure frameItems only has one child.. Why would
> you even bother to use a nsFrameItems if you only need to deal with one frame?

ConstructBlock and ConstructTableFrame both want to append the new frame in an nsFrameItems, so we have to use one. All four cases are setting frameItems.

> -                    isBlockFrame);
> +                    PR_FALSE);
> 
> If I'm following correct, in some cases here the aFrame argument to
> ProcessChildren will indeed be a block. Why is it always false for this call?

contentFrame can't be a block, since the only paths that set processChildren to true set contentFrame to be an SVG or XUL frame.

> So.. the reason these placeholders are in the docRootFrame and not the
> pageContentFrame (which is where they used to be) is because of bug 200774.
> Given bz's description in bug 200774 comment 5, I would suspect that you're
> reintroducing that crash here. You should test that.

I am pleased to report that the crash does not occur. I'll add a crashtest for 200774 though.

> If that's the case, then
> you need to push those placeholders back down into the docRootElement.

We can't do that because that root element might be positioned (abs-pos or fixed-pos) itself :-).

> Alternatively, maybe you can do some display list fiddling so that instead of
> replicating the fixed frames on each page, we just paint the
> pageContentFrame's FirstInFlow's frames over every page. Replicating frames
> like we do now is both hacky and buggy anyway.

That is a very good idea and I wish I'd thought of it. I will file a followup bug for that work, it would simplify things considerably.

> I don't understand nsCSSFrameConstructor's state-saving stuff, so I didn't
> review that. Also I didn't quite understand the changes in nsFrame.cpp.

No worries, dbaron or someone else can take care of that.

> Please document somewhere what the (entire) root frame hierarchy should look
> like and how it maps to the various data members of nsCSSFrameConstructor,
> from the reflow root down to the docRootElementFrame, in screen, print, and
> print preview modes. I don't remember if this is written down already, but it
> should be. And if it is, it'll need updating. :)

OK.

Comment 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v3

With those comments addressed.

Comment 52

[fantasai]

> Good catch. But why is the || needed? nsColumnSetFrame doesn't have it. Seems
> to me that just passing PR_TRUE for aForceNormal should be sufficient.

Because the abspos continuations are on the overflow containers list.

Comment 53

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v4

Yeah, OK. But I think this is the right way to do it, a straight || isn't correct for nsresults and we also need to honor aForceNormal here.

Comment 54

[Gabriele Best [:Gabri]]

Any progress?

Comment 55

[fantasai]

> ConstructBlock and ConstructTableFrame both want to append the new frame in an
> nsFrameItems, so we have to use one. All four cases are setting frameItems.

Ok, then can we localize the mess and create a frameItems inside the
if-block only for the calls where it's necessary? I think this code
would be more understandable if you used a single nsIFrame* variable
to represent the docRootElementFrame instead of a single-item
frameItems. I'm having a very hard time following what's going on
because it's very unclear what each variable represents. I shouldn't
have to read the contents of Construct*Frame in order to understand
which variable is holding the new docRootelementFrame.

> > -                    isBlockFrame);
> > +                    PR_FALSE);
> > ...
> 
> contentFrame can't be a block, since the only paths that set processChildren
> to true set contentFrame to be an SVG or XUL frame.

Ok, then please add a comment explaining this.

+       how the root frame hierarchy should look

Thanks for this.

+    mInitialContainingBlock is "root element frame".
+    mDocElementContainingBlock is the parent of mInitialContainingBlock
+      (i.e. CanvasFrame or nsRootBoxFrame)

Sounds like we need a bug open for renaming mInitialContainingBlock
to mRootElementFrame.

   // the placeholders must be kids of a block, so we want to move over the

This comment, and others in this function, are no longer accurate
given your changes, so you need to update them.

+    if (!aForceNormal &&
+        NS_SUCCEEDED(nsContainerFrame::StealFrame(aPresContext, aChild)))
+      return NS_OK;
+    return nsContainerFrame::StealFrame(aPresContext, aChild, PR_TRUE);

So the point of aForceNormal is to force StealFrame to check the
main child list even if this is an overflow continuation. It's
given a default value of PR_FALSE because you normally don't set
this: the main place this happens is when overriding StealFrame
like we do in nsColumnSetFrame or here. There's no concept of
"honoring" aForceNormal if both regular frames and overflow
continuations are on the same list.

There's only ever one child of the canvas frame, right? And
checking that list doesn't require a proptable lookup. So we
probably want to check that first. If we don't find the frame
there, then we pass through to nsContainerFrame::StealFrame.

     // Get replicated fixed frames' placeholders out of the way
     nsFrameList stolenPlaceholders = StealFixedPlaceholders(frame);
...
     // Put removed fixed placeholders back
     rv = ReplaceFixedPlaceholders(frame, stolenPlaceholders);
     NS_ENSURE_SUCCESS(rv, rv);

This should no longer be necessary now that the placeholders are
in a canvas frame and don't interfere with the docRootElement's
reflow.

I didn't review your tests. Let me know if you think that's necessary.

Comment 56

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #55)
> > ConstructBlock and ConstructTableFrame both want to append the new frame in
> > an nsFrameItems, so we have to use one. All four cases are setting
> > frameItems.
> 
> Ok, then can we localize the mess and create a frameItems inside the
> if-block only for the calls where it's necessary? I think this code
> would be more understandable if you used a single nsIFrame* variable
> to represent the docRootElementFrame instead of a single-item
> frameItems. I'm having a very hard time following what's going on
> because it's very unclear what each variable represents. I shouldn't
> have to read the contents of Construct*Frame in order to understand
> which variable is holding the new docRootelementFrame.

OK.

> > > -                    isBlockFrame);
> > > +                    PR_FALSE);
> > > ...
> > 
> > contentFrame can't be a block, since the only paths that set processChildren
> > to true set contentFrame to be an SVG or XUL frame.
> 
> Ok, then please add a comment explaining this.

OK.

> +    mInitialContainingBlock is "root element frame".
> +    mDocElementContainingBlock is the parent of mInitialContainingBlock
> +      (i.e. CanvasFrame or nsRootBoxFrame)
> 
> Sounds like we need a bug open for renaming mInitialContainingBlock
> to mRootElementFrame.

OK, I'll file one.

>    // the placeholders must be kids of a block, so we want to move over the
> 
> This comment, and others in this function, are no longer accurate
> given your changes, so you need to update them.

OK.

> +    if (!aForceNormal &&
> +        NS_SUCCEEDED(nsContainerFrame::StealFrame(aPresContext, aChild)))
> +      return NS_OK;
> +    return nsContainerFrame::StealFrame(aPresContext, aChild, PR_TRUE);
> 
> So the point of aForceNormal is to force StealFrame to check the
> main child list even if this is an overflow continuation. It's
> given a default value of PR_FALSE because you normally don't set
> this: the main place this happens is when overriding StealFrame
> like we do in nsColumnSetFrame or here. There's no concept of
> "honoring" aForceNormal if both regular frames and overflow
> continuations are on the same list.
> 
> There's only ever one child of the canvas frame, right? And
> checking that list doesn't require a proptable lookup. So we
> probably want to check that first. If we don't find the frame
> there, then we pass through to nsContainerFrame::StealFrame.

OK, should I assert that it's false here, then?

>      // Get replicated fixed frames' placeholders out of the way
>      nsFrameList stolenPlaceholders = StealFixedPlaceholders(frame);
> ...
>      // Put removed fixed placeholders back
>      rv = ReplaceFixedPlaceholders(frame, stolenPlaceholders);
>      NS_ENSURE_SUCCESS(rv, rv);
> 
> This should no longer be necessary now that the placeholders are
> in a canvas frame and don't interfere with the docRootElement's
> reflow.

OK.

Comment 57

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #55)
> Sounds like we need a bug open for renaming mInitialContainingBlock
> to mRootElementFrame.

Filed bug 452345.

Comment 58

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v5

Comment 59

[fantasai]

> OK, should I assert that it's false here, then?

You can. It should be false unless a class that inherits from CanvasFrame needs to override StealFrame.

> Ok, then can we localize the mess ... I shouldn't have to read the contents of
> Construct*Frame in order to understand which variable is holding the new
> docRootelementFrame.

Depending on where I'm reading from, in some cases it seems
  *aNewFrame == contentFrame && contentFrame == mInitialContainingBlock
and in others
  *aNewFrame != contentFrame && contentFrame == mInitialContainingBlock

What does contentFrame represent, and why is *aNewFrame only sometimes == mInitialContainingBlock? Or am I missing something?

Comment 60

[Robert O'Callahan (:roc) (email my personal email if necessary)]

contentFrame and *aNewFrame are in fact always the same. I'll fix the patch to eliminate the ambiguity.

Comment 61

[fantasai]

You probably also need to remove the code here:
  http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp#1535

   // If the containing block is the initial containing block and it has a
   // height that depends on its content, then use the viewport height instead.
   // This gives us a reasonable value against which to compute percentage
   // based heights and to do bottom relative positioning

If you set "position: relative" on the root element and give it some abspos kids, they should behave the same as if you'd set "position: relative" on the <body> element instead. Might need some testcases for that.

Comment 62

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #60)
> contentFrame and *aNewFrame are in fact always the same. I'll fix the patch to
> eliminate the ambiguity.

I lied. They can be different, in particular, when the root frame is absolute or fixed positioned, contentFrame is the out-of-flow frame and *aNewFrame is the placeholder.

So for SVG and XUL we don't support absolute or fixed positioning. It wouldn't be hard to add for SVG, but I'm not even sure the SVG WG would approve, so I'll leave that for now if that's OK.

I added a comment explaining this.

I removed the code you pointed out, thanks. Testing that exposed some other issues:
-- Positioning of a root block element didn't make it an abs-pos containing block. Fixed by passing IsPositioned into ConstructBlock in ConstructDocElementFrame.
-- Relative positioning of the root element didn't work. Fixed by adding code to CanvasFrame::Reflow to use the kid's mComputedOffsets.
-- CalculateContainingBlockSizeForAbsolutes in nsBlockFrame.cpp would, for root element frames, think that the block frame was not the outermost frame for the content and try to obtain the abs-pos containing block size from the outermost frame for the content, namely the viewport nsHTMLScrollFrame. This is not what we want, we should use the size for the block itself in this case.

Reftest 243519-7.html added.

Comment 63

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v6

Fix as per comment above. Except that I decided to make positioned SVG root elements work, for consistency (and they already do in Opera and Webkit too). So I added a call to AddChild in ConstructDocElementFrame, plus a test for that.

Comment 64

[fantasai]

-      if (aLastRS->ComputedWidth() != NS_UNCONSTRAINEDSIZE && !isCanvasBlock) {
+      if (aLastRS->ComputedWidth() != NS_UNCONSTRAINEDSIZE) {
         cbSize.width = PR_MAX(0,
           aLastRS->ComputedWidth() + aLastRS->mComputedPadding.LeftRight() - scrollbars.LeftRight());
-      }
-      if (aLastRS->ComputedHeight() != NS_UNCONSTRAINEDSIZE) {
         cbSize.height = PR_MAX(0,
           aLastRS->ComputedHeight() + aLastRS->mComputedPadding.TopBottom() - scrollbars.TopBottom());

You're removing the check for NS_UNCONSTRAINEDSIZE from the height calculation. Why?

Comment 65

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v7

Sorry, that was just a mistake.

Comment 66

[fantasai]

Comment on attachment 336425 [details] [diff] [review]
fix v7

Please make sure we have some tests for
  - percentage height/width calculation on root element, both abspos and unpositioned
  - percentage calculation on abspos children of root, both when root is and is not position: relative itself.

r=fantasai on the patch. Thanks roc!

Comment 67

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

(In reply to comment #66)
> Please make sure we have some tests for
>   - percentage height/width calculation on root element, both abspos and
> unpositioned

You might want this for both standards mode and quirks mode, too.

Comment 68

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

Comment on attachment 336425 [details] [diff] [review]
fix v7

sr=dbaron

Comment 69

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #66)
>   - percentage calculation on abspos children of root, both when root is and is
> not position: relative itself.

I'm not sure what you expect for percentage height of an abs-pos child where the root is relatively positioned. The root element by default has auto height and the child is going to size vertically relative to whatever that intrinsic height is --- not the viewport. So I'm leaving that test out.

Comment 70

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed 75919d3eb3d0 (including the extra tests asked for).

Comment 71

[Robert O'Callahan (:roc) (email my personal email if necessary)]

After landing this I discovered that with this patch, Acid3 crashes. With a little more digging I discovered that layout/base/crashtests/243519-1.html was broken --- JS syntax error meant that it didn't really test anything. Not sure when that crept in. Basically, ReconstructDocElementHierarchyInternal was not handling positioned elements correctly, it was failing to remove the placeholder and also RemoveFixedItems would remove the root element's frame too early if the root element was fixed-position.

So I was a bad person and fixed up the test and the failure on the spot. The changeset is here:
http://hg.mozilla.org/mozilla-central/rev/14ce7619e9c1
Please review...

Comment 72

[mdew]

I haven't seen any improvements in the acid3 test since this landed, was this expected?

Comment 73

[philippe (part-time)]

(In reply to comment #72)
> I haven't seen any improvements in the acid3 test since this landed, was this
> expected?
There _is_ a change. At the top right of the box, there is a 'x' with a magenta background. Before this patch, it was placed outside of the box (too low and too much to the right); now it is positioned correctly inside, in the top-right corner (exactly).

Comment 74

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I backed this out, my attempt in comment #71 caused crashes running other tests.

Comment 76

[fantasai]

> I'm not sure what you expect for percentage height of an abs-pos child where
> the root is relatively positioned. The root element by default has auto height
> and the child is going to size vertically relative to whatever that intrinsic
> height is --- not the viewport. So I'm leaving that test out.

A percentage height on an abspos child should be the ICB (i.e. the viewport) when the root element is not relatively positioned. When it is relatively positioned, then the percentage height needs to be relative to the height of the root element, because that is its containing block, not the ICB. I do want to see a test for this, as we currently have some code that mucks around with these things and treats abspos children of the root specially.

Comment 77

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, sure. That test did actually work. I'll add it.

Comment 78

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v8

The crashing issue was stupid. This patch passes crashtests and reftests, and doesn't crash on Acid3.

The changes from the last version:
-- added 234519-9f.html as fantasai requested
-- nsCSSFrameConstructor::RemoveFixedItems should not remove the root element's frame if the root element is fixed-pos
-- nsCSSFrameConstructor::ReconstructDocElementHierarchyInternal needs to handle the case where the root element frame is out-of-flow.

layout/base/crashtests/243519-1.html is already fixed on trunk, I didn't back out that change.

Comment 79

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

Comment on attachment 337362 [details] [diff] [review]
fix v8

r+sr=dbaron

Comment 80

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Relanded 679778dd198f.

Comment 81

[Tony Mechelynck [:tonymec]]

The "fix" for this bug has unacceptable side-effects in the Editor, see bug 454004 and in particular bug 454004 comment #16.

Comment 82

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Roc, are the bottom right testcases correct? I don't see anything on that position. The whole tab is blank.

Comment 83

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Which testcase is that?

Comment 84

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Roc, all three testcases with the bottom right text attached to this bug:
attachment 148430 [details], attachment 148431 [details], and attachment 148432 [details].

Comment 85

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #84)
> Roc, all three testcases with the bottom right text attached to this bug:
> attachment 148430 [details],

Is broken, most of its content is inside a <script>

> attachment 148431 [details]

Ditto

> and attachment 148432 [details].

Ditto

Comment 86

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified on trunk and 1.9.1 with builds on OS X and Windows by checking the given URL and the reftests.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090204 Shiretoko/3.1b3pre ID:20090204020327

Comment 87

[Boris Zbarsky [:bzbarsky]]

This caused bug 482886.

Comment 88

[Hr Gwea]

This bug is still present when the root element has display:table

See here: http://jsfiddle.net/Lvj4xf40/

Tested in Firefox 34

Comment 89

[Bill Gianopoulos [:WG9s]]

(In reply to Hr Gwea from comment #88)
> This bug is still present when the root element has display:table
> 
> See here: http://jsfiddle.net/Lvj4xf40/
> 
> Tested in Firefox 34

Well, then you should file a new bug with steps to reproduce and a test case etc.  Don't post on a closed bug because that will just be ignored.

Comment 90

[Boris Zbarsky [:bzbarsky]]

That testcase has nothing to do with this bug.  For one thing, the <div> there is not the root element.

That said the rendering of that testcase is not currently precisely defined by the CSS specification.  See thread starting at https://lists.w3.org/Archives/Public/www-style/2014Mar/0208.html from almost a year ago.