Changeset 19621 in webkit

Timestamp:

Feb 14, 2007 6:10:31 AM (17 years ago)

Author:

weinig

Message:

LayoutTests:

Reviewed by Hyatt.

• test for ​http://bugs.webkit.org/show_bug.cgi?id=12726 REGRESSION (r12073): Text wraps in the middle of a word instead of wrapping at the space before the word

• fast/text/break-word-expected.checksum: Added.
• fast/text/break-word-expected.png: Added.
• fast/text/break-word-expected.txt: Added.
• fast/text/break-word.html: Added.

WebCore:

Reviewed by Hyatt.

• fix ​http://bugs.webkit.org/show_bug.cgi?id=12726 REGRESSION (r12073): Text wraps in the middle of a word instead of wrapping at the space before the word

Test: fast/text/break-word.html

The wrapW variable used to keep track of the width of the characters scanned
so far by adding up the widths of individual characters. Because of the
rounding hack, the total ended up being bigger than the width of the same characters
when measured together as a single run.

The fix is to use wrapW only as an upper bound, and once it overflows the line's width,
fall back on measuring everything from the beginning of the line as one run.

• rendering/bidi.cpp: (WebCore::RenderBlock::findNextLineBreak): Implemented the above fix, including not measuring additional single characters once wrapW overflows the line. Also moved the assignment to breakNBSP out of the loop since it is constant for the entire text object, made breakWords and midWordBreak update only when they might change, and cleaned up a few things.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/text/break-word-expected.checksum (added)
• LayoutTests/fast/text/break-word-expected.png (added)
• LayoutTests/fast/text/break-word-expected.txt (added)
• LayoutTests/fast/text/break-word.html (added)
• WebCore/ChangeLog (modified) (1 diff)
• WebCore/rendering/bidi.cpp (modified) (12 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2007-02-14  Mitz Pettel  <mitz@webkit.org>
+
+        Reviewed by Hyatt.
+
+        - test for http://bugs.webkit.org/show_bug.cgi?id=12726
+          REGRESSION (r12073): Text wraps in the middle of a word instead of wrapping at the space before the word
+
+        * fast/text/break-word-expected.checksum: Added.
+        * fast/text/break-word-expected.png: Added.
+        * fast/text/break-word-expected.txt: Added.
+        * fast/text/break-word.html: Added.
+
 2007-02-14  Antti Koivisto  <antti@apple.com>
 

trunk/WebCore/ChangeLog

@@ +1 @@
+2007-02-14  Mitz Pettel  <mitz@webkit.org>
+
+        Reviewed by Hyatt.
+
+        - fix http://bugs.webkit.org/show_bug.cgi?id=12726
+          REGRESSION (r12073): Text wraps in the middle of a word instead of wrapping at the space before the word
+
+        Test: fast/text/break-word.html
+
+        The wrapW variable used to keep track of the width of the characters scanned
+        so far by adding up the widths of individual characters. Because of the
+        rounding hack, the total ended up being bigger than the width of the same characters
+        when measured together as a single run.
+
+        The fix is to use wrapW only as an upper bound, and once it overflows the line's width,
+        fall back on measuring everything from the beginning of the line as one run.
+
+        * rendering/bidi.cpp:
+        (WebCore::RenderBlock::findNextLineBreak): Implemented the above fix, including not measuring
+        additional single characters once wrapW overflows the line. Also moved the assignment
+        to breakNBSP out of the loop since it is constant for the entire text object, made breakWords and
+        midWordBreak update only when they might change, and cleaned up a few things.
+
 2007-02-14  Antti Koivisto  <antti@apple.com>
 

trunk/WebCore/rendering/bidi.cpp

@@ -2215 +2215 @@
                 tmpW += o->width() + o->marginLeft() + o->marginRight() + inlineWidth(o);
         } else if (o->isText()) {
-            RenderText *t = static_cast<RenderText *>(o);
+            RenderText* t = static_cast<RenderText*>(o);
             int strlen = t->textLength();
             int len = strlen - pos;
@@ -2221 +2221 @@
 
             const Font& f = t->style(m_firstLine)->font();
-            // proportional font, needs a bit more work.
+
             int lastSpace = pos;
             int wordSpacing = o->style()->wordSpacing();
@@ -2230 +2230 @@
             int wrapW = tmpW + inlineWidth(o, !appliedStartWidth, true);
             int nextBreakable = -1;
+            bool breakNBSP = autoWrap && o->style()->nbspMode() == SPACE;
+            // Auto-wrapping text should wrap in the middle of a word only if it could not wrap before the word,
+            // which is only possible if the word is the first thing on the line, that is, if |w| is zero.
+            bool breakWords = o->style()->wordWrap() == BREAK_WORD && ((autoWrap && !w) || currWS == PRE);
+            bool midWordBreak = false;
             
             while (len) {
@@ -2246 +2251 @@
                         BidiIterator endMid;
                         if (pos > 0)
-                            endMid = BidiIterator(0, o, pos-1);
+                            endMid = BidiIterator(0, o, pos - 1);
                         else
-                            endMid = BidiIterator(0, previous, previous->isText() ? static_cast<RenderText *>(previous)->textLength() - 1 : 0);
+                            endMid = BidiIterator(0, previous, previous->isText() ? static_cast<RenderText*>(previous)->textLength() - 1 : 0);
                         // Two consecutive soft hyphens. Avoid overlapping midpoints.
                         if (sNumMidpoints && smidpoints->at(sNumMidpoints - 1).obj == endMid.obj && smidpoints->at(sNumMidpoints - 1).pos > endMid.pos)
@@ -2256 +2261 @@
                         
                         // Add the width up to but not including the hyphen.
-                        tmpW += t->width(lastSpace, pos - lastSpace, f, w+tmpW) + lastSpaceWordSpacing;
+                        tmpW += t->width(lastSpace, pos - lastSpace, f, w + tmpW) + lastSpaceWordSpacing;
                         
                         // For wrapping text only, include the hyphen.  We need to ensure it will fit
                         // on the line if it shows when we break.
                         if (autoWrap)
-                            tmpW += t->width(pos, 1, f, w+tmpW);
+                            tmpW += t->width(pos, 1, f, w + tmpW);
                         
-                        BidiIterator startMid(0, o, pos+1);
+                        BidiIterator startMid(0, o, pos + 1);
                         addMidpoint(startMid);
                     }
@@ -2275 +2280 @@
                 
                 bool applyWordSpacing = false;
-                bool breakNBSP = autoWrap && o->style()->nbspMode() == SPACE;
                 
-                // FIXME: This check looks suspicious. Why does w have to be 0?  
-                bool breakWords = o->style()->wordWrap() == BREAK_WORD && ((autoWrap && w == 0) || currWS == PRE);
-
                 currentCharacterIsWS = currentCharacterIsSpace || (breakNBSP && c == noBreakSpace);
 
-                if (breakWords)
+                if (breakWords && !midWordBreak) {
                     wrapW += t->width(pos, 1, f, w+wrapW);
-                bool midWordBreak = breakWords && (w + wrapW > width);
-
-                if (c == '\n' || (currWS != PRE && isBreakable(str, pos, strlen, nextBreakable, breakNBSP)) || midWordBreak) {
+                    midWordBreak = w + wrapW > width;
+                }
+
+                bool betweenWords = c == '\n' || (currWS != PRE && isBreakable(str, pos, strlen, nextBreakable, breakNBSP));
+
+                if (betweenWords || midWordBreak) {
                     bool stoppedIgnoringSpaces = false;
                     if (ignoringSpaces) {
@@ -2295 +2299 @@
                             lastSpaceWordSpacing = 0;
                             lastSpace = pos; // e.g., "Foo    goo", don't add in any of the ignored spaces.
-                            BidiIterator startMid ( 0, o, pos );
+                            BidiIterator startMid(0, o, pos);
                             addMidpoint(startMid);
                             stoppedIgnoringSpaces = true;
@@ -2315 +2319 @@
                     applyWordSpacing =  wordSpacing && currentCharacterIsSpace && !previousCharacterIsSpace;
 
-                    if (autoWrap && w + tmpW > width && w == 0) {
+                    if (autoWrap && w + tmpW > width && !w) {
                         int fb = nearestFloatBottom(m_height);
                         int newLineWidth = lineWidth(fb);
@@ -2393 +2397 @@
                     }
 
-                    if (autoWrap) {
+                    if (autoWrap && betweenWords) {
                         w += tmpW;
                         tmpW = 0;
                         lBreak.obj = o;
                         lBreak.pos = pos;
+                        // Auto-wrapping text should not wrap in the middle of a word once it has had an
+                        // opportunity to break after a word.
+                        breakWords = false;
                     }
                     
@@ -2405 +2412 @@
                         lBreak.obj = o;
                         lBreak.pos = pos;
+                        midWordBreak &= breakWords;
                     } else {
                         lastSpaceWordSpacing = applyWordSpacing ? wordSpacing : 0;
@@ -2457 +2465 @@
             if (!ignoringSpaces)
                 tmpW += t->width(lastSpace, pos - lastSpace, f, w+tmpW) + lastSpaceWordSpacing;
-            if (!appliedStartWidth)
-                tmpW += inlineWidth(o, true, false);
-            tmpW += inlineWidth(o, false, true);
+            tmpW += inlineWidth(o, !appliedStartWidth, true);
         } else
-            ASSERT( false );
+            ASSERT_NOT_REACHED();
 
         RenderObject* next = bidiNext(start.block, o, bidi);
@@ -2528 +2534 @@
         if (!last->isFloatingOrPositioned() && last->isReplaced() && autoWrap && 
             (!last->isListMarker() || static_cast<RenderListMarker*>(last)->isInside())) {
-            // Go ahead and add in tmpW.
             w += tmpW;
             tmpW = 0;