309550 - clear inside fieldset doesn't work

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect)

Description

[Susanne Jaeger]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b4) Gecko/20050913 SeaMonkey/1.0a Mnenhy/0.7.2.10005
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b4) Gecko/20050913 SeaMonkey/1.0a Mnenhy/0.7.2.10005

Inside a fieldset CSS-property clear doesn't work as it should.
------ HTML ----------
<p>paragraph before clear</p>
<hr>
<p>paragraph after clear</p>
--------- CSS ---------
p {width: 30em; float: left; background: #fec; }
hr {clear: left; }

inside a fieldset the <hr> isn't cleared, in a div it is.


Reproducible: Always

Actual Results:  
inside the fieldset <hr> is displayed to the right of the floated paragraph 

Expected Results:  
<hr> should be cleared and displayed at the bottom of the first paragraph. 

Going back I found this working in Mozilla 1.8a5 2004-11-13 but broken in 1.8a6
2004-12-15

Comment 1

[Susanne Jaeger]

Attachment: testcase

Same content in fieldset and in div. Clearing works in the second (div) block
and doesn't in the first (fieldset).

Comment 2

[Martijn Wargers (dead)]

This regressed between 2004-11-24 and 2004-11-25:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-11-24+09%3A00%3A00&maxdate=2004-11-25+12%3A00%3A00&cvsroot=%2Fcvsroot
Bug 209694 seems to me the most likely cause for this changing behavior.

Comment 3

[dolphinling]

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

Comment 4

[Martijn Wargers (dead)]

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

Comment 5

[Steve England [:stevee]]

Optimistically requesting blocking1.8rc2 as this is one of the few rendering regressions (that i'm aware of anyway) that would be great if got fixed.

Comment 6

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

In our code, fieldsets establish a new block formatting context, and it looks like they have a space manager.  Perhaps there's code somewhere that assumes frames with a space manager are blocks?

Comment 7

[Mike Schroepfer]

Even though it is a regression, the impact is minor we are very close to lockdown for rc2 so it looks like it is not going to make it.  

Comment 8

[Martijn Wargers (dead)]

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

Comment 9

[Tonico Strasser]

This bug brakes tableless form layouts that have worked before, this is very bad, at least for me.

Comment 10

[FataL]

Attachment: Example of the tableless CSS form layout with was broked by this bug

Comment 11

[FataL]

(In reply to comment #7)
> Even though it is a regression, the impact is minor we are very close to
> lockdown for rc2 so it looks like it is not going to make it.  
Nasty bug! Broked couple pages with forms that previously worked fine in every browser. Big mistake was release browser with this regression - web designers work very hard especially on tableless form layouts... :(
Any suggestions how to fix it without removing FIELDSET element from source?

Comment 12

[Susanne Jaeger]

(In reply to comment #11)
> Any suggestions how to fix it without removing FIELDSET element from source?

It's often possible to leave no space for floating by declaring width: 100%; for an element. In the testcase it helps to add 
fieldset hr {float: left; width: 100%; }
Nasty workaround. but stuff like that worked for me in some cases. 

I'm not sure about your example, don't know what is the intended view. 

Comment 13

[Andrew Schultz]

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

Comment 14

[Peter]

Attachment: Floated label not cleared by inner fieldset - tableless layout

Here is another case:

1. label elements are floated left with display: block.
2. input elements are contained by the label elements and also have display: block (forces the input to be below the label text)
3. fieldset is set to clear: left

Attaching another test case that shows this example.

Comment 15

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

Attachment: fix

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.

Comment 16

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

Because the child block has its own space manager, nsFieldSetFrame's space manager is only used for floats inside a legend. I think legend frames would be better off having their own space manager, and then we can remove the space manager from nsFieldSetFrame. I filed bug 323946 for this.

Comment 17

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

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

If you want to add an assertion to nsBlockFrame::Init or something that it never has NS_BLOCK_SPACE_MGR without NS_BLOCK_MARGIN_ROOT, go ahead.  (That's right, isn't it?)

Comment 18

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

That would currently trigger assertions; some form controls create frames like that. That's probably a bug we should file separately.

Comment 19

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

checked into trunk.

Comment 20

[Peter]

I have stumbled upon a potential hack fix for this bug, which allows an item to clear floats within a fieldset.

There seems to be 2 requirements:
1. The item that clears the float must be a block item (for example, this could be a button given display: block;, or a <div>)
2. Within the fieldset, you must include another fieldset prior to the item that clears the floats.

I will include an attachment that displays this fix.

What does this mean?
The good news is that it's not uncommon to have nested fieldsets.  However, sometimes it doesn't make sense (especially for really short forms).  So we need a way to include an empty fieldset (it must also contain a legend to be valid).  If you try to include the fieldset but hide it by giving it display:none;, the bug does not get fixed.  But you can instead essentially hide the fieldset by setting the margin, border, padding, and height to zero, and giving the legend display:none;.  Thus your HTML output would look something like this:

<fieldset>
   <legend>Test</legend>
   <fieldset class="mozillaFloatBugFix"><legend></legend></fieldset>
   <label>Foo<input></label>
   <input class="submit" name="submit">
</fieldset>

My only concern with this approach is whether or not this psuedo-hidden fieldset will cause any confusion for screen readers or other non-visual UAs.  I would tend to think not, but if anyone has a way of testing this I'd like to know the results.

Attaching the sample hack.

Comment 21

[Peter]

Attachment: Contains hack to fix float clearing inside a fieldset

This includes examples that demonstrate how to fix the Mozilla float bug within a fieldset that prevents items from clearing the float.

Comment 22

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

That's not a fix, it's a workaround, and all adding comments like that does is make this bug report more confusing and therefore less likely to be accepted when evaluated for branch approval.

Comment 23

[Peter]

(In reply to comment #22)
> That's not a fix, it's a workaround, and all adding comments like that does is
> make this bug report more confusing and therefore less likely to be accepted
> when evaluated for branch approval.

Are you blind?  Did you not see "I have stumbled upon a potential hack fix..." and "Attaching the sample hack"?  I never said this was a real fix.  I said right off the bat that it was hack.  It is simply a way for those of us using tableless form layouts to work around this bug which took so long to fix.  

In addition, I explained that certain other form elements can affect this bug.  If you find it confusing, I'm sorry to hear that.  It was not overly complicated, and explained in a fairly detailed manner so as NOT to confuse anyone.  I'm sorry, but I'm not going to let your rather rude remarks sway me from posting something that could potentially help a lot of people work around this bug.

That said, it seems that it does not have to be a fieldset element that you hide.  In fact, any element seems to work.  For example, a span or a br given 0 height as described in my previous post will also let the item clear.

Comment 24

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

When someone has to evaluate hundreds of bug reports for branch approval, they get about 30 seconds per bug.  And off-topic comments can make things very confusing in a 30-second skim.

Comment 25

[Peter]

(In reply to comment #24)
> When someone has to evaluate hundreds of bug reports for branch approval, they
> get about 30 seconds per bug.  And off-topic comments can make things very
> confusing in a 30-second skim.
> 

My comment was not off topic... though yours clearly is.

Comment 26

[FataL]

So, there was Firefox 1.5.0.1, 1.5.0.2, Firefox 2.0 Alpha...
Fix not included anywhere and my pages with forms still broken in FF > 1.0

Comment 27

[Peter]

Yep.  Odd indeed.  Is there a different Bugzilla database for Firefox somewhere that I don't know about?

Comment 28

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

Releases are built from branches of the source code that are stabilized.  This has been checked in on the trunk, but not on the branch from which Firefox 1.5 and 2.0 are built.

Comment 29

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

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

Comment 30

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

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

roc, feel free to check this in to 1.8.1 as well

Comment 31

[Mike McNally]

(Sorry - I previously added this comment to a dup)

This bug has no target. In that it's a regression from the 1.0.x version of the
product, is it going to make it into a 1.5.0.x release? It seems like a pretty
nasty problem that messes up a lot of formerly-working page layouts.

Comment 32

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

checked into 1.8.1. This should be fixed in the next Firefox 2.0 alpha/beta release.

I'm ambivalent about whether this should go into 1.8.0.x...