[Webkit-unassigned] [Bug 48078] Editing a word with misspell or autocorrection underline should remove the underline when the editing changes the word.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=48078





--- Comment #4 from jpu at apple.com  2010-10-25 10:50:04 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.