Bug 391584. Don't miss word-break opportunities at points where whitespace has collapsed away. Also, note that a word-break opportunity exists at the start of a frame when it starts with a space.

layout/generic/nsTextFrameThebes.cpp

@@ -4408,7 +4408,7 @@ public:
   PRBool NextCluster();
   PRBool IsWhitespace();
   PRBool IsPunctuation();
-  PRBool HaveWordBreakBefore();
+  PRBool HaveWordBreakBefore() { return mHaveWordBreak; }
   PRInt32 GetAfterOffset();
   PRInt32 GetBeforeOffset();
 
@@ -4421,6 +4421,7 @@ private:
   PRInt32                     mCharIndex;
   nsTextFrame::TrimmedOffsets mTrimmed;
   nsTArray<PRPackedBool>      mWordBreaks;
+  PRPackedBool                mHaveWordBreak;
 };
 
 PRBool
@@ -4493,12 +4494,6 @@ ClusterIterator::IsPunctuation()
   return c == nsIUGenCategory::kPunctuation || c == nsIUGenCategory::kSymbol;
 }
 
-PRBool
-ClusterIterator::HaveWordBreakBefore()
-{
-  return mWordBreaks[GetBeforeOffset() - mTextFrame->GetContentOffset()];
-}
-
 PRInt32
 ClusterIterator::GetBeforeOffset()
 {
@@ -4520,30 +4515,32 @@ ClusterIterator::NextCluster()
     return PR_FALSE;
   gfxTextRun* textRun = mTextFrame->GetTextRun();
 
+  mHaveWordBreak = PR_FALSE;
   while (PR_TRUE) {
+    PRBool keepGoing = PR_FALSE;
     if (mDirection > 0) {
       if (mIterator.GetOriginalOffset() >= mTrimmed.GetEnd())
         return PR_FALSE;
-      if (mIterator.IsOriginalCharSkipped() ||
+      keepGoing = mIterator.IsOriginalCharSkipped() ||
           mIterator.GetOriginalOffset() < mTrimmed.mStart ||
-          !textRun->IsClusterStart(mIterator.GetSkippedOffset())) {
-        mIterator.AdvanceOriginal(1);
-        continue;
-      }
+          !textRun->IsClusterStart(mIterator.GetSkippedOffset());
       mCharIndex = mIterator.GetOriginalOffset();
       mIterator.AdvanceOriginal(1);
     } else {
       if (mIterator.GetOriginalOffset() <= mTrimmed.mStart)
         return PR_FALSE;
       mIterator.AdvanceOriginal(-1);
-      if (mIterator.IsOriginalCharSkipped() ||
+      keepGoing = mIterator.IsOriginalCharSkipped() ||
           mIterator.GetOriginalOffset() >= mTrimmed.GetEnd() ||
-          !textRun->IsClusterStart(mIterator.GetSkippedOffset()))
-        continue;
+          !textRun->IsClusterStart(mIterator.GetSkippedOffset());
       mCharIndex = mIterator.GetOriginalOffset();
     }
 
-    return PR_TRUE;
+    if (mWordBreaks[GetBeforeOffset() - mTextFrame->GetContentOffset()]) {
+      mHaveWordBreak = PR_TRUE;
+    }
+    if (!keepGoing)
+      return PR_TRUE;
   }
 }
 
