402950 - whatwg.org homepage line-breaking is off vs. Fx2 and Webkit

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect, P2)

Description

[Robert Sayre]

The text is extending past the boxes on trunk.

Comment 1

[Robert Sayre]

Attachment: reftest.html

Comment 2

[Robert Sayre]

Attachment: reftest.reference.html

Comment 3

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

Attachment: fix

The bug here is that the break opportunity induced by the breakable space is recorded as a "break before" the first character of the second span. But nsLineLayout determines that the second span a) does not fit in the line and b) does not have any break opportunities recorded before it (!aCanRollBackBeforeFrame in CanPlaceFrame). So it just declines to place the span, and that means the break opportunity found in the span is forgotten.

We fix this in two parts: 1) request back-up before we bail out of CanPlaceFrame, to give a second pass over the line a chance to stop at the recorded break opportunity, if any and 2) allow spans to always be placed ... just because some child didn't fit and caused the span to overflow the available width, doesn't mean we should fail to place the span. If we do fail to place the span, we may throw out a break opportunity in the span. This bug and the reftest show examples of this.

The poorness of the fit of the Reflow API to inline layout becomes more and more glaring...

Comment 4

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

I'm having trouble making sense of the comments in the reftest.  (Running off to dinner, haven't looked at anything else.)

Comment 5

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

+<!-- We should be able to break even when the only break opportunity is within a span
+that overflows the line width (the break opportunity after the whitespace is treated
+as a break before the first character of the second span) -->
+<p><span>mmmmmmmmmm</span>
+<span>mmmmm<a>mmmmm</a></span></p>

The whitespace creates a break opportunity at the start of the second span. Without this patch, the span is treated as "fails to fit" in CanPlaceFrame so we back out the break opportunity registered by the span, hence trunk decides that there are no break opportunities and everything ends up on one line.

+<!-- A span that overflows the line width should be allowed to fit, it might have a break
+opportunity inside it (the break opportunity after the whitespace is treated
+as a break before the first character of the second span) -->
+<p>m <span>mmmmmmmmmm</span>
+<span>m<a>mmmmmmmmm</a></span></p>

Similar problem, basically. The break opportunity at the beginning of the second span should be allowed to take effect. This part of the test is just testing whether having an existing break opportunity earlier in the line makes a difference.

Comment 6

[Worcester12345]

Not sure if this is the same bug, but I know I have filed many "text flowing outside the box" types of bugs (this one isn't one, but I haven't figured out the search yet) against the reporter tool. Anyhow, check out: http://reporter.mozilla.org/app/report/?report_id=RMO11860390821711&report_description=text&report_problem_type=6&selected%5B%5D=host_hostname&selected%5B%5D=report_file_date&show=25&product_family=&page=1

Comment 7

[Worcester12345]

P.S. The way I filed a bunch of reporter tool reports was using something like: "text goes outside the box". I imagine if you can get the database to filter on "box", "text", and "*out*"; under either appearance and/or behavior, you'd find a lot of them.

Comment 8

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

So what does it mean to set LL_NEEDBACKUP when you don't have something to roll back to?  It looks like that could cause nsBlockFrame::ReflowInlineFrames to go into an infinite loop.

Comment 9

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

No, because nsBlockFrame::ReflowInlineFrames has

  if (needsBackup) {
    // We need to try backing up to before a text run
    PRInt32 offset;
    nsIContent* breakContent = aLineLayout.GetLastOptionalBreakPosition(&offset);
    // XXX It's possible, in fact not unusual, for the break opportunity to already
    // be the end of the line. We should detect that and optimize to not
    // re-do the line.
    if (breakContent) {
      // We can back up!
      lineReflowStatus = LINE_REFLOW_REDO_NO_PULL;

So if we set LL_NEEDBACKUP and we don't have something to roll back to, we just don't roll back.

Comment 10

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

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

Er, that's in DoReflowInlineFrames, but ok.

r+sr=dbaron

Comment 11

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

checked in