GetFloatContainingBlock fails in some cases (BuildFloatList removal)

RESOLVED FIXED

Status

()

Core
Layout: Floats
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: bz, Assigned: roc)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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?
Created attachment 164102 [details]
Testcase for some of the issues

Note that this testcase crashes on unload due to bug 267039... So be careful.
Created attachment 174312 [details] [diff] [review]
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.
Assignee: nobody → roc
Status: NEW → ASSIGNED
Attachment #174312 - Flags: superreview?(bzbarsky)
Attachment #174312 - Flags: review?(bzbarsky)
*** Bug 267039 has been marked as a duplicate of this bug. ***
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.
Attachment #174312 - Flags: superreview?(bzbarsky)
Attachment #174312 - Flags: superreview+
Attachment #174312 - Flags: review?(bzbarsky)
Attachment #174312 - Flags: review+
Checked in and filed followup bugs 285164 and 285165.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
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

13 years ago
(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?
Please don't quote comments in their entirety?

The issue Martijn mentioned will likely get addressed in bug 283385...
You need to log in before you can comment on or make changes to this bug.