Bug 139862 - execDeleteCommand() does not update spellchecker sometimes
Summary: execDeleteCommand() does not update spellchecker sometimes
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Grzegorz Czajkowski
URL:
Keywords:
Depends on: 133939
Blocks:
  Show dependency treegraph
 
Reported: 2014-12-22 07:53 PST by Grzegorz Czajkowski
Modified: 2015-03-04 05:48 PST (History)
3 users (show)

See Also:


Attachments
proposed patch to discuss (25.32 KB, patch)
2015-01-22 03:38 PST, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff
proposed patch to discuss - rebased (25.38 KB, patch)
2015-01-22 04:49 PST, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-mavericks (519.15 KB, application/zip)
2015-01-22 05:38 PST, Build Bot
no flags Details
Make edit-dictated-text-with-alternative.html happy (25.76 KB, patch)
2015-01-23 01:28 PST, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff
Make edit-dictated-text-with-alternative.html happy (25.58 KB, patch)
2015-01-23 03:45 PST, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff
test case (634 bytes, text/html)
2015-02-16 03:16 PST, Grzegorz Czajkowski
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Grzegorz Czajkowski 2014-12-22 07:53:54 PST
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()
Comment 1 Alexey Proskuryakov 2014-12-31 13:22:08 PST
editing/spelling/grammar-edit-word.html crashes 100% of time. Updated test expectation in <http://trac.webkit.org/r177837>.
Comment 2 Grzegorz Czajkowski 2015-01-22 03:34:23 PST
(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
       ~~~~~~~~~
Comment 3 Grzegorz Czajkowski 2015-01-22 03:38:47 PST
Created attachment 245136 [details]
proposed patch to discuss
Comment 4 Grzegorz Czajkowski 2015-01-22 04:49:01 PST
Created attachment 245138 [details]
proposed patch to discuss - rebased
Comment 5 Build Bot 2015-01-22 05:38:36 PST
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
Comment 6 Build Bot 2015-01-22 05:38:38 PST
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
Comment 7 Grzegorz Czajkowski 2015-01-22 06:02:39 PST
(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.
Comment 8 Grzegorz Czajkowski 2015-01-23 01:28:12 PST
Created attachment 245218 [details]
Make edit-dictated-text-with-alternative.html happy
Comment 9 Grzegorz Czajkowski 2015-01-23 03:45:25 PST
Created attachment 245220 [details]
Make edit-dictated-text-with-alternative.html happy
Comment 10 Grzegorz Czajkowski 2015-01-28 02:19:20 PST
ping reviewers ?

I'm not sure whether it's correct fix. I'd appreciate your view on it.
Might be updateEditorUI() related.
Comment 11 Grzegorz Czajkowski 2015-02-16 03:16:29 PST
Created attachment 246641 [details]
test case
Comment 12 Grzegorz Czajkowski 2015-02-16 04:51:31 PST
(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!
Comment 13 Ryosuke Niwa 2015-03-03 21:12:12 PST
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.
Comment 14 Grzegorz Czajkowski 2015-03-04 05:48:40 PST
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.