Bug 48078 – Editing a word with misspell or autocorrection underline should remove the underline when the editing changes the word.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jia Pu]

After bug 47629, on Mac OS 10.7, when moving cursor onto a word, the underline (if the word has) is not removed. However, if now the user start editing the word, then we need to move the underline first before applying the underline. If after the editing, the word is still misspelled, we will rely on spell checking to add the underline back on.

Note, this is a 10.7 only change. 


At this moment, say we have a word "mispelling" with red misspelling underline. Now if we insert "s" to change it to "misspelling", the result is "mis" and "pelling" with red underline, but not "s". Also, noticed that the word is now correctly spelled. Similar behavior happens to blue autocorrection underline. 

To fix this, if an editing command changes a word that has misspell or autocorrection underline, we need to remove those underlines before applying the editing.

There're 3 scenarios to consider:
1. If the editing happens in the middle of the word. We always remove the underline.
2. If the editing happens at either end of the word, and the editing is to inserting a whitespace, we don't need to remove the underline, because this will not change the word.
3. If the editing happens at either end of the word, and the editing is not simply inserting a whitespace, we remove the underline.

[Jia Pu]

<rdar://problem/8579155>

[Jia Pu]

Created attachment 71510 [details]
Proposed Patch (v1)

Comment on attachment 71510 [details]
Proposed Patch (v1)

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

Seems like it should be possible to have tests to go with this change.

> WebCore/ChangeLog:13
> +        This patch part of on-going improvement of autocorrection feature on Mac OSX. When an editing
> +        occur, if it affect words (by deleting/inserting characters, spliting word, merging words) that
> +        has Spelling and/or CorrectionIndicator markers, we want to remove the markers. If subsequntial
> +        spelling checking found spelling error in newlly formed words, it will add the markers back in.
> +

Some typos in the above: “Mac OS X”, “When editing occurs”, “affects”, “that have Spelling”, “finds”, “newly”.

> WebCore/editing/Editor.cpp:145
> +#if PLATFORM(MAC) && !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)

Not for this patch, but we should consider defining a dedicated macro for all things autocorrection-related and using it instead of the above, for clarity and for other platforms that may want this behavior.

> WebCore/editing/Editor.cpp:146
> +    removeSpellAndCorrectionMarkersFromWordsToBeEdited(isSpaceOrNewline((event->data().characters())[0]));

Is it possible for event->data() to be zero-length?

> WebCore/editing/Editor.cpp:2935
> +    // We want to remove the markers from a word if an editing command will change the word. This can happend in one of

Typo: “happend”.

> WebCore/editing/Editor.cpp:2954
> +    VisiblePosition startOfFirstWord = startOfWord(startOfSelection, LeftWordIfOnBoundary).deepEquivalent();
> +    VisiblePosition endOfFirstWord = endOfWord(startOfSelection, LeftWordIfOnBoundary).deepEquivalent();
> +    // Last word is the word that begins before or on the end of selection
> +    VisiblePosition startOfLastWord = startOfWord(endOfSelection, RightWordIfOnBoundary).deepEquivalent();
> +    VisiblePosition endOfLastWord = endOfWord(endOfSelection, RightWordIfOnBoundary).deepEquivalent();

Why are you using deepEquivalent() here? You are going from a VisiblePosition to a Position and then back to a VisiblePosition.

> WebCore/editing/Editor.cpp:2988
> +    RefPtr<Document> document = m_frame->document();

Any reason to take a reference to the document here?

