Make it possible to insert editable images with a gesture
Created attachment 355557 [details] Patch
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)
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)
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.
(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).
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.
(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!
Created attachment 355587 [details] Patch
Created attachment 355588 [details] Patch
Comment on attachment 355588 [details] Patch Clearing flags on attachment: 355588 Committed r238475: <https://trac.webkit.org/changeset/238475>
All reviewed patches have been landed. Closing bug.
<rdar://problem/46230916>