Spell check whole content of an editable element when it gets focused.

Spell check whole content of an editable element when it gets focused.

This is how Gecko and Presto behave. Without this patch Blink spell checks
an editable after moving the caret/unfocusing the editable but the check spans
only the line the caret was previously at (possibly plus 1-2 lines around).
Textareas and contenteditables are only spell checked on
first focus.

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

[pstanek, 2013-08-02 13:19:56]

Last one split out of 20709004.

[please use gerrit instead, 2013-08-02 16:48:45]

https://chromiumcodereview.appspot.com/21694005/diff/1/LayoutTests/editing/sp...
File LayoutTests/editing/spelling/spellcheck-editable-on-focus.html (right):

https://chromiumcodereview.appspot.com/21694005/diff/1/LayoutTests/editing/sp...
LayoutTests/editing/spelling/spellcheck-editable-on-focus.html:12: <textarea>zz
zz zz.</textarea>
Could you please add <input type="text"> here, too?

https://chromiumcodereview.appspot.com/21694005/diff/1/LayoutTests/editing/sp...
LayoutTests/editing/spelling/spellcheck-editable-on-focus.html:20: var
testTextArea = document.getElementsByTagName('textarea')[0];
It's best to be consistent. Assign an id to the textarea and use that id in a
document.getElementById() call.

https://chromiumcodereview.appspot.com/21694005/diff/1/LayoutTests/editing/sp...
LayoutTests/editing/spelling/spellcheck-editable-on-focus.html:26:
document.getElementById("console").innerHTML = "Automatic testing impossible.
Test manually.\n";
There's an easier way to write this:

  log("Automatic testing impossible. Test manually.");

The log() function is defined in util.js, which you already include.

[tony, 2013-08-02 17:01:16]

https://chromiumcodereview.appspot.com/21694005/diff/1/Source/core/editing/Ed...
File Source/core/editing/Editor.cpp (right):

https://chromiumcodereview.appspot.com/21694005/diff/1/Source/core/editing/Ed...
Source/core/editing/Editor.cpp:106:
frame->editor()->markMisspellingsAndBadGrammar(selection);
Is it really OK to check all of a contentEditable or textarea?  If you have lots
of text (e.g., many megabytes), won't this cause the UI to hang?

https://chromiumcodereview.appspot.com/21694005/diff/1/Source/core/editing/Ed...
Source/core/editing/Editor.cpp:2110:
elementDidBeginEditing(toHTMLTextFormControlElement(e)->innerTextElement());
textFieldDidBeginEditing and textAreaDidBeginEditing seem the same. Why not have
a single function?  It could be called textAreaOrTextFieldDidBeginEditing.

[pstanek, 2013-08-02 17:18:42]

On 2013/08/02 17:01:16, tony wrote:
>
https://chromiumcodereview.appspot.com/21694005/diff/1/Source/core/editing/Ed...
> File Source/core/editing/Editor.cpp (right):
> 
>
https://chromiumcodereview.appspot.com/21694005/diff/1/Source/core/editing/Ed...
> Source/core/editing/Editor.cpp:106:
> frame->editor()->markMisspellingsAndBadGrammar(selection);
> Is it really OK to check all of a contentEditable or textarea?  If you have
lots
> of text (e.g., many megabytes), won't this cause the UI to hang?

Yes. Note however the problem is already there but to trigger it you need to
paste megabytes. But indeed limiting the selection to check sounds like a good
idea.

> 
>
https://chromiumcodereview.appspot.com/21694005/diff/1/Source/core/editing/Ed...
> Source/core/editing/Editor.cpp:2110:
> elementDidBeginEditing(toHTMLTextFormControlElement(e)->innerTextElement());
> textFieldDidBeginEditing and textAreaDidBeginEditing seem the same. Why not
have
> a single function?  It could be called textAreaOrTextFieldDidBeginEditing.

[tony, 2013-08-02 17:33:54]

