405577 - REGRESSION: white-space:nowrap; not honored for a series of inline elements if elements wider than viewport

Status: RESOLVED FIXED

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

Description

[bosspro]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 1.0.3705; .NET CLR 1.1.4322; Tablet PC 1.7; .NET CLR 2.0.50727; .NET CLR 3.0.04506.30)
Build Identifier: Mozilla/5.0 (X11;U;Linux i686; en-US; rv:1.9b2pre) Gecko/2007112604 Minefield/3.0b2pre

Inline elements with the white-space:nowrap; (class="toolitem" in example) CSS property may be broken apart if the inline elements take up more width than the viewport.

Reproducible: Always

Steps to Reproduce:
1.  Go to http://bosspro.phenominet.com/domain/portal/gfcprimer/ff3test.html
2.  Ensure the viewport is smaller than the elements displayed in the page.
3.  Inline elements with the white-space:nowrap; (class="toolitem" in example) CSS property may be broken apart, when they should have been wrapped to the next line.
Actual Results:  
Inline elements with the white-space:nowrap; (class="toolitem" in example) CSS property may be broken apart.

Expected Results:  
Inline elements with the white-space:nowrap; property should not be broken apart.

This originated somewhere between Firefox 3 alpha 4 and alpha 5, and it remains in the trunk.

Comment 1

[bosspro]

Attachment: Image demonstrating problem

Comment 2

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

Will probably be fixed by bug 403426.

Comment 3

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

Now that 403426 is checked in, this exhibits a different bug where the content isn't breaking where it should so it overflows the viewport vertically. I'll look into it.

Comment 4

[jason.thurston]

I posted this description and test code under bug 403426, but it might actually be this bug.  I'm not really sure.

It seems if there is any room, even one pixel, at the end of a "nowrap" text block then the entire next nowrap text block will be
placed outside the surrounding block on the same row and only if the current
nowrap text block didn't completely fit in the row will the next nowrap text
block be started on the following row.  The behavior I would expect is that
when the next nowrap block can not fit on the same row as the current one then
it is moved to a new row.  Below is my new test code to demonstrate this.

<div style="height: 100%;width: 75px; background:red">
     <span style="font-size: 7pt;white-space: normal">
          <span style="white-space: nowrap;"><input name="f2" value="2"
checked="checked" type="checkbox"/>
                &nbsp;1-aaaaaaaaaa
          </span>
          <!-- should wrap here but doesn't in ff3-->
          <span style="white-space: nowrap;"><input name="f2" value="2"
checked="checked" type="checkbox"/>
                &nbsp;2-aaaaaaaaaaa
          </span>
          <!-- should wrap here but doesn't in ff3-->
          <span style="white-space: nowrap;"><input name="f2" value="2"
checked="checked" type="checkbox"/>
                &nbsp;3-aaaaaaaaaaaaa
          </span>
          <!-- should wrap here but doesn't in ff3-->
          <span style="white-space: nowrap;"><input name="f2" value="2"
checked="checked" type="checkbox"/>
                &nbsp;4-aaaaaaaaaaaaaa
          </span>
          <!-- should wrap here but doesn't in ff3-->
     </span>
</div>

Comment 5

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

Bumping to P1 since it's a fairly major regression

Comment 6

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

The main problem is in the new code that detects a trailing break opportunity for a textframe:

    if (textMetrics.mAdvanceWidth - trimmableWidth > availWidth) {
      breakAfter = PR_TRUE;
    } else {
      lineLayout.NotifyOptionalBreakPosition(mContent, offset + length, PR_TRUE);
    }

The problem is, if we're already past the right edge of the available space, then availWidth is zero, and in this case textMetrics.mAdvanceWidth - trimmableWidth is also zero because the frame collapses away. So we just notify, we never take the opportunity ... even if there are many such textframes interspersed with other non-breakable content on the line.

