Closed Bug 551425 Opened 12 years ago Closed 12 years ago

Hiding a float does not update the layout correctly


(Core :: Layout: Floats, defect, P2)




Tracking Status
blocking2.0 --- final+
blocking1.9.2 --- -
status1.9.2 --- wanted
status1.9.1 --- unaffected


(Reporter: sebastian, Assigned: fantasai.bugs)




(Keywords: regression)


(3 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6 FireShot/0.32
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6 FireShot/0.32

I have this html:

<div class="holder">
 <div class="elementSmall" style="display: block;">
   Content Small
   <a href="#" />Show Big<a>
 <div class="elementBig" style="display: none;">
   Content Big
   <a href="#" />Show Small<a>

This is floating text in the div.
<br />
Uses br tags, not p tags.
<br />
Some more text.
</div><!-- end holder -->

Reproducible: Always

Steps to Reproduce:
1.I click the link to show the Big block - it's applied display: block; to the Big element and display: none; to the small div. The text floats around the div.
2. I click on the link in the Big block to display the small div again i.e. to hide the big div.
Actual Results:  
The small div is displayed again and the big div is applied display: none. Nevertheless the text does not float around the small div. It is stuck to the size of the big one.

Expected Results:  
The text to float around the small div after hiding the big div. This is normal behavior. It was all right in FF 3.5.8.

Here you can see screenshots. The actual big is the gap that stays (screen 3).
Can you give more exact steps to reproduce, e.g. where on the page is the link that should be clicked on?
Now this bug-case is gone. After extensive testing in the office now (as well as before submitting the bug yesterday) it is no longer present.

Link of the article is:

I have no idea what happened.
Closed: 12 years ago
Resolution: --- → WORKSFORME
Hello again,
I've detected again the bug in this article ->

The second window on the left column in the article marked with red square and bars in it.

Open with the link in the bottom right of the window (green box with arrow in it) and then close it with the same positioned link.
Resolution: WORKSFORME → ---
Component: General → Layout
Product: Firefox → Core
QA Contact: general → layout
Yep.  The page in comment 3 reproduces the problem.  Any chance of a reduced testcase?
Component: Layout → Layout: Floats
Ever confirmed: true
QA Contact: layout → layout.floats
Attached file testcase
Regression range:

Perhaps bug 499377?
Blocks: 499377
blocking1.9.2: --- → ?
blocking2.0: --- → ?
status1.9.2: --- → ?
Keywords: regression
OS: Mac OS X → All
Hardware: x86 → All
Summary: Layout change of CSS property Display: block to none and reverse of a floated element in div contating text does not affect text → Hiding a float does not update the layout correctly
Would definitely take a fix for this layout regression. Regressed after 1.9.1, so that branch is unaffected.
blocking1.9.2: ? → -
fantasai, this is probably yours, mind taking it?
As long as you don't mind me not getting to it until April.
Assignee: nobody → fantasai.bugs
That's only 11 days away for me :-)
Correction, late April. :)
blocking2.0: ? → final+
Priority: -- → P2
Attached patch work-in-progress (obsolete) — Splinter Review
Still working through some reftest failures, but basically my approach here is to get rid of the border-box <-> content-box coord transformation we're using for the float manager, since it's really confusing. (Thought I'd post the unfinished patch so dbaron can see what I'm up to.)
Attached patch patchSplinter Review
So, actually, the patch passes reftests when I push to tryserver. -__-;;
Attachment #449541 - Attachment is obsolete: true
Attachment #453604 - Flags: review?
fantasai, whom did you mean to ask for review?
Comment on attachment 453604 [details] [diff] [review]

David Baron. I thought I had marked him down...
Attachment #453604 - Flags: review? → review?(dbaron)
This required a bit of merging with bug 563584; I think this should be merged correctly.  There was one chunk removed; I couldn't find anything obvious to add, though it's possible there might be one chunk.  I haven't tested this.
Comment on attachment 453604 [details] [diff] [review]

This is really a review of the merged patch in the attachment 463662 [details] [diff] [review].

I found the two mistakes in my merging.

First of all, there was a chunk of the original patch in code in
CanPlaceFloat that was removed.  It does require a corresponding change
after bug 563584, which is adding this additional change in
-         mContentArea.height - floatY) ||
+         mContentArea.YMost() - floatY) ||
Please double-check that you agree with this, and add it.

