Closed
Bug 551425
Opened 15 years ago
Closed 14 years ago
Hiding a float does not update the layout correctly
Categories
(Core :: Layout: Floats, defect, P2)
Core
Layout: Floats
Tracking
()
RESOLVED
FIXED
mozilla2.0b4
Tracking | Status | |
---|---|---|
blocking2.0 | --- | final+ |
blocking1.9.2 | --- | - |
status1.9.2 | --- | wanted |
status1.9.1 | --- | unaffected |
People
(Reporter: sebastian, Assigned: fantasai.bugs)
References
()
Details
(Keywords: regression)
Attachments
(3 files, 1 obsolete file)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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•15 years ago
|
||
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•15 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
Closed: 15 years ago
Resolution: --- → WORKSFORME
Reporter | ||
Comment 3•15 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 → ---
Updated•15 years ago
|
Component: General → Layout
Product: Firefox → Core
QA Contact: general → layout
Comment 4•15 years ago
|
||
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
Comment 5•15 years ago
|
||
Comment 6•15 years ago
|
||
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
Comment 7•15 years ago
|
||
Would definitely take a fix for this layout regression. Regressed after 1.9.1, so that branch is unaffected.
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 :-)
blocking2.0: ? → final+
Priority: -- → P2
Assignee | ||
Comment 12•15 years ago
|
||
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•14 years ago
|
||
So, actually, the patch passes reftests when I push to tryserver. -__-;;
Attachment #449541 -
Attachment is obsolete: true
Attachment #453604 -
Flags: review?
Assignee | ||
Comment 15•14 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)
Comment 16•14 years ago
|
||
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•14 years ago
|
||
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+
Comment 18•14 years ago
|
||
(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 20•14 years ago
|
||
I addressed the review comments in: http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/d13353d4a2d6/fantasai-float-coords
Comment 21•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/fd26456949ad
Status: ASSIGNED → RESOLVED
Closed: 15 years ago → 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b4
You need to log in
before you can comment on or make changes to this bug.
Description
•