277420 - Setting overflow disables column layout [columns]

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

The basic issue is that ConstructFrameByDisplayType has several branches, for
positioned blocks, for blocks with overflow set, etc.  Not all of these end up
calling into ConstructBlock, which is where columns are handled; in particular
the "blocks with overflow" branch does not.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 2

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

This one's going to be harder because scrollframe construction is more icky. I
have to figure out how to marry it to ConstructBlock.

Comment 3

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

Attachment: fix

Okay, here it is. I haven't really modified the scroll stuff much. I basically
just inlined BuildScrollFrame, switched InitAndRestoreFrame to ConstructBlock,
and then simplified things a bit.

I simplified FinishBuildingScrollFrame because it didn't need most of its
parameters.

I removed SetScalingOfTwips(PR_FALSE) from BuildScrollFrame because it doesn't
seem to be necessary --- or effective. BeginBuildingScrollFrame sets it,
anyway, and then clears it on exit! So it was always cleared for the rest of
BuildScrollFrame already.

In ConstructBlock I removed the contentParentFrame ? contentParentFrame :
parentFrame logic from the call to AddChild. This was firing an assertion in
AddChild when contentParentFrame != null and contentParentFrame != parentFrame,
because *newFrame's parent is *always* aParentFrame. It's easy to verify this
within ConstructBlock.

I also had to change ua.css as shown, to make sure that the column styles
actually get set on the scrolled-content frame.

Comment 4

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

BTW fairly soon I'm going to modify scrollframes so that there is no scrollport
frame. That will simplify scrollframe construction quite a bit and hopefully
make it easier to rework the construction code.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> This was firing an assertion in AddChild

Which one?

Looking at the ConstructBlock code, aParentFrame is the parent of the new block,
right?  The last arg to AddChild() should be the frame which should be the
parent for the placeholder of the block in case the block is out-of-flow.  This
is absolutely not the same thing as aParentFrame.  Note that we really do want
aContentParentFrame here, not contentParentFrame.

Comment 6

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

Oh yeah. New patch coming up.

Comment 7

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

Attachment: fix #2

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 172402 [details] [diff] [review]
fix #2

So first of all, could you document the scrollframe-building methods?  At the
moment, for example, the comment where BuildScrollFrame is declared flat-out
lies about whether it sets the primary frame...

Also, is there a reason BeginBuildingScrollFrame takes a document?  Can't it
just get mDocument as needed?

>Index: layout/base/nsCSSFrameConstructor.cpp
>@@ -6540,77 +6504,53 @@ nsCSSFrameConstructor::ConstructFrameByD

>+    nsFrameItems blockItem;
>+    rv = ConstructBlock(aPresShell, aPresContext, aState, aDisplay, aContent,
>+                        scrollFrame, scrollFrame, scrolledContentStyle,
>+                        &scrolledFrame, blockItem, aDisplay->IsPositioned());
>+    NS_ASSERTION(blockItem.childList == scrolledFrame,
>+                 "Scrollframe's frameItems should be exactly the scrolled frame");

Hmm... ConstructBlock will call AddChild and pass it the new block you passed
in (scrolledFrame, in this case) and the display struct you passed in (which is
aDisplay, in this case).  We may have say a scrollable absolutely positioned
frame, in which case this will end up sticking it in the abs containing block's
childlist, not in blockItem (try it -- it should trigger your assert).	I think
you want to pass the display struct of scrolledContentStyle here, and add a
comment to ua.css that people should not set "position: inherit" or "float:
inherit" on the scrolled content.

It's also worth writing some testcases for various scrolled stuff (positioned,
not positioned, with positioned kids), check that we create the right frame
trees with this patch, and stick it in the regression tests.

