Bug 53255 – Reversion should not be marked as misspelled.

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

[Jia Pu]

Just like in AppKit, 
1. If the user explicitly reverts a correction, we should not mark that instance as misspelled. 
2. However, if the user subsequently edits it, we would want to mark it as usual.

<rdar://problem/8636726>

[Jia Pu]

Created attachment 80372 [details]
Proposed patch (v1)

[Jia Pu]

hmm, is there something terribly wrong? maybe merge conflicts?

[Jia Pu]

Created attachment 80413 [details]
Proposed patch (v1)

Re-submitting patch v1 due to the mysterious hiccup of build bots the first time.

[Jia Pu]

Created attachment 80458 [details]
Patch

Comment on attachment 80458 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80458&action=review

I am going to say r- because of the hasMarkers() change. I don’t understand why it’s needed and it can certainly be dealt with separately from this bug.

> Source/WebCore/ChangeLog:13
> +           is achieved by explicitly apply pending correction when user types space, line break or

explicitly applying?

> Source/WebCore/ChangeLog:43
> +        (WebCore::markerTypesForAutocorrection): Add SpellCheckingExemption marker when apply correction.

applying!

> Source/WebCore/ChangeLog:53
> +        (WebCore::Editor::markAllMisspellingsAndBadGrammarInRanges): Don't marker mispelling if the

typo: marker -> mark

> Source/WebCore/dom/DocumentMarker.h:44
> +        // correction. Text with Replacement marker don't necessarilly has CorrectionIndicator

typo: don't -> doesn't, necessarilly -> necessarily

> Source/WebCore/dom/DocumentMarker.h:45
> +        // marker. For instance, after some text has been corrected, it will has both Replacement

has->have

