<rdar://problem/41112433>
To reproduce: - Go to System Preferences > Keyboard > Text. - Add a text replacement from "yt?" to "you there?". - In Safari (or any other WebKit-based app), focus an editable area. - Type "yt?", and don't press space or enter. Expected: The editable area should contain "yt?". Subsequently inserting a space or newline should then replace "yt?" with "you there?". Observed: Autocorrection kicks in and changes "yt?" to "you there?" immediately.
This also reproduces on shipping macOS, but only if grammar checking is enabled.
Created attachment 344029 [details] Patch
Created attachment 344033 [details] Fix macOS build
Created attachment 344035 [details] Fix the iOS build
Created attachment 344039 [details] Win and 32-bit Mac build fix
Created attachment 344041 [details] More 32-bit build fixes
Attachment 344041 [details] did not pass style-queue: ERROR: Tools/TestRunnerShared/cocoa/LayoutTestSpellChecker.mm:160: Should not have spaces around = in property synthesis. [whitespace/property] [4] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 344042 [details] Patch
Comment on attachment 344042 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344042&action=review > Source/WebCore/editing/Editor.h:296 > + void markAllMisspellingsAndBadGrammarInRanges(TextCheckingTypeMask, Range* spellingRange, Range* automaticReplacementRange, Range* grammarRange); Why are these Range* instead of Range& or const Range&? Can all three be null? > Source/WebCore/editing/TextCheckingHelper.h:82 > + mutable int m_checkingStart { -1 }; > + mutable int m_checkingEnd { -1 }; > + mutable int m_checkingLength { -1 }; > + mutable int m_automaticReplacementStart { -1 }; > + mutable int m_automaticReplacementLength { -1 }; At some point we should consider using std::optional here instead of a magic number of -1. > Tools/TestRunnerShared/cocoa/LayoutTestSpellChecker.h:35 > +@interface LayoutTestTextCheckingResult : NSTextCheckingResult { I think this class should be private and not mentioned in the header. The data structures could just hold NSTextCheckingResult; no need to remember that our special type was used once the object is created. > Tools/TestRunnerShared/cocoa/LayoutTestSpellChecker.h:51 > ++ (instancetype)installIfNecessary; Seems slightly unnecessarily wordy. Such methods often have simpler names like "checker", with the "set this up so I can use it" part treated as implicit. > Tools/TestRunnerShared/cocoa/LayoutTestSpellChecker.mm:36 > +typedef void (^TextCheckingCompletionHandler)(NSInteger sequenceNumber, NSArray<NSTextCheckingResult *> *results, NSOrthography *orthography, NSInteger wordCount); New code should typically use "using" instead of "typedef". > Tools/TestRunnerShared/cocoa/LayoutTestSpellChecker.mm:38 > +static NeverDestroyed<RetainPtr<LayoutTestSpellChecker>> globalSpellChecker; Typically it’s overkill to use NeverDestroyed<RetainPtr>. Instead we just do a leakRef when initializing the global or just skip the adoptNS.
Comment on attachment 344042 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344042&action=review >> Source/WebCore/editing/Editor.h:296 >> + void markAllMisspellingsAndBadGrammarInRanges(TextCheckingTypeMask, Range* spellingRange, Range* automaticReplacementRange, Range* grammarRange); > > Why are these Range* instead of Range& or const Range&? Can all three be null? It does look like all three may be null (at least one call site passes a null grammarRange, and additionally, there's logic in markAllMisspellingsAndBadGrammarInRanges to bail when !spellingRange). I think the modern idiom for this would be to make these three ranges RefPtr<Range>&& instead — I can do this in a followup (along with refactoring the cached offsets in TextCheckingParagraph to be `std::optional`s). >> Source/WebCore/editing/TextCheckingHelper.h:82 >> + mutable int m_automaticReplacementLength { -1 }; > > At some point we should consider using std::optional here instead of a magic number of -1. Definitely agree! I'll do this in a followup. >> Tools/TestRunnerShared/cocoa/LayoutTestSpellChecker.h:35 >> +@interface LayoutTestTextCheckingResult : NSTextCheckingResult { > > I think this class should be private and not mentioned in the header. The data structures could just hold NSTextCheckingResult; no need to remember that our special type was used once the object is created. Good catch — moved to the implementation file. >> Tools/TestRunnerShared/cocoa/LayoutTestSpellChecker.h:51 >> ++ (instancetype)installIfNecessary; > > Seems slightly unnecessarily wordy. Such methods often have simpler names like "checker", with the "set this up so I can use it" part treated as implicit. Ok! Renamed to "checker". >> Tools/TestRunnerShared/cocoa/LayoutTestSpellChecker.mm:36 >> +typedef void (^TextCheckingCompletionHandler)(NSInteger sequenceNumber, NSArray<NSTextCheckingResult *> *results, NSOrthography *orthography, NSInteger wordCount); > > New code should typically use "using" instead of "typedef". Ah, I just realized that I don't actually need this typedef, so I just removed this. >> Tools/TestRunnerShared/cocoa/LayoutTestSpellChecker.mm:38 >> +static NeverDestroyed<RetainPtr<LayoutTestSpellChecker>> globalSpellChecker; > > Typically it’s overkill to use NeverDestroyed<RetainPtr>. Instead we just do a leakRef when initializing the global or just skip the adoptNS. Got it — removed the NeverDestroyed<> and adoptNS() here.
Created attachment 344064 [details] Patch for landing
Created attachment 344065 [details] Patch for landing
Comment on attachment 344065 [details] Patch for landing Clearing flags on attachment: 344065 Committed r233412: <https://trac.webkit.org/changeset/233412>