It might be enough to just change that > to an >=. Normally we would try to pack as much zero-width stuff on the line as possible, but here because we have a break opportunity at the end of a text run, it's probably OK to just break --- there can't be any more zero-width text following us. There could be zero-width replaced content but that's OK to bump to the next line, I think. I'll think about this some more on the way home.

Comment 7

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

Attachment: fix

Two parts to this fix. First the easy part in nsTextFrame: in bug 403426 we added support for signalling a potential break at the end of a text run/linebreaker run. But we didn't add support for actually breaking there in the second pass when a forced break was indicated. The code
  if (forceBreak >= offset + length) {
       forceBreak = -1;
would fire so we'd treat the break as not in our text frame. So this patch sets breakAfter to true when forceBreak == offset + length so we return BREAK_AFTER status. That helps a lot.

Then the hard part that I mentioned above. When the text frame has a break opportunity at its end and we are already beyond the available width, it should break-after. But we fail to detect that because availableWidth is zero and we have zero width so we let ourselves fit. Basically we have no way to distinguish "we are beyond the right edge" from "we are at the right edge". I'm fixing that by allowing nsInlineFrame to make availableWidth negative! That is actually the cleanest way I can think of to make this distinction. Removing that one line is enough to fix the rest of the issues here. This is a scary change, but it should only affect inline frames really --- nsInlineFrame, nsTextFrame, nsFirstLetterFrame. I had a look at the uses of availableWidth there and it seems like they'll cope fine.

This was added in bug 169620. I checked there and all the testcases (such as they are) continue to work fine. That one line I'm removing is actually the only availableWidth-related change in that patch.

Comment 8

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

David, we could land this for beta2 if you review it today. Otherwise I think it should slip.

Comment 9

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

This could cause a negative width to end up in nsHTMLReflowState::availableWidth through nsLineLayout::ReflowFrame, right?  That seems scary, particularly for inline-blockish or replaced boxes.  Do you know where the negative psd->mRightEdge - psd->mLeftEdge needs to propagate to in order to fix what you need?  Are there other places you could add a PR_MAX?

Comment 10

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

> This could cause a negative width to end up in
> nsHTMLReflowState::availableWidth through nsLineLayout::ReflowFrame, right? 

I don't think so, based on
  // Inline-ish and text-ish things don't compute their width;
  // everything else does.  We need to give them an available width that
  // reflects the space left on the line.
  NS_ASSERTION(psd->mRightEdge != NS_UNCONSTRAINEDSIZE,
               "shouldn't have unconstrained widths anymore");
  if (reflowState.ComputedWidth() == NS_UNCONSTRAINEDSIZE)
    reflowState.availableWidth = psd->mRightEdge - psd->mX;

inline-blockish and replaced-ish things shouldn't execute this line here. Their available width is being set to mBlockReflowState->ComputedWidth() via

  nsSize availSize(mBlockReflowState->ComputedWidth(), NS_UNCONSTRAINEDSIZE);

  // Setup reflow state for reflowing the frame
  nsHTMLReflowState reflowState(mPresContext, *psd->mReflowState,
                                aFrame, availSize);

right? It could definitely use an additional comment...

We could avoid setting a negative available width by instead giving PerSpanData a flag saying "starts beyond available width" and setting that via an extra parameter to BeginSpan. But that flag would need to be propagated around, and might not even be enough if there's negative-offset stuff that might move us back inside the available width. Plus we'd have to check the flag in various places that currently test the available width. All in all it seems simpler to allow available width to go negative at least for eLineParticipants.

Comment 11

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

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

Oops, sorry, I misread that bit of ReflowFrame.

r+sr=dbaron

Comment 12

[Karl Tomlinson (:karlt)]

I've disabled the reftest because of failure on linux.

Comment 13

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

I *think* it's just a problem with the test but I'm going to check and write a better test.

Comment 14

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

I tweaked the test and checked it back in. In the process, though, I found another line breaking bug! I'll file it.