@@ -4563,6 +4560,7 @@ ClusterIterator::ClusterIterator(nsTextFrame* aTextFrame, PRInt32 aPosition,
   mFrag = aTextFrame->GetContent()->GetText();
   mTrimmed = aTextFrame->GetTrimmedOffsets(mFrag, PR_TRUE);
 
+  PRInt32 textOffset = aTextFrame->GetContentOffset();
   PRInt32 textLen = aTextFrame->GetContentLength();
   if (!mWordBreaks.AppendElements(textLen + 1)) {
     mDirection = 0; // signal failure
@@ -4570,7 +4568,7 @@ ClusterIterator::ClusterIterator(nsTextFrame* aTextFrame, PRInt32 aPosition,
   }
   memset(mWordBreaks.Elements(), PR_FALSE, textLen + 1);
   nsAutoString text;
-  mFrag->AppendTo(text, aTextFrame->GetContentOffset(), textLen);
+  mFrag->AppendTo(text, textOffset, textLen);
   nsIWordBreaker* wordBreaker = nsContentUtils::WordBreaker();
   PRInt32 i = 0;
   while ((i = wordBreaker->NextWord(text.get(), textLen, i)) >= 0) {
@@ -4578,7 +4576,13 @@ ClusterIterator::ClusterIterator(nsTextFrame* aTextFrame, PRInt32 aPosition,
   }
   // XXX this never allows word breaks at the start or end of the frame, but to fix
   // this we would need to rewrite word-break detection to use the text from
-  // textruns or something. Not a regression, at least.
+  // textruns or something. Not a regression, at least. For now we can make things
+  // a little better by noting a word break opportunity when the frame starts
+  // or ends with white-space.
+  if (textLen > 0) {
+    mWordBreaks[0] = IsSelectionSpace(mFrag, textOffset);
+    mWordBreaks[textLen] = IsSelectionSpace(mFrag, textOffset + textLen - 1);
+  }
 }
 
 PRBool
@@ -5404,10 +5408,10 @@ nsTextFrame::TrimTrailingWhiteSpace(nsPresContext* aPresContext,
   const nsTextFragment* frag = mContent->GetText();
   TrimmedOffsets trimmed = GetTrimmedOffsets(frag, PR_TRUE);
   gfxSkipCharsIterator iter = start;
-  PRUint32 trimmedEnd = iter.ConvertOriginalToSkipped(trimmed.GetEnd());
   const nsStyleText* textStyle = GetStyleText();
   gfxFloat delta = 0;
-
+  PRUint32 trimmedEnd = iter.ConvertOriginalToSkipped(trimmed.GetEnd());
+  
   if (GetStateBits() & TEXT_TRIMMED_TRAILING_WHITESPACE) {
     aLastCharIsJustifiable = PR_TRUE;
   } else if (trimmed.GetEnd() < GetContentEnd()) {

layout/generic/test/Makefile.in

@@ -55,6 +55,7 @@ _TEST_FILES = 	test_bug323656.html \
 		test_bug392923.html \
 		test_bug394173.html \
 		test_bug394239.html \
+		test_word_movement.html \
 		$(NULL)
 
 libs:: $(_TEST_FILES)

layout/generic/test/test_word_movement.html

@@ -22,7 +22,26 @@ SimpleTest.waitForExplicitFinish();
 // This seems to be necessary because the selection is not set up properly otherwise
 setTimeout(test, 0);
 
+function getPrefs() {
+  const prefSvcContractID = "@mozilla.org/preferences-service;1";
+  const prefSvcIID = Components.interfaces.nsIPrefService;
+  return Components.classes[prefSvcContractID].getService(prefSvcIID)
+                                               .getBranch("layout.word_select.");
+}
+
+function setEatSpace(newValue) {
+  getPrefs().setBoolPref("eat_space_to_next_word", newValue);
+}
+
+function restoreEatSpace() {
+  try {
+    getPrefs().clearUserPref("eat_space_to_next_word");
+  } catch(ex) {}
+}
+
 function test() {
+  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
+
   var wordModifiers =
       (navigator.platform.indexOf("Mac") >= 0) ? {altKey:true} : {ctrlKey:true};
   var sel = window.getSelection();
@@ -40,6 +59,8 @@ function test() {
     is(sel.anchorOffset, offset, "Left movement broken in " + editor.innerHTML);
   }
 
+  setEatSpace(false);
+
   editor.innerHTML = "Hello Kitty";
   sel.collapse(editor.firstChild, 0);
   testRight(editor.firstChild, 5);
@@ -72,6 +93,8 @@ function test() {
   testLeft(editor.firstChild.firstChild, 4);
   testLeft(editor.firstChild.firstChild, 0);
 
+  restoreEatSpace();
+
   SimpleTest.finish();
 }