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

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Gabriel Chadwick]

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...

Comment 1

[Gabriel Chadwick]

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...

Comment 2

[Gabriel Chadwick]

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

[philippe (part-time)]

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.

Comment 4

[Gabriel Chadwick]

Confirming for further triage.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Something resembling a minimal testcase would be great.

Comment 6

[Martijn Wargers (dead)]

Attachment: 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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Hmm...  That looks like some sort of float/clearance bug.  The testcase looks an awful lot like bug 380199.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: testcase2

Here's a modified testcase.  To be correct, the black bar would need to appear below the other bars.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: 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.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: 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.

Comment 11

[Daniel Holbert [:dholbert]]

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.

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: partial patch v1

This fixes testcase2, but not the break.com page or the first testcase.

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: 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.

Comment 15

[Daniel Holbert [:dholbert]]

=== 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.

Comment 16

[Daniel Holbert [:dholbert]]

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.

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: 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 18

[Daniel Holbert [:dholbert]]

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.)

Comment 19

[Jeff Walden [:Waldo]]

(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]?

Comment 20

[Daniel Holbert [:dholbert]]

(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.

Comment 21

[Daniel Holbert [:dholbert]]

Attachment: 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])

Comment 22

[Daniel Holbert [:dholbert]]

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

[Boris Zbarsky [:bzbarsky]]

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.

Comment 24

[Daniel Holbert [:dholbert]]

(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.

Comment 25

[Daniel Holbert [:dholbert]]

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 26

[Gabriel Chadwick]

Comment on attachment 286892 [details] [diff] [review]
patch v2

As review could land after beta 1 ? Just asking...

Comment 27

[Boris Zbarsky [:bzbarsky]]

It's already a blocker. It doesn't need explicit approval post-beta.

Comment 28

[Daniel Holbert [:dholbert]]

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.

Comment 29

[Daniel Holbert [:dholbert]]

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

Comment 30

[Gabriel Chadwick]

Verified with todays nightly build.