Summary: | execDeleteCommand() does not update spellchecker sometimes | ||
---|---|---|---|
Product: | WebKit | Reporter: | Grzegorz Czajkowski <g.czajkowski> |
Component: | HTML Editing | Assignee: | Grzegorz Czajkowski <g.czajkowski> |
Status: | NEW --- | ||
Severity: | Normal | CC: | buildbot, darin, rniwa |
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | Unspecified | ||
OS: | Unspecified | ||
Bug Depends on: | 133939 | ||
Bug Blocks: | |||
Attachments: |
Description
Grzegorz Czajkowski
2014-12-22 07:53:54 PST
editing/spelling/grammar-edit-word.html crashes 100% of time. Updated test expectation in <http://trac.webkit.org/r177837>. (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 ~~~~~~~~~ Created attachment 245136 [details]
proposed patch to discuss
Created attachment 245138 [details]
proposed patch to discuss - rebased
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 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
(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. Created attachment 245218 [details]
Make edit-dictated-text-with-alternative.html happy
Created attachment 245220 [details]
Make edit-dictated-text-with-alternative.html happy
ping reviewers ? I'm not sure whether it's correct fix. I'd appreciate your view on it. Might be updateEditorUI() related. Created attachment 246641 [details]
test case
(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 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 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.
|