593 bytes, text/html
710 bytes, text/html
702 bytes, text/html
718 bytes, text/html
1.36 KB, patch
|Details | Diff | Splinter Review|
7.18 KB, patch
|Details | Diff | Splinter Review|
On this webpage http://www.break.com/pictures/break-photo-hunt-20372853.html the lower browsing bar (the buttons to switch images) is to high. Works in firefox 2.0.0.x but not in Mozilla/5.0 (Windows; U; Windows NT 6.0; fr; rv:1.9a9pre) Gecko/2007101405 Minefield/3.0a9pre ID:2007101405. Will be looking for regression range...
Regression range is 20061207 to 20061208 during landing period of reflow refactoring. So probably related to bug #300030. Waiting for some one to point to good component to confirm. Could be dupe...
Screenshots available here correct and wrong layout : Wrong http://picasaweb.google.com/lh/viewPhoto?uname=gabrielmayrandchadwick&aid=4984677857079263249&iid=5122387694852384466 Good http://picasaweb.google.com/lh/viewPhoto?uname=gabrielmayrandchadwick&aid=4984677857079263249&iid=5122387690557417154
This seems quite timing related. If I reload the page, then that grey box displays under the image, but positioned too low (some 30px lower than it should be). While reloading the page one can also see the image being resized (that is done with some js scripting) to fit in the box.
Confirming for further triage.
Something resembling a minimal testcase would be great.
Created attachment 286543 [details] testcase Well, I minimized it to this, except this also breaks on branch builds. I guess there is some subtle difference, which doesn't make it break on branch, but does on trunk.
Hmm... That looks like some sort of float/clearance bug. The testcase looks an awful lot like bug 380199.
Created attachment 286714 [details] testcase2 Here's a modified testcase. To be correct, the black bar would need to appear below the other bars.
Created attachment 286715 [details] testcase2, reference case #1 Here's a reference for testcase2, with the <br/> taken out. (letting the third div wrap naturally within the fixed-width body, rather than imposing it with the <br/>) This shows the correct behavior.
Created attachment 286716 [details] testcase2, reference case #2 Here's another reference case for testcase2. This one just has an extra <br/> added, enforcing the break between the first and second divs. (which happens naturally anyway, due to wrapping within the fixed-width body) This shows the correct behavior.
Side note: Interestingly, testcase2 looks fine on branch, even though it's similar to testcase1, which does break branch. Also, ref2 is broken on branch.
Created attachment 286888 [details] [diff] [review] partial patch v1 This fixes testcase2, but not the break.com page or the first testcase.
Created attachment 286892 [details] [diff] [review] patch v2 oops, there was bug in the last patch: old: nscoord lineYCombinedB = lineYA + aLine->GetCombinedArea().height; new: nscoord lineYCombinedB = lineYCombinedA + aLine->GetCombinedArea().height; Now that that's better, this patch fixes all this bug's testcases, plus the original break.com testcase, plus the bug 380199 testcase.
=== Explanation of / Justification for patch === This is the setup in testcase2, after we've done a reflow but before we've done any JS modifications: line 1: mBounds: y=0, height=1140 mCombinedArea: y=6001, height=1800 line 2: mBounds: y=1140, height=0 mCombinedArea: y=1800, height=600 Blue Float: y=0, height=1200 Green Float: y=1200, height=600 Black Float: y=1800, height=600 **Note A: Line 1 gets its height from the <br/>. **Note B: Green Float is *below* blue float (not beside it) because there isn't enough horizontal space for them to fit side-by-side. So in fact, the Green Float is positioned **below** line 2's mBounds, even though it's **in** line 1. So when we grow the Green Float via JS, line 2's mBounds (a zero-high sliver at y=1140) doesn't intersect the float damage region (which starts at y=1200). So nsBlockFrame::PropagateFloatDamage doesn't mark line 2 as dirty. That's bad -- line2 **needs** to be marked dirty, because its float content (the Black Float) **is** affected by the change. We weren't catching this because the Black Float isn't located inside its line's mBounds rectangle -- it's pushed downward due to wrapping. It *is* located inside it's lines GetCombinedArea() rectangle, however. So this patch checks the float-damage region for intersection with the line's GetCombinedArea() to catch this.
Note this comment from nsLineBox.h: 391 // Combined area is the area of the line that should influence the 392 // overflow area of its parent block. The combined area should be 393 // used for painting-related things, but should never be used for 394 // layout (except for handling of 'overflow'). I'm assuming that my patch's use of GetCombinedLayout() falls under the allowed "handling of 'overflow'" category, because I'm using GetCombinedLayout() to find the intersection between overflowed floats. But let me know if I'm misinterpreting this comment and if my use of GetCombinedLayout() is unsafe for some reason.
Created attachment 286912 [details] [diff] [review] reftests (as patch) This reftest checks this bug's "testcase2" attachment, along with its "reference case #1/2" variants, against a simple no-float, no-JS reference.
Comment on attachment 286912 [details] [diff] [review] reftests (as patch) The first reftest fails pre-patch and passes post-patch. The other two pass both before and after. (I'm including them as sanity checks, because all three should look the same.)
(In reply to comment #13) > oops, there was bug in the last patch: Was this caught by an existing reftest, or is such a reftest included in attachment 286912 [details] [diff] [review]?
(In reply to comment #19) > (In reply to comment #13) > > oops, there was bug in the last patch: > > Was this caught by an existing reftest, or is such a reftest included in > attachment 286912 [details] [diff] [review]? It was caught by the original testcase on this bug page, by virtue of the fact that that testcase's floats are much larger than the height of a line. (19px) The attached reftests wouldn't catch that issue -- I'll include a second copy of those reftests with larger floats, which would catch that.
Created attachment 286997 [details] [diff] [review] reftests, landed Here are the reftests, as landed (just now). Same as previously posted reftests, except I added 400171-2*.html, with a larger first float than 400171-1*.html This makes 400171-2*.html catch the bug in partial patch v1. (attachment 286888 [details] [diff] [review])
Just updated the reftests -- I had to increase the div.a size a bit to make 400171-1a fail on Windows. Now it's 30px high instead of 20px. The first div needs to be taller than the <br/> in order for that test to fail, and 20px was just *barely* taller on Linux. (The <br/> is 19px) Windows' default font must be slightly taller than Linux's, making the <br/> 20+ px tall, as large or larger than the first div, and making the test unexpectedly pass. http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1193948220&maxdate=1193948221&who=dholbert%cs.stanford.edu
Might make sense to do the heights in em instead of px. Then you don't have to worry about font sizes on the test system, etc. Also, it's not that the div needs to be taller than the <br> but that it needs to be taller than the line-height, right? Setting that explicitly instead of using the auto value might also make things more predictable.
(In reply to comment #23) > Might make sense to do the heights in em instead of px. Then you don't have to > worry about font sizes on the test system, etc. Ah, that makes sense. I haven't ever used em-units much, but that could definitely be handy here. > Also, it's not that the div needs to be taller than the <br> but that it needs > to be taller than the line-height, right? Setting that explicitly instead of > using the auto value might also make things more predictable. Yup, by "taller than the <br>" I just meant taller than the line height. I'd forgot that line-height was configurable via CSS, though -- you're right, that's probably the easiest way to enforce the desired behavior for these testcases.
Just checked in a minor change to reftests, adding an explicit line-height declaration: http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1193965923&maxdate=1193966040&who=dholbert%cs.stanford.edu
Comment on attachment 286892 [details] [diff] [review] patch v2 As review could land after beta 1 ? Just asking...
It's already a blocker. It doesn't need explicit approval post-beta.
Comment on attachment 286892 [details] [diff] [review] patch v2 (In reply to comment #27) > It's already a blocker. It doesn't need explicit approval post-beta. > Yup. And it's already in line for post-beta1 check-in: http://wiki.mozilla.org/Firefox3/Beta2CheckinQueue Canceling approval1.9 request.
checked in patch v2, and removed "fails" from this bug's lines in reftest.list. Checking in generic/nsBlockFrame.cpp; /cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v <-- nsBlockFrame.cpp new revision: 3.882; previous revision: 3.881 done Checking in reftests/bugs/reftest.list; /cvsroot/mozilla/layout/reftests/bugs/reftest.list,v <-- reftest.list new revision: 1.189; previous revision: 1.188 done
Verified with todays nightly build.