412607 - inline content not wrapping to next line, when it should

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[philippe (part-time)]

Attachment: testcase

The linked site shows unneeded scrollbars around each post.
In the testcase: 
An image at the end of a line, it should drop to the next line.
wrapped in a div {overflow:auto}, inside a table.

regression range: (tested OS X PPC-10.4)
fails
Gecko/2007102004 Minefield/3.0a9pre
works
Gecko/2007101904 Minefield/3.0a9pre
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-10-19+03%3A30%3A00&maxdate=2007-10-20+03%3A30%3A00&cvsroot=%2Fcvsroot

--> bug 393096

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

This reduced testcase reproduces the bug on both Mac & Linux. 
(haven't tried windows; it probably happens there too.  If not, I'm sure it would reproduce there after tweaking the image width)

As a first guess, it looks like this bug has something do with the whitespace between the "x" character and the image.  If I remove the whitespace betwen the text and the image, I can't get a scrollbar to appear, even when I increase the image's width 1px at a time so that it eventually wraps to the next line.

This is also is true of the original testcase on Mac -- if I remove the whitespace between the text and the image, the bug doesn't reproduce.

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to comment #1)
> Created an attachment (id=300542) [details]
> testcase 2
> 
> This reduced testcase reproduces the bug on both Mac & Linux. 
> (haven't tried windows

It reproduces on Windows as well. (Tested using Firefox 3b2)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 3

This reduced testcase shows that we need neither a table nor an image to reproduce this bug.

The table was just limiting the width of the "overflow: auto" div (which can be done with CSS), and the image was just acting as an inline fixed-width filler element (which can be done with an inline-block).

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 4

and this testcase shows that "overflow: auto" isn't needed either, actually.

The green div should appear on a separate line from the 'x', but it appears on the same line.  (tested on Windows/Mac/Linux)

When I increase the green div's width by 1px, however, it drops to the next line. (on my linux machine, at least)

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to comment #3)
> This reduced testcase shows that we need neither a table nor an image
(In reply to comment #4)
> and this testcase shows that "overflow: auto" isn't needed either, actually.

Generalizing summary to reflect these discoveries.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: testcase 5

Here's one more testcase  -- it just uses divs to get the point across.

This testcase makes it clear (if it wasn't already) that we're basically ignoring the width of the space when deciding whether the green div needs to wrap or not.

We have:
[wrapper div: 100px wide]
  [div a: 20px wide] [SPACE] [div b: 80px wide]

So the divs would fit perfectly if the whitespace were ignored, but not when we actually allocate space for it.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to comment #6)
> we're basically ignoring the width of the space when deciding whether
> the green div needs to wrap or not.

Specifically, that happens here, in nsLineLayout::CanPlaceFrame:
http://mxr.mozilla.org/seamonkey/source/layout/generic/nsLineLayout.cpp#1166

1166   PRBool outside = pfd->mBounds.XMost() - mTrimmableWidth + endMargin > psd->mRightEdge;
1167   if (!outside) {
...
1172     return PR_TRUE;
1173   }

At this point, in the code:
 - We're determining whether we can place the green div on the first line
 - "pfd" is the perFrameData for the green div
 - mTrimmableWidth = 300 = 5px, the width of the space before the green div

This seems clearly bad that we'd do this. AFAICT, we need to either:
  A) be clearing mTrimmableWidth at some point, so the space is no longer considered "trimmable"
  B) remove mTrimmableWidth from the subtraction in line 1166.

I think (A) seems more correct.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to comment #7)
> we need to 
>   A) be clearing mTrimmableWidth at some point, so the space is no longer
> considered "trimmable"

Ah -- we actually already do clear mTrimmableWidth during the call of nsLineLayout::ReflowFrame for the green div, BUT we do it a bit too late -- at nsLineLayout:cpp:1013, 17 lines after we've called CanPlaceFrame and decided that the green div fits.

The chunk where we clear it is:
  1011       if (!continuingTextRun) {
  1012         if (!pfd->GetFlag(PFD_SKIPWHENTRIMMINGWHITESPACE)) {
  1013           mTrimmableWidth = 0;
  1014         }

roc -- should we move this chunk to up above CanPlaceFrame?

Or, perhaps it should stay in its current position, but it should refer to the *next* frame in the line layout, rather than the current frame?  (i.e. so that during nsLineLayout::ReflowFrame on the textFrame, we'd look ahead and see the div coming up, and proactively clear mTrimmableWidth as a result of the upcoming div not being a textframe.)

To tell the truth, I'm not sure what purpose it serves in it current position, without these changes.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> roc -- should we move this chunk to up above CanPlaceFrame?

Yeah, that sounds right.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: fix v1

This patch fixes testcases 2-5 on linux, making the block wrap to the next line.
(original testcase doesn't reproduce the bug on linux)

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: reftests [checked in]

reftests, based on testcase 5.

Comment 12

[philippe (part-time)]

Your patch fixes the first test case as well (OS X 10.5). That one is very sensitive to the font chosen (or the font-metrics); make the box 1 or 2 px smaller or larger and you'll get hit..

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to comment #12)
> Your patch fixes the first test case as well (OS X 10.5).

Great, thanks for testing that!

> That one is very
> sensitive to the font chosen (or the font-metrics); make the box 1 or 2 px
> smaller or larger and you'll get hit..

That makes sense, and actually applies in all of the testcases -- the amount  of leeway for reproducing the bug is the width of a space character, which is pretty small.

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to comment #13)
> > That one is very
> > sensitive to the font chosen (or the font-metrics); make the box 1 or 2 px
> > smaller or larger and you'll get hit..
> 
> That makes sense, and actually applies in all of the testcases

Sorry -- to be clear, the "very sensitive to the font chosen" part *does* matter much more on the first testcase than on the other ones, because the first testcase has more text that grows/shrinks when the font changes.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 301568 [details] [diff] [review]
fix v1

+    if (!continuingTextRun) {
+      if (!pfd->GetFlag(PFD_SKIPWHENTRIMMINGWHITESPACE)) {

Use &&

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: fix v1a [checked in]

(In reply to comment #15)
> Use &&

Ah, good call.  Here's the patch again, with that change.

I'll land this when the tree quiets down.

Comment 17

[Daniel Holbert [:dholbert]]

fix v1a & reftests checked in.

Checking in generic/nsLineLayout.cpp;
/cvsroot/mozilla/layout/generic/nsLineLayout.cpp,v  <--  nsLineLayout.cpp
new revision: 3.297; previous revision: 3.296
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/412607-1-ref.html,v
done
Checking in reftests/bugs/412607-1-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/412607-1-ref.html,v  <--  412607-1-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/412607-1a.html,v
done
Checking in reftests/bugs/412607-1a.html;
/cvsroot/mozilla/layout/reftests/bugs/412607-1a.html,v  <--  412607-1a.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/412607-1b.html,v
done
Checking in reftests/bugs/412607-1b.html;
/cvsroot/mozilla/layout/reftests/bugs/412607-1b.html,v  <--  412607-1b.html
initial revision: 1.1
done
Checking in reftests/bugs/reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v  <--  reftest.list
new revision: 1.341; previous revision: 1.340
done