Closed Bug 209694 Opened 20 years ago Closed 19 years ago

[MARGIN-C]clear margin merged with bottom margin on empty last child

Categories

(Core :: Layout: Block and Inline, defect, P1)

Product:
Core
Component:
Layout: Block and Inline
Platform:
x86
All
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: forum, Assigned: roc)

References

(
URL
)

Details

(Keywords: testcase, Whiteboard: (py8ieh: take testcase and make a real one))

Attachments

(5 files, 3 obsolete files)

Testcase
563 bytes, text/html
Details
Testcase #2
1.32 KB, text/html
Details
testcase showing related problem
172 bytes, text/html
Details
diff -w
86.48 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
better testcase
2.51 KB, text/html
Details 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5a) Gecko/20030531
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5a) Gecko/20030531

http://www.marilouonline.com/videos.html

there should be a margin between the 2 <div class="videoPanel">. Displayed
correctly in IE and Opera

Reproducible: Always

Steps to Reproduce:
1.Open the link
2.Check for a margin between the 2 div's forementionned


Actual Results:  
there's no margin at all between the 2 divs

Expected Results:  
check it out in IE or Opera for expected results
Attached file Testcase — 

    
    

    
  
Confirming bug, 2003-06-12-22 trunk Linux.
Assignee: general → float
Status: UNCONFIRMED → NEW
Component: Browser-General → Layout: Floats
Ever confirmed: true
Keywords: testcase
OS: Windows XP → All
QA Contact: general → ian

    
    

    
  
We're probably collapsing the margin out the top.
Assignee: float → block-and-inline
Component: Layout: Floats → Layout: Block & Inline
Summary: margin-bottom is being ignored if no padding is declared → [MARGIN-C]margin-bottom is being ignored if no padding is declared

    
  
Summary: [MARGIN-C]margin-bottom is being ignored if no padding is declared → [MARGIN-C]margin-bottom ignored when last child empty and cleared

    
    

    
  

      
      
      
      

        Attached file
          
        Testcase #2
        — 
      

    


  
I don't think it's a margin collapsing problem.  The problem is that an
element that has "clear:both" does not have any effect if its height is zero
and there is no other content following it.  Even though it's height is zero
it should still be positioned below the float and thus make the parent
container
high enough to "wrap" the floater. (It works for <br clear=all>)

This testcase is a bit clearer I hope.

    
    

    
  
The behavior here is actually arguably correct per CSS2, since clear works by
increasing margin, and yet the margins in question are adjacent and should thus
collapse.  That said, I'm not sure it's the only possible interpretation, and
I'm not sure if it's a good thing even if it is.
Summary: [MARGIN-C]margin-bottom ignored when last child empty and cleared → [MARGIN-C]clear margin merged with bottom margin on empty last child
Whiteboard: [csswg]

    
    

    
  
(And, FWIW, my comment 3 didn't make any sense at all, since there was non-empty
content.)

    
    

    
  
(This bug is really a case where the requirements of the CSS spec are impossible
to meet simultaneously.)

    
    

    
  