On 2013/08/02 17:18:42, pstanek wrote:
> On 2013/08/02 17:01:16, tony wrote:
> >
>
https://chromiumcodereview.appspot.com/21694005/diff/1/Source/core/editing/Ed...
> > File Source/core/editing/Editor.cpp (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/21694005/diff/1/Source/core/editing/Ed...
> > Source/core/editing/Editor.cpp:106:
> > frame->editor()->markMisspellingsAndBadGrammar(selection);
> > Is it really OK to check all of a contentEditable or textarea?  If you have
> lots
> > of text (e.g., many megabytes), won't this cause the UI to hang?
> 
> Yes. Note however the problem is already there but to trigger it you need to
> paste megabytes. But indeed limiting the selection to check sounds like a good
> idea.

We should try to fix this bug first.  Ideally, we would only spellcheck text
that is visible to the user (and maybe a little bit off the screen).

[pstanek, 2013-08-02 20:13:13]

On 2013/08/02 17:33:54, tony wrote:
> On 2013/08/02 17:18:42, pstanek wrote:
> > On 2013/08/02 17:01:16, tony wrote:
> > >
> >
>
https://chromiumcodereview.appspot.com/21694005/diff/1/Source/core/editing/Ed...
> > > File Source/core/editing/Editor.cpp (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/21694005/diff/1/Source/core/editing/Ed...
> > > Source/core/editing/Editor.cpp:106:
> > > frame->editor()->markMisspellingsAndBadGrammar(selection);
> > > Is it really OK to check all of a contentEditable or textarea?  If you
have
> > lots
> > > of text (e.g., many megabytes), won't this cause the UI to hang?
> > 
> > Yes. Note however the problem is already there but to trigger it you need to
> > paste megabytes. But indeed limiting the selection to check sounds like a
good
> > idea.
> 
> We should try to fix this bug first.  Ideally, we would only spellcheck text
> that is visible to the user (and maybe a little bit off the screen).

I'm not totally convinced the visibility policy would be the best. Imagine
contenteditable with an image at the beginning covering all visible area and a
little bit off the screen and then text right below it. In such case the text
won't even be scratched. Or a very small textarea with a lot of text (moreover
this is more complex to implement). I'd rather vote for some fixed string
length/words count. It seems Firefox limits number of misspellings and stop
checking if the limit is hit. Such a limit could be added to the spellchecker to
be consistent no matter what is spellchecked. Anyway. I need to experiment and
come up with something. Could we in a meanwhile switch to
https://codereview.chromium.org/21235009/ which hopefully won't be problematic.
https://codereview.chromium.org/21130005/ becomes blocked by the same problem as
we have here.

[pstanek, 2013-08-02 20:16:20]

On 2013/08/02 20:13:13, pstanek wrote:
> On 2013/08/02 17:33:54, tony wrote:
> > On 2013/08/02 17:18:42, pstanek wrote:
> > > On 2013/08/02 17:01:16, tony wrote:
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/21694005/diff/1/Source/core/editing/Ed...
> > > > File Source/core/editing/Editor.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/21694005/diff/1/Source/core/editing/Ed...
> > > > Source/core/editing/Editor.cpp:106:
> > > > frame->editor()->markMisspellingsAndBadGrammar(selection);
> > > > Is it really OK to check all of a contentEditable or textarea?  If you
> have
> > > lots
> > > > of text (e.g., many megabytes), won't this cause the UI to hang?
> > > 
> > > Yes. Note however the problem is already there but to trigger it you need
to
> > > paste megabytes. But indeed limiting the selection to check sounds like a
> good
> > > idea.
> > 
> > We should try to fix this bug first.  Ideally, we would only spellcheck text
> > that is visible to the user (and maybe a little bit off the screen).
> 
> I'm not totally convinced the visibility policy would be the best. Imagine
> contenteditable with an image at the beginning covering all visible area and a
> little bit off the screen and then text right below it. In such case the text
> won't even be scratched. Or a very small textarea with a lot of text (moreover
> this is more complex to implement). I'd rather vote for some fixed string
> length/words count. It seems Firefox limits number of misspellings and stop
> checking if the limit is hit. Such a limit could be added to the spellchecker
to
> be consistent no matter what is spellchecked. Anyway. I need to experiment and
> come up with something. Could we in a meanwhile switch to
> https://codereview.chromium.org/21235009/ which hopefully won't be
problematic.
> https://codereview.chromium.org/21130005/ becomes blocked by the same problem
as
> we have here.

> Such a limit could be added to the spellchecker to
> be consistent no matter what is spellchecked

And it would fix the problem with freeze on spellchecking after pasting a huge
text ;).

[ojan, 2013-08-02 22:41:20]

Why is focus the right time to do this? Won't we already have spellchecked the
contents if it was created via user input? Maybe we should be doing it at
childrenChanged or something like that? Even then, I think we'd need some
mitigation to not hang the UI. I agree that just doing the visible text doesn't
make much sense. We could do like the find-in-page code and chunk up the work,
but do all the text eventually.

[pstanek, 2013-08-05 10:21:00]

On 2013/08/02 22:41:20, ojan wrote:
> Why is focus the right time to do this? Won't we already have spellchecked the
> contents if it was created via user input? Maybe we should be doing it at
> childrenChanged or something like that? Even then, I think we'd need some
> mitigation to not hang the UI. I agree that just doing the visible text
doesn't
> make much sense. We could do like the find-in-page code and chunk up the work,
> but do all the text eventually.