> WebCore/editing/Editor.cpp:2990
> +    for (TextIterator textIter(wordRange.get()); !textIter.atEnd(); textIter.advance()) {

Please don’t use an abbreviation in the name. “iterator”, or “textIterator” are good names for this variable.

> WebCore/editing/Editor.cpp:3000
> +                    document->markers()->removeMarkers(markerRange.get(), DocumentMarker::Spelling);
> +                    document->markers()->removeMarkers(markerRange.get(), DocumentMarker::CorrectionIndicator);

I don’t think it’s safe to call removeMarkers() while iterating over the markers vector!

> WebCore/editing/Editor.cpp:3015
> +            document->markers()->removeMarkers(node, 0, INT_MAX, DocumentMarker::Spelling);
> +            document->markers()->removeMarkers(node, 0, INT_MAX, DocumentMarker::CorrectionIndicator);

Using numeric_limits<int>::max() is preferred to INT_MAX. But this is not very elegant. Perhaps a version of removeMarkers() that takes a node and a marker type is needed.

[Jia Pu]

(In reply to comment #3)
> (From update of attachment 71510 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71510&action=review
> 
> Seems like it should be possible to have tests to go with this change.

I do plan to have tests. At this moment DumpRenderTree is having trouble to compile using recent AppKit. I will see what I can do.

> 
> > WebCore/editing/Editor.cpp:145
> > +#if PLATFORM(MAC) && !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)
> 
> Not for this patch, but we should consider defining a dedicated macro for all things autocorrection-related and using it instead of the above, for clarity and for other platforms that may want this behavior.

Where should such a macro go? I can add it for this patch, and follow up with another patch which replaces all existing long #if's with the new macro.

> 
> > WebCore/editing/Editor.cpp:146
> > +    removeSpellAndCorrectionMarkersFromWordsToBeEdited(isSpaceOrNewline((event->data().characters())[0]));
> 
> Is it possible for event->data() to be zero-length?

Probably possible. I would add code for that.

> 
> > WebCore/editing/Editor.cpp:2935
> > +    // We want to remove the markers from a word if an editing command will change the word. This can happend in one of
> 
> Typo: “happend”.
> 
> > WebCore/editing/Editor.cpp:2954
> > +    VisiblePosition startOfFirstWord = startOfWord(startOfSelection, LeftWordIfOnBoundary).deepEquivalent();
> > +    VisiblePosition endOfFirstWord = endOfWord(startOfSelection, LeftWordIfOnBoundary).deepEquivalent();
> > +    // Last word is the word that begins before or on the end of selection
> > +    VisiblePosition startOfLastWord = startOfWord(endOfSelection, RightWordIfOnBoundary).deepEquivalent();
> > +    VisiblePosition endOfLastWord = endOfWord(endOfSelection, RightWordIfOnBoundary).deepEquivalent();
> 
> Why are you using deepEquivalent() here? You are going from a VisiblePosition to a Position and then back to a VisiblePosition.

I must have misread the code. Somehow, I thought endOfWord() and startOfWord() take Position, instead of VisiblePosition as argument.


> > WebCore/editing/Editor.cpp:3000
> > +                    document->markers()->removeMarkers(markerRange.get(), DocumentMarker::Spelling);
> > +                    document->markers()->removeMarkers(markerRange.get(), DocumentMarker::CorrectionIndicator);
> 
> I don’t think it’s safe to call removeMarkers() while iterating over the markers vector!

Thanks for pointing that out. 

> 
> > WebCore/editing/Editor.cpp:3015
> > +            document->markers()->removeMarkers(node, 0, INT_MAX, DocumentMarker::Spelling);
> > +            document->markers()->removeMarkers(node, 0, INT_MAX, DocumentMarker::CorrectionIndicator);
> 
> Using numeric_limits<int>::max() is preferred to INT_MAX. But this is not very elegant. Perhaps a version of removeMarkers() that takes a node and a marker type is needed.

Will add such function.

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 71510 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=71510&action=review
> > 
> > Seems like it should be possible to have tests to go with this change.
> 
> I do plan to have tests. At this moment DumpRenderTree is having trouble to compile using recent AppKit. I will see what I can do.

http://trac.webkit.org/changeset/70404 might help.

[Jia Pu]

Created attachment 72135 [details]
Proposed patch (v2)

In addition to changes based on comment #3, there're following major differences compared with previous patch:
1. Refactored DocumentMarkerController::removeMarkers() and added DocumentMarkerController::hasMarker() to make marker manipulation more convenient.
2. Moved some calls to Editor::removeSpellAndCorrectionMarkersFromWordsToBeEdited() from Editor to TypingCommand, so that it gets called by both testing script and actual typing.
3. Added tests.

[Early Warning System Bot]

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

[Jia Pu]

Created attachment 72140 [details]
Proposed patch (v3)

Fixed build failure on non-mac platforms.

Comment on attachment 72140 [details]
Proposed patch (v3)

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

r- because of config.h and the problem in hasMarker().

> WebCore/config.h:176
> +// Some platforms provide UI for suggesting autocorrection.
> +#define SUPPORT_AUTOCORRECTION_PANEL 0
> +// Some platforms use spelling and autocorrection markers to provide visual cue.
> +// On such platform, if word with marker is edited, we need to remove the marker.
> +#define REMOVE_MARKERS_UPON_EDITING 0
> +
>  #if PLATFORM(MAC)
>  // New theme
>  #define WTF_USE_NEW_THEME 1
> +#if !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)
> +#define SUPPORT_AUTOCORRECTION_PANEL 1
> +#define REMOVE_MARKERS_UPON_EDITING 1
> +#endif // !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)
>  #endif // PLATFORM(MAC)

config.h isn’t the right place for defining these things. These are only relevant to one or two files in editing and should probably be defined in some header in editing (that’s included by those files).

> WebCore/dom/DocumentMarkerController.cpp:329
> +    if (iterator == m_markers.end())
> +        return;
> +    removeMarkersFromMarkerMapVectorPair(node, iterator->second, markerType);

In this case, the “early return” actually makes the function longer, since there’s only one statement after the return.

> WebCore/dom/DocumentMarkerController.cpp:535
> +bool DocumentMarkerController::hasMarker(Range *range, const Vector<DocumentMarker::MarkerType>& markerTypes)

The space should go on the other side of the star.

> WebCore/dom/DocumentMarkerController.cpp:546
> +    Node* startContainer = range->startContainer(ec);
> +    if (ec)
> +        return false;
> +    Node* endContainer = range->endContainer(ec);
> +    if (ec)
> +        return false;

I wonder if we can guarantee that the range has start and end containers and replace those returns with assertions.

> WebCore/dom/DocumentMarkerController.cpp:553
> +            for (Vector<DocumentMarker>::const_iterator markerIterator = markers.begin(); markerIterator != end; ++markerIterator) {

markerIterator is a good name, but we often use 'it' as the name of a loop iterator.

> WebCore/dom/DocumentMarkerController.cpp:555
> +                if (foundIndex != notFound && markerIterator->endOffset > static_cast<unsigned>(range->startOffset()) && markerIterator->startOffset < static_cast<unsigned>(range->endOffset()))

This test is wrong. If node is the startContainer, then you don’t care about the endOffset unless the startContainer is also the endContainer.

> WebCore/dom/DocumentMarkerController.cpp:560
> +            Vector<DocumentMarker> markers = markersForNode(node);
> +            Vector<DocumentMarker>::const_iterator end = markers.end();

Instead of repeating these two statements in the if and else blocks, you could just move them outside.

Perhaps it would be simpler to organize everything in a single for loop and test for the edge cases inside the loop.

> WebCore/editing/Editor.cpp:2977
> +    Vector<DocumentMarker::MarkerType> markerTypeToRemove;

This should be called markerTypesToRemove and initialized with a size of 2:
    Vector<DocumentMarker::MarkerType> markerTypesToRemove(2);
then instead of appending you can assign to the 0th and 1st elements.
Better yet, make this a static and make a helper function to initialize this once.

    const Vector<DocumentMarker::MarkerType>& markerTypesToRemove = createMarkerTypesToRemove();

Another idea is to use a bit mask instead of a vector. Checking if the marker type is in the vector is slower than testing a bit mask.

> WebCore/editing/Editor.cpp:2984
> +    Vector<RangeMarkerPair> markerToRemove;

This should be called markersToRemove. To avoid allocation on the heap, you can give it a small inline capacity, e.g.
    Vector<RangeMarkerPair, 16> markerToRemove;

[Jia Pu]

(In reply to comment #9)
> (From update of attachment 72140 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72140&action=review
> 
> r- because of config.h and the problem in hasMarker().
> 
> > WebCore/config.h:176
> > +// Some platforms provide UI for suggesting autocorrection.
> > +#define SUPPORT_AUTOCORRECTION_PANEL 0
> > +// Some platforms use spelling and autocorrection markers to provide visual cue.
> > +// On such platform, if word with marker is edited, we need to remove the marker.
> > +#define REMOVE_MARKERS_UPON_EDITING 0
> > +
> >  #if PLATFORM(MAC)
> >  // New theme
> >  #define WTF_USE_NEW_THEME 1
> > +#if !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)
> > +#define SUPPORT_AUTOCORRECTION_PANEL 1
> > +#define REMOVE_MARKERS_UPON_EDITING 1
> > +#endif // !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)
> >  #endif // PLATFORM(MAC)
> 
> config.h isn’t the right place for defining these things. These are only relevant to one or two files in editing and should probably be defined in some header in editing (that’s included by those files).

Moving these macros to Editor.h. 

> > WebCore/dom/DocumentMarkerController.cpp:546
> > +    Node* startContainer = range->startContainer(ec);
> > +    if (ec)
> > +        return false;
> > +    Node* endContainer = range->endContainer(ec);
> > +    if (ec)
> > +        return false;
> 
> I wonder if we can guarantee that the range has start and end containers and replace those returns with assertions.

This is indeed guaranteed at call site. Using assertions instead.

> > WebCore/editing/Editor.cpp:2977
> > +    Vector<DocumentMarker::MarkerType> markerTypeToRemove;
> 
> This should be called markerTypesToRemove and initialized with a size of 2:
>     Vector<DocumentMarker::MarkerType> markerTypesToRemove(2);
> then instead of appending you can assign to the 0th and 1st elements.
> Better yet, make this a static and make a helper function to initialize this once.
> 
>     const Vector<DocumentMarker::MarkerType>& markerTypesToRemove = createMarkerTypesToRemove();
> 
> Another idea is to use a bit mask instead of a vector. Checking if the marker type is in the vector is slower than testing a bit mask.

Changing DocumentMarker::MarkerType from regular enum to bit masks.

[Jia Pu]

Created attachment 72204 [details]
Proposed patch (v4)

Addressed issues in comment #9.

Comment on attachment 72204 [details]
Proposed patch (v4)

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

This is very close but I still have a few comments.

> WebCore/dom/DocumentMarker.h:36
> +    // Changed MarkerType from regular enum to bit mask to make searching for multiple types easier.

This comment is appropriate for a change log, but it will not make sense to anyone reading this file in the future. Just remove it.

> WebCore/dom/DocumentMarker.h:46
>      MarkerType type;

You should also typedef unsigned MarkerTypes.

> WebCore/dom/DocumentMarkerController.cpp:534
> +bool DocumentMarkerController::hasMarkers(Range* range, unsigned markerTypes)

Use the MarkerTypes typedef here.

> WebCore/editing/Editor.cpp:2977
> +    static const unsigned markerTypesToRemove = DocumentMarker::Spelling | DocumentMarker::CorrectionIndicator;

“static” doesn’t do anything here. I’d not use a variable at all for this, but that’s just a matter of taste.

[Early Warning System Bot]

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

[Early Warning System Bot]

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

[Jia Pu]

Created attachment 72240 [details]
Proposed patch (v5)

Revised per comment #12. And Fixed build issue on Qt.

[WebKit Commit Bot]

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

Clearing flags on attachment: 72240

Committed r70826: <http://trac.webkit.org/changeset/70826>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/70826 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
media/video-seek-by-small-increment.html

[Jia Pu]

(In reply to comment #18)
> http://trac.webkit.org/changeset/70826 might have broken GTK Linux 64-bit Debug
> The following tests are not passing:
> media/video-seek-by-small-increment.html

Is there to find out more about this failure, such as expected and actual result?