Hiding a float does not update the layout correctly

RESOLVED FIXED in mozilla2.0b4

Status

()

Core
Layout: Floats
P2
normal
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Kostantin Stambolov, Assigned: fantasai)

Tracking

({regression})

unspecified
mozilla2.0b4
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+, blocking1.9.2 -, status1.9.2 wanted, status1.9.1 unaffected)

Details

(URL)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
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
Can you give more exact steps to reproduce, e.g. where on the page is the link that should be clicked on?
(Reporter)

Comment 2

8 years ago
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.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → WORKSFORME
(Reporter)

Comment 3

8 years ago
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.
Status: RESOLVED → UNCONFIRMED
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?
Status: UNCONFIRMED → NEW
Component: Layout → Layout: Floats
Ever confirmed: true
QA Contact: layout → layout.floats
Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5923620fafe8&tochange=08c3952a49b3

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: ? → -
status1.9.1: --- → unaffected
status1.9.2: ? → wanted
fantasai, this is probably yours, mind taking it?
(Assignee)

Comment 9

8 years ago
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 :-)
(Assignee)

Comment 11

8 years ago
Correction, late April. :)
blocking2.0: ? → final+
Priority: -- → P2
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 12

8 years ago
Created attachment 449541 [details] [diff] [review]
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.)
(Assignee)

Comment 13

8 years ago
Created attachment 453604 [details] [diff] [review]
patch

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?
(Assignee)

Comment 15

8 years ago
Comment on attachment 453604 [details] [diff] [review]
patch

David Baron. I thought I had marked him down...
Attachment #453604 - Flags: review? → review?(dbaron)
Created attachment 463662 [details] [diff] [review]
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 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
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 +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?
And I really do like this change; it makes the code a good bit cleaner.
http://hg.mozilla.org/mozilla-central/rev/fd26456949ad
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b4

Updated

8 years ago
Depends on: 590417

Updated

8 years ago
No longer depends on: 590417

Updated

8 years ago
Blocks: 392826
You need to log in before you can comment on or make changes to this bug.