> nsCSSFrameConstructor::ConstructBlock(nsIPresShell*            

The second cosmetic change doesn't seem that useful, but your call.  ;)

Comment 9

[Boris Zbarsky [:bzbarsky]]

One other thing.  While you're messing with this, could you document that
aNewFrame is an inout param for ConstructBlock and that it will get reset to the
"outermost" frame for the block?

Comment 10

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

Attachment: fix #3

Okay. I did all the stuff you said. A testcase is included that tests various
elements with absolute children:
-- positioned, scrolled
-- scrolled
-- positioned, columns
-- positioned, scrolled, columns
-- scrolled, columns

One tricky issue is how to handle cases where a columns element is positioned
and has abs-pos children. I've changed ConstructBlock so that the anonymous
block child of nsColumnSetFrame becomes the abs-pos container. This seems to
give the right effect, although I had to fix nsBlockFrame's
CalculateContainingBlock to make it work for non-ICB blocks with absolute
children that aren't themselves positioned. Eventually I want to make such
absolute frames splittable across column/page breaks, but this is a step in the
right direction.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 172502 [details] [diff] [review]
fix #3

>Index: layout/base/nsCSSFrameConstructor.h

>-  //  NOTE: this method does NOT set the primary frame for the content element
>-  //

Why not just change this to say that it DOES set the primary frame (to the
*aNewFrame it outs.

Also, document that aNewFrame is in fact just an out param?

>+  // aFrameItems is where we want to put the block in case it's in-flow.
>+  // aNewFrame is an out parameter that will be reset to the outermost
>+  // frame constructed (e.g. if we need to wrap the block in an
>+  // nsColumnSetFrame.

Well, aNewFrame is a inout parameter.  We pass in the frame for the block in
*aNewFrame, and get back the "outermost frame" for the block in *aNewFrame.  I
find this rather confusing, hence my request that we document it....

Could we also document that aParentFrame is the parent for *aNewFrame and
aContentParentFrame is what the parent would have been had *aNewFrame been
in-flow?

>Index: layout/base/nsLayoutUtils.cpp
>+PRBool
>+nsLayoutUtils::IsInitialContainingBlock(nsIFrame* aFrame)
>+{
>+  nsIContent* content = aFrame->GetContent();
>+
>+  if (content && !content->GetParent()) {
>+    // The containing block corresponds to the document element so it's
>+    // the initial containing block
>+    return PR_TRUE;

This is wrong.	For example, consider the following HTML file:

<html style="display: table">
aaa
</html>

The frame model we build looks like this (only the relevant part):

  HTMLScroll(html)(-1)@0x8587740 {0,0,8540,5250} [state=81c40084]
[content=0x84b48d0] [sc=0x85876f0] pst=:-moz-viewport-scroll<
    ScrollBox(html)(-1)@0x85878b8 [view=0x858dfb0] next=0x8587bdc
{0,0,8540,5250} [state=80c42094] [content=0x84b48d0] [sc=0x858f0a4]
pst=:-moz-scrolled-content<
      Canvas(html)(-1)@0x8587650 [view=0x8590720] {0,0,8540,5250}
[state=00002014] [content=0x84b48d0] [sc=0x858f168] pst=:-moz-canvas<
	TableOuter(html)(-1)@0x85908e8 {0,0,644,462} [state=00010004]
[content=0x84b48d0] [sc=0x858f314] pst=:-moz-table-outer<
	  Table(html)(-1)@0x85909fc {0,0,644,462} [state=00010004]
[content=0x84b48d0] [sc=0x858f260]<
	    TableRowGroup(html)(-1)@0x8590cc4 {0,0,644,462} [state=00010004]
[content=0x84b48d0] [sc=0x8590c18] pst=:-moz-table-row-group<
	      TableRow(html)(-1)@0x8590dac {0,0,644,462} [state=00010004]
[content=0x84b48d0] [sc=0x8590d5c] pst=:-moz-table-row<
		TableCell(html)(-1)@0x8590f08 {0,0,644,462} [state=00000004]
[content=0x84b48d0] [sc=0x8590eb8] pst=:-moz-table-cell<
		  Block(html)(-1)@0x8590fc4 {0,0,644,462} [state=00c00014]
sc=0x8591070(i=0,b=1) pst=:-moz-cell-content<

That's 9 different frames all with the same mContent, and only one of these
ought to be the initial containing block...

>Index: layout/base/nsLayoutUtils.h
>+   * If you can't figure out what this function does, you are stupid.
>+   */
>+  static PRBool IsInitialContainingBlock(nsIFrame* aFrame);

How about we just say this returns whether aFrame is the CSS initial containing
block for aFrame's presshell?  ;)

Comment 12

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

Attachment: fix #4

OK, all those comments addressed.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 173680 [details] [diff] [review]
fix #4

>Index: layout/base/nsCSSFrameConstructor.h
>+  // @param aContentParent is the parent the block would have if it
>+  // was in-flow

s/was/were/

r+sr=bzbarsky

Comment 14

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

checked in. thanks.

Comment 15

[Mats Palmgren (inactive)]

This caused bug 282754