407111 - Text won't break before an image

Status: VERIFIED FIXED

(Core :: Layout: Block and Inline, defect, P2)

Description

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

Attachment: testcase

The testcase should wrap to two lines, but it doesn't.

Comment 1

[Jesse Ruderman]

The <br> in the testcase doesn't seem to be necessary.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Seems like we ought to break before the second image, no?  Isn't that the only possible break point in the line?

Comment 3

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

Yeah. I didn't mean the summary to imply that we should break directly before the <br>

Comment 4

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

Attachment: fix

Turns out the <br> is necessary, actually. I'm not sure what Jesse is seeing, maybe a build before 405577 was fixed.

Somewhere recently I exposed a bug that's been around for at least a little while. When nsLineLayout places a frame that doesn't continue a textrun and is wrappable CSS white-space, it records a break opportunity. The problem is, it passes PR_TRUE for the aFits parameter, which is sometimes incorrect ... sometimes, such as in this testcase after the <br>, the break position after the frame is beyond the available width. Thus this "bad" break opportunity wipes out the "good" one that really did fit. The fix is to return in CanPlaceFrame whether the trailing edge of the frame fits in the available width, pass that for aFits.

This also shows the "shouldn't be updating the break position after we've already flagged an overrun" assertion firing incorrectly. It's legitimate to get a NotifyOptionalBreakPosition call after we've detected an overrun, but aFits must be false.

Comment 5

[Mike Schroepfer]

Block p2 since I'd hope we'd squash the layout/text changes before B3..

Comment 6

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

I don't think that's going to happen unfortunately.

Comment 7

[Mike Schroepfer]

(In reply to comment #6)
> I don't think that's going to happen unfortunately.
> 

I can dream right :-)?  Please adjust if you think P2 is not right...

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 291856 [details] [diff] [review]
fix

r+sr=dbaron

Comment 9

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

I'm not sure if we should take this for beta2. David, what do you think?

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't have strong feelings either way; you probably have a better handle on the bugs that have been filed.

Comment 11

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

Comment on attachment 291856 [details] [diff] [review]
fix

Let's do it. This is a tough call but I think the risk is low and I'm afraid that this bug could be common.

Comment 12

[Mike Schroepfer]

Comment on attachment 291856 [details] [diff] [review]
fix

k - if you think it's good let's do it roc

Comment 13

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

checked in

Comment 14

[Carsten Book [:Tomcat]]

verified fixed using the testcase from roc and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050621 Firefox/3.0pre