[reflow branch] Break.com browsing bar is displaced (on image)

VERIFIED FIXED

Status

()

Core
Layout
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: Gabriel Chadwick, Assigned: dholbert)

Tracking

({regression})

Trunk
regression
Points:
---
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [dbaron-1.9:RwCr], URL)

Attachments

(6 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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

10 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: 300030
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

10 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

10 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

10 years ago
Confirming for further triage.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.9? → blocking1.9+
Whiteboard: [dbaron-1.9:RwCr]
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.
Depends on: 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.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
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.
Attachment #286888 - Attachment is obsolete: true
Duplicate of this bug: 380199
No longer depends on: 380199
=== 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.
Attachment #286892 - Flags: superreview?(roc)
Attachment #286892 - Flags: review?(roc)
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.)
Attachment #286912 - Flags: review?(roc)
(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+
(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])
Attachment #286912 - Attachment is obsolete: true
Flags: in-testsuite+
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
(Reporter)

Updated

10 years ago
Keywords: qawanted
Whiteboard: [dbaron-1.9:RwCr] → [dbaron-1.9:RwCr][needs landing]
(Reporter)

Comment 26

10 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?
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.
Attachment #286892 - Flags: approval1.9?
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
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [dbaron-1.9:RwCr][needs landing] → [dbaron-1.9:RwCr]
(Reporter)

Comment 30

10 years ago
Verified with todays nightly build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.