This is a regression introduced by the patch for bug 46986. Two issues need to be fixed. 1. In word "shouldn't", the "shouldn'" part is marked as misspelled. 2. After typing "wouldn", wait for autocorrection panel to show up suggesting "would". Now if user types "'", the autocorrection suggestion will be applied. But it shouldn't. <rdar://problem/8654206>
Created attachment 73743 [details] Proposed patch (v1)
Comment on attachment 73743 [details] Proposed patch (v1) View in context: https://bugs.webkit.org/attachment.cgi?id=73743&action=review > WebCore/ChangeLog:15 > + (WebCore::Editor::markMisspellingsAfterTypingToWord): Renamed markAllMisspellingsAndBadGrammarInRanges() I think you meant to say “Renamed markMisspellingsAfterTypingToPosition()”. > WebCore/ChangeLog:20 > + (WebCore::Editor::markAllMisspellingsAndBadGrammarInRanges): Renamed. Um… I don’t think you did. > WebCore/editing/Editor.cpp:2031 > + if (isAmbiguousBoundaryCharacter(wordText[wordText.length()-1])) There should be spaces around the - sign. But more importantly, I think it’s worth checking that wordText.length() is non-zero first, even though I can’t think of when the word range would contain no text. > WebCore/editing/Editor.cpp:2274 > + if (shouldMarkSpelling && result->type == TextCheckingTypeSpelling && resultLocation >= spellingRangeStartOffset && resultLocation + resultLength <= spellingRangeEndOffset && (ambiguousBoundaryOffset || resultLocation + resultLength != ambiguousBoundaryOffset)) { I don’t understand this condition, and I don’t see this change explained in the change log. As far as I can tell, the or expression always evaluates to true.
(In reply to comment #2) > (From update of attachment 73743 [details]) > > > WebCore/editing/Editor.cpp:2274 > > + if (shouldMarkSpelling && result->type == TextCheckingTypeSpelling && resultLocation >= spellingRangeStartOffset && resultLocation + resultLength <= spellingRangeEndOffset && (ambiguousBoundaryOffset || resultLocation + resultLength != ambiguousBoundaryOffset)) { > > I don’t understand this condition, and I don’t see this change explained in the change log. As far as I can tell, the or expression always evaluates to true. Mutliple results can be returned from text checking. And not all of them involve the ambiguous boundary. For instance, given phrase "This probelm wouldn'", the condition in question is only true for the spelling result "wouldn' -> would'", but not for "probelm -> problem". Since for "probelm", resultLocation + resultLength < ambiguousBoundaryOffset.
(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 73743 [details] [details]) > > > > > WebCore/editing/Editor.cpp:2274 > > > + if (shouldMarkSpelling && result->type == TextCheckingTypeSpelling && resultLocation >= spellingRangeStartOffset && resultLocation + resultLength <= spellingRangeEndOffset && (ambiguousBoundaryOffset || resultLocation + resultLength != ambiguousBoundaryOffset)) { > > > > I don’t understand this condition, and I don’t see this change explained in the change log. As far as I can tell, the or expression always evaluates to true. > > Mutliple results can be returned from text checking. And not all of them involve the ambiguous boundary. > > For instance, given phrase "This probelm wouldn'", the condition in question is only true for the spelling result "wouldn' -> would'", but not for "probelm -> problem". Since for "probelm", resultLocation + resultLength < ambiguousBoundaryOffset. Ah, never mind, I see now that the logic is wrong for what I'm trying to check.
Created attachment 73804 [details] Proposed patch (v2) 1. Revised patch according to comment #2. 2. Regenerated patch relative to r71955 to resolve conflicts.
Attachment 73804 [details] did not build on qt: Build output: http://queues.webkit.org/results/5990004
Created attachment 73806 [details] Proposed patch (v3) Fixed build failure on non-Mac platforms.
Comment on attachment 73806 [details] Proposed patch (v3) View in context: https://bugs.webkit.org/attachment.cgi?id=73806&action=review > WebCore/ChangeLog:13 > + (WebCore::isAmbiguousBoundaryCharacter): Moved function to the top of the file so that it. So that it what? :) > WebCore/ChangeLog:20 > + is not a partial contraction, such as "wouldn'", before mark it as misspelled. Also update update what? > WebCore/editing/Editor.cpp:2021 > +void Editor::markMisspellingsAfterTypingToWord(const VisiblePosition &wordStart, const VisibleSelection& selectionAfterTyping) Can we ASSERT_ARG here that selectionAfterTyping is a caret selection? > WebCore/editing/Editor.cpp:2057 > + VisibleSelection adjacentWords = VisibleSelection(startOfWord(wordStart, LeftWordIfOnBoundary), endOfWord(wordStart, RightWordIfOnBoundary)); You don’t need to call startOfWord() on wordStart, because the only caller passes a word start already. Maybe this should be ASSERT_ARG()ed right at the beginning. Anyway, your patch just made this obvious by improving the naming. You don’t need to change anything now. > WebCore/editing/Editor.cpp:2071 > + markMisspellings(VisibleSelection(startOfWord(wordStart, LeftWordIfOnBoundary), endOfWord(wordStart, RightWordIfOnBoundary)), misspellingRange); Ditto.
(In reply to comment #8) > (From update of attachment 73806 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=73806&action=review > > > > WebCore/editing/Editor.cpp:2021 > > +void Editor::markMisspellingsAfterTypingToWord(const VisiblePosition &wordStart, const VisibleSelection& selectionAfterTyping) > > Can we ASSERT_ARG here that selectionAfterTyping is a caret selection? Not sure we can assert that since the endingSelection() is implemented in EditCommand which probably involves many code paths that I'm not familiar with. > > > WebCore/editing/Editor.cpp:2057 > > + VisibleSelection adjacentWords = VisibleSelection(startOfWord(wordStart, LeftWordIfOnBoundary), endOfWord(wordStart, RightWordIfOnBoundary)); > > You don’t need to call startOfWord() on wordStart, because the only caller passes a word start already. Maybe this should be ASSERT_ARG()ed right at the beginning. Anyway, your patch just made this obvious by improving the naming. You don’t need to change anything now. It seems we will have to call startOfWord() to assert. Or we could just assume that the caller passes in correct value.
Created attachment 74155 [details] Proposed patch (v4) Fixed typos in change log.
(In reply to comment #9) > > > WebCore/editing/Editor.cpp:2057 > > > + VisibleSelection adjacentWords = VisibleSelection(startOfWord(wordStart, LeftWordIfOnBoundary), endOfWord(wordStart, RightWordIfOnBoundary)); > > > > You don’t need to call startOfWord() on wordStart, because the only caller passes a word start already. Maybe this should be ASSERT_ARG()ed right at the beginning. Anyway, your patch just made this obvious by improving the naming. You don’t need to change anything now. > > It seems we will have to call startOfWord() to assert. Or we could just assume that the caller passes in correct value. It’s okay to call startOfWord() in an assertion, since that will only happen in debug builds.
Comment on attachment 73806 [details] Proposed patch (v3) Cleared Dan Bernstein's review+ from obsolete attachment 73806 [details] so that this bug does not appear in http://webkit.org/pending-commit.
The commit-queue encountered the following flaky tests while processing attachment 74155 [details]: fast/preloader/script.html compositing/iframes/overlapped-nested-iframes.html Please file bugs against the tests. These tests were authored by abarth@webkit.org and simon.fraser@apple.com. The commit-queue is continuing to process your patch.
Comment on attachment 74155 [details] Proposed patch (v4) Rejecting patch 74155 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'land-attachment', '--force-clean', '--ignore-builders', '--non-interactive', '--parent-command=commit-queue', 74155]" exit_code: 2 Last 500 characters of output: ocorrection-contraction.html A repository hook failed: Commit blocked by pre-commit hook (exit code 1) with output: The following files contain tab characters: trunk/LayoutTests/platform/mac/editing/spelling/autocorrection-contraction.html Please use spaces instead to indent. If you must commit a file with tabs, use svn propset to set the "allow-tabs" property. at /usr/local/git/libexec/git-core/git-svn line 572 Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1 Full output: http://queues.webkit.org/results/6237061
Created attachment 74387 [details] Proposed patch (v5) Fixed tab vs. space issue.
Comment on attachment 74387 [details] Proposed patch (v5) Clearing flags on attachment: 74387 Committed r72469: <http://trac.webkit.org/changeset/72469>
All reviewed patches have been landed. Closing bug.