> Source/WebCore/dom/DocumentMarkerController.cpp:591
> -    Node* startContainer = range->startContainer();
> -    ASSERT(startContainer);
> -    Node* endContainer = range->endContainer();
> -    ASSERT(endContainer);
> -
> -    Node* pastLastNode = range->pastLastNode();
> -    for (Node* node = range->firstNode(); node != pastLastNode; node = node->traverseNextNode()) {
> +    TextIterator it(range);
> +
> +    // Need special handling on first and last node.
> +    RefPtr<Range> firstRange = it.range();
> +    Node* firstNode = firstRange->startContainer();
> +    RefPtr<Range> previousRange = firstRange;
> +    it.advance();
> +    for (; !it.atEnd(); it.advance()) {
> +        Node* node = previousRange->startContainer();
> +        // In this loop, we check every node between (excluding) the first and last node.
> +        if (node == firstNode)
> +            continue;
>          Vector<DocumentMarker> markers = markersForNode(node);
> -        Vector<DocumentMarker>::const_iterator end = markers.end();
> -        for (Vector<DocumentMarker>::const_iterator it = markers.begin(); it != end; ++it) {
> -            if (!(markerTypes & it->type))
> -                continue;
> -            if (node == startContainer && node == endContainer) {
> -                // The range spans only one node.
> -                if (it->endOffset > static_cast<unsigned>(range->startOffset()) && it->startOffset < static_cast<unsigned>(range->endOffset()))
> -                    return true;
> -            } else {
> -                if (node == startContainer) {
> -                    if (it->endOffset > static_cast<unsigned>(range->startOffset()))
> -                        return true;
> -                } else if (node == endContainer) {
> -                    if (it->startOffset < static_cast<unsigned>(range->endOffset()))
> -                        return true;
> -                } else
> -                    return true;
> -            }
> +        size_t size = markers.size();
> +        for (size_t i = 0; i < size; ++i) {
> +            const DocumentMarker& marker = markers[i];
> +            if (marker.type & markerTypes)
> +                return true;
> +        }
> +        previousRange = it.range();
> +    }
> +
> +    RefPtr<Range> lastRange = previousRange;
> +    Node* lastNode = lastRange->startContainer();
> +
> +    // Check the first node.
> +    Vector<DocumentMarker> markers = markersForNode(firstNode);
> +    size_t size = markers.size();
> +    for (size_t i = 0; i < size; ++i) {
> +        const DocumentMarker& marker = markers[i];
> +        if (!(marker.type & markerTypes))
> +            continue;
> +        if (firstNode == lastNode) {
> +            if (marker.endOffset > static_cast<unsigned>(firstRange->startOffset()) && marker.startOffset < static_cast<unsigned>(lastRange->endOffset()))
> +                return true;
> +        } else {
> +            if (marker.endOffset > static_cast<unsigned>(firstRange->startOffset()))
> +                return true;
>          }
>      }
> +
> +    if (firstNode == lastNode)
> +        return false;
> +    // Didn't find relevant mark in all other nodes. Now check the last node.
> +    markers = markersForNode(lastNode);
> +    size = markers.size();
> +    for (size_t i = 0; i < size; ++i) {
> +        const DocumentMarker& marker = markers[i];
> +        if (!(marker.type & markerTypes))
> +            continue;
> +        if (marker.startOffset < static_cast<unsigned>(lastRange->endOffset()))
> +            return true;
> +    }
> +

The new code is more complex and harder to follow than the old code. I also don’t think that a TextIterator is more efficient than node traversal.

> Source/WebCore/editing/Editor.cpp:2678
> +    document->markers()->removeMarkers(wordRange.get(), DocumentMarker::Spelling | DocumentMarker::CorrectionIndicator | DocumentMarker::SpellCheckingExemption, true);

This mysterious “true” at the call site suggests that the parameter should be an enum.

> Source/WebCore/editing/Editor.cpp:2736
> +    // Only apply correction if current correction panel type is PanelTypeCorrection.

This comment is redundant with the following code.

> Source/WebCore/editing/Editor.cpp:2740
> +    // Only apply correction if the caret is at the end of the range to be replaced.

Even this comment doesn’t add much. Perhaps you could explain the motivation instead.

[Jia Pu]

(In reply to comment #5)
> (From update of attachment 80458 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80458&action=review
> 
> I am going to say r- because of the hasMarkers() change. I don’t understand why it’s needed and it can certainly be dealt with separately from this bug.
> 
> > Source/WebCore/dom/DocumentMarkerController.cpp:591
> > -    Node* startContainer = range->startContainer();
> > -    ASSERT(startContainer);
> > -    Node* endContainer = range->endContainer();
> > -    ASSERT(endContainer);
> > -
> > -    Node* pastLastNode = range->pastLastNode();
> > -    for (Node* node = range->firstNode(); node != pastLastNode; node = node->traverseNextNode()) {
> > +    TextIterator it(range);
> > +
> > +    // Need special handling on first and last node.
> > +    RefPtr<Range> firstRange = it.range();
> > +    Node* firstNode = firstRange->startContainer();
> > +    RefPtr<Range> previousRange = firstRange;
> > +    it.advance();
> > +    for (; !it.atEnd(); it.advance()) {
> > +        Node* node = previousRange->startContainer();
> > +        // In this loop, we check every node between (excluding) the first and last node.
> > +        if (node == firstNode)
> > +            continue;
> >          Vector<DocumentMarker> markers = markersForNode(node);
> > -        Vector<DocumentMarker>::const_iterator end = markers.end();
> > -        for (Vector<DocumentMarker>::const_iterator it = markers.begin(); it != end; ++it) {
> > -            if (!(markerTypes & it->type))
> > -                continue;
> > -            if (node == startContainer && node == endContainer) {
> > -                // The range spans only one node.
> > -                if (it->endOffset > static_cast<unsigned>(range->startOffset()) && it->startOffset < static_cast<unsigned>(range->endOffset()))
> > -                    return true;
> > -            } else {
> > -                if (node == startContainer) {
> > -                    if (it->endOffset > static_cast<unsigned>(range->startOffset()))
> > -                        return true;
> > -                } else if (node == endContainer) {
> > -                    if (it->startOffset < static_cast<unsigned>(range->endOffset()))
> > -                        return true;
> > -                } else
> > -                    return true;
> > -            }
> > +        size_t size = markers.size();
> > +        for (size_t i = 0; i < size; ++i) {
> > +            const DocumentMarker& marker = markers[i];
> > +            if (marker.type & markerTypes)
> > +                return true;
> > +        }
> > +        previousRange = it.range();
> > +    }
> > +
> > +    RefPtr<Range> lastRange = previousRange;
> > +    Node* lastNode = lastRange->startContainer();
> > +
> > +    // Check the first node.
> > +    Vector<DocumentMarker> markers = markersForNode(firstNode);
> > +    size_t size = markers.size();
> > +    for (size_t i = 0; i < size; ++i) {
> > +        const DocumentMarker& marker = markers[i];
> > +        if (!(marker.type & markerTypes))
> > +            continue;
> > +        if (firstNode == lastNode) {
> > +            if (marker.endOffset > static_cast<unsigned>(firstRange->startOffset()) && marker.startOffset < static_cast<unsigned>(lastRange->endOffset()))
> > +                return true;
> > +        } else {
> > +            if (marker.endOffset > static_cast<unsigned>(firstRange->startOffset()))
> > +                return true;
> >          }
> >      }
> > +
> > +    if (firstNode == lastNode)
> > +        return false;
> > +    // Didn't find relevant mark in all other nodes. Now check the last node.
> > +    markers = markersForNode(lastNode);
> > +    size = markers.size();
> > +    for (size_t i = 0; i < size; ++i) {
> > +        const DocumentMarker& marker = markers[i];
> > +        if (!(marker.type & markerTypes))
> > +            continue;
> > +        if (marker.startOffset < static_cast<unsigned>(lastRange->endOffset()))
> > +            return true;
> > +    }
> > +
> 
> The new code is more complex and harder to follow than the old code. I also don’t think that a TextIterator is more efficient than node traversal.

The concern here is mainly about correctness, rather than efficiency. My understanding is that TextIterator guarantees to iterate on leave text nodes, while regular node iteration doesn't decsend into children node. Seeing how addMarker() is implemented, I thought using TextIterator is the correct thing to do. If this change is entirely unnecessary, I could pull out the change on that function. If motivation is plausible, I will try to restructure the code for better readability.

[Jia Pu]

Created attachment 80902 [details]
Proposed patch (v2)

Updated patch based on comment 5.

[Jia Pu]

(In reply to comment #5)
> (From update of attachment 80458 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80458&action=review
> 
> > Source/WebCore/editing/Editor.cpp:2740
> > +    // Only apply correction if the caret is at the end of the range to be replaced.
> 
> Even this comment doesn’t add much. Perhaps you could explain the motivation instead.

The if statement probably should always evaluate to true. But since I'm not 100% sure that will be the case given the complexity of editing code, I added it to guard against upexpected case. I have rephrased the comment a bit.

Also, I just noticed that the name of this function really should be applyAutocorrectionBeforeTypingIfAppropriate() instead of applyAutocorrectionAfterTypingIfAppropriate(). I have changed it in latest patch.

Comment on attachment 80902 [details]
Proposed patch (v2)

View in context: https://bugs.webkit.org/attachment.cgi?id=80902&action=review

Looks good. One comment/question though.

> Source/WebCore/editing/Editor.cpp:2745
> +    else
> +        // Pending correction should always be where caret is. But in case this is not always true, we still want to dismiss the panel without accepting the correction.
> +        dismissCorrectionPanel(ReasonForDismissingCorrectionPanelIgnored);
> +}

Since this is a 2-line else block, it should have braces around it. More importantly, though, by “should always be where [the] caret is”, do you mean it’s a programming mistake if it isn’t? In that case, there should be an assertion here (but we can keep the code that recovers from this situation gracefully for release builds).

[Jia Pu]

Created attachment 81146 [details]
Proposed patch (v3)

new patch based on comment 9.

Fixed in <http://trac.webkit.org/changeset/77577>.