Bug 208661 – Add support for inserting and removing a text placeholder

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

Add support for inserting and removing a text placeholder. A text placeholder is an element that occupies space in a line (or multiple lines), but does not have a visual appearance. One application of a text placeholder is to show the iOS dictation placeholder, which is shown while the system is waiting for dictation results. This bug is about implementing the infrastructure to support that, but it does not implement the requisite UIKit delegate callbacks to actually show the dictation landing UI. I will do that in another bug.

[Daniel Bates]

<rdar://problem/59371073>

[Daniel Bates]

Created attachment 392611 [details]
For the bots

[Daniel Bates]

Created attachment 392616 [details]
For the bots

[Daniel Bates]

Created attachment 392618 [details]
For the bots

[Daniel Bates]

Comment on attachment 392618 [details]
For the bots

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

> Source/WebCore/editing/Editor.h:648
> +    RefPtr<TextPlaceholderElement> m_textPlaceholderElement;

This is unused. Remove.

> Source/WebCore/html/shadow/TextPlaceholderElement.cpp:56
> +        if (auto* shadowHost = parentOfInsertedTree.shadowHost(); is<HTMLTextFormControlElement>(shadowHost))

Maybe this should use document().ancestorNodeInThisScope(parentOfInsertedTree)? <-- that covers nested shadow host elements to be future proof.

> Source/WebCore/html/shadow/TextPlaceholderElement.cpp:67
> +    if (auto* shadowHost = oldParentOfRemovedTree.shadowHost(); is<HTMLTextFormControlElement>(shadowHost))

Maybe this should use document().ancestorNodeInThisScope(oldParentOfRemovedTree)?

[Daniel Bates]

Comment on attachment 392618 [details]
For the bots

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

>> Source/WebCore/html/shadow/TextPlaceholderElement.cpp:56
>> +        if (auto* shadowHost = parentOfInsertedTree.shadowHost(); is<HTMLTextFormControlElement>(shadowHost))
> 
> Maybe this should use document().ancestorNodeInThisScope(parentOfInsertedTree)? <-- that covers nested shadow host elements to be future proof.

Actually, ^^^ that wouldn't be correct. What we want is the enclosing HTMLTextFormControlElement (the first one we find walking up our parents).

[Daniel Bates]

Comment on attachment 392618 [details]
For the bots

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

> Source/WebCore/html/shadow/TextPlaceholderElement.cpp:54
> +    HTMLDivElement::insertedIntoAncestor(insertionType, parentOfInsertedTree);

Move this to the end.

> Source/WebCore/html/shadow/TextPlaceholderElement.cpp:64
> +    HTMLDivElement::removedFromAncestor(removalType, oldParentOfRemovedTree);

Move this to the end and don't use an early return below.

[Daniel Bates]

Comment on attachment 392618 [details]
For the bots

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

>>> Source/WebCore/html/shadow/TextPlaceholderElement.cpp:56
>>> +        if (auto* shadowHost = parentOfInsertedTree.shadowHost(); is<HTMLTextFormControlElement>(shadowHost))
>> 
>> Maybe this should use document().ancestorNodeInThisScope(parentOfInsertedTree)? <-- that covers nested shadow host elements to be future proof.
> 
> Actually, ^^^ that wouldn't be correct. What we want is the enclosing HTMLTextFormControlElement (the first one we find walking up our parents).

Wait, can you really have nested shadow hosts?

[Daniel Bates]

Created attachment 392624 [details]
For the bots

[Daniel Bates]

Created attachment 392637 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 392637 [details]
Patch

I'm concerned that this patch add some web-exposed behavior changes. It mutates the DOM, and adds a pseudo class that isn't hidden from the web. Inserting an element will have layout side-effects (akin to those for phone number linkification) which are known to trigger rendering bugs.

[Daniel Bates]

