Bug 214331 – Should represent `TextPlaceholderElement` as an `NSTextAttachmentCharacter` instead of a `\n`

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Devin Rousso]

This can muck with whitespace addition/removal with iOS smart editing.

[Devin Rousso]

<rdar://problem/64779558>

[Devin Rousso]

Created attachment 404306 [details]
Patch

[Devin Rousso]

Created attachment 404311 [details]
Patch

[Devin Rousso]

Created attachment 404312 [details]
Patch

... stupid semicolons (╯°□°）╯︵ ┻━┻

[Wenson Hsieh]

Comment on attachment 404311 [details]
Patch

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

> Source/WebCore/editing/TextIterator.cpp:807
> +static bool shouldEmitReplacementInsteadOfNode(Node& node)

This should probably take `const Node&` as the argument type.

> Source/WebCore/editing/TextIterator.cpp:851
> +    // Placeholders should eventually disappear, so treating them as a line break doesn't make sense
> +    // as when they are removed the text after it is combined with the text before it.

Nit - I think this comment would be more effective inside `shouldEmitReplacementInsteadOfNode`, since that is where the notion of “emitting replacement characters instead of nodes" becomes relevant to `TextPlaceholderElement`.

> Source/WebKit/UIProcess/API/ios/WKWebViewPrivateForTestingIOS.h:61
> +- (void)_insertTextPlaceholderWithSize:(CGSize)size completionHandler:(void (^)(UITextPlaceholder *))completionHandler;
> +- (void)_removeTextPlaceholder:(UITextPlaceholder *)placeholder willInsertText:(BOOL)willInsertText completionHandler:(void (^)(void))completionHandler;

Nit - I think you could actually omit this extra plumbing on WKWebView, and just use -textInputContentView instead to grab the WKContentView (exposed as a <UIWKInteractionViewProtocol>-conformant responder).

> Source/WebKit/UIProcess/API/ios/WKWebViewTestingIOS.mm:117
> +- (void)_insertTextPlaceholderWithSize:(CGSize)size completionHandler:(void (^)(UITextPlaceholder *))completionHandler
> +{
> +    [_contentView insertTextPlaceholderWithSize:size completionHandler:completionHandler];
> +}
> +
> +- (void)_removeTextPlaceholder:(UITextPlaceholder *)placeholder willInsertText:(BOOL)willInsertText completionHandler:(void (^)(void))completionHandler
> +{
> +    [_contentView removeTextPlaceholder:placeholder willInsertText:willInsertText completionHandler:completionHandler];
> +}
> +

(Ditto)

> Tools/TestWebKitAPI/Tests/WebKitCocoa/DocumentEditingContext.mm:863
> +    auto* placeholder = [webView synchronouslyInsertTextPlaceholderWithSize:CGSizeMake(5, 5)];

Nit - the * should be on the other side here. But also, we can just use `auto` for these.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/DocumentEditingContext.mm:880
> +    auto* placeholder = [webView synchronouslyInsertTextPlaceholderWithSize:CGSizeMake(5, 5)];

Ditto.

[Wenson Hsieh]

Comment on attachment 404312 [details]
Patch

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

> Source/WebKit/Platform/spi/ios/UIKitSPI.h:708
> +- (void)insertTextPlaceholderWithSize:(CGSize)size completionHandler:(void (^)(UITextPlaceholder *))completionHandler;
> +- (void)removeTextPlaceholder:(UITextPlaceholder *)placeholder willInsertText:(BOOL)willInsertText completionHandler:(void (^)(void))completionHandler;

This should also go inside the `@optional` section, to match the actual SPI header.

[Devin Rousso]

Comment on attachment 404311 [details]
Patch

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

>> Source/WebCore/editing/TextIterator.cpp:807
>> +static bool shouldEmitReplacementInsteadOfNode(Node& node)
> 
> This should probably take `const Node&` as the argument type.

I was matching the other `shouldEmit*` functions, but yeah I could do that.

[Devin Rousso]

Created attachment 404314 [details]
Patch

[Devin Rousso]

Created attachment 404355 [details]
Patch

[Wenson Hsieh]

Comment on attachment 404355 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * editing/TextIterator.cpp:

It would be nice to have a few words here to explain why we want to emit the replacement character instead of a newline (something similar to what you already wrote in the comment below).

[Devin Rousso]

Created attachment 404376 [details]
Patch

[EWS]

Committed r264420: <https://trac.webkit.org/changeset/264420>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404376 [details].

[Darin Adler]

Comment on attachment 404376 [details]
Patch

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

> Source/WebCore/editing/TextIterator.cpp:1014
> +            emitCharacter(0xFFFC, *m_node->parentNode(), m_node, 0, 0);

Should write:

    objectReplacementCharacter

Instead of:

    0xFFFC

It’s in the header CharacterNames.h.

[Devin Rousso]

Comment on attachment 404376 [details]
Patch

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

>> Source/WebCore/editing/TextIterator.cpp:1014
>> +            emitCharacter(0xFFFC, *m_node->parentNode(), m_node, 0, 0);
> 
> Should write:
> 
>     objectReplacementCharacter
> 
> Instead of:
> 
>     0xFFFC
> 
> It’s in the header CharacterNames.h.

Oh neat!  I did not know that existed :)

Will land a followup.

[Devin Rousso]

Committed r264439: Unreviewed followup, use objectReplacementCharacter instead of 0xFFFC