Bug 191937 – Make it possible to insert editable images with a gesture

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

Make it possible to insert editable images with a gesture

[Tim Horton]

Created attachment 355557 [details]
Patch

[Tim Horton]

Comment on attachment 355557 [details]
Patch

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

> Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm:657
> +    case EditAction::ConvertToUnorderedList: return UI_STRING_KEY_INTERNAL("Convert to Unordered List", "Convert to Unordered List (Undo action name)", "Undo action name");

I didn't mean to change this one in this patch (if I did, I would have done them both)

[Tim Horton]

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

> Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm:657
> +    case EditAction::ConvertToUnorderedList: return UI_STRING_KEY_INTERNAL("Convert to Unordered List", "Convert to Unordered List (Undo action name)", "Undo action name");

I didn't mean to change this one in this patch (if I did, I would have done them both)

[Wenson Hsieh]

Comment on attachment 355557 [details]
Patch

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

> Source/WebCore/editing/VisibleSelection.cpp:34
> +#include "ShadowRoot.h"

Nit - is this needed?

> Source/WebKit/UIProcess/WebEditCommandProxy.cpp:-79
> -        return String();

Was this change intended?

>> Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm:657
>> +    case EditAction::ConvertToUnorderedList: return UI_STRING_KEY_INTERNAL("Convert to Unordered List", "Convert to Unordered List (Undo action name)", "Undo action name");
> 
> I didn't mean to change this one in this patch (if I did, I would have done them both)

Oops. I think I'll clean these four up in a separate patch (it looks like WebEditCommandProxy.cpp is also affected).

> LayoutTests/editing/images/basic-editable-image-from-execCommand.html:11
> +    window.getSelection().setBaseAndExtent(document.body, 0, document.body, 0);

Nit - setPosition(document.body, 0) would be cleaner here.

[Tim Horton]

(In reply to Wenson Hsieh from comment #4)
> Comment on attachment 355557 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355557&action=review
> 
> > Source/WebCore/editing/VisibleSelection.cpp:34
> > +#include "ShadowRoot.h"
> 
> Nit - is this needed?

I think whenever you see something like this in a patch that touches Sources*.txt, assume it's a unified sources build fix :P

> > Source/WebKit/UIProcess/WebEditCommandProxy.cpp:-79
> > -        return String();
> 
> Was this change intended?

Definitely not.

> >> Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm:657
> >> +    case EditAction::ConvertToUnorderedList: return UI_STRING_KEY_INTERNAL("Convert to Unordered List", "Convert to Unordered List (Undo action name)", "Undo action name");
> > 
> > I didn't mean to change this one in this patch (if I did, I would have done them both)
> 
> Oops. I think I'll clean these four up in a separate patch (it looks like
> WebEditCommandProxy.cpp is also affected).

Thank you!

> > LayoutTests/editing/images/basic-editable-image-from-execCommand.html:11
> > +    window.getSelection().setBaseAndExtent(document.body, 0, document.body, 0);
> 
> Nit - setPosition(document.body, 0) would be cleaner here.

Ooh, somehow I didn't know that was a thing (really the whole DOM selection API feels very oddly named/designed to me).

[Wenson Hsieh]

Comment on attachment 355557 [details]
Patch

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

> Source/WebCore/editing/InsertEditableImageCommand.cpp:34
> +InsertEditableImageCommand::InsertEditableImageCommand(Document& document)

One could imagine moving the selection to the pencil position and using ReplaceSelectionCommand with EditAction::InsertEditableImage as an alternative to introducing a new edit command.

However, it seems like this may be followed up with more complex logic for inserting editable images, such as paragraph splitting, in which case we would eventually want to add a new edit command

> Source/WebCore/editing/InsertEditableImageCommand.cpp:50
> +    insertNodeAt(imgElement.copyRef(), endingSelection().start());

Nit - We can WTFMove(imgElement) here instead of creating a new Ref.

> Source/WebCore/editing/InsertEditableImageCommand.cpp:51
> +}

I think you also probably want to set the ending selection to either encompass the new image, or place it right after the new image?

> Source/WebCore/editing/InsertEditableImageCommand.h:43
> +    EditAction editingAction() const override { return EditAction::InsertEditableImage; }

Nit - override => final?

> LayoutTests/editing/images/basic-editable-image-from-execCommand.html:10
> +addEventListener("load", async () => {

It might also be good to also test undo-redoing, and then check to see where the selection ends up.

[Tim Horton]

(In reply to Wenson Hsieh from comment #6)
> Comment on attachment 355557 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355557&action=review
> 
> > Source/WebCore/editing/InsertEditableImageCommand.cpp:34
> > +InsertEditableImageCommand::InsertEditableImageCommand(Document& document)
> 
> One could imagine moving the selection to the pencil position and using
> ReplaceSelectionCommand with EditAction::InsertEditableImage as an
> alternative to introducing a new edit command.
> 
> However, it seems like this may be followed up with more complex logic for
> inserting editable images, such as paragraph splitting, in which case we
> would eventually want to add a new edit command

Right, that's what I'm envisioning. We'll need to break out of blockquotes, containers, indentation, etc. But in a future patch - this is just setting us up for that.

> 
> > Source/WebCore/editing/InsertEditableImageCommand.cpp:50
> > +    insertNodeAt(imgElement.copyRef(), endingSelection().start());
> 
> Nit - We can WTFMove(imgElement) here instead of creating a new Ref.
> 
> > Source/WebCore/editing/InsertEditableImageCommand.cpp:51
> > +}
> 
> I think you also probably want to set the ending selection to either
> encompass the new image, or place it right after the new image?

Yeah, that's in the WebPageIOS code but I should move it here instead.

> > Source/WebCore/editing/InsertEditableImageCommand.h:43
> > +    EditAction editingAction() const override { return EditAction::InsertEditableImage; }
> 
> Nit - override => final?
> 
> > LayoutTests/editing/images/basic-editable-image-from-execCommand.html:10
> > +addEventListener("load", async () => {
> 
> It might also be good to also test undo-redoing, and then check to see where
> the selection ends up.

Sure!

[Tim Horton]

Created attachment 355587 [details]
Patch

[Tim Horton]

Created attachment 355588 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 355588 [details]
Patch

Clearing flags on attachment: 355588

Committed r238475: <https://trac.webkit.org/changeset/238475>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46230916>