Make sure the last selection gets spell checked when focusing different frame.

Make sure the last selection gets spell checked when focusing different frame.

BUG=264742
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155725

[pstanek, 2013-07-30 22:08:53]

In the end I based on on master this is why isSelectionInTextField() is
duplicated here and in 21024004. I hope this is ok.

[please use gerrit instead, 2013-07-30 22:12:14]

On 2013/07/30 22:08:53, pstanek wrote:
> In the end I based on on master this is why isSelectionInTextField() is
> duplicated here and in 21024004. I hope this is ok.

Let's land 21024004 first, then we can start reviewing the rest of the CLs.

[pstanek, 2013-07-30 22:14:12]

On 2013/07/30 22:12:14, Rouslan Solomakhin wrote:
> On 2013/07/30 22:08:53, pstanek wrote:
> > In the end I based on on master this is why isSelectionInTextField() is
> > duplicated here and in 21024004. I hope this is ok.
> 
> Let's land 21024004 first, then we can start reviewing the rest of the CLs.

Ok. Will wait with uploading the last one too, then.

[tony, 2013-08-02 20:35:03]

https://codereview.chromium.org/21235009/diff/6001/LayoutTests/editing/spelli...
File LayoutTests/editing/spelling/focusing-other-frame.html (right):

https://codereview.chromium.org/21235009/diff/6001/LayoutTests/editing/spelli...
LayoutTests/editing/spelling/focusing-other-frame.html:31: alert("Automatic
testing impossible.");
This should say that the test requires content_shell and provide instructions on
how to run the test manually (or say it can't be done manually).

https://codereview.chromium.org/21235009/diff/6001/Source/core/editing/Editor...
File Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/21235009/diff/6001/Source/core/editing/Editor...
Source/core/editing/Editor.cpp:2301: if (!activeAndFocused) {
Nit: It's a little easier to read if you early return if (activeAndFocused). One
less level of indent.

https://codereview.chromium.org/21235009/diff/6001/Source/core/editing/Editor...
Source/core/editing/Editor.cpp:2311: // by activating different frame.
This is a 'what' comment, which we normally avoid in blink code.  Instead, you
could make a helper function like, 'spellCheckDuringBlur' or something.  I would
probably just rename Editor::respondToChangedFocusedOrActiveState to
Editor::spellCheckAfterBlur and do the !activeAndFocused in FrameSelection.

https://codereview.chromium.org/21235009/diff/6001/Source/core/editing/Editor...
Source/core/editing/Editor.cpp:2312: VisiblePosition
start(m_frame->selection()->selection().visibleStart());
This code seems similar to the code in respondToChangedSelection.  Is it
possible to share some code between the two?

https://codereview.chromium.org/21235009/diff/6001/Source/core/editing/Editor...
Source/core/editing/Editor.cpp:2316:
markMisspellingsAndBadGrammar(adjacentWords, true, selectedSentence);
A good cleanup change would be converting the bool into an enum.

[please use gerrit instead, 2013-08-02 20:41:36]

https://codereview.chromium.org/21235009/diff/6001/LayoutTests/editing/spelli...
File LayoutTests/editing/spelling/focusing-other-frame.html (right):

https://codereview.chromium.org/21235009/diff/6001/LayoutTests/editing/spelli...
LayoutTests/editing/spelling/focusing-other-frame.html:31: alert("Automatic
testing impossible.");
In addition, it would be better to use log() function from resources/util.js.
(That would require a <div id="console"></div> element, by the way.)

[pstanek, 2013-08-04 00:00:06]

https://codereview.chromium.org/21235009/diff/6001/Source/core/editing/Editor...
File Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/21235009/diff/6001/Source/core/editing/Editor...
Source/core/editing/Editor.cpp:2316:
markMisspellingsAndBadGrammar(adjacentWords, true, selectedSentence);
On 2013/08/02 20:35:03, tony wrote:
> A good cleanup change would be converting the bool into an enum.

Sure but note I'm not doing this within this bug fix.

[pstanek, 2013-08-07 17:48:33]

Tony please take a look at this in a spare moment. Thanks in advance.

[tony, 2013-08-07 18:21:56]

LGTM!

[commit-bot: I haz the power, 2013-08-07 18:22:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pstanek@opera.com/21235009/21001

[commit-bot: I haz the power, 2013-08-07 18:22:22]

Failed to apply patch for Source/core/editing/Editor.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/editing/Editor.cpp
  Hunk #1 FAILED at 62.
  Hunk #2 succeeded at 98 with fuzz 2 (offset 11 lines).
  Hunk #3 FAILED at 2263.
  Hunk #4 succeeded at 2333 (offset 38 lines).
  2 out of 4 hunks FAILED -- saving rejects to file
Source/core/editing/Editor.cpp.rej

Patch:       Source/core/editing/Editor.cpp
Index: Source/core/editing/Editor.cpp
diff --git a/Source/core/editing/Editor.cpp b/Source/core/editing/Editor.cpp
index
9bbc1f14ce565e3967f4f96271cb57016a02d0d6..3e6f9637ba895765cf5616f7cbefb2dffee60ac3
100644
--- a/Source/core/editing/Editor.cpp
+++ b/Source/core/editing/Editor.cpp
@@ -62,6 +62,7 @@
 #include "core/editing/htmlediting.h"
 #include "core/editing/markup.h"
 #include "core/html/HTMLImageElement.h"
+#include "core/html/HTMLInputElement.h"
 #include "core/html/HTMLTextAreaElement.h"
 #include "core/loader/cache/ResourceFetcher.h"
 #include "core/page/EditorClient.h"
@@ -87,6 +88,16 @@ using namespace HTMLNames;
 using namespace WTF;
 using namespace Unicode;
 
+namespace {
+
+bool isSelectionInTextField(const VisibleSelection& selection)
+{
+    HTMLTextFormControlElement* textControl =
enclosingTextFormControl(selection.start());
+    return textControl && textControl->hasTagName(inputTag) &&
toHTMLInputElement(textControl)->isTextField();
+}
+
+} // namespace
+
 // When an event handler has moved the selection outside of a text control
 // we should use the target control's selection for this editing operation.
 VisibleSelection Editor::selectionForCommand(Event* event)
@@ -2253,15 +2264,7 @@ void Editor::respondToChangedSelection(const
VisibleSelection& oldSelection, Fra
         // If this is a change in selection resulting from a delete operation,
         // oldSelection may no longer be in the document.
         if (shouldCheckSpellingAndGrammar && closeTyping &&
oldSelection.isContentEditable() && oldSelection.start().deprecatedNode() &&
oldSelection.start().anchorNode()->inDocument()) {
-            VisiblePosition oldStart(oldSelection.visibleStart());
-            VisibleSelection oldAdjacentWords =
VisibleSelection(startOfWord(oldStart, LeftWordIfOnBoundary),
endOfWord(oldStart, RightWordIfOnBoundary));
-            if (oldAdjacentWords != newAdjacentWords) {
-                if (isContinuousGrammarCheckingEnabled) {
-                    VisibleSelection oldSelectedSentence =
VisibleSelection(startOfSentence(oldStart), endOfSentence(oldStart));
-                    markMisspellingsAndBadGrammar(oldAdjacentWords,
oldSelectedSentence != newSelectedSentence, oldSelectedSentence);
-                } else
-                    markMisspellingsAndBadGrammar(oldAdjacentWords, false,
oldAdjacentWords);
-            }
+            spellCheckOldSelection(oldSelection, newAdjacentWords,
newSelectedSentence);
         }
 
         if (!textChecker() ||
textChecker()->shouldEraseMarkersAfterChangeSelection(TextCheckingTypeSpelling))
{
@@ -2285,6 +2288,34 @@ void Editor::respondToChangedSelection(const
VisibleSelection& oldSelection, Fra
     notifyComponentsOnChangedSelection(oldSelection, options);
 }
 
+void Editor::spellCheckAfterBlur()
+{
+    if (!m_frame->selection()->selection().isContentEditable())
+        return;
+
+    if (isSelectionInTextField(m_frame->selection()->selection())) {
+        // textFieldDidEndEditing() and textFieldDidBeginEditing() handle this.
+        return;
+    }
+
+    VisibleSelection empty;
+    spellCheckOldSelection(m_frame->selection()->selection(), empty, empty);
+}
+
+void Editor::spellCheckOldSelection(const VisibleSelection& oldSelection, const
VisibleSelection& newAdjacentWords, const VisibleSelection& newSelectedSentence)
+{
+    VisiblePosition oldStart(oldSelection.visibleStart());
+    VisibleSelection oldAdjacentWords = VisibleSelection(startOfWord(oldStart,
LeftWordIfOnBoundary), endOfWord(oldStart, RightWordIfOnBoundary));
+    if (oldAdjacentWords  != newAdjacentWords) {
+        if (isContinuousSpellCheckingEnabled() && isGrammarCheckingEnabled()) {
+            VisibleSelection selectedSentence =
VisibleSelection(startOfSentence(oldStart), endOfSentence(oldStart));
+            markMisspellingsAndBadGrammar(oldAdjacentWords, selectedSentence !=
newSelectedSentence, selectedSentence);
+        } else {
+            markMisspellingsAndBadGrammar(oldAdjacentWords, false,
oldAdjacentWords);
+        }
+    }
+}
+
 static Node* findFirstMarkable(Node* node)
 {
     while (node) {

[tony, 2013-08-07 18:23:32]

Can you rebase and upload the patch again?

[pstanek, 2013-08-07 21:06:50]

On 2013/08/07 18:23:32, tony wrote:
> Can you rebase and upload the patch again?

Done.

[commit-bot: I haz the power, 2013-08-07 21:14:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pstanek@opera.com/21235009/29001

[commit-bot: I haz the power, 2013-08-08 00:45:35]

Change committed as 155725