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).
Created attachment 12688 [details] Screenshot of overlapping text
Created attachment 12689 [details] Test file to validate Use this file in Step 3 of "Steps to reproduce".
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.
Created attachment 12691 [details] Real test file to validate Use this file in Step 3 of "Steps to reproduce", not Attachment 12688 [details].
(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).
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.
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 <style> and <span style="white-space: pre;">type="text/javascript"</span> for <script>. </div>
(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.
Created attachment 12719 [details] Layout test case
Created attachment 12720 [details] Patch v1 Proposed patch.
(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
(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
Comment on attachment 12720 [details] Patch v1 This looks suspicious to me. I'll need to study it some more.
(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.
Why does this only happen when text-align is justify?
(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]).
Comment on attachment 12720 [details] Patch v1 + autoWrapWasEverTrueOnLine = autoWrapWasEverTrueOnLine || autoWrap; You can write that as ||= if you like. Looks fine, r=me
(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.
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.
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);
Committed revision 19206.