267037 - GetFloatContainingBlock fails in some cases (BuildFloatList removal)

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We have some cases where GetFloatContainingBlock() actually returns the wrong
thing....  Right now, BuildFloatList() covers up these issues so we get the
right layout, but we trigger the following assert:

###!!! ASSERTION: Float frame has wrong parent: 'floatFrame->GetParent() ==
mBlock', file
/home/bzbarsky/mozilla/xlib/mozilla/layout/html/base/src/nsBlockReflowState.cpp,
line 862

This happens because GetFloatContainingBlock() thinks that only block-display
frames can be float containing blocks.  That's not quite true, since legends,
table cells, and <button> frames can all be float containing blocks.

At the moment, table cells happen to work because they implement
GetContentInsertionFrame().  Perhaps we should do the same for <button>.  But
that won't help legends.

Thoughts on how to fix it?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase for some of the issues

Note that this testcase crashes on unload due to bug 267039... So be careful.

Comment 2

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

Attachment: fix

This implements GetContentInsertionFrame as you suggest for buttons. That is
certainly the right thing.

It also sets display:block on legends, which I think is also clearly the right
thing. This is enough to fix this bug and the crash in bug 267039.

Comment 3

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

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 174312 [details] [diff] [review]
fix

>Index: layout/forms/nsHTMLButtonControlFrame.h
>+  virtual nsIFrame* GetContentInsertionFrame() {
>+    return GetFirstChild(nsnull)->GetContentInsertionFrame();

Might be worth checking whether we can replace the InsertFrames/etc code in
this class with asserts...  (not sure what happens for out-of-flow buttons, for
example).  File a separate bug, cc me and mats.palm?

I just looked at all callers of PushFloatContainingBlock in the frame
constructor.  The following still wouldn't get flagged (as far as I can tell)
as containing blocks by GetFloatContainingBlock after this patch:

1)  An SVGOuterFrame if the documentElement is <svg:svg>.  Given that
    that's not a blockframe, the fact that it's pushed at all is probably
    a bug...
2)  The scrolled frame inside a select (not sure about this one; see
    InitializeSelectFrame()).
3)  SVG <foreignObject> frames (since we'll actually create a blockframe
    no matter what the display value of the node is).

May be worth filing separate bugs on the SVG issues, I guess, but we should
sort out whether <select> is a problem (that is, whether the display of the
scrolled content there will be block).

r+sr=bzbarsky for the part you have attached here, though.

Comment 5

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

Checked in and filed followup bugs 285164 and 285165.

Comment 6

[Martijn Wargers (dead)]

So legend {display:none} is not working anymore, because of this?
This causes an issue here: http://gathering.tweakers.net/
The Quicksearch text has become visible because of this (at the top right, right
above the text input), while it didn't show up before this fix.
Fwiw, Opera8b and IE6 support legend {display:none}.

Comment 7

[Ger Teunis]

(In reply to comment #6)
> So legend {display:none} is not working anymore, because of this?
> This causes an issue here: http://gathering.tweakers.net/
> The Quicksearch text has become visible because of this (at the top right, right
> above the text input), while it didn't show up before this fix.
> Fwiw, Opera8b and IE6 support legend {display:none}.
> 

I confirm.
Robert, should we reopen this bug or file a new one?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Please don't quote comments in their entirety?

The issue Martijn mentioned will likely get addressed in bug 283385...