|Summary:||execDeleteCommand() does not update spellchecker sometimes|
|Product:||WebKit||Reporter:||Grzegorz Czajkowski <g.czajkowski>|
|Component:||HTML Editing||Assignee:||Grzegorz Czajkowski <g.czajkowski>|
|Severity:||Normal||CC:||buildbot, darin, rniwa|
|Version:||528+ (Nightly build)|
|Bug Depends on:||133939|
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.