It's not always spellchecked even if typed in by a user e.g. the user closed the
browser and now the textarea with the text is brought back from the session. Or
the user turned spellchecker off an on. This is why onfocus seems good for
spellchecking it (others spell check on focus too).
Chunking up the work should do, indeed, but only if asynchronous spellchecking
is on. Otherwise only one chunk should be done (size of it should be set to
something big enough but not too big to avoid spell checker choking).

[ojan, 2013-08-05 17:25:52]

On 2013/08/05 10:21:00, pstanek wrote:
> On 2013/08/02 22:41:20, ojan wrote:
> > Why is focus the right time to do this? Won't we already have spellchecked
the
> > contents if it was created via user input? Maybe we should be doing it at
> > childrenChanged or something like that? Even then, I think we'd need some
> > mitigation to not hang the UI. I agree that just doing the visible text
> doesn't
> > make much sense. We could do like the find-in-page code and chunk up the
work,
> > but do all the text eventually.
> 
> It's not always spellchecked even if typed in by a user e.g. the user closed
the
> browser and now the textarea with the text is brought back from the session.
Or
> the user turned spellchecker off an on. This is why onfocus seems good for
> spellchecking it (others spell check on focus too).

Thinking about this more, I just don't want us to do this work repeatedly
everytime you focus. So, I'd be happy with doing it on focus if we kept a bit so
that we only do it on the first focus. We could also clear that bit if the
spellcheck property is turned off so that we redo it if it's turned on and
focused later.

> Chunking up the work should do, indeed, but only if asynchronous spellchecking
> is on. Otherwise only one chunk should be done (size of it should be set to
> something big enough but not too big to avoid spell checker choking).

Sounds good to me. I didn't realize we still had a sync spellchecking codepath.
I'm not too familiar with the code...can we get rid of the sync version now that
we've forked?

[pstanek, 2013-08-07 17:46:38]

On 2013/08/05 17:25:52, ojan wrote:
> On 2013/08/05 10:21:00, pstanek wrote:
> > On 2013/08/02 22:41:20, ojan wrote:
> > > Why is focus the right time to do this? Won't we already have spellchecked
> the
> > > contents if it was created via user input? Maybe we should be doing it at
> > > childrenChanged or something like that? Even then, I think we'd need some
> > > mitigation to not hang the UI. I agree that just doing the visible text
> > doesn't
> > > make much sense. We could do like the find-in-page code and chunk up the
> work,
> > > but do all the text eventually.
> > 
> > It's not always spellchecked even if typed in by a user e.g. the user closed
> the
> > browser and now the textarea with the text is brought back from the session.
> Or
> > the user turned spellchecker off an on. This is why onfocus seems good for
> > spellchecking it (others spell check on focus too).
> 
> Thinking about this more, I just don't want us to do this work repeatedly
> everytime you focus. So, I'd be happy with doing it on focus if we kept a bit
so
> that we only do it on the first focus. We could also clear that bit if the
> spellcheck property is turned off so that we redo it if it's turned on and
> focused later.

True. Such info would have to be kept per element (maybe a list of pointers to
elements) and would have to be cleared if spelling markers are removed from the
document e.g. on reload, on turning spell checker off, on navigating away etc.

> 
> > Chunking up the work should do, indeed, but only if asynchronous
spellchecking
> > is on. Otherwise only one chunk should be done (size of it should be set to
> > something big enough but not too big to avoid spell checker choking).
> 
> Sounds good to me. I didn't realize we still had a sync spellchecking
codepath.
> I'm not too familiar with the code...can we get rid of the sync version now
that
> we've forked?

Looks like some tests still use this so this might be a bit bigger undertake so
I'd prefer to not do this within this bug fix.

[ojan, 2013-08-07 22:58:09]

