[webkit-reviews] review denied: [Bug 208661] Add support for inserting and removing a text placeholder : [Attachment 392637] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Daniel Bates
<dbates at webkit.org>'s request for review:
Bug 208661: Add support for inserting and removing a text placeholder
https://bugs.webkit.org/show_bug.cgi?id=208661

Attachment 392637: Patch

https://bugs.webkit.org/attachment.cgi?id=392637&action=review




--- Comment #14 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 392637
  --> https://bugs.webkit.org/attachment.cgi?id=392637
Patch

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

r- to hide the new pseudo from the web. Editing people should review more.

> Source/WebCore/css/html.css:701
> +::-webkit-text-placeholder {

Please call this ::-apple-text-placeholder and make it so that it's only valid
inside a UA stylesheet. It should not be possible to apply this pseudo element
from web content.

> Source/WebCore/editing/Editor.cpp:3313
> +RefPtr<TextPlaceholderElement> Editor::insertTextPlaceholder(const IntSize&
size)

Is size dimensions in pixels? Maybe supplying a LengthSize would be more
appropriate? is there any need to match the font size of the surrounding text?

> Source/WebCore/editing/Editor.cpp:3320
> +    TypingCommand::deleteSelection(document);

Should this really be a replaceSelection operation?

> Source/WebCore/editing/Editor.cpp:3325
> +    // To match the Legacy WebKit implementation, set the text insertion
point to be after the placeholder.

Do we really need to match legacy webkit?

> Source/WebCore/editing/Editor.cpp:3342
> +    auto savedRootEditableElement = placeholder.rootEditableElement();

rootEditableElement() doesn't seem to return a RefPtr<> (it should).

> Source/WebCore/editing/Editor.cpp:3343
> +    auto* savedParentNode = placeholder.parentNode();

RefPtr

> Source/WebCore/editing/Editor.cpp:3350
> +    // To match the Legacy WebKit implementation, set the text insertion
point to be before where the placeholder use to be.

Do we have to?

> Source/WebCore/platform/ios/SelectionRect.h:38
> -    WEBCORE_EXPORT explicit SelectionRect(const IntRect&, bool isHorizontal,
int columnNumber);
> +    WEBCORE_EXPORT explicit SelectionRect(const IntRect&, bool isHorizontal,
int pageNumber);

Is this related?

> Source/WebKit/UIProcess/ios/WKTextPlaceholder.h:38
> +- (const WebCore::ElementContext&)elementContext;

@property?

> Source/WebKit/UIProcess/ios/WKTextPlaceholder.mm:53
> +    return @[ [[[WKTextSelectionRect alloc]
initWithCGRect:_elementContext.boundingRect] autorelease] ];

What coordinate space is _elementContext.boundingRect? Does that match
UITextSelectionRect coordinates? Does this work in subframes and
overflow:scroll?

>
LayoutTests/fast/text-placeholder/insert-and-remove-into-text-field-expected.ht
ml:4
> +<meta name="viewport" content="initial-scale=1.0">

If you want viewport behavior you need to add <!-- webkit-test-runner [
useFlexibleViewport=true ] -->