Bug 13072 - REGRESSION (r15617): white-space: pre-wrap breaks off the last character of a wide word
Summary: REGRESSION (r15617): white-space: pre-wrap breaks off the last character of a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Nobody
URL:
Keywords: HasReduction, Regression
Depends on:
Blocks:
 
Reported: 2007-03-14 12:10 PDT by mitz
Modified: 2007-03-14 18:36 PDT (History)
1 user (show)

See Also:


Attachments
Test case (166 bytes, text/html)
2007-03-14 12:12 PDT, mitz
no flags Details
Patch (36.98 KB, patch)
2007-03-14 14:53 PDT, mitz
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2007-03-14 12:10:13 PDT
[I noticed this when looking at the test case for bug 13071]

The last character of the last word in white-space:pre-wrap that is too wide to fit gets wrapped to the next line. This regressed in <http://trac.webkit.org/projects/webkit/changeset/15617>.
Comment 1 mitz 2007-03-14 12:12:14 PDT
Created attachment 13634 [details]
Test case
Comment 2 mitz 2007-03-14 13:58:14 PDT
This is the change in r15617:

     if (lBreak == start && !lBreak.obj->isBR()) {
         // we just add as much as possible
-        if (style()->whiteSpace() == PRE) {
+        if (style()->preserveNewline()) {
             // FIXME: Don't really understand this case.
             if (pos != 0) {

And this is the explanation from the change log:

+        * rendering/bidi.cpp: (WebCore::RenderBlock::findNextLineBreak): Took a rule
+        that specifically called out the pre whitespace mode and made it work for all
+        the modes that preserve newlines. This makes sure we get a last line box for
+        text after the last "\n" even in cases where there is no <br> afterward.

I'm not sure that rule was about preserving newlines. I don't understand it either, but at least the (pos != 0) case seems wrong even for PRE.

Reverting the above change fixes the present bug and passes all layout tests.

Comment 3 Dave Hyatt 2007-03-14 14:08:28 PDT
Attach a patch and I will r= this.
Comment 4 mitz 2007-03-14 14:53:00 PDT
Created attachment 13638 [details]
Patch
Comment 5 Dave Hyatt 2007-03-14 14:54:23 PDT
Comment on attachment 13638 [details]
Patch

My guess is this was just unintentional cleanup.

r=me
Comment 6 Sam Weinig 2007-03-14 18:36:45 PDT
Landed in r20200.