On 2013/08/07 17:46:38, pstanek wrote:
> On 2013/08/05 17:25:52, ojan wrote:
> > On 2013/08/05 10:21:00, pstanek wrote:
> > > On 2013/08/02 22:41:20, ojan wrote:
> > > > Why is focus the right time to do this? Won't we already have
spellchecked
> > the
> > > > contents if it was created via user input? Maybe we should be doing it
at
> > > > childrenChanged or something like that? Even then, I think we'd need
some
> > > > mitigation to not hang the UI. I agree that just doing the visible text
> > > doesn't
> > > > make much sense. We could do like the find-in-page code and chunk up the
> > work,
> > > > but do all the text eventually.
> > > 
> > > It's not always spellchecked even if typed in by a user e.g. the user
closed
> > the
> > > browser and now the textarea with the text is brought back from the
session.
> > Or
> > > the user turned spellchecker off an on. This is why onfocus seems good for
> > > spellchecking it (others spell check on focus too).
> > 
> > Thinking about this more, I just don't want us to do this work repeatedly
> > everytime you focus. So, I'd be happy with doing it on focus if we kept a
bit
> so
> > that we only do it on the first focus. We could also clear that bit if the
> > spellcheck property is turned off so that we redo it if it's turned on and
> > focused later.
> 
> True. Such info would have to be kept per element (maybe a list of pointers to
> elements) and would have to be cleared if spelling markers are removed from
the
> document e.g. on reload, on turning spell checker off, on navigating away etc.

Yup. It looks like we have bits to spare on Element.h by stealing from
m_arraySize.

> > > Chunking up the work should do, indeed, but only if asynchronous
> spellchecking
> > > is on. Otherwise only one chunk should be done (size of it should be set
to
> > > something big enough but not too big to avoid spell checker choking).
> > 
> > Sounds good to me. I didn't realize we still had a sync spellchecking
> codepath.
> > I'm not too familiar with the code...can we get rid of the sync version now
> that
> > we've forked?
> 
> Looks like some tests still use this so this might be a bit bigger undertake
so
> I'd prefer to not do this within this bug fix.

Yeah, totally. Sorry, I was just thinking out loud.

[Hajime Morrita, 2013-08-08 06:02:50]

I hope this to be done asynchronously because 1) the text can be large and 2)
grammar check needs IPC.
We do asynchronous checking for pasted text.

[pstanek, 2013-08-12 14:58:06]

On 2013/08/08 06:02:50, morrita1 wrote:
> I hope this to be done asynchronously because 1) the text can be large and 2)
> grammar check needs IPC.
> We do asynchronous checking for pasted text.

It's done asynchronously but still in one chunk so when pasting e.g. 1 MB text
into an editable the page becomes unresponsive.
I'm working on chunking the text up and it seems I have some working fix ready.
I'll push it to https://codereview.chromium.org/21130005/ and I hope you don't
mind. The rationale is that https://codereview.chromium.org/21130005/ seems to
be only blocked by this issue (possible freeze) where here I have to still fix
needless spell checking on every focus.

[pstanek, 2013-08-20 18:24:22]

On 2013/08/12 14:58:06, pstanek wrote:
> On 2013/08/08 06:02:50, morrita1 wrote:
> > I hope this to be done asynchronously because 1) the text can be large and
2)
> > grammar check needs IPC.
> > We do asynchronous checking for pasted text.
> 
> It's done asynchronously but still in one chunk so when pasting e.g. 1 MB text
> into an editable the page becomes unresponsive.
> I'm working on chunking the text up and it seems I have some working fix
ready.
> I'll push it to https://codereview.chromium.org/21130005/ and I hope you don't
> mind. The rationale is that https://codereview.chromium.org/21130005/ seems to
> be only blocked by this issue (possible freeze) where here I have to still fix
> needless spell checking on every focus.

Back to this since chunking up was sorted out and integrated. I rebased and
addressed review comments i.e. amongst others I implemented keeping track of
editables which have been spell checked on first focus.

PTAL

[pstanek, 2013-08-20 18:25:09]

On 2013/08/20 18:24:22, pstanek wrote:
> On 2013/08/12 14:58:06, pstanek wrote:
> > On 2013/08/08 06:02:50, morrita1 wrote:
> > > I hope this to be done asynchronously because 1) the text can be large and
> 2)
> > > grammar check needs IPC.
> > > We do asynchronous checking for pasted text.
> > 
> > It's done asynchronously but still in one chunk so when pasting e.g. 1 MB
text
> > into an editable the page becomes unresponsive.
> > I'm working on chunking the text up and it seems I have some working fix
> ready.
> > I'll push it to https://codereview.chromium.org/21130005/ and I hope you
don't
> > mind. The rationale is that https://codereview.chromium.org/21130005/ seems
to
> > be only blocked by this issue (possible freeze) where here I have to still
fix
> > needless spell checking on every focus.
> 
> Back to this since chunking up was sorted out and integrated. I rebased and
> addressed review comments i.e. amongst others I implemented keeping track of
> editables which have been spell checked on first focus.
> 
> PTAL

Looks like rebase introduced a lot of unrelated changes. Sorry for that.

[pstanek, 2013-08-20 18:29:24]

