Bug 49423 – Regression: contraction is considered misspelling.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jia Pu]

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>

[Jia Pu]

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.

[Jia Pu]

(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.

[Jia Pu]

(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.

[Jia Pu]

Created attachment 73804 [details]
Proposed patch (v2)

1. Revised patch according to comment #2.
2. Regenerated patch relative to r71955 to resolve conflicts.

[Early Warning System Bot]

Attachment 73804 [details] did not build on qt:
Build output: http://queues.webkit.org/results/5990004

[Jia Pu]

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.

[Jia Pu]

(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.

[Jia Pu]

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.

[Eric Seidel (no email)]

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.

[WebKit Commit Bot]

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 [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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

[Jia Pu]

Created attachment 74387 [details]
Proposed patch (v5)

Fixed tab vs. space issue.

[WebKit Commit Bot]

Comment on attachment 74387 [details]
Proposed patch (v5)

Clearing flags on attachment: 74387

Committed r72469: <http://trac.webkit.org/changeset/72469>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.