Bug 139862 – execDeleteCommand() does not update spellchecker sometimes

• Component: HTML Editing
• Status: NEW

[Grzegorz Czajkowski]

The following tests are failing:

LayoutTests/editing/spelling/editing-multiple-words-with-markers.html
LayoutTests/editing/spelling/grammar-edit-word.html

There are three issues:

1. In one task in TypingCommand::deleteKeyPressed we are doing two selection changes:

deleteSelection(selectionToDelete, m_smartDelete);
   calls respondToChangedSelection() and editorUIUpdateTimer.startOneShot(0)

typingAddedToOpenCommand(DeleteKey); 
   calls respondToChangedSelection() but editorUIUpdateTimer is still active and we do an early return and do not notify spellchecker about this selection change.

Invoking updateEditorUINowIfScheduled() does not help after applying command as we already lost one selection change.

2. Internals::hasGrammarMarker() does not invoke updateEditorUINowIfScheduled()

3. After non-editing selection changes the tests should invoke internals.updateEditorUINowIfScheduled()

[Alexey Proskuryakov]

editing/spelling/grammar-edit-word.html crashes 100% of time. Updated test expectation in <http://trac.webkit.org/r177837>.

[Grzegorz Czajkowski]

(In reply to comment #0)
> The following tests are failing:
> 
> LayoutTests/editing/spelling/editing-multiple-words-with-markers.html
> LayoutTests/editing/spelling/grammar-edit-word.html
> 
> There are three issues:
> 
> 1. In one task in TypingCommand::deleteKeyPressed we are doing two selection
> changes:
> 
> deleteSelection(selectionToDelete, m_smartDelete);
>    calls respondToChangedSelection() and editorUIUpdateTimer.startOneShot(0)
> 
> typingAddedToOpenCommand(DeleteKey); 
>    calls respondToChangedSelection() but editorUIUpdateTimer is still active
> and we do an early return and do not notify spellchecker about this
> selection change.
> 
> Invoking updateEditorUINowIfScheduled() does not help after applying command
> as we already lost one selection change.
> 

Updated description as I was debugging it:
On asynchronous spellchecker, the response containing the positions and lengths of potential markers may include results which are not sill valid besides the currently present text is not supposed to have a markers.

That happens when selection change causes a caret position inside the word.
In this case WebKit should not render spelling/grammar markers. For example,

User types:
it's a meagesga meagesga   (async spellcheck request #1)

User selects a range:
it's a meag|esga meag|esga

User deletes a range:
it's a meag^esga           (^ - caret position)

Spellchecker gets response for request #1 and applies it!
it's a meag^esga
       ~~~~~~~~~

[Grzegorz Czajkowski]

Created attachment 245136 [details]
proposed patch to discuss

[Grzegorz Czajkowski]

Created attachment 245138 [details]
proposed patch to discuss - rebased

[Build Bot]

Comment on attachment 245138 [details]
proposed patch to discuss - rebased

Attachment 245138 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4760864099926016

New failing tests:
platform/mac/editing/input/edit-dictated-text-with-alternative.html

[Build Bot]

Created attachment 245139 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Grzegorz Czajkowski]

(In reply to comment #5)
> Comment on attachment 245138 [details]
> proposed patch to discuss - rebased
> 
> Attachment 245138 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.appspot.com/results/4760864099926016
> 
> New failing tests:
> platform/mac/editing/input/edit-dictated-text-with-alternative.html

Need to have a look at it.

[Grzegorz Czajkowski]

Created attachment 245218 [details]
Make edit-dictated-text-with-alternative.html happy

[Grzegorz Czajkowski]

Created attachment 245220 [details]
Make edit-dictated-text-with-alternative.html happy

[Grzegorz Czajkowski]

ping reviewers ?

I'm not sure whether it's correct fix. I'd appreciate your view on it.
Might be updateEditorUI() related.

[Grzegorz Czajkowski]

Created attachment 246641 [details]
test case

[Grzegorz Czajkowski]

(In reply to comment #11)
> Created attachment 246641 [details]
> test case

document.execCommand("InsertText", false, "it's a meagesga meagesga ");
// a few spellcheker requests are sent while typing.
// at this time async spellchecker did not get any response to apply markers

testElement.setSelectionRange(11, 20);
// partially selects two misspelled words (the are the same)
// "it's a meag|esga meag|esga "
// at this time async spellchecker did not get any response to apply markers

document.execCommand("Delete");
// deletes selection which in fact creates a word for which spellchecker was already invoked
// while typing ("meagesga") and applies it.
// This is unexpected behaviour as the caret is placed in the middle of the word
// which means that spelling markers should disappear.

This is reproducible via scripts only!

[Ryosuke Niwa]

Comment on attachment 245220 [details]
Make edit-dictated-text-with-alternative.html happy

View in context: https://bugs.webkit.org/attachment.cgi?id=245220&action=review

> Source/WebCore/ChangeLog:32
> +        Since closing spellchecker's request in this particular case can lead to skipping
> +        valid markers reuse updateMarkersForWordsAffectedByEditing() functionality to fix
> +        this issue.
> +
> +        After deleting selection, keep a 'Range' of selection for which markers are not
> +        allowed to be drawn and respect it when the response from spellchecker comes.

I don't think keeping a single range is sufficient. It's possible for authors or some page scripts to delete multiple nodes.

I can think of two approaches:
1. Store the range for which an async spellchecking is requested, and ignore the result if the DOM inside that range is ever modified, and re-request spellchecking.
2. Store multiple ranges while an async spellchecking is in fly, and update the results as needed when the results come back.

Both approaches have some drawbacks. 1 is very simple in terms of implementation but it could result in deletion & spellchecking code racing each other and never showing spellchecking.

2 on the other hand is very complicated to implement and could result in "incorrect" spellchecking since grammar, spelling, etc… could change as the user (or in-page spellchecker) edits the text but it doesn't waste the results of async spellchecking as much.

> Source/WebCore/editing/TypingCommand.cpp:634
> +
> +    // FIXME:: consider calling prohibitIncomingMarkersFromApplyingAfterDeletingRange.

We should definitely call prohibitIncomingMarkersFromApplyingAfterDeletingRange here.
You can easily create a test case by using ForwardDelete command.

[Grzegorz Czajkowski]

Comment on attachment 245220 [details]
Make edit-dictated-text-with-alternative.html happy

Thanks you for comments. I'll take all of them into account. 
Clearing review flag until next patch is ready.