On 2013/08/20 18:25:09, pstanek wrote:
> On 2013/08/20 18:24:22, pstanek wrote:
> > On 2013/08/12 14:58:06, pstanek wrote:
> > > On 2013/08/08 06:02:50, morrita1 wrote:
> > > > I hope this to be done asynchronously because 1) the text can be large
and
> > 2)
> > > > grammar check needs IPC.
> > > > We do asynchronous checking for pasted text.
> > > 
> > > It's done asynchronously but still in one chunk so when pasting e.g. 1 MB
> text
> > > into an editable the page becomes unresponsive.
> > > I'm working on chunking the text up and it seems I have some working fix
> > ready.
> > > I'll push it to https://codereview.chromium.org/21130005/ and I hope you
> don't
> > > mind. The rationale is that https://codereview.chromium.org/21130005/
seems
> to
> > > be only blocked by this issue (possible freeze) where here I have to still
> fix
> > > needless spell checking on every focus.
> > 
> > Back to this since chunking up was sorted out and integrated. I rebased and
> > addressed review comments i.e. amongst others I implemented keeping track of
> > editables which have been spell checked on first focus.
> > 
> > PTAL
> 
> Looks like rebase introduced a lot of unrelated changes. Sorry for that.

Ah, but only in deltas. Side by side diffs are ok. Good.

[please use gerrit instead, 2013-08-20 18:33:52]

https://codereview.chromium.org/21694005/diff/17001/LayoutTests/editing/spell...
File LayoutTests/editing/spelling/spellcheck-editable-on-focus.html (right):

https://codereview.chromium.org/21694005/diff/17001/LayoutTests/editing/spell...
LayoutTests/editing/spelling/spellcheck-editable-on-focus.html:33:
internals.settings.setAsynchronousSpellCheckingEnabled(false);
Please also add the same test for asynchronous spellchecker.

[pstanek, 2013-08-20 20:44:43]

https://codereview.chromium.org/21694005/diff/17001/LayoutTests/editing/spell...
File LayoutTests/editing/spelling/spellcheck-editable-on-focus.html (right):

https://codereview.chromium.org/21694005/diff/17001/LayoutTests/editing/spell...
LayoutTests/editing/spelling/spellcheck-editable-on-focus.html:33:
internals.settings.setAsynchronousSpellCheckingEnabled(false);
On 2013/08/20 18:33:52, Rouslan Solomakhin wrote:
> Please also add the same test for asynchronous spellchecker.

Done.

[please use gerrit instead, 2013-08-20 21:24:02]

