Bug 12423 – Mixing white-space:pre text with non white-space:pre text does not wrap properly

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[David Kilzer (:ddkilzer)]

Summary:

After validating an HTML document, some error descriptions contain a mix of <code> tags in them.  When the Safari window is narrowed horizontally, the text does not wrap properly.

Steps to reproduce:

1. Open Safari/WebKit.
2. Open URL: http://validator.w3.org/
3. Upload test file (will be attached shortly).
4. Narrow width of browser until text starts wrapping in Error #1.

Expected results:

Text with <code> tags should wrap properly.

Actual results:

Text with <code> tags does not wrap properly, but insteads overlaps.

Notes:

Tested with locally-built debug build of WebKit r19151 with Safari 2.0.4 (419.3) on Mac OS X 10.4.8 (8N1037).

[David Kilzer (:ddkilzer)]

Created attachment 12688 [details]
Screenshot of overlapping text

[David Kilzer (:ddkilzer)]

Created attachment 12689 [details]
Test file to validate

Use this file in Step 3 of "Steps to reproduce".

[David Kilzer (:ddkilzer)]

Comment on attachment 12689 [details]
Test file to validate

Oops!  This attachment was for Bug 12407, but it will show the issue as well if validated by URL:

http://bugs.webkit.org/attachment.cgi?id=12689

Scroll down to Error #8, then reduce the width of the browser window.

[David Kilzer (:ddkilzer)]

Created attachment 12691 [details]
Real test file to validate

Use this file in Step 3 of "Steps to reproduce", not Attachment 12688 [details].

[David Kilzer (:ddkilzer)]

(In reply to comment #0)
> Notes:
> 
> Tested with locally-built debug build of WebKit r19151 with Safari 2.0.4
> (419.3) on Mac OS X 10.4.8 (8N1037).

Shipping Safari 2.0.4 (419.3) on Mac OS X 10.4.8 (8N1037) does the same thing, although the effect isn't quite as bad (it wraps before overlapping as much as ToT).

[David Kilzer (:ddkilzer)]

Created attachment 12710 [details]
Reduced test case

Mixing text with white-space:pre and without white-space:pre within a text-align:justify block causes the text to render overlapped (depending on the width of the containing block).

Note that the "width: 372px;" style may be removed from <p>, but then the browser must be resized to view the bug.

[David Kilzer (:ddkilzer)]

I've tried tracing through the rendering code, and found that in RenderBlock::computeHorizontalPositionsForLine(), availableWidth = 372 while totWidth = 509 by the time the code reaches the "Armed with the total width of the line (without justification),..." comment.

It seems that the text with the white-space:pre style is lying about its width during layout somewhere.

Note that Opera 9.10, Firefox 2.0.0.1 and Mac MSIE 5.23 all render this without overlapping text.

I think the root cause is that line breaks are not computed correctly. See this (left-aligned) example:

<div style="width: 350px; background-color: #ffe;">
Typical for type are type="text/css" for &lt;style&gt; and <span style="white-space: pre;">type="text/javascript"</span> for &lt;script&gt;.
</div>

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> I think the root cause is that line breaks are not computed correctly. See this
> (left-aligned) example:

I'm not positive, but the text with the white-space:pre style may be "binding" to a space that precedes it instead of breaking.  I need to trace through RenderBlock::findNextLineBreak() some more to grok it.

[David Kilzer (:ddkilzer)]

Created attachment 12719 [details]
Layout test case

[David Kilzer (:ddkilzer)]

Created attachment 12720 [details]
Patch v1

Proposed patch.

[David Kilzer (:ddkilzer)]

(In reply to comment #10)
> Created an attachment (id=12719) [edit]
> Layout test case

This test case passes on Opera 9.10 and MSIE 6.  Two of the five cases (3rd and 5th) fail to wrap on Firefox 2.0.0.1.  Filed B.M.O. 368458:

https://bugzilla.mozilla.org/show_bug.cgi?id= 368458

[David Kilzer (:ddkilzer)]

(In reply to comment #12)
> This test case passes on Opera 9.10 and MSIE 6.  Two of the five cases (3rd and
> 5th) fail to wrap on Firefox 2.0.0.1.  Filed B.M.O. 368458:

https://bugzilla.mozilla.org/show_bug.cgi?id=368458

[Dave Hyatt]

Comment on attachment 12720 [details]
Patch v1

This looks suspicious to me.  I'll need to study it some more.

[David Kilzer (:ddkilzer)]

(In reply to comment #14)
> (From update of attachment 12720 [details] [edit])
> This looks suspicious to me.  I'll need to study it some more.

I wouldn't expect anything less from my first ever patch to bidi.cpp.  :)

Note that all layout tests passed with this change.

I started with something like this:

    if (!willFitOnLine && !autoWrap && last && last->isText() && last->style()->autoWrap())
        canPlaceOnLine = false;

Then I came up with some more test cases (see Attachment 12719 [details]) that required this concept to be generalized for any text run on the line.  Thus if any text run on the current line had autoWrap set to true, then this text run shouldn't be placed on the current line.   Instead, it should wrap.

Of course, I could be missing something since I'm not real familiar with this code.

[Dave Hyatt]

Why does this only happen when text-align is justify?

[David Kilzer (:ddkilzer)]

(In reply to comment #16)
> Why does this only happen when text-align is justify?

It doesn't just happen when text-align is justify.  That's how I noticed it in the first place (and is what caused the overlapping text).

Mitz's test cases show that in the default case (no text-align specified), the text is drawn outside the bounding box.  See "Layout test case" (Attachment 12719 [details]).

[Darin Adler]

Comment on attachment 12720 [details]
Patch v1

+        autoWrapWasEverTrueOnLine = autoWrapWasEverTrueOnLine || autoWrap;

You can write that as ||= if you like.

Looks fine, r=me

[David Kilzer (:ddkilzer)]

(In reply to comment #18)
> (From update of attachment 12720 [details] [edit])
> +        autoWrapWasEverTrueOnLine = autoWrapWasEverTrueOnLine || autoWrap;
> You can write that as ||= if you like.

I don't think C/C++/ObjC[++] supports the ||= logical assignment operator.  I know Perl does, though.

> Looks fine, r=me

Is Hyatt okay with this patch?  He expressed doubts in previous comments.

[Dave Hyatt]

Something is still bothering me about this... I feel like there's a scenario I could craft where the break point would be incorrect, but since I can't come up with it after thinking about it for a bit, r=me as well.

[David Kilzer (:ddkilzer)]

After looking at this expression again, I think it may be simplified, but I will NOT make this change when committing:

    bool canPlaceOnLine = willFitOnLine || !autoWrap;
    if (!willFitOnLine && !autoWrap && autoWrapWasEverTrueOnLine)
        canPlaceOnLine = false;

Could be simplified to (parens added for visual syntactic sugar):

    bool canPlaceOnLine = willFitOnLine || (!willFitOnLine && !autoWrapWasEverTrueOnLine);

[David Kilzer (:ddkilzer)]

Committed revision 19206.