Bug 201085 – [iOS] [WebKit2] Tapping on the “I’m” text suggestion after typing “i’” does nothing

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wenson Hsieh]

<rdar://problem/53056118>

[Wenson Hsieh]

Created attachment 377151 [details]
Patch

[Darin Adler]

Comment on attachment 377151 [details]
Patch

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

> Source/WebCore/editing/TextIterator.h:57
> +WEBCORE_EXPORT String foldQuoteMarks(String);

Seems a little peculiar to have the argument the be "String" rather than "const String&". Also, I am a little concerned using this outside the context of ICU’s usearch. The folding here was specifically to work around what usearch does and does not do. This folding alone without all the other folding that usearch does is a sort of peculiar subset. Finally, it’s not great to have this in this header since it’s not really an "iteration" thing. Just because the file-local version was here doesn’t mean this is a good logical home for the function.

[Darin Adler]

Comment on attachment 377151 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2342
>          textForRange = plainTextReplacingNoBreakSpace(range.get());

Might be cleaner to integrate the quote mark folding into a replacement for the "plainTextReplacingNoBreakSpace" function instead of doing it as a separate step.

[Wenson Hsieh]

(In reply to Darin Adler from comment #3)
> Comment on attachment 377151 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377151&action=review
> 
> > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2342
> >          textForRange = plainTextReplacingNoBreakSpace(range.get());
> 
> Might be cleaner to integrate the quote mark folding into a replacement for
> the "plainTextReplacingNoBreakSpace" function instead of doing it as a
> separate step.

This was my first thought, but then I realized that we’d still need a way to fold quote marks for the original text passed into applyAutocorrectionInternal.

[Darin Adler]

Comment on attachment 377151 [details]
Patch

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

>>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2342
>>>          textForRange = plainTextReplacingNoBreakSpace(range.get());
>> 
>> Might be cleaner to integrate the quote mark folding into a replacement for the "plainTextReplacingNoBreakSpace" function instead of doing it as a separate step.
> 
> This was my first thought, but then I realized that we’d still need a way to fold quote marks for the original text passed into applyAutocorrectionInternal.

Does that text perhaps also ultimately come from a call to plainTextReplacingNoBreakSpace?

[Wenson Hsieh]

Comment on attachment 377151 [details]
Patch

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

>>>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2342
>>>>          textForRange = plainTextReplacingNoBreakSpace(range.get());
>>> 
>>> Might be cleaner to integrate the quote mark folding into a replacement for the "plainTextReplacingNoBreakSpace" function instead of doing it as a separate step.
>> 
>> This was my first thought, but then I realized that we’d still need a way to fold quote marks for the original text passed into applyAutocorrectionInternal.
> 
> Does that text perhaps also ultimately come from a call to plainTextReplacingNoBreakSpace?

It comes from UIKit (which, in turn, gets it from kbd).

[Wenson Hsieh]

(In reply to Darin Adler from comment #2)
> Comment on attachment 377151 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377151&action=review
> 
> > Source/WebCore/editing/TextIterator.h:57
> > +WEBCORE_EXPORT String foldQuoteMarks(String);
> 
> Seems a little peculiar to have the argument the be "String" rather than
> "const String&”.

Agreed. On closer inspection, we should be able to make this const String& (the only call site that depends on this is one of the SearchBuffer constructors that could be tweaked). I’ll post a v2 that does this.

> Also, I am a little concerned using this outside the
> context of ICU’s usearch. The folding here was specifically to work around
> what usearch does and does not do. This folding alone without all the other
> folding that usearch does is a sort of peculiar subset.

I see. Perhaps the function could be renamed in such a way that makes this clear?

> Finally, it’s not
> great to have this in this header since it’s not really an "iteration"
> thing. Just because the file-local version was here doesn’t mean this is a
> good logical home for the function.

Yeah, I had considered moving this (either to WTFString.h as an instance method of WTF::String, or a standalone function in that file, or just a function in StringCommon.h, or even a new header in WebCore) but decided to leave it out of this patch to make this change simpler.

One thing to note is that wherever we decide to move it, we should probably keep foldQuoteMarks and foldQuoteMark in the same place.

Would it be okay to do this in a followup patch?

[Wenson Hsieh]

Created attachment 377154 [details]
Make foldQuoteMarks take a const String&

[Wenson Hsieh]

> One thing to note is that wherever we decide to move it, we should probably
> keep foldQuoteMarks and foldQuoteMark in the same place.
> 
> Would it be okay to do this in a followup patch?

I filed <https://bugs.webkit.org/show_bug.cgi?id=201092> for this.

[WebKit Commit Bot]

Comment on attachment 377154 [details]
Make foldQuoteMarks take a const String&

Clearing flags on attachment: 377154

Committed r249074: <https://trac.webkit.org/changeset/249074>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 377154 [details]
Make foldQuoteMarks take a const String&

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

> Source/WebCore/editing/TextIterator.cpp:-2422
> -    foldQuoteMarks(m_target);

What you did here seems to be fixing a serious bug! This was calling a function and ignoring its result so it wasn’t folding the quote marks. Glad you fixed it, but not at all happy that we didn’t have test coverage that showed us what was happening.

[Wenson Hsieh]

(In reply to Darin Adler from comment #12)
> Comment on attachment 377154 [details]
> Make foldQuoteMarks take a const String&
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377154&action=review
> 
> > Source/WebCore/editing/TextIterator.cpp:-2422
> > -    foldQuoteMarks(m_target);
> 
> What you did here seems to be fixing a serious bug! This was calling a
> function and ignoring its result so it wasn’t folding the quote marks. Glad
> you fixed it, but not at all happy that we didn’t have test coverage that
> showed us what was happening.

Indeed!

One thing to note here is that this codepath is only compiled if UCONFIG_NO_COLLATION, which might explain the lack of test coverage (at least, on macOS and iOS bots).

[Darin Adler]

Comment on attachment 377154 [details]
Make foldQuoteMarks take a const String&

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

>>> Source/WebCore/editing/TextIterator.cpp:-2422
>>> -    foldQuoteMarks(m_target);
>> 
>> What you did here seems to be fixing a serious bug! This was calling a function and ignoring its result so it wasn’t folding the quote marks. Glad you fixed it, but not at all happy that we didn’t have test coverage that showed us what was happening.
> 
> Indeed!
> 
> One thing to note here is that this codepath is only compiled if UCONFIG_NO_COLLATION, which might explain the lack of test coverage (at least, on macOS and iOS bots).

Aha, yes, that makes sense. And I think that means this does not affect any Cocoa platforms.