LGTM for layout tests. I'm not familiar with Editor code and defer that review
to the others. (I'm more familiar with the spellchecker code itself.)

[tony, 2013-08-20 22:10:38]

https://codereview.chromium.org/21694005/diff/33001/Source/core/editing/Edito...
File Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/21694005/diff/33001/Source/core/editing/Edito...
Source/core/editing/Editor.cpp:1073: // the passed in element is put into
m_editablesSpellCheckedOnFocus (it's the root editable element).
This style of comment that describes behavior is better for the commit
description.  The behavior is likely to change in the future and this comment
likely to bitrot.

I would rename m_editablesSpellCheckedOnFocus to something like
m_alreadySpellCheckedEditables or something.  Maybe
editablesAlreadySpellChecked?

https://codereview.chromium.org/21694005/diff/33001/Source/core/editing/Edito...
Source/core/editing/Editor.cpp:1091:
m_editablesSpellCheckedOnFocus.append(parent);
You could put a 'why' comment here: We always recheck textfields because markers
are removed on blur.

https://codereview.chromium.org/21694005/diff/33001/Source/core/editing/Editor.h
File Source/core/editing/Editor.h (right):

https://codereview.chromium.org/21694005/diff/33001/Source/core/editing/Edito...
Source/core/editing/Editor.h:218: void didEndEditing(Element*);
It doesn't look like we use Element in didEndEditing.

https://codereview.chromium.org/21694005/diff/33001/Source/core/editing/Edito...
Source/core/editing/Editor.h:313: // This gets called when any editable element
gets focused.
I would remove this comment.  It's a 'what' comment instead of a 'why' comment
and it seems like it would easily get out of date.

https://codereview.chromium.org/21694005/diff/33001/Source/core/editing/Edito...
Source/core/editing/Editor.h:328: Vector<RefPtr<Node> >
m_editablesSpellCheckedOnFocus;
This should be a HashSet, not a Vector.

Also, why do you hold a ref?  If someone removes the node from the DOM, the ref
is going to keep it alive until the page is unloaded.

Even if you switch it to a HashSet<Node*>, it seems like it's possible for
unbounded growth if you dynamically create editable areas.  On a site like
gmail, that's bad.

https://codereview.chromium.org/21694005/diff/33001/Source/core/html/HTMLText...
File Source/core/html/HTMLTextAreaElement.cpp (right):

https://codereview.chromium.org/21694005/diff/33001/Source/core/html/HTMLText...
Source/core/html/HTMLTextAreaElement.cpp:270: void
HTMLTextAreaElement::handleFocusEvent(Element* oldFocusedNode, FocusDirection
direction)
Nit: unused parameters should not be named.

[pstanek, 2013-08-21 06:05:17]

https://codereview.chromium.org/21694005/diff/33001/Source/core/editing/Editor.h
File Source/core/editing/Editor.h (right):

https://codereview.chromium.org/21694005/diff/33001/Source/core/editing/Edito...
Source/core/editing/Editor.h:328: Vector<RefPtr<Node> >
m_editablesSpellCheckedOnFocus;
On 2013/08/20 22:10:39, tony wrote:
> This should be a HashSet, not a Vector.
> 
> Also, why do you hold a ref?  If someone removes the node from the DOM, the
ref
> is going to keep it alive until the page is unloaded.
> 
> Even if you switch it to a HashSet<Node*>, it seems like it's possible for
> unbounded growth if you dynamically create editable areas.  On a site like
> gmail, that's bad.

I was thinking the same i.e. it's only a key so Node* would do but then I looked
how spelling and grammar markers are kept. They also keep refs in their hashset
which also looks unneeded so I came to conclusion that I might not know
something and there is some reason behind this so I used refs here as well.

[pstanek, 2013-08-21 07:55:48]

PTAL

[tony, 2013-08-21 17:15:23]

This is better, but I'm still worried about m_editablesAlreadySpellChecked
growing and never clearing elements that are removed from the page.  Here's
another idea: instead of keeping a list of elements we check in Editor, maybe we
can set a bit on Element when we check it.  Specifically, it looks like there
are bits available in Node's m_nodeFlags.

[pstanek, 2013-08-21 17:32:16]

On 2013/08/21 17:15:23, tony wrote:
> This is better, but I'm still worried about m_editablesAlreadySpellChecked
> growing and never clearing elements that are removed from the page.  Here's
> another idea: instead of keeping a list of elements we check in Editor, maybe
we
> can set a bit on Element when we check it.  Specifically, it looks like there
> are bits available in Node's m_nodeFlags.

Was thinking about it. The problem is that turning spell checker off/on should
clear the list. Now it's easy. With a bit in Element traversing the whole tree
would have to be done just to clear the bit. Is it ok?

[pstanek, 2013-08-21 17:35:47]

On 2013/08/21 17:32:16, pstanek wrote:
> On 2013/08/21 17:15:23, tony wrote:
> > This is better, but I'm still worried about m_editablesAlreadySpellChecked
> > growing and never clearing elements that are removed from the page.  Here's
> > another idea: instead of keeping a list of elements we check in Editor,
maybe
> we
> > can set a bit on Element when we check it.  Specifically, it looks like
there
> > are bits available in Node's m_nodeFlags.
> 
> Was thinking about it. The problem is that turning spell checker off/on should
> clear the list. Now it's easy. With a bit in Element traversing the whole tree
> would have to be done just to clear the bit. Is it ok?

Note that other frames' trees must be traversed as well (this helped me to
realize there's a bug here since the list is cleared in one editor only).

[tony, 2013-08-21 17:58:58]

> On 2013/08/21 17:32:16, pstanek wrote:
> > On 2013/08/21 17:15:23, tony wrote:
> > > This is better, but I'm still worried about m_editablesAlreadySpellChecked
> > > growing and never clearing elements that are removed from the page. 
Here's
> > > another idea: instead of keeping a list of elements we check in Editor,
> maybe
> > we
> > > can set a bit on Element when we check it.  Specifically, it looks like
> there
> > > are bits available in Node's m_nodeFlags.
> > 
> > Was thinking about it. The problem is that turning spell checker off/on
should
> > clear the list. Now it's easy. With a bit in Element traversing the whole
tree
> > would have to be done just to clear the bit. Is it ok?

You mean when someone goes to settings, finds and unchecks the "Enable spell
checking" option?  That almost never happens and it's OK to do a full tree walk
then.  According to our user metrics, that option is selected by 0.01% of our
users and I doubt it's something toggled more than once.

[pstanek, 2013-08-21 18:04:42]

On 2013/08/21 17:58:58, tony wrote:
> > On 2013/08/21 17:32:16, pstanek wrote:
> > > On 2013/08/21 17:15:23, tony wrote:
> > > > This is better, but I'm still worried about
m_editablesAlreadySpellChecked
> > > > growing and never clearing elements that are removed from the page. 
> Here's
> > > > another idea: instead of keeping a list of elements we check in Editor,
> > maybe
> > > we
> > > > can set a bit on Element when we check it.  Specifically, it looks like
> > there
> > > > are bits available in Node's m_nodeFlags.
> > > 
> > > Was thinking about it. The problem is that turning spell checker off/on
> should
> > > clear the list. Now it's easy. With a bit in Element traversing the whole
> tree
> > > would have to be done just to clear the bit. Is it ok?
> 
> You mean when someone goes to settings, finds and unchecks the "Enable spell
> checking" option?  That almost never happens and it's OK to do a full tree
walk
> then.  According to our user metrics, that option is selected by 0.01% of our
> users and I doubt it's something toggled more than once.

Yeah but what users want is when they have some text and they switch the spell
checking dictionary the text gets spell checked using the new dictionary. This
could be implemented like: change dictionary; turn spell checker off; turn it
on; (and it'll work thanks to https://codereview.chromium.org/23332004/ ;)). So
how does stats for switching dictionaries look? :)

[tony, 2013-08-21 18:11:31]

On 2013/08/21 18:04:42, pstanek wrote:

> Yeah but what users want is when they have some text and they switch the spell
> checking dictionary the text gets spell checked using the new dictionary. This
> could be implemented like: change dictionary; turn spell checker off; turn it
> on; (and it'll work thanks to https://codereview.chromium.org/23332004/ ;)).
So
> how does stats for switching dictionaries look? :)

Still very small.  In total, about twice as often as disabling spellcheck so
we're talking like 0.03% for either disabling spell check or changing spell
check language.

[pstanek, 2013-08-21 18:12:50]

On 2013/08/21 18:11:31, tony wrote:
> On 2013/08/21 18:04:42, pstanek wrote:
> 
> > Yeah but what users want is when they have some text and they switch the
spell
> > checking dictionary the text gets spell checked using the new dictionary.
This
> > could be implemented like: change dictionary; turn spell checker off; turn
it
> > on; (and it'll work thanks to https://codereview.chromium.org/23332004/ ;)).
> So
> > how does stats for switching dictionaries look? :)
> 
> Still very small.  In total, about twice as often as disabling spellcheck so
> we're talking like 0.03% for either disabling spell check or changing spell
> check language.

Ok. Will change hash set to a bit in Element tomorrow. Stay tuned.

[pstanek, 2013-08-22 14:09:30]

On 2013/08/21 18:12:50, pstanek wrote:
> On 2013/08/21 18:11:31, tony wrote:
> > On 2013/08/21 18:04:42, pstanek wrote:
> > 
> > > Yeah but what users want is when they have some text and they switch the
> spell
> > > checking dictionary the text gets spell checked using the new dictionary.
> This
> > > could be implemented like: change dictionary; turn spell checker off; turn
> it
> > > on; (and it'll work thanks to https://codereview.chromium.org/23332004/
;)).
> > So
> > > how does stats for switching dictionaries look? :)
> > 
> > Still very small.  In total, about twice as often as disabling spellcheck so
> > we're talking like 0.03% for either disabling spell check or changing spell
> > check language.
> 
> Ok. Will change hash set to a bit in Element tomorrow. Stay tuned.

