Add/remove spell checking markers in text inputs depending on focus.

Add/remove spell checking markers in text inputs depending on focus.

Inputs are spell checked by default. However since it's very
likely user names, cities, addresses and other stuff that
possibly isn't in the dictionary will be put into them when
unfocused misspelling markers are removed from them (so
they are not all red all the time). However they are
added again on input re-focusing.

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

[please use gerrit instead, 2013-07-30 21:31:59]

Thank you for splitting up the patch! This is much easier to review. 

Please do not be discouraged by the large number of comments. Reviews will
become easier as you learn style guidelines and Blink's overall architecture.

My comments are mainly stylistic. I defer the design and architecture decisions
for editing and spellcheck to the more experienced reviewers.

https://codereview.chromium.org/21024004/diff/1/LayoutTests/editing/spelling/...
File LayoutTests/editing/spelling/markers-input-type-text.html (right):

https://codereview.chromium.org/21024004/diff/1/LayoutTests/editing/spelling/...
LayoutTests/editing/spelling/markers-input-type-text.html:20:
description("Markers in text inpus should only be visible if an input is being
edited" +
spelling: inpus -> inputs

https://codereview.chromium.org/21024004/diff/1/LayoutTests/editing/spelling/...
LayoutTests/editing/spelling/markers-input-type-text.html:21: " but removing
markers from the input must not touch other markers." +
Please add a space before " at the end of the line. Remove the space after " at
the beginning of the line. For example: 

description("Markers in text input should only be visible if an input is being
edited, " +
"but removing markers from the input must not touch other markers.");

https://codereview.chromium.org/21024004/diff/1/LayoutTests/editing/spelling/...
LayoutTests/editing/spelling/markers-input-type-text.html:27: function
findFirstTextNode(node)
These functions look like they can be reused in other files. Could you please
put them into a utility file? For example, you can put them in
LayoutTests/editing/spelling/resources/util.js. Or create a new file, up to you.

https://codereview.chromium.org/21024004/diff/1/LayoutTests/editing/spelling/...
LayoutTests/editing/spelling/markers-input-type-text.html:43: 
Please remove the second blank line here.

https://codereview.chromium.org/21024004/diff/1/LayoutTests/editing/spelling/...
LayoutTests/editing/spelling/markers-input-type-text.html:57: if
(!window.internals) {
Please add a space after the closing brace of typeText() function.

https://codereview.chromium.org/21024004/diff/1/LayoutTests/editing/spelling/...
File LayoutTests/editing/spelling/spellcheck-async-mutation.html (right):

https://codereview.chromium.org/21024004/diff/1/LayoutTests/editing/spelling/...
LayoutTests/editing/spelling/spellcheck-async-mutation.html:214: // In move,
marker should be there unless it's input which is moved.
This sentence is not clear. Please explain in more details. Perhaps give an
example.

https://codereview.chromium.org/21024004/diff/1/LayoutTests/editing/spelling/...
LayoutTests/editing/spelling/spellcheck-async-mutation.html:215: if (elem
instanceof HTMLInputElement && elem !=document.activeElement)
Please add a space after !=.

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

https://codereview.chromium.org/21024004/diff/1/Source/core/editing/Editor.cp...
Source/core/editing/Editor.cpp:2107: if (isGrammarCheckingEnabled())
Also check for unified spellchecker setting. If unified checking is enabled,
then you should remove both grammar and spelling marker types.

https://codereview.chromium.org/21024004/diff/1/Source/core/editing/Editor.cp...
Source/core/editing/Editor.cpp:2289: && !isSelectionInTextField(oldSelection)) {
We don't go through code looking for style violations, but if you're editing a
line of code that does not adhere to style guidelines, then it's OK to modify
the style to match the guidelines. In this particular example, the line expands
more than 125 characters. I believe that Blink style mandates 125 character
lines. (Correct me, if I'm wrong.) Please modify this line as follows:

    if (shouldCheckSpellingAndGrammar &&
        closeTyping &&
        oldSelection.isContentEditable() &&
        oldSelection.start().deprecatedNode() &&
        oldSelection.start().anchorNode()->inDocument() &&
        !isSelectionInTextField(oldSelection)) {
        ...
    }

(&& and || go at the end of the line.)

https://codereview.chromium.org/21024004/diff/1/Source/core/editing/SpellChec...
File Source/core/editing/SpellChecker.h (right):

https://codereview.chromium.org/21024004/diff/1/Source/core/editing/SpellChec...
Source/core/editing/SpellChecker.h:86: void cancelProcessingRequest();
Can you reuse cancelCheck() instead? The implementation seems to be doing
exactly same task. If you think that implementation of cancelProcessingRequest()
is better, you can try modifying cancelCheck() implementation instead. We have
layout tests and unit tests in place to make sure your modifications don't break
spellchecker. You're already familiar with the layout tests:
LayoutTest/editing/spelling. You can run Blink's spellcheck unit tests like so:

  ./out/Release/webkit_unit_tests --gtest_filter="*Spell*:*spell*"

[pstanek, 2013-07-30 21:50:39]

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

https://codereview.chromium.org/21024004/diff/1/Source/core/editing/Editor.cp...
Source/core/editing/Editor.cpp:2289: && !isSelectionInTextField(oldSelection)) {
On 2013/07/30 21:32:00, Rouslan Solomakhin wrote:
> We don't go through code looking for style violations, but if you're editing a
> line of code that does not adhere to style guidelines, then it's OK to modify
> the style to match the guidelines. In this particular example, the line
expands
> more than 125 characters. I believe that Blink style mandates 125 character
> lines. (Correct me, if I'm wrong.) Please modify this line as follows:
> 
>     if (shouldCheckSpellingAndGrammar &&
>         closeTyping &&
>         oldSelection.isContentEditable() &&
>         oldSelection.start().deprecatedNode() &&
>         oldSelection.start().anchorNode()->inDocument() &&
>         !isSelectionInTextField(oldSelection)) {
>         ...
>     }
> 
> (&& and || go at the end of the line.)

Been there. && and || on the right is something presubmit checks complain about.

https://codereview.chromium.org/21024004/diff/1/Source/core/editing/SpellChec...
File Source/core/editing/SpellChecker.h (right):

https://codereview.chromium.org/21024004/diff/1/Source/core/editing/SpellChec...
Source/core/editing/SpellChecker.h:86: void cancelProcessingRequest();
On 2013/07/30 21:32:00, Rouslan Solomakhin wrote:
> Can you reuse cancelCheck() instead? The implementation seems to be doing
> exactly same task. If you think that implementation of
cancelProcessingRequest()
> is better, you can try modifying cancelCheck() implementation instead. We have
> layout tests and unit tests in place to make sure your modifications don't
break
> spellchecker. You're already familiar with the layout tests:
> LayoutTest/editing/spelling. You can run Blink's spellcheck unit tests like
so:
> 
>   ./out/Release/webkit_unit_tests --gtest_filter="*Spell*:*spell*"

This is something layout tests showed me as in the original patch I was using
cancelCheck(). The problem was that if some checks were added to the queue
before unfocusing an input they were also cancelled causing spellcheck-queue
test to fail. This is why I created this function to cancel only the current
request. I could reuse cancelCheck() however by adding an extra param to it.

[please use gerrit instead, 2013-07-30 21:54:42]

On 2013/07/30 21:50:39, pstanek wrote:
> This is something layout tests showed me as in the original patch I was using
> cancelCheck(). The problem was that if some checks were added to the queue
> before unfocusing an input they were also cancelled causing spellcheck-queue
> test to fail. This is why I created this function to cancel only the current
> request. I could reuse cancelCheck() however by adding an extra param to it.

Try to remove the queue clearing code from cancelCheck() and see if the tests
are still passing. I suspect that the queue clearing is not required. (It was I
that originally added it.)

void SpellChecker::cancelCheck()
{
    if (m_processingRequest)
        m_processingRequest->didCancel();
}

[please use gerrit instead, 2013-07-30 21:57:01]

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

https://codereview.chromium.org/21024004/diff/1/Source/core/editing/Editor.cp...
Source/core/editing/Editor.cpp:2289: && !isSelectionInTextField(oldSelection)) {
On 2013/07/30 21:50:39, pstanek wrote:
> && and || on the right is something presubmit checks complain about.

Presubmit checks are usually correct. Do what they ask :-). (I might be mixing
in Chromium style in here, sorry.)

[pstanek, 2013-07-31 12:10:07]

Added patches with Rouslan's comments addressed.

[please use gerrit instead, 2013-07-31 16:27:31]

Thank you for fixing the style!

https://codereview.chromium.org/21024004/diff/12001/LayoutTests/editing/spell...
File LayoutTests/editing/spelling/markers-input-type-text.html (right):

https://codereview.chromium.org/21024004/diff/12001/LayoutTests/editing/spell...
LayoutTests/editing/spelling/markers-input-type-text.html:11: <script
src="../editing.js" language="JavaScript" type="text/JavaScript" ></script>
   language="JavaScript" type="text/JavaScript" 

Nit: This is not necessary.

[please use gerrit instead, 2013-07-31 16:30:18]

I think that Adam will ask you to add a longer description to the patch, similar
to the other CL that he reviewed.

[please use gerrit instead, 2013-07-31 16:31:47]

Overall, this looks good to me, but I would like to have a more experienced
Blink developer take a look at the CL.

[tony, 2013-07-31 20:59:28]

https://codereview.chromium.org/21024004/diff/12001/LayoutTests/editing/spell...
File LayoutTests/editing/spelling/markers-input-type-text.html (right):

https://codereview.chromium.org/21024004/diff/12001/LayoutTests/editing/spell...
LayoutTests/editing/spelling/markers-input-type-text.html:34: return;
Does it work to return when you're not in a function?  Normally it's better to
put the testing code in an onload handler and return from that.  Then you also
don't need to set successfullyParsed.

https://codereview.chromium.org/21024004/diff/12001/LayoutTests/editing/spell...
File LayoutTests/editing/spelling/resources/util.js (right):

https://codereview.chromium.org/21024004/diff/12001/LayoutTests/editing/spell...
LayoutTests/editing/spelling/resources/util.js:55: if (node instanceof
HTMLInputElement || node instanceof HTMLTextAreaElement) {
The style guide says "If one part of an if-else statement uses curly braces, the
other part must too.".  Either remove these {} or add them to the else.

https://codereview.chromium.org/21024004/diff/12001/Source/core/editing/Spell...
File Source/core/editing/SpellChecker.cpp (left):

https://codereview.chromium.org/21024004/diff/12001/Source/core/editing/Spell...
Source/core/editing/SpellChecker.cpp:195: m_requestQueue.clear();
I don't understand this change.  Why are we no longer emptying the queue?

https://codereview.chromium.org/21024004/diff/12001/Source/web/EditorClientIm...
File Source/web/EditorClientImpl.cpp (right):

https://codereview.chromium.org/21024004/diff/12001/Source/web/EditorClientIm...
Source/web/EditorClientImpl.cpp:111: return true;
It looks like we already spellcheck inputs by default.  I think this is because
of this condition: if the spellcheck attribute is not set anywhere, we return
true here.

I'm not sure what the purpose of the change to this function is.

It's also weird that if the spellcheck attribute is false, we can still return
true from this function.  I guess we check the attribute somewhere else.

[pstanek, 2013-07-31 21:39:42]

On 2013/07/31 20:59:28, tony wrote:
>
https://codereview.chromium.org/21024004/diff/12001/LayoutTests/editing/spell...
> File LayoutTests/editing/spelling/markers-input-type-text.html (right):
> 
>
https://codereview.chromium.org/21024004/diff/12001/LayoutTests/editing/spell...
> LayoutTests/editing/spelling/markers-input-type-text.html:34: return;
> Does it work to return when you're not in a function?  Normally it's better to
> put the testing code in an onload handler and return from that.  Then you also
> don't need to set successfullyParsed.

This works ok. Chrome shows alert and that the page is successfully parsed.

> 
>
https://codereview.chromium.org/21024004/diff/12001/LayoutTests/editing/spell...
> File LayoutTests/editing/spelling/resources/util.js (right):
> 
>
https://codereview.chromium.org/21024004/diff/12001/LayoutTests/editing/spell...
> LayoutTests/editing/spelling/resources/util.js:55: if (node instanceof
> HTMLInputElement || node instanceof HTMLTextAreaElement) {
> The style guide says "If one part of an if-else statement uses curly braces,
the
> other part must too.".  Either remove these {} or add them to the else.
> 
>
https://codereview.chromium.org/21024004/diff/12001/Source/core/editing/Spell...
> File Source/core/editing/SpellChecker.cpp (left):
> 
>
https://codereview.chromium.org/21024004/diff/12001/Source/core/editing/Spell...
> Source/core/editing/SpellChecker.cpp:195: m_requestQueue.clear();
> I don't understand this change.  Why are we no longer emptying the queue?

Please see https://codereview.chromium.org/21024004/#msg3 and
https://codereview.chromium.org/21024004/#msg4

> 
>
https://codereview.chromium.org/21024004/diff/12001/Source/web/EditorClientIm...
> File Source/web/EditorClientImpl.cpp (right):
> 
>
https://codereview.chromium.org/21024004/diff/12001/Source/web/EditorClientIm...
> Source/web/EditorClientImpl.cpp:111: return true;
> It looks like we already spellcheck inputs by default.  I think this is
because
> of this condition: if the spellcheck attribute is not set anywhere, we return
> true here.
> 
> I'm not sure what the purpose of the change to this function is.
> 
> It's also weird that if the spellcheck attribute is false, we can still return
> true from this function.  I guess we check the attribute somewhere else.

Since inputs have underlying TextControlInnerElement spell checking is	 
enabled for them by default (by editor->isSpellCheckingEnabledInFocusedNode()
check in EditorClientImpl::shouldSpellcheckByDefault() which checks a node with
a selection and this is where the spellcheck attribute is checked too) causing
misspellings to be marked when typing in an input. However when unfocusing the
input (a node with a selection becomes null) the spell checker gets disabled
(because of returning false here which resulted in
isContinuousSpellCheckingEnabled() returning false) causing Blink to clear *all*
misspelling markers within document.

[pstanek, 2013-07-31 21:49:17]

On 2013/07/31 16:27:31, Rouslan Solomakhin wrote:
> Thank you for fixing the style!
> 
>
https://codereview.chromium.org/21024004/diff/12001/LayoutTests/editing/spell...
> File LayoutTests/editing/spelling/markers-input-type-text.html (right):
> 
>
https://codereview.chromium.org/21024004/diff/12001/LayoutTests/editing/spell...
> LayoutTests/editing/spelling/markers-input-type-text.html:11: <script
> src="../editing.js" language="JavaScript" type="text/JavaScript" ></script>
>    language="JavaScript" type="text/JavaScript" 
> 
> Nit: This is not necessary.

It actually is as it has function utils.js uses.

[please use gerrit instead, 2013-07-31 21:51:25]

On 2013/07/31 21:49:17, pstanek wrote:
> On 2013/07/31 16:27:31, Rouslan Solomakhin wrote:
> > Thank you for fixing the style!
> > 
> >
>
https://codereview.chromium.org/21024004/diff/12001/LayoutTests/editing/spell...
> > File LayoutTests/editing/spelling/markers-input-type-text.html (right):
> > 
> >
>
https://codereview.chromium.org/21024004/diff/12001/LayoutTests/editing/spell...
> > LayoutTests/editing/spelling/markers-input-type-text.html:11: <script
> > src="../editing.js" language="JavaScript" type="text/JavaScript" ></script>
> >    language="JavaScript" type="text/JavaScript" 
> > 
> > Nit: This is not necessary.
> 
> It actually is as it has function utils.js uses.

I meant that language and type attributes are not necessary. This should work:

  <script src="../editing.js"></script>

Should it not?

[tony, 2013-07-31 21:53:30]

On 2013/07/31 21:39:42, pstanek wrote:
> On 2013/07/31 20:59:28, tony wrote:
> >
>
https://codereview.chromium.org/21024004/diff/12001/LayoutTests/editing/spell...
> > File LayoutTests/editing/spelling/markers-input-type-text.html (right):
> > 
> >
>
https://codereview.chromium.org/21024004/diff/12001/LayoutTests/editing/spell...
> > LayoutTests/editing/spelling/markers-input-type-text.html:34: return;
> > Does it work to return when you're not in a function?  Normally it's better
to
> > put the testing code in an onload handler and return from that.  Then you
also
> > don't need to set successfullyParsed.
> 
> This works ok. Chrome shows alert and that the page is successfully parsed.

Yes, but in the console, there is a javascript error.  You can't return if
you're not in a function.

>> Nit: This is not necessary.

>It actually is as it has function utils.js uses.

Rouslan is saying that the language and type attributes are not necessary.

[pstanek, 2013-07-31 22:07:32]

On 2013/07/31 21:53:30, tony wrote:
> On 2013/07/31 21:39:42, pstanek wrote:
> > On 2013/07/31 20:59:28, tony wrote:
> > >
> >
>
https://codereview.chromium.org/21024004/diff/12001/LayoutTests/editing/spell...
> > > File LayoutTests/editing/spelling/markers-input-type-text.html (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/21024004/diff/12001/LayoutTests/editing/spell...
> > > LayoutTests/editing/spelling/markers-input-type-text.html:34: return;
> > > Does it work to return when you're not in a function?  Normally it's
better
> to
> > > put the testing code in an onload handler and return from that.  Then you
> also
> > > don't need to set successfullyParsed.
> > 
> > This works ok. Chrome shows alert and that the page is successfully parsed.
> 
> Yes, but in the console, there is a javascript error.  You can't return if
> you're not in a function.
> 
> >> Nit: This is not necessary.
> 
> >It actually is as it has function utils.js uses.
> 
> Rouslan is saying that the language and type attributes are not necessary.

patch set #3 with test fixes.

[tony, 2013-07-31 22:31:45]

code change LGTM.  A few more style nits with the test change.

https://codereview.chromium.org/21024004/diff/28001/LayoutTests/editing/spell...
File LayoutTests/editing/spelling/markers-input-type-text.html (right):

https://codereview.chromium.org/21024004/diff/28001/LayoutTests/editing/spell...
LayoutTests/editing/spelling/markers-input-type-text.html:29: 
Nit: Remove blank line.

https://codereview.chromium.org/21024004/diff/28001/LayoutTests/editing/spell...
LayoutTests/editing/spelling/markers-input-type-text.html:44: if
(window.internals) {
You don't need to test for window.internals again since you checked for it at
the beginning of the function.

https://codereview.chromium.org/21024004/diff/28001/LayoutTests/editing/spell...
LayoutTests/editing/spelling/markers-input-type-text.html:49: if
(window.internals) {
Ditto.

https://codereview.chromium.org/21024004/diff/28001/LayoutTests/editing/spell...
LayoutTests/editing/spelling/markers-input-type-text.html:56: if
(window.internals) {
Ditto.

https://codereview.chromium.org/21024004/diff/28001/LayoutTests/editing/spell...
LayoutTests/editing/spelling/markers-input-type-text.html:62: if
(window.internals) {
Ditto.

[pstanek, 2013-08-01 07:28:29]

On 2013/07/31 22:31:45, tony wrote:
> code change LGTM.  A few more style nits with the test change.
> 
>
https://codereview.chromium.org/21024004/diff/28001/LayoutTests/editing/spell...
> File LayoutTests/editing/spelling/markers-input-type-text.html (right):
> 
>
https://codereview.chromium.org/21024004/diff/28001/LayoutTests/editing/spell...
> LayoutTests/editing/spelling/markers-input-type-text.html:29: 
> Nit: Remove blank line.
> 
>
https://codereview.chromium.org/21024004/diff/28001/LayoutTests/editing/spell...
> LayoutTests/editing/spelling/markers-input-type-text.html:44: if
> (window.internals) {
> You don't need to test for window.internals again since you checked for it at
> the beginning of the function.
> 
>
https://codereview.chromium.org/21024004/diff/28001/LayoutTests/editing/spell...
> LayoutTests/editing/spelling/markers-input-type-text.html:49: if
> (window.internals) {
> Ditto.
> 
>
https://codereview.chromium.org/21024004/diff/28001/LayoutTests/editing/spell...
> LayoutTests/editing/spelling/markers-input-type-text.html:56: if
> (window.internals) {
> Ditto.
> 
>
https://codereview.chromium.org/21024004/diff/28001/LayoutTests/editing/spell...
> LayoutTests/editing/spelling/markers-input-type-text.html:62: if
> (window.internals) {
> Ditto.

Fixed in the patch set #4 + it seems I failed to commit the new expected output
file after changes in the test which ofc caused the test to fail (other failures
don't look related). This is fixed now too.

[pstanek, 2013-08-01 21:16:26]

Some unrelated failures again?

[commit-bot: I haz the power, 2013-08-01 21:17:57]

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

[commit-bot: I haz the power, 2013-08-02 01:07:39]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-08-02 06:34:54]

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

[commit-bot: I haz the power, 2013-08-02 09:16:55]

Change committed as 155420

[pstanek, 2013-08-02 09:19:59]

On 2013/08/02 09:16:55, I haz the power (commit-bot) wrote:
> Change committed as 155420

Great now we cam move to other related patches and I may rebase and file yet
another one...