Closed
Bug 400171
Opened 17 years ago
Closed 17 years ago
[reflow branch] Break.com browsing bar is displaced (on image)
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
VERIFIED
FIXED
People
(Reporter: chadwickgab+mozilla, Assigned: dholbert)
References
()
Details
(Keywords: regression, Whiteboard: [dbaron-1.9:RwCr])
Attachments
(6 files, 2 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
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...
Reporter | ||
Comment 1•17 years ago
|
||
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...
Blocks: reflow-refactor
Flags: blocking1.9?
Summary: Break.com browsing bar is displaced (on image) → [reflow branch] Break.com browsing bar is displaced (on image)
Reporter | ||
Comment 2•17 years ago
|
||
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
Comment 3•17 years ago
|
||
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.
Reporter | ||
Comment 4•17 years ago
|
||
Confirming for further triage.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.9? → blocking1.9+
Updated•17 years ago
|
Whiteboard: [dbaron-1.9:RwCr]
Comment 5•17 years ago
|
||
Something resembling a minimal testcase would be great.
Comment 6•17 years ago
|
||
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.
Comment 7•17 years ago
|
||
Hmm... That looks like some sort of float/clearance bug. The testcase looks an awful lot like bug 380199.
Depends on: 380199
Assignee | ||
Comment 8•17 years ago
|
||
Here's a modified testcase. To be correct, the black bar would need to appear below the other bars.
Assignee | ||
Comment 9•17 years ago
|
||
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.
Assignee | ||
Comment 10•17 years ago
|
||
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.
Assignee | ||
Comment 11•17 years ago
|
||
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.
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → dholbert
Assignee | ||
Updated•17 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 12•17 years ago
|
||
This fixes testcase2, but not the break.com page or the first testcase.
Assignee | ||
Comment 13•17 years ago
|
||
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.
Attachment #286888 -
Attachment is obsolete: true
Assignee | ||
Comment 15•17 years ago
|
||
=== 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.
Assignee | ||
Updated•17 years ago
|
Attachment #286892 -
Flags: superreview?(roc)
Attachment #286892 -
Flags: review?(roc)
Assignee | ||
Comment 16•17 years ago
|
||
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.
Assignee | ||
Comment 17•17 years ago
|
||
This reftest checks this bug's "testcase2" attachment, along with its "reference case #1/2" variants, against a simple no-float, no-JS reference.
Assignee | ||
Comment 18•17 years ago
|
||
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.)
Attachment #286912 -
Flags: review?(roc)
Comment 19•17 years ago
|
||
(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]?
Attachment #286892 -
Flags: superreview?(roc)
Attachment #286892 -
Flags: superreview+
Attachment #286892 -
Flags: review?(roc)
Attachment #286892 -
Flags: review+
Attachment #286912 -
Flags: review?(roc) → review+
Assignee | ||
Comment 20•17 years ago
|
||
(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.
Assignee | ||
Comment 21•17 years ago
|
||
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])
Attachment #286912 -
Attachment is obsolete: true
Assignee | ||
Updated•17 years ago
|
Flags: in-testsuite+
Assignee | ||
Comment 22•17 years ago
|
||
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
Comment 23•17 years ago
|
||
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.
Assignee | ||
Comment 24•17 years ago
|
||
(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.
Assignee | ||
Comment 25•17 years ago
|
||
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
Assignee | ||
Updated•17 years ago
|
Whiteboard: [dbaron-1.9:RwCr] → [dbaron-1.9:RwCr][needs landing]
Reporter | ||
Comment 26•17 years ago
|
||
Comment on attachment 286892 [details] [diff] [review] patch v2 As review could land after beta 1 ? Just asking...
Attachment #286892 -
Flags: approval1.9?
Comment 27•17 years ago
|
||
It's already a blocker. It doesn't need explicit approval post-beta.
Assignee | ||
Comment 28•17 years ago
|
||
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.
Attachment #286892 -
Flags: approval1.9?
Assignee | ||
Comment 29•17 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Whiteboard: [dbaron-1.9:RwCr][needs landing] → [dbaron-1.9:RwCr]
You need to log in
before you can comment on or make changes to this bug.
Description
•