Done. PTAL

[tony, 2013-08-22 18:44:33]

LGTM

https://codereview.chromium.org/21694005/diff/54001/Source/core/editing/Edito...
File Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/21694005/diff/54001/Source/core/editing/Edito...
Source/core/editing/Editor.cpp:1020: if (!isContinuousSpellCheckingEnabled()) {
Nit: If you early return, when enabled, the rest of the code can be indented
less.

https://codereview.chromium.org/21694005/diff/54001/Source/core/editing/Editor.h
File Source/core/editing/Editor.h (right):

https://codereview.chromium.org/21694005/diff/54001/Source/core/editing/Edito...
Source/core/editing/Editor.h:313: // This gets called when any editable element
gets focused.
I would remove this comment. We don't normally include 'what' comments in
headers.

[pstanek, 2013-08-22 19:41:28]

On 2013/08/22 18:44:33, tony wrote:
> LGTM
> 
>
https://codereview.chromium.org/21694005/diff/54001/Source/core/editing/Edito...
> File Source/core/editing/Editor.cpp (right):
> 
>
https://codereview.chromium.org/21694005/diff/54001/Source/core/editing/Edito...
> Source/core/editing/Editor.cpp:1020: if (!isContinuousSpellCheckingEnabled())
{
> Nit: If you early return, when enabled, the rest of the code can be indented
> less.
> 
>
https://codereview.chromium.org/21694005/diff/54001/Source/core/editing/Editor.h
> File Source/core/editing/Editor.h (right):
> 
>
https://codereview.chromium.org/21694005/diff/54001/Source/core/editing/Edito...
> Source/core/editing/Editor.h:313: // This gets called when any editable
element
> gets focused.
> I would remove this comment. We don't normally include 'what' comments in
> headers.

Both done & sending this for integration.

[commit-bot: I haz the power, 2013-08-22 19:43:36]

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

[commit-bot: I haz the power, 2013-08-22 22:19:41]

Change committed as 156583

[ojan, 2013-08-23 01:52:19]

spellcheck-editable-on-focus.html looks to be flaky.

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...

[pstanek, 2013-08-23 11:46:18]

On 2013/08/23 01:52:19, ojan wrote:
> spellcheck-editable-on-focus.html looks to be flaky.
> 
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%25...

Will take a look into this when working on 278119. I can reproduce flakiness but
only when running all editing tests. Does not happen when running
editing/spelling only. Weird.

[please use gerrit instead, 2013-08-23 14:33:14]

On 2013/08/23 11:46:18, pstanek wrote:
> On 2013/08/23 01:52:19, ojan wrote:
> > spellcheck-editable-on-focus.html looks to be flaky.
> > 
> >
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%25...
> 
> Will take a look into this when working on 278119. I can reproduce flakiness
but
> only when running all editing tests. Does not happen when running
> editing/spelling only. Weird.

That sounds like other tests are switching some settings that affect your test,
but you do not reset them. LayoutTests/editing/spelling/resources/util.js, for
example, sets these settings:

    internals.settings.setAsynchronousSpellCheckingEnabled(true);
    internals.settings.setSmartInsertDeleteEnabled(true);
    internals.settings.setSelectTrailingWhitespaceEnabled(false);
    internals.settings.setUnifiedTextCheckerEnabled(true);
    internals.settings.setEditingBehavior("win");

[pstanek, 2013-08-23 14:35:35]

On 2013/08/23 14:33:14, Rouslan Solomakhin wrote:
> On 2013/08/23 11:46:18, pstanek wrote:
> > On 2013/08/23 01:52:19, ojan wrote:
> > > spellcheck-editable-on-focus.html looks to be flaky.
> > > 
> > >
> >
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%25...
> > 
> > Will take a look into this when working on 278119. I can reproduce flakiness
> but
> > only when running all editing tests. Does not happen when running
> > editing/spelling only. Weird.
> 
> That sounds like other tests are switching some settings that affect your
test,
> but you do not reset them. LayoutTests/editing/spelling/resources/util.js, for
> example, sets these settings:
> 
>     internals.settings.setAsynchronousSpellCheckingEnabled(true);
>     internals.settings.setSmartInsertDeleteEnabled(true);
>     internals.settings.setSelectTrailingWhitespaceEnabled(false);
>     internals.settings.setUnifiedTextCheckerEnabled(true);
>     internals.settings.setEditingBehavior("win");

Actually it turned out to be simpler: a bug in the code :).

[pstanek, 2013-08-23 14:37:19]

On 2013/08/23 14:35:35, pstanek wrote:
> On 2013/08/23 14:33:14, Rouslan Solomakhin wrote:
> > On 2013/08/23 11:46:18, pstanek wrote:
> > > On 2013/08/23 01:52:19, ojan wrote:
> > > > spellcheck-editable-on-focus.html looks to be flaky.
> > > > 
> > > >
> > >
> >
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%25...
> > > 
> > > Will take a look into this when working on 278119. I can reproduce
flakiness
> > but
> > > only when running all editing tests. Does not happen when running
> > > editing/spelling only. Weird.
> > 
> > That sounds like other tests are switching some settings that affect your
> test,
> > but you do not reset them. LayoutTests/editing/spelling/resources/util.js,
for
> > example, sets these settings:
> > 
> >     internals.settings.setAsynchronousSpellCheckingEnabled(true);
> >     internals.settings.setSmartInsertDeleteEnabled(true);
> >     internals.settings.setSelectTrailingWhitespaceEnabled(false);
> >     internals.settings.setUnifiedTextCheckerEnabled(true);
> >     internals.settings.setEditingBehavior("win");
> 
> Actually it turned out to be simpler: a bug in the code :).

Test's code that is. Anyway. Fix pushed to
https://codereview.chromium.org/22859062/