(In reply to Simon Fraser (smfr) from comment #11)
> Comment on attachment 392637 [details]
> Patch
> 
> I'm concerned that this patch add some web-exposed behavior changes. It
> mutates the DOM, and adds a pseudo class that isn't hidden from the web.
> Inserting an element will have layout side-effects (akin to those for phone
> number linkification) which are known to trigger rendering bugs.

How can I move forward with this patch?

[Daniel Bates]

(In reply to Daniel Bates from comment #12)
> (In reply to Simon Fraser (smfr) from comment #11)
> > Comment on attachment 392637 [details]
> > Patch
> > 
> > I'm concerned that this patch add some web-exposed behavior changes. It
> > mutates the DOM, and adds a pseudo class that isn't hidden from the web.
> > Inserting an element will have layout side-effects (akin to those for phone
> > number linkification) which are known to trigger rendering bugs.
> 
> How can I move forward with this patch?

FYI, the point of this feature is to have layout changed

[Simon Fraser (smfr)]

Comment on attachment 392637 [details]
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.html:4
> +<meta name="viewport" content="initial-scale=1.0">

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

[Simon Fraser (smfr)]

(In reply to Simon Fraser (smfr) from comment #14)
> Comment on attachment 392637 [details]
> 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.

See CSSParserMode::UASheetMode

[Daniel Bates]

Comment on attachment 392637 [details]
Patch

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

>>> 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.
> 
> See CSSParserMode::UASheetMode

For now, I decided to remove this pseudo and hardcode the styles.

>> 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?

Yes, it's in pixels. LengthSize does not seem appropriate because this feature only needs to be pixel aware.

>> Source/WebCore/editing/Editor.cpp:3320
>> +    TypingCommand::deleteSelection(document);
> 
> Should this really be a replaceSelection operation?

🤷‍♂️ I wanted to initially and tried, but there looks to be a bug in replaceSelectionWithFragment() as it deletes the first character if there is a caret selection and the caret is at a word start boundary. I will add a FIXME and file a bug shortly to fix this up. For now, I matched the Legacy WebKit impl and delete the selection.

>> 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?

Due to time limitations it is easier for me to match Legacy WebKit. Otherwise, the impls will be asymmetric wrt to this feature and I would have to update it to have a good user experience.

>> Source/WebCore/editing/Editor.cpp:3342
>> +    auto savedRootEditableElement = placeholder.rootEditableElement();
> 
> rootEditableElement() doesn't seem to return a RefPtr<> (it should).

OK

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

OK

>> 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?

Due to time limitations it is easier for me to match Legacy WebKit. Otherwise, the impls will be asymmetric wrt to this feature and I would have to update it to have a good user experience.

[Daniel Bates]

Created attachment 392789 [details]
Patch

[Daniel Bates]

Created attachment 392790 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 392637 [details]
Patch

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

>>> Source/WebCore/editing/Editor.cpp:3320
>>> +    TypingCommand::deleteSelection(document);
>> 
>> Should this really be a replaceSelection operation?
> 
> 🤷‍♂️ I wanted to initially and tried, but there looks to be a bug in replaceSelectionWithFragment() as it deletes the first character if there is a caret selection and the caret is at a word start boundary. I will add a FIXME and file a bug shortly to fix this up. For now, I matched the Legacy WebKit impl and delete the selection.

This call to TypingCommand would mean that the deletion will be a part of the open typing command.
Is that really expected? If not, call deleteSelectionWithSmartDelete instead.

> Source/WebCore/editing/Editor.cpp:3323
> +    m_frame.selection().toNormalizedRange()->insertNode(placeholder.copyRef());

There is no guarantee that toNormalizedRange() would be not null here.
Also, this would mean that this operation is not undoable even though the deletion is.
That doesn't make much sense. Either all of this should be undoable, or none should be.
Note that scripts can trigger undo/redo at any moment via execCommand('undo'/'redo'~)
so it's not sufficient that UIKit / AppKit wouldn't try to undo / redo in certain orders.

> Source/WebCore/editing/Editor.cpp:3329
> +    m_frame.selection().setSelectedRange(newSelection.ptr(), DOWNSTREAM, FrameSelection::ShouldCloseTyping::Yes);

It makes zero sense that the deletion is a part of the open typing command but inserting of this element isn't.

Also, just do specify that this is user-triggered selection change.
m_frame.selection().setSelection(VisibleSelection(positionBeforeNode(placeholder), positionAfterNode(placeholder), FrameSelection::defaultSetSelectionOptions(UserTriggered)));

> Source/WebCore/html/shadow/TextPlaceholderElement.cpp:54
> +    if (insertionType.connectedToDocument) {

This check isn't semantically right even if it worked for the test cases we have.
Conceptually, we want to check this condition whenever this elements shadow root changes
so we should be checking treeScopeChanged instead.

> Source/WebCore/html/shadow/TextPlaceholderElement.cpp:63
> +    if (removalType.disconnectedFromDocument) {

Ditto about checking treeScopeChanged instead.

> Source/WebCore/html/shadow/TextPlaceholderElement.h:40
> +    // Element

I don't think this comment is useful.

> Source/WebCore/html/shadow/TextPlaceholderElement.h:43
> +    // Node

Ditto.

[Ryosuke Niwa]

It's unclear to me how this feature is supposed to interact with user or script initiated undo & redo.

[Simon Fraser (smfr)]

Comment on attachment 392790 [details]
Patch

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

> Source/WebCore/editing/Editor.cpp:3322
> +    // FIXME: Write in terms of replaceSelectionWithFragment(). As of 03/06/2020, using
> +    // replaceSelectionWithFragment() when there is a caret selection at a word start
> +    // deletes the first character in the word.

Rather than giving dates which will get you laughed at two years from now, file a bug and reference it.

> Source/WebCore/editing/Editor.cpp:3351
> +    placeholder.remove();

This is pretty scary. This function has a TextPlaceholderElement& so we don't know if anyone is holding a ref to it, so remove() might cause it to be destroyed.

> Source/WebCore/html/shadow/TextPlaceholderElement.cpp:47
> +    // FIXME: Move to User Agent stylesheet.

File and reference a bug.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:7448
> +    // FIXME: Implement support for willInsertText.

This only means something to you. File a bug and reference it.

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

@property?

[Daniel Bates]

Comment on attachment 392790 [details]
Patch

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

>> Source/WebCore/editing/Editor.cpp:3322
>> +    // deletes the first character in the word.
> 
> Rather than giving dates which will get you laughed at two years from now, file a bug and reference it.

Filed bug #208744.

>> Source/WebCore/editing/Editor.cpp:3351
>> +    placeholder.remove();
> 
> This is pretty scary. This function has a TextPlaceholderElement& so we don't know if anyone is holding a ref to it, so remove() might cause it to be destroyed.

The caller has to hold a ref to it. The only way for the caller to have a TextPlaceholderElement is to have previously called insertTextPlaceholder(), which returns a RefPtr.

>> Source/WebCore/html/shadow/TextPlaceholderElement.cpp:47
>> +    // FIXME: Move to User Agent stylesheet.
> 
> File and reference a bug.

Filed bug #208745.

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:7448
>> +    // FIXME: Implement support for willInsertText.
> 
> This only means something to you. File a bug and reference it.

Filed bug #208747.

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

I didn't know you could do it, but you can.

[Daniel Bates]

(In reply to Ryosuke Niwa from comment #20)
> It's unclear to me how this feature is supposed to interact with user or
> script initiated undo & redo.

The placeholder itself isn't undo/redo-able. But any range selection that was deleted when the placeholder was inserted is.

[Daniel Bates]

Created attachment 392807 [details]
Patch

[Daniel Bates]

Comment on attachment 392807 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:7448
> +        placeholderWithWebViewBoundplaceholderToUseingRect.boundingRect = [strongSelf convertRect:placeholderWithWebViewBoundingRect.boundingRect fromView:[strongSelf webView]];

yikes!

[Daniel Bates]

Created attachment 392810 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 392807 [details]
Patch

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

> Source/WebCore/html/shadow/TextPlaceholderElement.cpp:47
> +    // FIXME: Move to User Agent stylesheet. See <https://bugs.webkit.org/show_bug.cgi?id=208745>.

You can say webkit.org/b/208745

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:7448
> +        placeholderWithWebViewBoundplaceholderToUseingRect.boundingRect = [strongSelf convertRect:placeholderWithWebViewBoundingRect.boundingRect fromView:[strongSelf webView]];

Not sure about this.

Search for "InRootViewCoordinates" in WebKit to see how this is normally done.

> Source/WebKit/UIProcess/ios/WKTextPlaceholder.mm:54
> +- (NSArray<UITextSelectionRect *> *)rects
> +{
> +    return @[ [[[WKTextSelectionRect alloc] initWithCGRect:_elementContext.boundingRect] autorelease] ];
> +}

I very much doubt these rects are correct, especially if you scroll after inserting the placeholder, and inside overflow:scroll and scrolled subframes, and on a zoomed page.

[Simon Fraser (smfr)]

Comment on attachment 392810 [details]
Patch

See comments from previous patch.

[Ryosuke Niwa]

Comment on attachment 392810 [details]
Patch

r- because I don't think this works with undo/redo.

[Ryosuke Niwa]

(In reply to Daniel Bates from comment #23)
> (In reply to Ryosuke Niwa from comment #20)
> > It's unclear to me how this feature is supposed to interact with user or
> > script initiated undo & redo.
> 
> The placeholder itself isn't undo/redo-able. But any range selection that
> was deleted when the placeholder was inserted is.

As in inserting of the placeholder is not undoable? That's fine but what happens when undo/redo is triggered when the placeholder is present? Or when the selection is moved to elsewhere when the placeholder is present?

e.g. Let's say we had (where [] denotes where the selection start & end is):
<div>hello[]</div>

And we've inserted a new paragraph separator as its own undo step:
<div>hello</div>
<div>[]<br></div>

Then we've inserted this placeholder element:

<div>hello</div>
<div>[<placeholder>]<br></div>

What happens when the undo is triggered at this point? Or when the selection is moved to the beginning of "hello"?

[Daniel Bates]

(In reply to Ryosuke Niwa from comment #30)
> (In reply to Daniel Bates from comment #23)
> > (In reply to Ryosuke Niwa from comment #20)
> > > It's unclear to me how this feature is supposed to interact with user or
> > > script initiated undo & redo.
> > 
> > The placeholder itself isn't undo/redo-able. But any range selection that
> > was deleted when the placeholder was inserted is.
> 
> As in inserting of the placeholder is not undoable? That's fine but what
> happens when undo/redo is triggered when the placeholder is present? Or when
> the selection is moved to elsewhere when the placeholder is present?
> 

Yes, I know. I still need to address that and forgot a FIXME. Would you mind if I address in a follow up?

[Ryosuke Niwa]

It appears to me that there needs to be a mechanism to cancel whatever is causing this placeholder to appear at the caret when the selection moves to elsewhere or some other execCommand is about to get executed.

At minimum, we need tests to make sure undo & redo works when this placeholder element is present in the DOM with various editing actions.

[Daniel Bates]

(In reply to Ryosuke Niwa from comment #32)
> It appears to me that there needs to be a mechanism to cancel whatever is
> causing this placeholder to appear at the caret when the selection moves to
> elsewhere or some other execCommand is about to get executed.
> 
> At minimum, we need tests to make sure undo & redo works when this
> placeholder element is present in the DOM with various editing actions.

Can I please address this in a follow up? I actually have a FIXME for this:

// FIXME: Save the current selection if it has changed since the placeholder was inserted and restore it after text insertion.

[Ryosuke Niwa]

Comment on attachment 392810 [details]
Patch

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

> Source/WebCore/editing/Editor.cpp:3321
> +    // FIXME: Write in terms of replaceSelectionWithFragment(). See <https://bugs.webkit.org/show_bug.cgi?id=208744>.
> +    TypingCommand::deleteSelection(document);

r- because it makes zero sense that this is a part of the previous typing command but we'd immediately close it below upon selection change.
If this is really used for dictation, then the whole dictation sequence should be undo.
e.g. let's say we had "hello [world]" where [ & ] and indicates start & end of selection.
If the user were to initiate the dictation so that we'd replace it with a placeholder like so:
"hello "[<placeholder>]
then when the user inserts text like "WebKit" as in:
"hello WebKit".
Then undoing this dictation should restore the state back to "hello world", not "hello"[<placeholder>] or "hello".

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

This comment doesn't match what the code says.

> Source/WebCore/editing/Editor.cpp:3330
> +    auto newSelection = Range::create(document);
> +    newSelection->setStartAfter(placeholder);
> +    newSelection->setEndAfter(placeholder);
> +    m_frame.selection().setSelectedRange(newSelection.ptr(), DOWNSTREAM, FrameSelection::ShouldCloseTyping::Yes);

r- again due to the superfluous creation of a Range as well as the typing command closing behavior above.
Use FrameSelection::setSelection as I've suggested above.

[Ryosuke Niwa]

(In reply to Daniel Bates from comment #33)
> (In reply to Ryosuke Niwa from comment #32)
> > It appears to me that there needs to be a mechanism to cancel whatever is
> > causing this placeholder to appear at the caret when the selection moves to
> > elsewhere or some other execCommand is about to get executed.
> > 
> > At minimum, we need tests to make sure undo & redo works when this
> > placeholder element is present in the DOM with various editing actions.
> 
> Can I please address this in a follow up? I actually have a FIXME for this:
> 
> // FIXME: Save the current selection if it has changed since the placeholder
> was inserted and restore it after text insertion.

Sure, addressing it in a follow up seems fine but let's address my other comments about the typing commands in this patch since it's silly to land broken code just to fix it later.

[Ryosuke Niwa]

Comment on attachment 392810 [details]
Patch

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

> Source/WebCore/editing/Editor.cpp:3354
> +        auto newSelection = Range::create(document, savedParentNode.get(), savedPlaceholderIndex, savedParentNode.get(), savedPlaceholderIndex);
> +        m_frame.selection().setSelectedRange(newSelection.ptr(), DOWNSTREAM, FrameSelection::ShouldCloseTyping::Yes);

Again, please don't create a Range just to update selection like this.

> Source/WebCore/html/shadow/TextPlaceholderElement.cpp:57
> +    if (insertionType.connectedToDocument) {

Did you not see my comment? We should be checking treeScopeChanged here.

> Source/WebCore/html/shadow/TextPlaceholderElement.cpp:58
> +        if (auto* shadowHost = parentOfInsertedTree.shadowHost(); is<HTMLTextFormControlElement>(shadowHost))

Please use RefPtr.

> Source/WebCore/html/shadow/TextPlaceholderElement.cpp:66
> +    if (removalType.disconnectedFromDocument) {

Ditto.

> Source/WebCore/html/shadow/TextPlaceholderElement.cpp:67
> +        if (auto* shadowHost = oldParentOfRemovedTree.shadowHost(); is<HTMLTextFormControlElement>(shadowHost))

Ditto.

> Source/WebCore/html/shadow/TextPlaceholderElement.h:40
> +    // Element

I don't think this comment is useful. Remove?

> Source/WebCore/html/shadow/TextPlaceholderElement.h:43
> +    // Node

Ditto.

[Daniel Bates]

Created attachment 392825 [details]
Patch

Updated aptch to address Ryosuke's feedback.

[Daniel Bates]

(In reply to Ryosuke Niwa from comment #36)
> Comment on attachment 392810 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=392810&action=review
> 
> > Source/WebCore/editing/Editor.cpp:3354
> > +        auto newSelection = Range::create(document, savedParentNode.get(), savedPlaceholderIndex, savedParentNode.get(), savedPlaceholderIndex);
> > +        m_frame.selection().setSelectedRange(newSelection.ptr(), DOWNSTREAM, FrameSelection::ShouldCloseTyping::Yes);
> 
> Again, please don't create a Range just to update selection like this.

How to do it then?

> 
> > Source/WebCore/html/shadow/TextPlaceholderElement.cpp:57
> > +    if (insertionType.connectedToDocument) {
> 
> Did you not see my comment? We should be checking treeScopeChanged here.

Fixed in patch I just posted.

> 
> > Source/WebCore/html/shadow/TextPlaceholderElement.cpp:58
> > +        if (auto* shadowHost = parentOfInsertedTree.shadowHost(); is<HTMLTextFormControlElement>(shadowHost))
> 
> Please use RefPtr.

OK

> 
> > Source/WebCore/html/shadow/TextPlaceholderElement.cpp:66
> > +    if (removalType.disconnectedFromDocument) {
> 
> Ditto.

Fixed in patch I just posted.

> 
> > Source/WebCore/html/shadow/TextPlaceholderElement.cpp:67
> > +        if (auto* shadowHost = oldParentOfRemovedTree.shadowHost(); is<HTMLTextFormControlElement>(shadowHost))
> 
> Ditto.

OK

> 
> > Source/WebCore/html/shadow/TextPlaceholderElement.h:40
> > +    // Element
> 
> I don't think this comment is useful. Remove?
> 
> > Source/WebCore/html/shadow/TextPlaceholderElement.h:43
> > +    // Node
> 
> Ditto.

[Ryosuke Niwa]

(In reply to Daniel Bates from comment #38)
> (In reply to Ryosuke Niwa from comment #36)
> > Comment on attachment 392810 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=392810&action=review
> > 
> > > Source/WebCore/editing/Editor.cpp:3354
> > > +        auto newSelection = Range::create(document, savedParentNode.get(), savedPlaceholderIndex, savedParentNode.get(), savedPlaceholderIndex);
> > > +        m_frame.selection().setSelectedRange(newSelection.ptr(), DOWNSTREAM, FrameSelection::ShouldCloseTyping::Yes);
> > 
> > Again, please don't create a Range just to update selection like this.
> 
> How to do it then?

As I've already stated in https://bugs.webkit.org/show_bug.cgi?id=208661#c19,

m_frame.selection().setSelection(VisibleSelection(positionBeforeNode(placeholder), positionAfterNode(placeholder), FrameSelection::defaultSetSelectionOptions(UserTriggered)));

[Daniel Bates]

Created attachment 392826 [details]
Patch

Update patch to address more feedback I accidentally missed. I did not know how to remove the Range code from removeTextPlaceholder at the moment. Filed bug #208751 to fix up while I look into.

[Daniel Bates]

(In reply to Ryosuke Niwa from comment #39)
> (In reply to Daniel Bates from comment #38)
> > (In reply to Ryosuke Niwa from comment #36)
> > > Comment on attachment 392810 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=392810&action=review
> > > 
> > > > Source/WebCore/editing/Editor.cpp:3354
> > > > +        auto newSelection = Range::create(document, savedParentNode.get(), savedPlaceholderIndex, savedParentNode.get(), savedPlaceholderIndex);
> > > > +        m_frame.selection().setSelectedRange(newSelection.ptr(), DOWNSTREAM, FrameSelection::ShouldCloseTyping::Yes);
> > > 
> > > Again, please don't create a Range just to update selection like this.
> > 
> > How to do it then?
> 
> As I've already stated in https://bugs.webkit.org/show_bug.cgi?id=208661#c19,
> 
> m_frame.selection().
> setSelection(VisibleSelection(positionBeforeNode(placeholder),
> positionAfterNode(placeholder),
> FrameSelection::defaultSetSelectionOptions(UserTriggered)));

This code is doing a different range.

[Ryosuke Niwa]

(In reply to Daniel Bates from comment #41)
> (In reply to Ryosuke Niwa from comment #39)
> > (In reply to Daniel Bates from comment #38)
> > > (In reply to Ryosuke Niwa from comment #36)
> > > > Comment on attachment 392810 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=392810&action=review
> > > > 
> > > > > Source/WebCore/editing/Editor.cpp:3354
> > > > > +        auto newSelection = Range::create(document, savedParentNode.get(), savedPlaceholderIndex, savedParentNode.get(), savedPlaceholderIndex);
> > > > > +        m_frame.selection().setSelectedRange(newSelection.ptr(), DOWNSTREAM, FrameSelection::ShouldCloseTyping::Yes);
> > > > 
> > > > Again, please don't create a Range just to update selection like this.
> > > 
> > > How to do it then?
> > 
> > As I've already stated in https://bugs.webkit.org/show_bug.cgi?id=208661#c19,
> > 
> > m_frame.selection().
> > setSelection(VisibleSelection(positionBeforeNode(placeholder),
> > positionAfterNode(placeholder),
> > FrameSelection::defaultSetSelectionOptions(UserTriggered)));
> 
> This code is doing a different range.

If you want selection to be inside the place holder, then VisibleSelection(firstPositionInNode(placeholder), lastPositionInNode(placeholder)).

[Daniel Bates]

Created attachment 392830 [details]
Patch

No more ranges.

[Daniel Bates]

(In reply to Ryosuke Niwa from comment #42)
> (In reply to Daniel Bates from comment #41)
> > (In reply to Ryosuke Niwa from comment #39)
> > > (In reply to Daniel Bates from comment #38)
> > > > (In reply to Ryosuke Niwa from comment #36)
> > > > > Comment on attachment 392810 [details]
> > > > > Patch
> > > > > 
> > > > > View in context:
> > > > > https://bugs.webkit.org/attachment.cgi?id=392810&action=review
> > > > > 
> > > > > > Source/WebCore/editing/Editor.cpp:3354
> > > > > > +        auto newSelection = Range::create(document, savedParentNode.get(), savedPlaceholderIndex, savedParentNode.get(), savedPlaceholderIndex);
> > > > > > +        m_frame.selection().setSelectedRange(newSelection.ptr(), DOWNSTREAM, FrameSelection::ShouldCloseTyping::Yes);
> > > > > 
> > > > > Again, please don't create a Range just to update selection like this.
> > > > 
> > > > How to do it then?
> > > 
> > > As I've already stated in https://bugs.webkit.org/show_bug.cgi?id=208661#c19,
> > > 
> > > m_frame.selection().
> > > setSelection(VisibleSelection(positionBeforeNode(placeholder),
> > > positionAfterNode(placeholder),
> > > FrameSelection::defaultSetSelectionOptions(UserTriggered)));
> > 
> > This code is doing a different range.
> 
> If you want selection to be inside the place holder, then
> VisibleSelection(firstPositionInNode(placeholder),
> lastPositionInNode(placeholder)).

I want to restore the caret to before the placeholder now that the placeholder was removed.

[Ryosuke Niwa]

(In reply to Daniel Bates from comment #44)
>
> I want to restore the caret to before the placeholder now that the
> placeholder was removed.

Ah, then you can do what you're doing with positionBeforeNode(&placeholder) before the removal and just use parentAnchoredEquivalent() instead so:

Position savedPositionBeforePlaceholder = positionBeforeNode(&placeholder).parentAnchoredEquivalent()

placeholder.remove();

m_frame.selection().setSelection(VisibleSelection(savedPositionBeforePlaceholder), ~)

[Daniel Bates]

(In reply to Ryosuke Niwa from comment #45)
> (In reply to Daniel Bates from comment #44)
> >
> > I want to restore the caret to before the placeholder now that the
> > placeholder was removed.
> 
> Ah, then you can do what you're doing with positionBeforeNode(&placeholder)
> before the removal and just use parentAnchoredEquivalent() instead so:
> 
> Position savedPositionBeforePlaceholder =
> positionBeforeNode(&placeholder).parentAnchoredEquivalent()
> 
> placeholder.remove();
> 
> m_frame.selection().
> setSelection(VisibleSelection(savedPositionBeforePlaceholder), ~)

there you go! Thanks

[Daniel Bates]

Created attachment 392833 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 392830 [details]
Patch

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

> Source/WebCore/editing/Editor.cpp:3344
> +    auto savedPositionBeforePlaceholder = positionBeforeNode(&placeholder);

Save parentAnchoredEquivalent() instead as I commented here.

> Source/WebCore/editing/Editor.cpp:3352
> +        VisibleSelection newSelection { savedPositionBeforePlaceholder, savedPositionBeforePlaceholder };

You can just pass a single savedPositionBeforePlaceholder to VisibleSelection's constructor.

> Source/WebCore/html/shadow/TextPlaceholderElement.cpp:37
> +using namespace HTMLNames;

Do we really need to have this using?

> LayoutTests/ChangeLog:11
> +        * fast/text-placeholder/insert-and-remove-into-text-field-expected.html: Added.

We probably have these tests in LayoutTests/editing instead.

[Ryosuke Niwa]

Comment on attachment 392833 [details]
Patch

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

> Source/WebCore/editing/Editor.cpp:3352
> +        VisibleSelection newSelection { savedPositionBeforePlaceholder, savedPositionBeforePlaceholder };

You can just pass one of them to VisibleSelection's constructor.

> Source/WebCore/html/shadow/TextPlaceholderElement.cpp:37
> +using namespace HTMLNames;

Can't we just use HTMLNames::divTag below?

[Daniel Bates]

Created attachment 392834 [details]
To land

[Daniel Bates]

Comment on attachment 392830 [details]
Patch

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

>> Source/WebCore/editing/Editor.cpp:3344
>> +    auto savedPositionBeforePlaceholder = positionBeforeNode(&placeholder);
> 
> Save parentAnchoredEquivalent() instead as I commented here.

OK

>> Source/WebCore/editing/Editor.cpp:3352
>> +        VisibleSelection newSelection { savedPositionBeforePlaceholder, savedPositionBeforePlaceholder };
> 
> You can just pass a single savedPositionBeforePlaceholder to VisibleSelection's constructor.

Cool

>> Source/WebCore/html/shadow/TextPlaceholderElement.cpp:37
>> +using namespace HTMLNames;
> 
> Do we really need to have this using?

Ok will not use

>> LayoutTests/ChangeLog:11
>> +        * fast/text-placeholder/insert-and-remove-into-text-field-expected.html: Added.
> 
> We probably have these tests in LayoutTests/editing instead.

OK

[Daniel Bates]

Created attachment 392835 [details]
To land for realz

[Daniel Bates]

Created attachment 392837 [details]
To land for realz realz

[Daniel Bates]

Comment on attachment 392837 [details]
To land for realz realz

Clearing flags on attachment: 392837

Committed r258050: <https://trac.webkit.org/changeset/258050>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.