From:  bugzilla-request-daemon@mozilla.org
Date:  19 Jan 2006 07:20:49 Hong Kong Time
Newsgroup:  news.mozilla.org/netscape.public.mozilla.reviewers
Subject:  

superreview requested: [Bug 309550] clear inside fieldset doesn't work : [Attachment 208910] fix

NNTP-Posting-Host:  207.126.111.202

Robert O'Callahan (Novell)  has asked David Baron
 for superreview:
Bug 309550: clear inside fieldset doesn't work
https://bugzilla.mozilla.org/show_bug.cgi?id=309550

Attachment 208910: fix
https://bugzilla.mozilla.org/attachment.cgi?id=208910&action=edit

------- Additional Comments from Robert O'Callahan (Novell) 
The problem is not actually to do with fieldset having a space manager. Turns
out, that's irrelevant because its child block frame also has a space manager
and that takes over.

The problem is that the child block frame has a space manager yet is not a
margin-root. When we reflow the BODY containing the FIELDSET, we call
nsBlockReflowContext::ComputeCollapsedTopMargin on the fieldset, and if
fieldset was a regular block we'd drill down to see the HR with clearance and
set up state for an optimistic reflow to test whether clearance is needed. But
ComputeCollapsedTopMargin bails out when it sees a non-block frame and doesn't
set up the optimistic reflow. When we reflow the fieldset's child block and it
gets to the HR frame, we see that the HR's top margin is collapsed with the
child block's top margin, and assume that clearance is being tested/controlled
by whoever called ComputeCollapsedTopMargin, and don't apply clearance
ourselves. In this case that assumption is wrong.

This fix makes the fieldset's anonymous child block a margin root. This is
correct ... I don't think margins should be collapsing through fieldsets. They
only do so because the fieldset border is set on the fieldset frame and the
child block doesn't see any border style on itself.

In general it doesn't seem correct to allow a frame to have a space manager
(i.e. be a block formatting context) but also allow margin collapsing through
it. It's going to interfere with float/clearance/margin-collapse interaction in
ways that likely violate standards. I made an exception for this to allow
columns to collapse top and bottom margins, even though they contain their own
floats ... I think that's OK because that really assumes special wording will
be added to the columns spec.

The fix is dead simple and is very low risk from a technical point of view. It
might conceivably break layouts where someone is relying on the bug of margin
collapsing happening through a fieldset. But assuming IE doesn't allow that,
that seems unlikely.