551425 - Hiding a float does not update the layout correctly

Status: RESOLVED FIXED

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

Description

[Kostantin Stambolov]

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>
 <div class="elementBig" style="display: none;">
   Content Big
   <a href="#" />Show Small<a>
 </div>

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).
http://tmp.sebastianz55.org/ff36-1.png
http://tmp.sebastianz55.org/ff36-2.png
http://tmp.sebastianz55.org/ff36-3-BUG.png

Comment 1

[Ria Klaassen (not reading all bugmail)]

Can you give more exact steps to reproduce, e.g. where on the page is the link that should be clicked on?

Comment 2

[Kostantin Stambolov]

Hi,
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: http://www.capital.bg/politika_i_ikonomika/bulgaria/2010/03/05/868631_ulici_s_iztekul_srok_na_godnost/

I have no idea what happened.

Comment 3

[Kostantin Stambolov]

Hello again,
I've detected again the bug in this article ->
http://www.capital.bg/politika_i_ikonomika/bulgaria/2010/03/12/872387_reitingut_ili_reformite/

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yep.  The page in comment 3 reproduces the problem.  Any chance of a reduced testcase?

Comment 5

[Martijn Wargers (dead)]

Attachment: testcase

Comment 6

[Boris Zbarsky [:bzbarsky]]

Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5923620fafe8&tochange=08c3952a49b3

Perhaps bug 499377?

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Would definitely take a fix for this layout regression. Regressed after 1.9.1, so that branch is unaffected.

Comment 8

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

fantasai, this is probably yours, mind taking it?

Comment 9

[fantasai]

As long as you don't mind me not getting to it until April.

Comment 10

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

That's only 11 days away for me :-)

Comment 11

[fantasai]

Correction, late April. :)

Comment 12

[fantasai]

Attachment: work-in-progress

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.)

Comment 13

[fantasai]

Attachment: patch

So, actually, the patch passes reftests when I push to tryserver. -__-;;

Comment 14

[Boris Zbarsky [:bzbarsky]]

fantasai, whom did you mean to ask for review?

Comment 15

[fantasai]

Comment on attachment 453604 [details] [diff] [review]
patch

David Baron. I thought I had marked him down...

Comment 16

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

Attachment: patch merged to trunk

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 17

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

Comment on attachment 453604 [details] [diff] [review]
patch

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
nsBlockReflowState::FlowAndPlaceFloat:
-         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
merging).

========================================================================

The bug fix for this bug is in nsBlockFrame::PropagateFloatDamage, where
prior to this patch there should have been a +borderPadding.top 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
follows:

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 /
  nsBlockFrame::RecoverFloatsFor.
- 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
   already)
 - 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

Comment 18

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

(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 +borderPadding.top when
> calling GetFloatAvailableSpaceForHeight.

That bug, by the way, goes back to:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/generic&command=DIFF_FRAMESET&file=nsBlockFrame.cpp&rev2=3.464&rev1=3.463
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?

Comment 19

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

And I really do like this change; it makes the code a good bit cleaner.

Comment 20

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

I addressed the review comments in:
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/d13353d4a2d6/fantasai-float-coords

Comment 21

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

http://hg.mozilla.org/mozilla-central/rev/fd26456949ad