Actually, I realized that the CSS spec isn't contradictory.  I'd always
translated the section in 10.6.3 which says:

  If it has block-level children, the height is the distance between the top
  border-edge of the topmost block-level child box and the bottom border-edge
  of the bottommost block-level child box. [...]

  [ http://www.w3.org/TR/2003/WD-CSS21-20030128/visudet.html#q19 ]

into the concept that child/parent collapsed margins always end up outside the
parent.  However, this need not be the case when margins collapse through an
empty block, so the spec isn't contradictory, and this testcase should do what
you think it ought to do.

    
    

    
  
(And, combining that with a new sentence likely to be in CSS 2.1 would actually
make bug 44242 invalid.)
Priority: -- → P1

    
    

    
  
This is IMHO a dupe of bug 45830.
Blocks: 224057

    
    

    
  
*** Bug 226487 has been marked as a duplicate of this bug. ***
Blocks: 204831

    
    

    
  
We need to fix this because an empty DIV with a 'clear:both' is the easiest way
to get a float container to grow to the size of the float, which is what IE does
by default, so this bug blocks people modifying their layouts to be
standards-compliant.
Assignee: core.layout.block-and-inline → roc

    
    

    
  
So given the markup:

<div style="background-color:yellow;">
  <img style="float:left" src="http://mozilla.org/images/hack.gif">
  <div style="clear:both"></div>
</div>

the yellow DIV gets zero height because the delta-Y induced by the clear becomes
part of the margin, which collapses all the way through the content. So if
that's wrong, what's supposed to happen here?

    
    

    
  
See: http://www.w3.org/TR/CSS21/box.html#collapsing-margins

The margin of the yellow box can't collapse with the margin of the empty DIV 
because "The top margin of an in-flow block-level element is adjoining to its 
first in-flow block-level child's top margin if the element has no top border, 
no top padding, *and the child has no clearance*".

This then doesn't collapse with the bottom margin of the yellow div because 
"When an element's own margins collapse, and that element has had clearance 
applied to it, its top margin collapses with the adjoining margins of subsequent 
siblings but that resulting margin does not collapse with the bottom margin of 
the parent block".

So the yellow box in this example is exactly the height of the float, and has 
zero margins before and after.


Please check the CSS2.1 rules, they have been significantly updated and 
clarified since CSS2 and are now pretty explicit.

    
    

    
  
Does "has clearance" mean that the space required by the 'clear' property is
non-zero?

If so, then what about the following testcase:

<div style="float:left; height:100px;"></div>
<div id='parent' style="margin-top:50px;">
  <div id='child' style="margin-top:80px; clear:both;"></div>
</div>

What should the result be? It seems to me that the logical result would put
child's top adjacent to the bottom of the float. But that would require 'partial
collapse'. If we're restricted to the margins either collapsing or not, then I
don't see a solution consistent with the CSS 2.1 rules.

    
    

    
  
See: http://www.w3.org/TR/CSS21/visuren.html#clearance

Computing the clearance of an element on which 'clear' is set is done by first 
determining the hypothetical position of the element's top border edge within 
its parent block. This position is determined after the top margin of the 
element has been collapsed with previous adjacent margins (including the top 
margin of the parent block). 

If the element's top border edge has not passed the relevant floats, then its 
clearance is set to the amount necessary to place the border edge of the block 
even with the bottom outer edge of the lowest float that must be cleared.


| <div style="float:left; height:100px;"></div>
| <div id='parent' style="margin-top:50px;">
|   <div id='child' style="margin-top:80px; clear:both;"></div>
| </div>

What this looks like depends on the parent element (and in particular, on 
whether it is the root element or not). Assuming the markup above is contained 
in a normal in-flow block-level element X with a non-zero border and zero 
padding, then the rendering would be determined as follows:

   At the top left of the X box's borders, a zero width float of height 100px is 
   placed.

   The two next elements ('parent' and 'child') are then collapsed together to 
   find the hypothetical position of 'child' to see if it needs to be cleared.
   The hypothetical position is determined as being 80px from the inner top of 
   X. (This is the collapsing of 50px, 80px, 0px, and 0px, the four 
   hypothetically adjacent margins at that point.)

   The hypothetical top border edge position hasn't cleared the float in this 
   example, so clearance is introduced. Do the collapsing with that in mind.
   The 'parent' box is not adjacent to the 'child' box's top margin, since it 
   has clearance. So its top border edge is 50px from the top of X. The 'child'
   box is next. Its top margin collapses with its bottom margin and the parent's
   bottom margin, but nothing else. Its clearance is 50px (the distance from the 
   parent's top border edge) - 80px (the resulting collapsed top margin of the 
   'child' box). The clearance is thus -30px. The clearance conceptually goes 
   before the top margin of the element, and is then followed by the margin
   (which is 80px). The top border edge of the 'child' box is thus -30px+80px 
   from the 'parent' box's top border edge, which places it exactly 100px from 
   the top of X (which is exactly where it was supposed to be -- 'clear' means
   "but the top border edge even with the bottom of the float").

   The bottom margin of the 'parent' box is part of the collapsing above, and
   thus the bottom of the 'parent' box is even with the 'child' box.

This is, at least, my reading (and for that matter, writing) of the spec. Is 
there something in that which seems inconsistent? There shouldn't be... :-)

    
    

    
  
I get it. I hadn't considered *negative* clearance values. It would be nice if
the spec mentioned this possibility in the paragraph "If the element's top
border edge has not passed the relevant floats, then its clearance is set to the
amount necessary to place the border edge of the block even with the bottom
outer edge of the lowest float that must be cleared."

I hope you realize that this is phenomenally complex, even for authors, and even
more so for implementors.

    
    

    
  
(In reply to comment #17)
> I get it. I hadn't considered *negative* clearance values.

The definition says "Clearance is introduced as spacing above the margin-top of 
an element. It is used to push the element vertically (typically downward), past 
the float." -- note the "typically downward" bit. That was the result of an 
argument in the working group regarding whether or not to explicitly mention 
that it could be negative, if I recall correctly.


> I hope you realize that this is phenomenally complex, even for authors, and 
> even more so for implementors.

Yes, it is. The working group attempted to make CSS2.1 a lot clearer than CSS2 
and CSS1 were on this issue. Unfortunately the basic model was pretty well 
established and all we could do is explain how on earth it was actually supposed 
to work. If I could go back to 1995 and help the development of CSS, I would, 
but unfortunately it's mostly too late now. :-(

(Havint said that, I _can_ help reduce the complexity of new specs I'm involved 
with, especially those of www.whatwg.org. If you see anything in those specs 
that is too complex for your liking, let us know whatwg@whatwg.org and I'll do 
my best to make it easier.)

    
    

    
  
I remember reading that text and not being able to imagine how 'clear' could
push the element "up past the float", and thus ignoring it.

I'll have to think about how this is going to work. I suspect we're going to
have to move the float clearing logic from nsBlockFrame::ReflowBlockFrame into
nsBlockReflowContext::ComputeCollapsedTopMargin. ComputeCollapsedTopMargin will
have to follow the leading blocks down the frame tree, computing the clearance
(if any) of each block encountered, and then recalculate the collapsed margin if
we encountered a block with clearance. We'll need to store state in the lines,
probably an mHasClearance bit, to record that a block had clearance and it will
have to apply its own top margin and clearance.

    
    

    
  
Can you clarify exactly what "its top margin collapses with the adjoining
margins of subsequent siblings but that resulting margin does not collapse with
the bottom margin of the parent block" means?

<div>
  <div style="clear:both; margin-top:100px"></div>


    
    

    
  
Can you clarify exactly what "its top margin collapses with the adjoining
margins of subsequent siblings but that resulting margin does not collapse with
the bottom margin of the parent block" means?

<div id="float" style="float:left; height:100px"></div>
<div id="outer" style="margin-bottom:100px;">
  <div id="clear" style="clear:both; margin-top:100px"></div>
  <div id="sib" style="margin-bottom:100px;"></div>
</div>
<div id="end" style="border:1px solid red"></div>

My impression is that there will be a 200px margin between the bottom of "float"
and the border-box of "end", because the margin-top of "clear" collapses with
the margin-bottom of "sib" but not the margin-bottom of "outer".

But what if we change the border-bottom of "sib" to 150px?

<div id="float" style="float:left; height:100px"></div>
<div id="outer" style="margin-bottom:100px;">
  <div id="clear" style="clear:both; margin-top:100px"></div>
  <div id="sib" style="margin-bottom:150px;"></div>
</div>
<div id="end" style="border:1px solid red"></div>

Is it true that we now have a 250px margin between the bottom of "float" and the
border-box of "end" because exactly the same reasoning applies as in the first case?

Then what about this case?

<div id="float" style="float:left; height:100px"></div>
<div id="outer" style="margin-bottom:100px;">
  <div id="clear" style="clear:both; margin-top:0.0001px"></div>
  <div id="sib" style="margin-bottom:100px;"></div>
</div>
<div id="end" style="border:1px solid red"></div>

Do we still see a 200.0001px margin? It seems strange for the existence of a
nonzero margin-top on "clear" to 'poison' the collapsing margin and prevent a
potentially distant sibling from collapsing its bottom margin with its parent.

    
    

    
  
Also, what if "clear"'s margin-top is zero? Can it still be said to collapse
with adjoining margins and thus prevent later siblings from collapsing with the
parent? The testcase would be

<div id="float" style="float:left; height:100px"></div>
<div id="outer" style="margin-bottom:100px;">
  <div id="clear" style="clear:both;"></div>
  <div id="sib" style="margin-bottom:100px;"></div>
</div>
<div id="end" style="border:1px solid red"></div>

Is the margin now only 100px or still 200px between between the bottom of
"float" and the border-box of "end"?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        work in progress (obsolete)
        — 
          — Splinter Review
      

    


  
I'm quite certain this doesn't work, because I've only just finished typing it
in, but it shows the approach I'm trying to take.

    
    

    
  
(BTW, I can attempt to understand what the spec says about clearance, if you
need any help, but I really have no clue from memory, because it's been through
so many iterations and proposals.  We spent a good part of the evening on the
last day of the Oslo meeting last year ironing out all the messy issues, during
which time I repeatedly regretted giving in (on the second day of the meeting)
on my alternative (simpler and more consistent with prior specs, but somewhat
non-intuitive) proposal for how the interaction of clearing and margin
collapsing should work.  Not that I remember what that was either...)

    
    

    
  
Ian's doing a great job so far :-)

    
    

    
  
> Can you clarify exactly what "its top margin collapses with the
> adjoining margins of subsequent siblings but that resulting margin
> does not collapse with the bottom margin of the parent block" means?

The definition of "adjoining margins" is given in 8.3.1 after the
bulleted list (each case that could be considered adjoining is listed,
no other cases are adjoining).

To answer your question with reference to your example:

> <div id="float" style="float:left; height:100px"></div>
> <div id="outer" style="margin-bottom:100px;">
>   <div id="clear" style="clear:both; margin-top:100px"></div>
>   <div id="sib" style="margin-bottom:100px;"></div>
> </div>
> <div id="end" style="border:1px solid red"></div>

(Again assuming a box X around the whole thing that is not the root
element and that has a solid border and zero padding.)

The float is at the top left of X's content area. The 'clear' box is
self-collapsing and so we use the special rules for finding the top of
self-collapsing blocks (that's the bullet point starting with "If the
top and bottom margins of a box are adjoining, then..."). Per these
rules, we collapse the top margin of the 'outer' block (0) and the top
margin of the 'clear' block (100px) together to find the top border
edge of the 'clear' block. (Remember, this is still just hypothetical,
we're trying to work out whether the 'clear' box needs any clearance.) 
This tells us that the hypothetical top of the 'clear' box is 100px
down from the top of X's content area, which means it is past the
float and that we don't need any clearance, so the 'clear' box has no
clearance and six margins are adjacent (0, 100px, 0, 0, 100px, 100px
respectively). Thus the total distance from the top of 'outer' to the
bottom of 'outer' is 100px and the red line is immediately below the
float, and the height of 'outer' is zero. (This matters -- it means if
it had any background, it wouldn't be visible.) The height of X in
this example is thus 101px (100px + 1px for the red border)

Read that again until you understand it because if it doesn't make any
sense the next bit will just make matters worse. Once you're sure you
understand it:

Note that if we had the _exact_ same markup but changed the margin-top
of the 'clear' box to one less pixels -- 99px -- that the result would
be quite different: The hypothetical position of the box would now not
be enough to push the top border edge of the 'clear' box past the
float (collapse 0 and 99px and you get 99px, less than the float's
100px) and so the box would need clearance, in this case of exactly
1px. If the 'clear' box has clearance, that means it doesn't collapse
with the 'outer' box, whose top border edge would therefore be at the
top of X. Thus the top border edge of the self-collapsing 'clear' box
would be defined as being 1px+99px (clearance and top margin
respectively) below the top of the parent 'outer' box and thus 100px
below the top of X. This position would be relevant if 'clear'
contained any floats of its own, or was a containing block for any
positioned elements.

Then, the margins of the self-collapsing box and its siblings would
collapse together -- 99px, 0, 0, 100px, collapsing to 100px (note that
the margins collapse straight through the top border edge of
self-collapsing blocks -- their top border edge position is defined as
not taking into account later margins, but doesn't stop the adjacent
margins from collapsing anyway), meaning that the bottom of 'outer' is
1px+100px (that pesky clearance, plus the collapsed margin) below the
top of 'outer'. So the height of 'outer' is 101px. A decrease in the
margin of the 'clear' box by 1px increased the height of the 'outer'
box from 0 to 101px, making its background visible in the process.

The bottom margin of 'outer' is then added on top of that -- it
doesn't collapse with the margins that collapsed with the
self-collapsing block that had clearance -- and thus the red line is
201px below the top of X, and 101px below the float. The height of X
in this example would thus be 202px (201px + 1px for the red border).

This discontinuity is visible in this example which uses similar
(although not identical) markup:

   http://www.hixie.ch/tests/adhoc/css/box/float/clear/004-demo.html

...if you implemented it right.


> <div id="float" style="float:left; height:100px"></div>
> <div id="outer" style="margin-bottom:100px;">
>   <div id="clear" style="clear:both; margin-top:100px"></div>
>   <div id="sib" style="margin-bottom:150px;"></div>
> </div>
> <div id="end" style="border:1px solid red"></div>

(Still assuming the wrapping X block.)

Here the 'clear' box still gets past the float on the strength of its
own margin and so the initial description above still applies, except
that the resulting collapsed margin is 150px instead of 100px. All the
margins still collapse fine, because there is no clearance in this
example.

If 'clear' had only had 99px of top margin, then it would have been
similar to the second example above, with the resulting distance
between the top of X and the red line being 251px (1px clearance,
150px margin collapsed around the 'clear' and 'sib' boxes, and 100px
bottom margin from 'outer'), and thus 151px from the bottom of the
float.


> <div id="float" style="float:left; height:100px"></div>
> <div id="outer" style="margin-bottom:100px;">
>   <div id="clear" style="clear:both; margin-top:0.0001px"></div>
>   <div id="sib" style="margin-bottom:100px;"></div>
> </div>
> <div id="end" style="border:1px solid red"></div>

It doesn't make any difference if the margin is 0.0001px or 0, 0
margins still fully take part in the collapsing margin model.

In this example, the hypothetical top border edge of the 'clear' block
would be 0.0001px from the top of the ever-assumed wrapper X, which if
clearly not enough to clear the float, and thus clearance of 99.9999px
is introduced. The margins of 'clear' and 'sib' then collapse (to
100px), and are added after the clearance, meaning that 'outer' has
height 199.9999px, and the distance from the bottom of the float to
the red line is 199.9999px (distance from top of X to the bottom of
'outer') + 100px (bottom margin of 'outer') - 100px (height of
'float') which is 199.9999px.



> ... prevent a potentially distant sibling from collapsing its bottom
> margin with its parent.

The sibling can't be "distant" if it is collapsing its margins with
the 'clear' block. Margins only collapse when they are adjacent.


> Also, what if "clear"'s margin-top is zero? Can it still be said to
> collapse with adjoining margins and thus prevent later siblings from
> collapsing with the parent?

Yes, zero margins take part in collapsing just like any others. The
key here is not whether there is a margin, but whether the float's
hypothetical (i.e., pre-clearance) top border edge position is beyond
the float or not (where "beyond" is defined as "").


> The testcase would be
> 
> <div id="float" style="float:left; height:100px"></div>
> <div id="outer" style="margin-bottom:100px;">
>   <div id="clear" style="clear:both;"></div>
>   <div id="sib" style="margin-bottom:100px;"></div>
> </div>
> <div id="end" style="border:1px solid red"></div>

Clearance here is 100px, height of 'outer' is 200px starting at the
top of X, height of X is 301px (including the 1px red line).


There are a number of margin collapsing testcases (although few that
involve 'clear', sadly) on my site, at:

   http://www.hixie.ch/tests/adhoc/css/box/block/margin-collapse/

    
    

    
  
> (where "beyond" is defined as "")

Oops, forgot to paste the definition. I meant to say: a block has cleared its
floats if its top border edge is below the bottom of the floats. So if you have
a one pixel float, and the top of the box with 'clear' is on the second vertical
pixel, it has cleared the float:

    _____________________
   [FLOAT]   _________   '
   |        | CLEARED |
   |        |         '
   '

    
    

    
  
Thanks. My examples weren't quite what I wanted because they didn't have
clearance, but you guessed what I meant and it's all pretty clear now. Thanks.

    
    

    
  
Well, one more question. When computing the clearance we're to assume that the
cleared block's top margin collapses with the margin of its parent or previous
sibling. What if the cleared block's first child has a top margin, should we
collapse with that too when computing clearance? Consider, under the usual
assumptions:

<div id="float" style="float:left; height:100px"></div>
<div id="clear" style="clear:both;">
  <div id="child" style="margin-top:200px;"></div>
</div>
<div id="end" style="border:1px solid red"></div>

Does "clear" have clearance or not? The current patch assumes that it does,
because we don't consider the child's top margin when computing the clearance,
but reading the spec I'm beginning to think this is incorrect.

A gigantic problem with the current patch is that ComputeCollapsedTopMargin
isn't considering the floats whose placeholders occur between the start of the
top-margin-root block and the descendant with clearance when it calculates
whether clearance is needed. But hacking ComputeCollapsedTopMargin to reflow and
place those floats would be a lot of code and really slow, not to mention that
we'd have to push and and pop state since this would only be a speculative
reflow. However, if we must compute the correct top margin value before
reflowing the top-margin-root block, then I see no other choice. It might be
possible to move away from this design but I don't yet see what an alternative
would look like.

I realize that I let something go past that I should have nailed down, in your
comment #16. You appear to interpret this text, "If the element's top border
edge has not passed the relevant floats, then its clearance is set to the amount
necessary to place the border edge of the block even with the bottom outer edge
of the lowest float that must be cleared.", to mean that that this computation
takes place assuming a layout where the element's top margin DID NOT collapse
with its parent's. So the hypothetical position is used only to determine
*whether* there is clearance, but not the value of the clearance. Right?

Then I have another question about an example, to test if my understanding is
correct.

<div id="outer" style="border:0.000001px solid red; float:left">
  <div id="interior" style="margin-top:100px;">
    <div id="float" style="float:left; height:100px"></div>
    <div id="clear" style="clear:left; margin-top:150px;"></div>
  </div>
</div>

We start by assuming that the top margins of "clear" and "interior" collapse.
The top border-edges of "interior", "float" and "clear" would then be 150px
below the top of "outer". So "clear" has clearance. Now we assume that the top
margins of "clear" and "interior" do not collapse. The top edges of "interior"
and "float" are now 100px below the top of "outer", and the top edge of "clear"
is 200px below the top of "outer". Is that correct?

If so, then this example confirms that we need to reflow floats at different
places for each element whose clearance we're testing. Think about this example
where the floats get laid out quite differently in the hypothetical vs the
actual cases.

<div id="outer" style="border:0.000001px solid red; float:left; width:1000px">
  <div id="right" style="float:right; width:900px; height:150px;"></div>
  <div id="interior" style="margin-top:100px;">
    <div id="float1" style="float:left; height:100px; width:100px;"></div>
    <div id="float2" style="float:left; height:100px; width:100px;"></div>
    <div id="clear" style="clear:left; margin-top:150px;"></div>
  </div>
</div>

Hmm, I wonder if a good way to implement all this would be to use our
ComputeCollapsedTopMargin approach and just reflow everything assuming that no
blocks have clearance, and then when we find that a block needs clearance we
back out to the nearest block whose margin collapse was affected and re-reflow
it. That approach might not actually be too bad in performance and complexity.

    
    

    
  
> Well, one more question. When computing the clearance we're to assume that the
> cleared block's top margin collapses with the margin of its parent or previous
> sibling. What if the cleared block's first child has a top margin, should we
> collapse with that too when computing clearance?

Yes. In the hypothetical case, the element (and any subsequent elements) don't
have clearance, and so margins collapse well. (Some of the previous siblings
might have clearance already; those, per rules in 8.3.1, have various
restrictions on where they collapse.)


> <div id="float" style="float:left; height:100px"></div>
> <div id="clear" style="clear:both;">
>   <div id="child" style="margin-top:200px;"></div>
> </div>
> <div id="end" style="border:1px solid red"></div>
> 
> Does "clear" have clearance or not?

The hypothetical case collapses all the margins togother and finds that 'clear'
has a top margin of 200px. This is enough to clear the float so there is no
clearance.



> So the hypothetical position is used only to determine *whether* there is
> clearance, but not the value of the clearance. Right?

That is correct. The hypothetical position decides whether there is clearance.
If there is, then that affects the rules in 8.3.1 -- so when you do the "real"
collapsing, different things actually collapse. It's when you're doing the
"real" collapse that you find the position of the 'clear' block's top border edge.


> <div id="outer" style="border:0.000001px solid red; float:left">
>   <div id="interior" style="margin-top:100px;">
>     <div id="float" style="float:left; height:100px"></div>
>     <div id="clear" style="clear:left; margin-top:150px;"></div>
>   </div>
> </div>

(Note that the outer element being a float doesn't affect clear or inner floats
at all, it just means that it'll shrink wrap to contain everything, including
descendent floats. Just like a root element would, in fact.)

Here the float's position is between two margins that might collapse, so we
invoke section 9.5.1 rule 4 sentence 2: "When the float occurs between two
collapsing margins, the float is positioned as if it had an otherwise empty
anonymous block parent taking part in the flow. The position of such a parent is
defined by the rules in the section on margin collapsing."

Thus the markup is now equivalent to:

 <div id="outer" style="border:0.000001px solid red; float:left">
   <div id="interior" style="margin-top:100px;">
     <div id="anonymous">
      <div id="float" style="float:left; height:100px"></div>
     </div>
     <div id="clear" style="clear:left; margin-top:150px;"></div>
   </div>
 </div>

The 'anonymous' block is self-collapsing, so we invoke 8.3.1, 6th bullet point,
first bullet point: "If the element's margins are collapsed with its parent's
top margin, the top border edge of the box is defined to be the same as the
parent's."

Now we have to find the top border edge of 'interior', but that depends on the
collapsing of various margins including one that might be affected by whether
clearance exists. We therefore find the hypothetical position of 'clear' by
hypothetically collapsing the margins at that point -- 100px, (0, 0), 150px, 0,
0, respectively -- and that gives us a top margin of 150px on the 'interior'
div, which puts the float at the same level as the 'clear' block, which means
the 'clear' block hasn't cleared the floats it needs to clear. We therefore now
assume 'clear' has clearance.

Knowing that 'clear' has clearance, we can do the real collapsing. We collapse
the adjacent margins -- just 100px and the two anonymous 0, 0 margins -- and so
find that there is a 100px margin at the top of 'interior'. This puts the float
at the top of 'interior', according to the two rules quoted above, 100px below
the outer red border. Next we position 'clear', which means discovering its
clearance. We need to place that element so that its top border edge is touching
the bottom border edge of the float. That's easy enough: 100px distance from the
bottom of the last in-flow box to the bottom of the floats we have to clear, -
150px of our margin (that's 150px collapsed with 0 and 0 bottom margins) in that
area already covering the float, = -50px clearance.

Thus the distance from inner top to inner bottom of the 'outer' block is 200px,
the last 100px of which is the float and 'interior' blocks.


> We start by assuming that the top margins of "clear" and "interior" collapse.
> The top border-edges of "interior", "float" and "clear" would then be 150px
> below the top of "outer". So "clear" has clearance. Now we assume that the top
> margins of "clear" and "interior" do not collapse. The top edges of "interior"
> and "float" are now 100px below the top of "outer", and the top edge of "clear"
> is 200px below the top of "outer". Is that correct?

Yes.


> <div id="outer" style="border:0.000001px solid red; float:left; width:1000px">
>   <div id="right" style="float:right; width:900px; height:150px;"></div>
>   <div id="interior" style="margin-top:100px;">
>     <div id="float1" style="float:left; height:100px; width:100px;"></div>
>     <div id="float2" style="float:left; height:100px; width:100px;"></div>
>     <div id="clear" style="clear:left; margin-top:150px;"></div>
>   </div>
> </div>

(Note that here the 'right' float is _not_ between two collapsing margins, so it
doesn't get an anonymous div -- it's just placed at the top of the 'outer' div.)

This is quite a nice example. The two renderings aren't actually that different;
the second float, for instance, has the same position in the real rendering as
in the hypothetical layout used to find if 'clear' has clearance. But yes, this
can cascade to quite big differences.

The key here is that the moment you start collapsing margins, you find all the
involved margins, then size the floats that are introduced between the first and
last margins involved (float sizes cannot depend on position), then see if any
of the in-flow blocks might have clearance, and if they do, for each one in turn
use the hypothetical mechanism to work out if it should have clearance or not.
When one of them does, you lay out everything up to that point and start again
from there (since clearance is like padding -- it blocks collapsing). If none of
them do, then you just do the layout at that point.

There are probably other, more performant ways of doing this, but that's the one
that makes the most sense when calculating things by hand IMHO.

    
    

    
  
> the second float, for instance, has the same position in the real rendering as
> in the hypothetical layout used to find if 'clear' has clearance.

Is that true? My understanding was that in the hypothetical layout, "float1" and
"float2" would be placed next to each other with their top edges 150px below the
top of "outer". But in the actual layout, the margins don't collapse, "float1"'s
top edge is placed 100px below the top of "outer", there's no room to put
"float2" beside it because of the right float, and therefore "float2" goes below
it, with its top edge 200px below the top of "outer".

> (float sizes cannot depend on position)

Are you sure? In this testcase, if I'd omitted the width property on the left
floats, and given them some wrappable text content, I think float1's size could
have changed with position because (in Mozilla parlance) the available width
would have been different (1000px in the hypothetical layout, 100px in the
actual layout), and text would have wrapped differently.

I'm pretty sure that I understand how to do this now. Thanks very much for all
the help.

    
    

    
  
(In reply to comment #31)
> 
> Is that true? My understanding was that in the hypothetical layout, "float1" 
> and "float2" would be placed next to each other with their top edges 150px 
> below the top of "outer".

Right.


> But in the actual layout, the margins don't collapse, "float1"'s
> top edge is placed 100px below the top of "outer", there's no room to put
> "float2" beside it because of the right float, and therefore "float2" goes 
> below it, with its top edge 200px below the top of "outer".

No, there is room, since the right float ends half way down the first left
float. The layout would be:

 +-----------------------------------------+
 |      +--------------------------------+ |
 |      |                                | |
 |      |                                | |
 |      |                                | |
 | XXXX |                                | |
 | XXXX +--------------------------------+ |
 | XXXX YYYY                               |
 | XXXX YYYY                               |
 |      YYYY                               |
 '      ''''                               '

...etc. If I'm not mistaken.


> Are you sure? In this testcase, if I'd omitted the width property on the left
> floats, and given them some wrappable text content, I think float1's size 
> could have changed with position because (in Mozilla parlance) the available 
> width would have been different (1000px in the hypothetical layout, 100px in 
> the actual layout), and text would have wrapped differently.

Available width is only dependent on the containing block, which is defined by
the float's original position in the DOM, not by layout. See section 10.3.5.

    
    

    
  
I'm not sure about the meaning of hypothetical constructions like the one used here:

> Computing the clearance of an element on which 'clear' is set is done by first
> determining the hypothetical position of the element's top border edge within
> its parent block. This position is determined after the top margin of the
> element has been collapsed with previous adjacent margins (including the top
> margin of the parent block).

The word "after" implies that layout is computed in some order determined by the
spec, but that's not really so. You assume it's an outside-in model, and
therefore we detect clearance on parents before children, which is natural
enough, but as far as I know it's not part of the spec. So it seems unclear
exactly what constraints are to apply in the hypothetical world we construct to
detect clearance.

For example, consider this example:

<div id="outer" style="border:0.000001px solid red;">
  <div id="float" style="float:left; height:100px;"></div>
  <div id="clear" style="clear:left; margin-top:120px;">
    <div id="child" style="clear:left; margin-top:-50px;">Hello</div>
  </div>
</div>

Is there really anything in the spec which forces the conclusion that "clear"
has clearance and "child" does not, precluding the possibility that "child" has
clearance and "clear" does not?

    
    

    
  
CSS is always done in document order. I can't find anything in the spec that
says so, but it is. I'll raise it with the working group.

    
    

    
  
Hmm. Most of the CSS spec seems to describe constraints on a valid layout, not a
layout algorithm. (In some places it describes algorithms, but these are really
just functions used to describe complex, but local, constraints.) I'm not sure
that just saying "CSS follows document order" is meaningful.

What we have here is a situation where multiple layouts may satisfy the spec as
written, and we need to say which layout is preferred.

What we really need is a formalization of CSS using something like constraint
logic programming (Prolog with arithmetic constraints). It seems that
Prolog-style choice points and a suitable ordering on the rules would suffice to
select the preferred layout among many possibilities. I wish I had the time.

    
    

    
  
Implementing CSS in Prolog would just mean we had yet another buggy
implementation to worry about...

But if you want it in terms of constraints, how about: "The hypothetical
position of an element is calculated assuming that all later elements have no
clearance."

    
    

    
  
> Implementing CSS in Prolog would just mean we had yet another buggy
> implementation to worry about...

The point of formalizing specs is that at some point the formalism is designated
as normative. But don't worry, actually formalizing CSS would be a mighty
project that no-one will do in the forseeable future :-).

> But if you want it in terms of constraints, how about: "The hypothetical
> position of an element is calculated assuming that all later elements have no
> clearance."

That helps a lot. But is it obvious that we can talk about "an element" having
clearance? Might an element have multiple boxes some of which have clearance and
others not? I suppose clearance only applies to the first-in-flow. But then
there are some situations where elements can have multiple distinct
presentations; 'position:fixed' and table header/footer repetition, at least. I
guess in those cases all copies are guaranteed to have the same clearance. (I do
wonder how well the CSS spec language, and CSS itself, are going to hold up in
these situations...)

    
    

    
  
> But is it obvious that we can talk about "an element" having clearance?

'clear' applies to elements and pseudo-elements that are block-level. I guess at
the technical level one box of an element in paged media has clearance and the
others don't... but then we talk about the margins of an element too, so I don't
really see the problem there.


> Might an element have multiple boxes some of which have clearance and
> others not?

Only insofar as the an element has multiple boxes some of which have a top
margin and others don't.


> But then there are some situations where elements can have multiple distinct
> presentations; 'position:fixed' and table header/footer repetition, at least.

Good lord, 'clear' shouldn't apply to absolutely positioned elements. Let me
file another issue... (Table headers and footers aren't block-level and so
'clear' doesn't apply to them.)

    
    

    
  
I have coded up a patch for this. There's still a lot of work to do to get it
working properly.

    
    

    
  
The patch is starting to look good. I have to look at the incremental reflow
issues though

    
    

    
  
In fact my patch is good enough that I've burned a lot of time trying to figure
out why certain testcases don't work, and it turned out that the code was right
and I was wrong.

    
    

    
  
(In reply to comment #26)
> To answer your question with reference to your example:
> 
> > <div id="float" style="float:left; height:100px"></div>
> > <div id="outer" style="margin-bottom:100px;">
> >   <div id="clear" style="clear:both; margin-top:100px"></div>
> >   <div id="sib" style="margin-bottom:100px;"></div>
> > </div>
> > <div id="end" style="border:1px solid red"></div>
> 
> (Again assuming a box X around the whole thing that is not the root
> element and that has a solid border and zero padding.)
...
> Note that if we had the _exact_ same markup but changed the margin-top
> of the 'clear' box to one less pixels -- 99px -- that the result would
> be quite different: The hypothetical position of the box would now not
> be enough to push the top border edge of the 'clear' box past the
> float (collapse 0 and 99px and you get 99px, less than the float's
> 100px) and so the box would need clearance, in this case of exactly
> 1px. If the 'clear' box has clearance, that means it doesn't collapse
> with the 'outer' box, whose top border edge would therefore be at the
> top of X. Thus the top border edge of the self-collapsing 'clear' box
> would be defined as being 1px+99px (clearance and top margin
> respectively) below the top of the parent 'outer' box and thus 100px
> below the top of X. This position would be relevant if 'clear'
> contained any floats of its own, or was a containing block for any
> positioned elements.

I believe this was actually incorrect. Here's the adjusted markup:

<div id="float" style="float:left; height:100px"></div>
<div id="outer" style="margin-bottom:100px;">
  <div id="clear" style="clear:both; margin-top:99px"></div>
  <div id="sib" style="margin-bottom:100px;"></div>
</div>
<div id="end" style="border:1px solid red"></div>

In this case, when we try to determine clearance, the hypothetical box position
will still be 100px below the top, because the margin-bottom on 'sib' collapses
with the margins of 'clear'. So to get the example to show what we wanted it to
show, you also have to reduce the margin-bottom on 'sib' to 99px.

    
    

    
  
Regarding float sizes depending on position:  it can happen in quirks mode, I
think (and it's an ugly hack that prevents us from fixing some rather serious
real bugs).

    
    

    
  
> <div id="float" style="float:left; height:100px"></div>
> <div id="outer" style="margin-bottom:100px;">
>   <div id="clear" style="clear:both; margin-top:99px"></div>
>   <div id="sib" style="margin-bottom:100px;"></div>
> </div>
> <div id="end" style="border:1px solid red"></div>
> 
> In this case, when we try to determine clearance, the hypothetical box 
> position will still be 100px below the top, because the margin-bottom on 'sib'
> collapses with the margins of 'clear'.

Er, yes, indeed. In fact you'd have to change both of the margin-bottom:100px
cases to 99px because until there is clearance, they would all collapse together
and the total collapsed margin has to be less than 100px for clearance to be
needed. My bad.

If you're enjoying your time with floats, please feel free to take a stab at the
other float bugs we have filed, or just to go through these tests:
   http://www.hixie.ch/tests/adhoc/css/box/block/margin-collapse/

    
    

    
  
Urrrgh. So given

<div id="float" style="float:left; height:100px"></div>
<div id="outer" style="margin-bottom:90px;">
  <div id="clear" style="clear:both; margin-top:90px"></div>
  <div id="sib" style="margin-bottom:90px;"></div>
</div>
<div style="margin-top:90px"></div>
...
<div style="margin-top:90px"></div>
<div style="margin-top:100px"></div>
<div id="end" style="border:1px solid red"></div>

... we have to wait indefinitely long before we can decide whether "clear" has
clearance. That makes this considerably harder than I thought.

    
    

    
  
I guess we can compute the collapsed margin just by looking ahead at the
following elements, without actually reflowing them, so that won't be so bad.

I'm having trouble figuring out how to make this all work efficiently
incrementally though. It's going to be really tough to fix all this without
hurting Tp.

    
    

    
  


  
I believe the float should be pushed down below the margin, adjacent to the
text. Currently we don't do that.

Note that fixing this will be hard because of the following sort of situation:

<div id="float" style="float:left; height:100px"></div>
<div style="margin-top:90px"></div>
...
<div style="margin-top:90px;"></div>
<div style="margin-top:90px;">
  <div style="margin-top:100px; clear:both;"></div>
</div>

Once we discover that clearance is required, we have to roll back all the way
to where we put the float, and place it again, then fix up all the following
lines. Ugh.

    
    

    
  
I wonder if this means it would be a good idea to make floats be children of,
and reflowed by, the block with the space manager.

    
    

    
  
It's not indefinitely long, it's just until the end of the set of margins that
are collapsing with each other at that point.

    
    

    
  
(which is rarely many steps, since you get very few empty elements generally)

    
    

    
  
> Note that fixing this will be hard because of the following sort of situation:
> 
> <div id="float" style="float:left; height:100px"></div>
> <div style="margin-top:90px"></div>
> ...
> <div style="margin-top:90px;"></div>
> <div style="margin-top:90px;">
>   <div style="margin-top:100px; clear:both;"></div>
> </div>
> 
> Once we discover that clearance is required, we have to roll back all the way
> to where we put the float, and place it again, then fix up all the following
> lines. Ugh.

No, that float is not affected by later elements. A float between two adjacent
margins is placed as if it had an empty element around it, with the top of that
element defined as, _for the purposes of positioning floats_:

#    * If the element's margins are collapsed with its parent's top margin, the 
#      top border edge of the box is defined to be the same as the parent's.
#    * Otherwise, either the element's parent is not taking part in the margin 
#      collapsing, or only the parent's bottom margin is involved. The position 
#      of the element's top border edge is the same as it would have been if the 
#      element had a non-zero top border.

Note how later margins never affect that.

    
    

    
  
(In reply to comment #49)
> I wonder if this means it would be a good idea to make floats be children of,
> and reflowed by, the block with the space manager.

I've thought for a while that that might be better, although there are a few
tricky issues.

    
    

    
  
> The position of the element's top border edge is the same as it would have been
> if the element had a non-zero top border.

OK, so our current rendering is actually correct. Thanks for reminding me about
this. I'm finding it hard to keep it all straight in my head.

    
    

    
  

      
      
      
      

        Attached file
          
        testcase (obsolete)
        — 
      

    


  
Here are the testcase summarizing most of the issues raised in this bug. My
current patch passes all of them.

    
    

    
  
1.8a4 passes none of them! except for the first one. Which is strange since
that's the one I originally wanted to fix here. Oh well.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        work in progress (obsolete)
        — 
          — Splinter Review
      

    


  
Another checkpoint. This is pretty good, I think, for static layout. It still
falls down badly in incremental reflow. In particular ReflowDirtyLines is
nowhere near in sync with what ReflowBlockFrame is doing.

        Attachment #159375 -
        Attachment is obsolete: true

    
    

    
  
To get clearance working properly with incremental reflow, I had to subsume the
fix for bug 260938 into this patch.
Blocks: 260938

    
    

    
  

      
      
      
      

        Attached patch
          
          
        diff -w
        — 
          — Splinter Review
      

    


  
I think this patch is ready for review. I'm afraid it's another monster. I'll
provide a detailed commentary soon. There are a few places that have scary
performance implications.

        Attachment #160705 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        better testcase
        — 
      

    


  
I improved my previous testcase(s) and added testing of various dynamic changes
when you hover over the testcases. My patch passes all these tests, as far as I
can tell. Sometimes I've had a hard time figuring out if it's my code or my own
evaluation of the test that's (in)correct :-).

        Attachment #160701 -
        Attachment is obsolete: true

    
    

    
  
At some point I'll take that test case and make real test cases with actual pass 
conditions and so forth and add them to the CSS 2.1 test suite...
Whiteboard: [csswg] → (py8ieh: take testcase and make a real one)
Blocks: 262771
Blocks: 263190

    
    

    
  
Okay, here's an overview of this patch.

The basic strategy for handling clearance and parent-child collapsing top
margins is in nsBlockFrame::ReflowBlockFrame and is similar to what Hixie
described. When we compute the collapsed top margin for a  child block in a line
L, we assume that none of the clearing elements have clearance. When those
elements are actually reflowed we check to see whether they need clearance. If
one does, then we complete its reflow as normal but we pass back (via
nsHTMLReflowState::mDiscoveredClearance) a notification that this frame needed
clearance. When the reflow for line L notices that this notification has
occurred, it marks the frame as having clearance, recomputes the correct top
margin, and repeats the reflow of the line. This approach doesn't require much
rearchitecture and is quite flexible. In particular it does not rely on floats
having the same size wherever they are positioned.

If the optimistic reflow fails, then we have to revert the space manager state
before we retry. But most of the time it won't fail. So we push the space
manager state and if we don't fail, we call the new method
nsSpaceManager::DiscardState which pops the state without actually restoring it.

We have to add a bit to the line state to record whether the line has clearance.
For lines whose top margin collapsed with an ancestor's, the clearance bit is
managed by the topmost ancestor whose top margin collapses with the line. For
all other lines, we compute the clearance bit when reflowing the block in
nsBlockFrame::ReflowBlockFrame.

nsBlockFrame::CheckForCollapsedBottomMarginFromClearanceLine applies the rule
from CSS 2.1 section 8.3.1. Basically whenever we compute the final block size,
we scan the line list backwards to see if a child block with clearance had its
bottom margin adjacent to our bottom margin. If so, then we convert the
carried-out bottom margin of the children into real vertical space which is
added to the block height, and restrict the carried out bottom margin to just
the block's own bottom margin.

The comments in https://bugzilla.mozilla.org/show_bug.cgi?id=260938#c6 apply to
this patch too.

The combination of incremental reflow and clearance is really broken right now
as you can see if you look at the incremental reflow bugs that have been exposed
by the reflow-on-focus changes. Of course adding more complex clearing
processing to fix this bug made the problem worse. The basic approach now is
that whenever ReflowDirtyLines sees a frame with 'clear' set, we call
ClearFloats to see what vertical position is required to for clearance, and we
compare the results with the line's actual vertical position. If they're
different, or the line didn't have clearance before but seems to need it now, or
the line had clearance before and doesn't seem to need it now, then we reflow
the line.

The good news is that according to Martjin this fixes a number of existing
clearance/incremental reflow bugs, and in my tests it takes care of even really
complex clearance/margins/incremental reflow cases. The bad news is that when we
reflow a block with a space manager (i.e. a containing block for floats), we
must reflow any block with a child line which has 'clear' set --- and of course
the block parents of such blocks, etc. That could potentially be bad for
performance in some cases. I just don't see a way around it. We could add code
to optimize this away in some restricted cases. To make it reasonably efficient
I've added a new block state bit NS_BLOCK_HAS_CLEAR_CHILDREN which is set when
the block has descendants with 'clear' not 'none'. It should probably be called
NS_BLOCK_HAS_CLEAR_DESCENDANTS. The bit is set by ReflowDirtyLines.

The interaction between BR-clearance and CSS clearance is a little unclear but I
think what this code does is reasonable. To simplify things I've stopped
accumulating BR-clearance across lines like we used to ... I'm not sure it was
ever valuable.

To simplify code and make things clearer, I consolidated the float-clearing
stuff in nsBlockReflowState into a single ClearFloats method which does not
change the state at all; it just returns what position is required to clear the
floats. This avoids some confusing aState.mY juggling.

+        clearance = aState.mY - (currentY + topMargin);
Note that this line computes exactly the actual CSS 2.1 clearance value for the
block. Or so I claim :-).

Note that ComputeCollapsedTopMargin tells us whether optimism is really needed
in mayNeedRetry --- i.e. whether it encountered a 'clear' element that had to be
assumed to have no clearance. If not (the common case I guess) then we can avoid
a little bit of work.

I've moved the call to nsBlockReflowContext::ComputeCollapsedTopMargin in
nsBlockReflowContext::ReflowBlock out to its callers in
nsBlockFrame::ReflowBlockFrame and nsBlockFrame::ReflowFloat, because that's
where the retry loops are. This also required moving initialization of a block's
nsHTMLReflowState out of nsBlockReflowContext::ReflowBlock into the callers.

nsBlockReflowContext::ComputeCollapsedTopMargin has changed quite a bit. On the
optimistic pass it unsets clearance on lines it encounters. On the second pass
it sets clearance on the line that has been determined to need it. Whenever it
changes the clearance on a line, it dirties the line and the ancestor lines it
traversed, to make sure such lines get reflowed.

I've changed nsBlockReflowContext::PlaceBlock to do margin collapsing when lines
really are empty rather than just have zero height.

Phew! The thing that most worries me about this is performance. We're definitely
setting ourselves up for extra work, both in incremental reflow and in normal
reflow. I really don't know what the impact will be but I'll be surprised if Tp
doesn't feel it. OTOH I can't predict right now what would be the right way to
optimize this better.

    
    

    
  
Comment on attachment 160890 [details] [diff] [review]
diff -w

Sorry David, but might as well get this on the radar.

        Attachment #160890 -
        Flags: superreview?(dbaron)

        Attachment #160890 -
        Flags: review?(dbaron)
Blocks: 265860

    
    

    
  
(In reply to comment #34)
> I'm not sure about the meaning of hypothetical constructions like the one used 
> here:
> 
> # Computing the clearance of an element on which 'clear' is set is done by 
> # first determining the hypothetical position of the element's top border edge
> # within its parent block. This position is determined after the top margin of
> # the element has been collapsed with previous adjacent margins (including the
> # top margin of the parent block).
> 
> The word "after" implies that layout is computed in some order determined by 
> the spec, but that's not really so. You assume it's an outside-in model, and
> therefore we detect clearance on parents before children, which is natural
> enough, but as far as I know it's not part of the spec. So it seems unclear
> exactly what constraints are to apply in the hypothetical world we construct 
> to detect clearance.

So I raised this with the CSS working group, and pretty much everyone here told
me that the layout was done in document order and even quoted parts of the spec
to that effect (although I can't find them now), so the working group didn't
want to add the sentence I proposed ("The hypothetical position of an element is
calculated assuming that all later elements have no clearance.").

I can try pushing harder if you want. Should I?

    
    

    
  
No, I can deal, although I don't believe that "done in document order" is really
a well-defined concept when you have intrinsic sizing.

    
    

    
  
The sentence that I thought already covered the issue was in 9.5.2:

  This position is determined after the top margin of the element has been
  collapsed with previous adjacent margins (including the top margin of the
  parent block).

(*previous* adjacent margins)

    
  
Blocks: 266454
Blocks: 267274
Blocks: 267116
Blocks: 268644

    
    

    
  
Comment on attachment 160890 [details] [diff] [review]
diff -w

I really wanted to review this in more detail, since I did the last rewrite of
margin collapsing, but I don't think that's going to happen.  So rubber-stamp
r+sr=dbaron.  Sorry for the delay.

>+PRBool
>+nsBlockFrame::CheckForCollapsedBottomMarginFromClearanceLine()
>+{
>+  for (line_iterator line = --end_lines(), begin = begin_lines();
>+       line != begin; --line) {

This doesn't check the first line.  Should it?

        Attachment #160890 -
        Flags: superreview?(dbaron)

        Attachment #160890 -
        Flags: superreview+

        Attachment #160890 -
        Flags: review?(dbaron)

        Attachment #160890 -
        Flags: review+

    
    

    
  
Yes it should. I'll fix that.

BTW I mentioned in email that the performance impact of this bug might be
mitigated somewhat if we cache the IsEmpty() status of frames. I'd like to do
that before checking this in; Boris and I have a plan, but it depends on (or at
least would conflict grossly with) the work in bug 261064...
Blocks: 263966
Blocks: 265342
Blocks: 268894
Blocks: 271433

    
    

    
  
Checked in. Mimimal Tp damage on btek, *perhaps* a 5-10ms loss. I'll keep an eye
on the other tboxes.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED

    
    

    
  
There's about a 0.5% Tp loss across the board, plus maybe 2% hit on Tdhtml. It's
unfortunate, but some loss was inevitable, we simply have to do more work to
check that blocks with clearance are correctly positioned after incremental reflow.

    
    

    
  
These are my observations on the dependencies, using a 2004-11-25 trunk build,
which has the fix: 

Bug 204831, partly fixed, second testcases still shows a bug in Mozilla, probably
Bug 224057, fixed
Bug 260938, fixed
Bug 262538, fixed
Bug 262771, fixed
Bug 263190, fixed
Bug 263966, fixed
Bug 265342, fixed
Bug 265860, not fixed
Bug 266454, fixed
Bug 267116, fixed
Bug 267274, fixed
Bug 268644, fixed
Bug 268894, I don't know
Bug 271433, fixed

There are some more bugs here and there, that are probably fixed by the fix for
this bug. I'll look for these tomorrow.

    
    

    
  
Thanks again Martjin. Please mark those fixed bugs as fixed.
Blocks: 261153
Blocks: 262104
Blocks: 264628
Blocks: 262027
Blocks: 262347
Blocks: 262420
Blocks: 262576
No longer blocks: 268894

    
    

    
  
This bug is in 1.8A5. Requesting "blocing 1.8a6" flag status
Flags: blocking1.8a6?

    
    

    
  
The fix is checked in. What is the point of making this block 1.8a6?
Flags: blocking1.8a6?
Blocks: 273193

    
  
No longer blocks: 273193
Depends on: 273193

    
    

    
  
Hmm. In hindsight, comment 73 bear all tokens of a Flatus Cerebralis - a fart of
the brain, if you will. Sorry about that.
Depends on: 273293
Depends on: 273946
Depends on: 274249
Blocks: 220114
Depends on: 282714
Depends on: 309550
Depends on: 320378
Depends on: 326085
Depends on: 375462

          You need to log in
          before you can comment on or make changes to this bug.