From f270904496c9273ebcdb6573cf9ed4c8328db535 Mon Sep 17 00:00:00 2001 From: "roc+@cs.cmu.edu" Date: Mon, 3 Mar 2008 00:37:34 -0800 Subject: [PATCH] Bug 392809. Fix word-based caret movement around punctuation and whitespace. r=smontagu,a=beltzner --- layout/generic/nsFrame.cpp | 30 ++++++++-- layout/generic/nsFrame.h | 14 +++-- layout/generic/nsIFrame.h | 18 +++++- layout/generic/nsTextFrameThebes.cpp | 10 ++-- layout/generic/test/test_word_movement.html | 62 ++++++++++++++++++--- 5 files changed, 109 insertions(+), 25 deletions(-) diff --git a/layout/generic/nsFrame.cpp b/layout/generic/nsFrame.cpp index 0f99537467f4..e4b9d9c649eb 100644 --- a/layout/generic/nsFrame.cpp +++ b/layout/generic/nsFrame.cpp @@ -4991,7 +4991,7 @@ nsFrame::PeekOffsetWord(PRBool aForward, PRBool aWordSelectEatSpace, PRBool aIsK if (!aState->mAtStart) { if (aState->mLastCharWasPunctuation) { // We're not punctuation, so this is a punctuation boundary. - if (BreakWordBetweenPunctuation(aForward, aIsKeyboardSelect)) + if (BreakWordBetweenPunctuation(aState, aForward, PR_FALSE, PR_FALSE, aIsKeyboardSelect)) return PR_TRUE; } else { // This is not a punctuation boundary. @@ -5001,7 +5001,9 @@ nsFrame::PeekOffsetWord(PRBool aForward, PRBool aWordSelectEatSpace, PRBool aIsK } // Otherwise skip to the other side and note that we encountered non-whitespace. *aOffset = 1 - startOffset; - aState->Update(PR_FALSE); + aState->Update(PR_FALSE, // not punctuation + PR_FALSE // not whitespace + ); if (!aWordSelectEatSpace) aState->SetSawBeforeType(); } @@ -5009,18 +5011,34 @@ nsFrame::PeekOffsetWord(PRBool aForward, PRBool aWordSelectEatSpace, PRBool aIsK } PRBool -nsFrame::BreakWordBetweenPunctuation(PRBool aAfterPunct, PRBool aIsKeyboardSelect) +nsFrame::BreakWordBetweenPunctuation(const PeekWordState* aState, + PRBool aForward, + PRBool aPunctAfter, PRBool aWhitespaceAfter, + PRBool aIsKeyboardSelect) { + NS_ASSERTION(aPunctAfter != aState->mLastCharWasPunctuation, + "Call this only at punctuation boundaries"); + if (aState->mLastCharWasWhitespace) { + // We always stop between whitespace and punctuation + return PR_TRUE; + } if (!nsContentUtils::GetBoolPref("layout.word_select.stop_at_punctuation")) { - // When this pref is false, we never stop at a punctuation boundary. + // When this pref is false, we never stop at a punctuation boundary unless + // it's after whitespace return PR_FALSE; } if (!aIsKeyboardSelect) { // mouse caret movement (e.g. word selection) always stops at every punctuation boundary return PR_TRUE; } - // keyboard caret movement stops after punctuation, not before it - return aAfterPunct; + PRBool afterPunct = aForward ? aState->mLastCharWasPunctuation : aPunctAfter; + if (!afterPunct) { + // keyboard caret movement only stops after punctuation (in content order) + return PR_FALSE; + } + // Stop only if we've seen some non-punctuation since the last whitespace; + // don't stop after punctuation that follows whitespace. + return aState->mSeenNonPunctuationSinceWhitespace; } NS_IMETHODIMP diff --git a/layout/generic/nsFrame.h b/layout/generic/nsFrame.h index ef54dcb68070..3f4333a8f8b8 100644 --- a/layout/generic/nsFrame.h +++ b/layout/generic/nsFrame.h @@ -251,11 +251,17 @@ public: PRInt32* aOffset, PeekWordState *aState); /** * Check whether we should break at a boundary between punctuation and - * non-punctuation. - * @param aAfterPunct true when this point is logically *after* punctuation. + * non-punctuation. Only call it at a punctuation boundary + * (i.e. exactly one of the previous and next characters are punctuation). + * @param aForward true if we're moving forward in content order + * @param aPunctAfter true if the next character is punctuation + * @param aWhitespaceAfter true if the next character is whitespace */ - PRBool BreakWordBetweenPunctuation(PRBool aAfterPunct, PRBool aIsKeyboardSelect); - + PRBool BreakWordBetweenPunctuation(const PeekWordState* aState, + PRBool aForward, + PRBool aPunctAfter, PRBool aWhitespaceAfter, + PRBool aIsKeyboardSelect); + NS_IMETHOD CheckVisibility(nsPresContext* aContext, PRInt32 aStartIndex, PRInt32 aEndIndex, PRBool aRecurse, PRBool *aFinished, PRBool *_retval); NS_IMETHOD GetOffsets(PRInt32 &aStart, PRInt32 &aEnd) const; diff --git a/layout/generic/nsIFrame.h b/layout/generic/nsIFrame.h index 6502f7921fa7..cd76af591512 100644 --- a/layout/generic/nsIFrame.h +++ b/layout/generic/nsIFrame.h @@ -2191,15 +2191,27 @@ protected: PRPackedBool mSawBeforeType; // true when the last character encountered was punctuation PRPackedBool mLastCharWasPunctuation; + // true when the last character encountered was whitespace + PRPackedBool mLastCharWasWhitespace; + // true when we've seen non-punctuation since the last whitespace + PRPackedBool mSeenNonPunctuationSinceWhitespace; // text that's *before* the current frame when aForward is true, *after* - // the current frame when aForward is false. + // the current frame when aForward is false. Only includes the text + // on the current line. nsAutoString mContext; PeekWordState() : mAtStart(PR_TRUE), mSawBeforeType(PR_FALSE), - mLastCharWasPunctuation(PR_FALSE) {} + mLastCharWasPunctuation(PR_FALSE), mLastCharWasWhitespace(PR_FALSE), + mSeenNonPunctuationSinceWhitespace(PR_FALSE) {} void SetSawBeforeType() { mSawBeforeType = PR_TRUE; } - void Update(PRBool aAfterPunctuation) { + void Update(PRBool aAfterPunctuation, PRBool aAfterWhitespace) { mLastCharWasPunctuation = aAfterPunctuation; + mLastCharWasWhitespace = aAfterWhitespace; + if (aAfterWhitespace) { + mSeenNonPunctuationSinceWhitespace = PR_FALSE; + } else if (!aAfterPunctuation) { + mSeenNonPunctuationSinceWhitespace = PR_TRUE; + } mAtStart = PR_FALSE; } }; diff --git a/layout/generic/nsTextFrameThebes.cpp b/layout/generic/nsTextFrameThebes.cpp index a5184085c401..a7f21ee82390 100644 --- a/layout/generic/nsTextFrameThebes.cpp +++ b/layout/generic/nsTextFrameThebes.cpp @@ -4788,23 +4788,23 @@ nsTextFrame::PeekOffsetWord(PRBool aForward, PRBool aWordSelectEatSpace, PRBool do { PRBool isPunctuation = cIter.IsPunctuation(); - if (aWordSelectEatSpace == cIter.IsWhitespace() && !aState->mSawBeforeType) { + PRBool isWhitespace = cIter.IsWhitespace(); + if (aWordSelectEatSpace == isWhitespace && !aState->mSawBeforeType) { aState->SetSawBeforeType(); - aState->Update(isPunctuation); + aState->Update(isPunctuation, isWhitespace); continue; } // See if we can break before the current cluster if (!aState->mAtStart) { PRBool canBreak = isPunctuation != aState->mLastCharWasPunctuation - ? BreakWordBetweenPunctuation(aForward ? aState->mLastCharWasPunctuation : isPunctuation, - aIsKeyboardSelect) + ? BreakWordBetweenPunctuation(aState, aForward, isPunctuation, isWhitespace, aIsKeyboardSelect) : cIter.HaveWordBreakBefore() && aState->mSawBeforeType; if (canBreak) { *aOffset = cIter.GetBeforeOffset() - mContentOffset; return PR_TRUE; } } - aState->Update(isPunctuation); + aState->Update(isPunctuation, isWhitespace); } while (cIter.NextCluster()); *aOffset = cIter.GetAfterOffset() - mContentOffset; diff --git a/layout/generic/test/test_word_movement.html b/layout/generic/test/test_word_movement.html index 38c7c6ea5219..be9594a94f4e 100644 --- a/layout/generic/test/test_word_movement.html +++ b/layout/generic/test/test_word_movement.html @@ -31,15 +31,19 @@ function getPrefs() { .getBranch("layout.word_select."); } -function setEatSpace(newValue) { - getPrefs().setBoolPref("eat_space_to_next_word", newValue); - eatSpace = newValue; +function setPrefs(eat_space, stop_at_punctuation) { + getPrefs().setBoolPref("eat_space_to_next_word", eat_space); + getPrefs().setBoolPref("stop_at_punctuation", stop_at_punctuation); + eatSpace = eat_space; } -function restoreEatSpace() { +function restorePrefs() { try { getPrefs().clearUserPref("eat_space_to_next_word"); } catch(ex) {} + try { + getPrefs().clearUserPref("stop_at_punctuation"); + } catch(ex) {} } function test() { @@ -69,7 +73,7 @@ function test() { var afterEditorNode = document.getElementById("catch").firstChild; - setEatSpace(false); + setPrefs(false, true); editor.innerHTML = "Hello Kitty"; sel.collapse(editor.firstChild, 0); @@ -103,7 +107,29 @@ function test() { testLeft(editor.firstChild.firstChild, 4); testLeft(editor.firstChild.firstChild, 0); - setEatSpace(true); + editor.innerHTML = "http://www.mozilla.org"; + sel.collapse(editor.firstChild, 0); + testRight(editor.firstChild, 7); + testRight(editor.firstChild, 11); + testRight(editor.firstChild, 19); + testLeft(editor.firstChild, 11); + testLeft(editor.firstChild, 7); + testLeft(editor.firstChild, 0); + + editor.innerHTML = "Set .rc to 'quiz'"; + sel.collapse(editor.firstChild, 0); + testRight(editor.firstChild, 3); + testRight(editor.firstChild, 7); + testRight(editor.firstChild, 10); + testRight(editor.firstChild.nextSibling.nextSibling, 5); + testLeft(editor.firstChild.nextSibling.firstChild, 1); + testLeft(editor.firstChild, 10); + testLeft(editor.firstChild, 8); + testLeft(editor.firstChild, 5); + testLeft(editor.firstChild, 3); + testLeft(editor.firstChild, 0); + + setPrefs(true, true); // test basic word movement with eat_space_next_to_word true. @@ -137,7 +163,29 @@ function test() { testLeft(editor.firstChild.firstChild, 4); testLeft(editor.firstChild.firstChild, 0); - restoreEatSpace(); + editor.innerHTML = "http://www.mozilla.org"; + sel.collapse(editor.firstChild, 0); + testRight(editor.firstChild, 7); + testRight(editor.firstChild, 11); + testRight(editor.firstChild, 19); + testLeft(editor.firstChild, 11); + testLeft(editor.firstChild, 7); + testLeft(editor.firstChild, 0); + + editor.innerHTML = "Set .rc to 'quiz'"; + sel.collapse(editor.firstChild, 0); + testRight(editor.firstChild, 4); + testRight(editor.firstChild, 8); + testRight(editor.firstChild.nextSibling.firstChild, 0); + testRight(afterEditorNode, 0); + testLeft(editor.firstChild.nextSibling.firstChild, 1); + testLeft(editor.firstChild, 10); + testLeft(editor.firstChild, 8); + testLeft(editor.firstChild, 5); + testLeft(editor.firstChild, 3); + testLeft(editor.firstChild, 0); + + restorePrefs(); SimpleTest.finish(); }