Bug 187225 – [macOS] Text replacements that end with symbols are expanded immediately

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wenson Hsieh]

<rdar://problem/41112433>

[Wenson Hsieh]

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.

[Wenson Hsieh]

This also reproduces on shipping macOS, but only if grammar checking is enabled.

[Wenson Hsieh]

Created attachment 344029 [details]
Patch

[Wenson Hsieh]

Created attachment 344033 [details]
Fix macOS build

[Wenson Hsieh]

Created attachment 344035 [details]
Fix the iOS build

[Wenson Hsieh]

Created attachment 344039 [details]
Win and 32-bit Mac build fix

[Wenson Hsieh]

Created attachment 344041 [details]
More 32-bit build fixes

[EWS Watchlist]

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.

[Wenson Hsieh]

Created attachment 344042 [details]
Patch

[Darin Adler]

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.

[Wenson Hsieh]

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.

[Wenson Hsieh]

Created attachment 344064 [details]
Patch for landing

[Wenson Hsieh]

Created attachment 344065 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 344065 [details]
Patch for landing

Clearing flags on attachment: 344065

Committed r233412: <https://trac.webkit.org/changeset/233412>