Second, I made a simple mistake in the merging as well, I believe.  In
the first nsBlockReflowState::GetFloatAvailableSpace, where the patch
currently looks like this:
>    nscoord height = (mContentArea.height == nscoord_MAX)
> -                     ? nscoord_MAX : NS_MAX(mContentArea.height - y, 0);
> +                     ? nscoord_MAX : NS_MAX(mContentArea.height - aY, 0);
I did the merging wrong; it should instead look like this:
>    nscoord height = (mContentArea.height == nscoord_MAX)
> -                     ? nscoord_MAX : NS_MAX(mContentArea.height - y, 0);
> +                     ? nscoord_MAX : NS_MAX(mContentArea.YMost() - aY, 0);
You should double-check this as well, and fix it.  (I think this
chunk of the patch was the most substantive merge that I had to do.)

Additionally, on a related note, please add comments around the
definition of mContentArea in nsBlockReflowState.h noting that since
mContentArea.height is NS_UNCONSTRAINEDSIZE in the normal case,
mContentArea.YMost() should only be called after checking that
mContentArea.height is *not* NS_UNCONSTRAINEDSIZE since otherwise
overflow could occur.  Also, the comment there that suggests that width
can be NS_UNCONSTRAINEDSIZE is wrong.  I'd suggest rewriting the entire
comment to read:
  // The content area to reflow child frames within.  This is within
  // this frame's coordinate system, which means mContentArea.x ==
  // BorderPadding().left and mContentArea.y == BorderPadding().top.
  // The height may be NS_UNCONSTRAINEDSIZE, which indicates that there
  // is no page/column boundary below (the common case).
  // mContentArea.YMost() should only be called after checking that
  // mContentArea.height is not NS_UNCONSTRAINEDSIZE; otherwise
  // coordinate overflow may occur.

Also, as a result of merging with bug 563584, ~nsBlockReflowState is
now empty; please remove it (from both .h and .cpp).

r=dbaron with that fixed

If you'd like me to address the review comments and land the patch,
please let me know (I'd be happy to), but I'd like you to double-check
that you agree with my comments above (particularly the first two about


The bug fix for this bug is in nsBlockFrame::PropagateFloatDamage, where
prior to this patch there should have been a when
calling GetFloatAvailableSpaceForHeight.

An additional bug fix contained here, I think, is that
nsBlockFrame::ReflowPushedFloats now adds the floats correctly to the
float manager; previously it added them with an incorrect
borderpadding offset in the case where it skipped reflowing them (though
I expect that happened rarely if ever).

There was a similar bug fix in equally unlikely-to-hit code in
nsContainerFrame::ReflowOverflowContainerChildren.  It called
nsBlockFrame::RecoverFloatsFor with incorrect float manager translation,
since that was one of the functions that required the float manager be
in border-box translation.

I believe that, once the review comments are addressed, those three
changes are the only substantive changes in this patch.


My notes on what this patch changed, which I made to help myself while
reviewing (and track which changes I had reviewed so far), are as

changes nsBlockReflowState::mContentArea from nsSize to nsRect, with
TopLeft() == BorderPadding().TopLeft()

float manager translation is relative to the border edge instead of
the content edge (though it appears to sometimes be in an intermediate
state pre-patch, nsBlockReflowState ctor/dtor are the entry/exit points
for the good state)

- float manager had border rect translation when calling
  nsBlockReflowState::RecoverFloats / nsBlockFrame::RecoverFloats /
- float manager had border rect translation when crossing Reflow API

Changes meaning of following from content box to border box:
 - calls into float manager as described above
 - nsBlockReflowState::mFloatManager{X,Y}
 - results of nsBlockReflowState::GetFloatAvailableSpace*
 - inout aFloatAvailableSpace param to DoReflowInlineFrames/PlaceLine
 - input to nsBlockReflowState::ComputeBlockAvailSpace (output was
 - input to nsBlockFrame::WidthToClearPastFloats
 - input to nsBlockReflowState::ComputeReplacedBlockOffsetsForFloats
 - perhaps another param or two that are trivially passed through to one
   of the above that I didn't need to note
Attachment #453604 - Flags: review?(dbaron) → review+
(In reply to comment #17)
> The bug fix for this bug is in nsBlockFrame::PropagateFloatDamage, where
> prior to this patch there should have been a when
> calling GetFloatAvailableSpaceForHeight.

That bug, by the way, goes back to:
which was part of bug 86947, which I fixed in 2001.

I don't understand why your changes exposed it, though.  Could they have caused something to be marked dirty less often?
And I really do like this change; it makes the code a good bit cleaner.
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b4
Depends on: 590417
No longer depends on: 590417
Blocks: 392826
You need to log in before you can comment on or make changes to this bug.