- Revision
- 290578
- Author
- wenson_hs...@apple.com
- Date
- 2022-02-27 19:24:13 -0800 (Sun, 27 Feb 2022)
Log Message
Invoking "Markup Image" should preserve the existing selection range
https://bugs.webkit.org/show_bug.cgi?id=237242
Reviewed by Darin Adler.
Source/WebCore:
Move the implementation of `replaceNodeFromPasteboard` out of the macOS-specific implementation file and into
EditorCocoa instead, so that we can use it on both iOS and macOS to handle the "Markup Image" action. See WebKit
ChangeLog for more details.
* editing/Editor.h:
* editing/cocoa/EditorCocoa.mm:
(WebCore::maybeCopyNodeAttributesToFragment):
(WebCore::Editor::replaceNodeFromPasteboard):
Also make a few minor adjustments while we're moving this code:
- Make this take a reference instead of a pointer (the method always expects a non-null Node pointer anyways).
- Add a FIXME mentioning that we can just delete `setInsertionPasteboard` altogether once Mail compose on
macOS uses WebKit2.
- Unconditionally pass `false` for whether or not we should enable smart paste when replacing the node. This
prevents us from inserting spaces around the replacement image element when triggering the "Markup Image"
item.
- Use `makeRangeSelectingNode` instead of `makeRangeSelectingNodeContents` when selecting the node to replace.
This allows us to handle the case where the node to replace cannot contain children for editing (in
particular, image elements).
Note that this codepath (`replaceNodeFromPasteboard`) is currently only exercised by WebKitLegacy service
controls code that was originally intended for use by Mail compose, but never ended up being used.
* editing/mac/EditorMac.mm:
(WebCore::Editor::pasteWithPasteboard):
(WebCore::maybeCopyNodeAttributesToFragment): Deleted.
(WebCore::Editor::replaceNodeFromPasteboard): Deleted.
Source/WebKit:
Make a few adjustments to `replaceWithPasteboardData`, such that it attempts to restore the previous selection
range after replacing the target element. If the previous selection is no longer valid (i.e., it was orphaned as
a result of the editing command used to replace the element), then we fall back to restoring the visible
character range of the previous selection, relative to the editable root (or the body if there is none).
API tests will be added in https://webkit.org/b/236519, once we're able to swizzle out the underlying VisionKit
methods in order to make these tests run reliably.
* WebProcess/WebPage/Cocoa/WebPageCocoa.mm:
(WebKit::OverridePasteboardForSelectionReplacement::OverridePasteboardForSelectionReplacement):
(WebKit::OverridePasteboardForSelectionReplacement::~OverridePasteboardForSelectionReplacement):
Add a helper RAII object that encapsulates logic for setting and unsetting data on the override pasteboard. Use
this in the two methods below.
(WebKit::WebPage::replaceWithPasteboardData):
Switch to using the refactored `Editor::replaceNodeFromPasteboard` method instead of calling to the adjacent
`replaceSelectionWithPasteboardData` method below. Using this method allows us to avoid manually selecting the
target element here, and also avoids smart pasteboard replacement (which may insert spaces before or after the
image); the latter is important in order to correctly restore the previous selection's character range in the
case where the previous selection range was orphaned by the replacement.
(WebKit::WebPage::replaceSelectionWithPasteboardData):
Source/WebKitLegacy/mac:
See WebKit and WebCore ChangeLogs for more detail.
* Misc/WebSharingServicePickerController.mm:
(-[WebSharingServicePickerController didShareImageData:confirmDataIsValidTIFFData:]):
Adjust this call site to pass in a reference instead of a pointer. Also deploy RefPtr in some adjacent code.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (290577 => 290578)
--- trunk/Source/WebCore/ChangeLog 2022-02-28 03:03:39 UTC (rev 290577)
+++ trunk/Source/WebCore/ChangeLog 2022-02-28 03:24:13 UTC (rev 290578)
@@ -1,3 +1,38 @@
+2022-02-27 Wenson Hsieh <wenson_hs...@apple.com>
+
+ Invoking "Markup Image" should preserve the existing selection range
+ https://bugs.webkit.org/show_bug.cgi?id=237242
+
+ Reviewed by Darin Adler.
+
+ Move the implementation of `replaceNodeFromPasteboard` out of the macOS-specific implementation file and into
+ EditorCocoa instead, so that we can use it on both iOS and macOS to handle the "Markup Image" action. See WebKit
+ ChangeLog for more details.
+
+ * editing/Editor.h:
+ * editing/cocoa/EditorCocoa.mm:
+ (WebCore::maybeCopyNodeAttributesToFragment):
+ (WebCore::Editor::replaceNodeFromPasteboard):
+
+ Also make a few minor adjustments while we're moving this code:
+ - Make this take a reference instead of a pointer (the method always expects a non-null Node pointer anyways).
+ - Add a FIXME mentioning that we can just delete `setInsertionPasteboard` altogether once Mail compose on
+ macOS uses WebKit2.
+ - Unconditionally pass `false` for whether or not we should enable smart paste when replacing the node. This
+ prevents us from inserting spaces around the replacement image element when triggering the "Markup Image"
+ item.
+ - Use `makeRangeSelectingNode` instead of `makeRangeSelectingNodeContents` when selecting the node to replace.
+ This allows us to handle the case where the node to replace cannot contain children for editing (in
+ particular, image elements).
+
+ Note that this codepath (`replaceNodeFromPasteboard`) is currently only exercised by WebKitLegacy service
+ controls code that was originally intended for use by Mail compose, but never ended up being used.
+
+ * editing/mac/EditorMac.mm:
+ (WebCore::Editor::pasteWithPasteboard):
+ (WebCore::maybeCopyNodeAttributesToFragment): Deleted.
+ (WebCore::Editor::replaceNodeFromPasteboard): Deleted.
+
2022-02-27 Matt Woodrow <mattwood...@apple.com>
Compute correct containing block override size for items that are subgridden in one dimension only.
Modified: trunk/Source/WebCore/editing/Editor.h (290577 => 290578)
--- trunk/Source/WebCore/editing/Editor.h 2022-02-28 03:03:39 UTC (rev 290577)
+++ trunk/Source/WebCore/editing/Editor.h 2022-02-28 03:24:13 UTC (rev 290578)
@@ -535,10 +535,10 @@
void takeFindStringFromSelection();
WEBCORE_EXPORT void replaceSelectionWithAttributedString(NSAttributedString *, MailBlockquoteHandling = MailBlockquoteHandling::RespectBlockquote);
WEBCORE_EXPORT void readSelectionFromPasteboard(const String& pasteboardName);
+ WEBCORE_EXPORT void replaceNodeFromPasteboard(Node&, const String& pasteboardName);
#endif
#if PLATFORM(MAC)
- WEBCORE_EXPORT void replaceNodeFromPasteboard(Node*, const String& pasteboardName);
WEBCORE_EXPORT RefPtr<SharedBuffer> dataSelectionForPasteboard(const String& pasteboardName);
#endif
Modified: trunk/Source/WebCore/editing/cocoa/EditorCocoa.mm (290577 => 290578)
--- trunk/Source/WebCore/editing/cocoa/EditorCocoa.mm 2022-02-28 03:03:39 UTC (rev 290577)
+++ trunk/Source/WebCore/editing/cocoa/EditorCocoa.mm 2022-02-28 03:24:13 UTC (rev 290578)
@@ -36,6 +36,7 @@
#import "Editing.h"
#import "EditingStyle.h"
#import "EditorClient.h"
+#import "ElementInlines.h"
#import "FontAttributes.h"
#import "FontCascade.h"
#import "Frame.h"
@@ -321,4 +322,67 @@
pasteAsPlainTextWithPasteboard(pasteboard);
}
+static void maybeCopyNodeAttributesToFragment(const Node& node, DocumentFragment& fragment)
+{
+ // This is only supported for single-Node fragments.
+ RefPtr firstChild = fragment.firstChild();
+ if (!firstChild || firstChild != fragment.lastChild())
+ return;
+
+ // And only supported for HTML elements.
+ if (!node.isHTMLElement() || !firstChild->isHTMLElement())
+ return;
+
+ // And only if the source Element and destination Element have the same HTML tag name.
+ Ref oldElement = downcast<HTMLElement>(node);
+ Ref newElement = downcast<HTMLElement>(*firstChild);
+ if (oldElement->localName() != newElement->localName())
+ return;
+
+ for (auto& attribute : oldElement->attributesIterator()) {
+ if (newElement->hasAttribute(attribute.name()))
+ continue;
+ newElement->setAttribute(attribute.name(), attribute.value());
+ }
}
+
+void Editor::replaceNodeFromPasteboard(Node& node, const String& pasteboardName)
+{
+ if (node.document() != m_document)
+ return;
+
+ auto range = makeRangeSelectingNode(node);
+ if (!range)
+ return;
+
+ Ref protectedDocument = m_document;
+ m_document.selection().setSelection({ *range }, FrameSelection::DoNotSetFocus);
+
+ Pasteboard pasteboard(PagePasteboardContext::create(m_document.pageID()), pasteboardName);
+ if (!m_document.selection().selection().isContentRichlyEditable()) {
+ pasteAsPlainTextWithPasteboard(pasteboard);
+ return;
+ }
+
+#if PLATFORM(MAC)
+ ALLOW_DEPRECATED_DECLARATIONS_BEGIN
+ // FIXME: How can this hard-coded pasteboard name be right, given that the passed-in pasteboard has a name?
+ // FIXME: We can also remove `setInsertionPasteboard` altogether once Mail compose on macOS no longer uses WebKitLegacy,
+ // since it's only implemented for WebKitLegacy on macOS, and the only known client is Mail compose.
+ client()->setInsertionPasteboard(NSGeneralPboard);
+ ALLOW_DEPRECATED_DECLARATIONS_END
+#endif
+
+ bool chosePlainText;
+ if (auto fragment = webContentFromPasteboard(pasteboard, *range, true, chosePlainText)) {
+ maybeCopyNodeAttributesToFragment(node, *fragment);
+ if (shouldInsertFragment(*fragment, *range, EditorInsertAction::Pasted))
+ pasteAsFragment(fragment.releaseNonNull(), false, false, MailBlockquoteHandling::IgnoreBlockquote);
+ }
+
+#if PLATFORM(MAC)
+ client()->setInsertionPasteboard({ });
+#endif
+}
+
+}
Modified: trunk/Source/WebCore/editing/mac/EditorMac.mm (290577 => 290578)
--- trunk/Source/WebCore/editing/mac/EditorMac.mm 2022-02-28 03:03:39 UTC (rev 290577)
+++ trunk/Source/WebCore/editing/mac/EditorMac.mm 2022-02-28 03:24:13 UTC (rev 290578)
@@ -184,64 +184,6 @@
client()->setInsertionPasteboard(String());
}
-static void maybeCopyNodeAttributesToFragment(const Node& node, DocumentFragment& fragment)
-{
- // This is only supported for single-Node fragments.
- Node* firstChild = fragment.firstChild();
- if (!firstChild || firstChild != fragment.lastChild())
- return;
-
- // And only supported for HTML elements.
- if (!node.isHTMLElement() || !firstChild->isHTMLElement())
- return;
-
- // And only if the source Element and destination Element have the same HTML tag name.
- const HTMLElement& oldElement = downcast<HTMLElement>(node);
- HTMLElement& newElement = downcast<HTMLElement>(*firstChild);
- if (oldElement.localName() != newElement.localName())
- return;
-
- for (const Attribute& attribute : oldElement.attributesIterator()) {
- if (newElement.hasAttribute(attribute.name()))
- continue;
- newElement.setAttribute(attribute.name(), attribute.value());
- }
-}
-
-void Editor::replaceNodeFromPasteboard(Node* node, const String& pasteboardName)
-{
- ASSERT(node);
-
- if (node->document() != m_document)
- return;
-
- Ref<Document> protector(m_document);
-
- auto range = makeRangeSelectingNodeContents(*node);
- m_document.selection().setSelection(VisibleSelection(range), FrameSelection::DoNotSetFocus);
-
- Pasteboard pasteboard(PagePasteboardContext::create(m_document.pageID()), pasteboardName);
-
- if (!m_document.selection().selection().isContentRichlyEditable()) {
- pasteAsPlainTextWithPasteboard(pasteboard);
- return;
- }
-
- ALLOW_DEPRECATED_DECLARATIONS_BEGIN
- // FIXME: How can this hard-coded pasteboard name be right, given that the passed-in pasteboard has a name?
- client()->setInsertionPasteboard(NSGeneralPboard);
- ALLOW_DEPRECATED_DECLARATIONS_END
-
- bool chosePlainText;
- if (auto fragment = webContentFromPasteboard(pasteboard, range, true, chosePlainText)) {
- maybeCopyNodeAttributesToFragment(*node, *fragment);
- if (shouldInsertFragment(*fragment, range, EditorInsertAction::Pasted))
- pasteAsFragment(fragment.releaseNonNull(), canSmartReplaceWithPasteboard(pasteboard), false, MailBlockquoteHandling::IgnoreBlockquote);
- }
-
- client()->setInsertionPasteboard(String());
-}
-
RefPtr<SharedBuffer> Editor::imageInWebArchiveFormat(Element& imageElement)
{
auto archive = LegacyWebArchive::create(imageElement);
Modified: trunk/Source/WebKit/ChangeLog (290577 => 290578)
--- trunk/Source/WebKit/ChangeLog 2022-02-28 03:03:39 UTC (rev 290577)
+++ trunk/Source/WebKit/ChangeLog 2022-02-28 03:24:13 UTC (rev 290578)
@@ -1,3 +1,35 @@
+2022-02-27 Wenson Hsieh <wenson_hs...@apple.com>
+
+ Invoking "Markup Image" should preserve the existing selection range
+ https://bugs.webkit.org/show_bug.cgi?id=237242
+
+ Reviewed by Darin Adler.
+
+ Make a few adjustments to `replaceWithPasteboardData`, such that it attempts to restore the previous selection
+ range after replacing the target element. If the previous selection is no longer valid (i.e., it was orphaned as
+ a result of the editing command used to replace the element), then we fall back to restoring the visible
+ character range of the previous selection, relative to the editable root (or the body if there is none).
+
+ API tests will be added in https://webkit.org/b/236519, once we're able to swizzle out the underlying VisionKit
+ methods in order to make these tests run reliably.
+
+ * WebProcess/WebPage/Cocoa/WebPageCocoa.mm:
+ (WebKit::OverridePasteboardForSelectionReplacement::OverridePasteboardForSelectionReplacement):
+ (WebKit::OverridePasteboardForSelectionReplacement::~OverridePasteboardForSelectionReplacement):
+
+ Add a helper RAII object that encapsulates logic for setting and unsetting data on the override pasteboard. Use
+ this in the two methods below.
+
+ (WebKit::WebPage::replaceWithPasteboardData):
+
+ Switch to using the refactored `Editor::replaceNodeFromPasteboard` method instead of calling to the adjacent
+ `replaceSelectionWithPasteboardData` method below. Using this method allows us to avoid manually selecting the
+ target element here, and also avoids smart pasteboard replacement (which may insert spaces before or after the
+ image); the latter is important in order to correctly restore the previous selection's character range in the
+ case where the previous selection range was orphaned by the replacement.
+
+ (WebKit::WebPage::replaceSelectionWithPasteboardData):
+
2022-02-27 Chris Dumez <cdu...@apple.com>
Omit template parameter for SetForScope<> variables
Modified: trunk/Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm (290577 => 290578)
--- trunk/Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm 2022-02-28 03:03:39 UTC (rev 290577)
+++ trunk/Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm 2022-02-28 03:24:13 UTC (rev 290578)
@@ -44,6 +44,7 @@
#import <WebCore/FocusController.h>
#import <WebCore/FrameView.h>
#import <WebCore/GraphicsContextCG.h>
+#import <WebCore/HTMLBodyElement.h>
#import <WebCore/HTMLConverter.h>
#import <WebCore/HTMLOListElement.h>
#import <WebCore/HTMLUListElement.h>
@@ -457,6 +458,27 @@
return string;
}
+class OverridePasteboardForSelectionReplacement {
+ WTF_MAKE_NONCOPYABLE(OverridePasteboardForSelectionReplacement);
+ WTF_MAKE_FAST_ALLOCATED;
+public:
+ OverridePasteboardForSelectionReplacement(const Vector<String>& types, const IPC::DataReference& data)
+ : m_types(types)
+ {
+ for (auto& type : types)
+ WebPasteboardOverrides::sharedPasteboardOverrides().addOverride(replaceSelectionPasteboardName(), type, { data });
+ }
+
+ ~OverridePasteboardForSelectionReplacement()
+ {
+ for (auto& type : m_types)
+ WebPasteboardOverrides::sharedPasteboardOverrides().removeOverride(replaceSelectionPasteboardName(), type);
+ }
+
+private:
+ Vector<String> m_types;
+};
+
void WebPage::replaceWithPasteboardData(const ElementContext& elementContext, const Vector<String>& types, const IPC::DataReference& data)
{
Ref frame = CheckedRef(m_page->focusController())->focusedOrMainFrame();
@@ -464,26 +486,58 @@
if (!element || !element->isContentEditable())
return;
- if (frame->document() != &element->document())
+ Ref document = element->document();
+ if (frame->document() != document.ptr())
return;
- auto replacementRange = makeRangeSelectingNode(*element);
- if (!replacementRange)
+ auto originalSelection = frame->selection().selection();
+ RefPtr selectionHost = originalSelection.rootEditableElement() ?: document->body();
+ if (!selectionHost)
return;
- frame->selection().setSelection(VisibleSelection { *replacementRange });
- replaceSelectionWithPasteboardData(types, data);
+ constexpr OptionSet iteratorOptions = TextIteratorBehavior::EmitsCharactersBetweenAllVisiblePositions;
+ std::optional<CharacterRange> rangeToRestore;
+ uint64_t numberOfCharactersInSelectionHost = 0;
+ if (auto range = originalSelection.range()) {
+ auto selectionHostRangeBeforeReplacement = makeRangeSelectingNodeContents(*selectionHost);
+ rangeToRestore = characterRange(selectionHostRangeBeforeReplacement, *range, iteratorOptions);
+ numberOfCharactersInSelectionHost = characterCount(selectionHostRangeBeforeReplacement, iteratorOptions);
+ }
+
+ {
+ OverridePasteboardForSelectionReplacement overridePasteboard { types, data };
+ IgnoreSelectionChangeForScope ignoreSelectionChanges { frame.get() };
+ frame->editor().replaceNodeFromPasteboard(*element, replaceSelectionPasteboardName());
+ }
+
+ constexpr auto restoreSelectionOptions = FrameSelection::defaultSetSelectionOptions(UserTriggered);
+ if (!originalSelection.isNoneOrOrphaned()) {
+ frame->selection().setSelection(originalSelection, restoreSelectionOptions);
+ return;
+ }
+
+ if (!rangeToRestore || !selectionHost->isConnected())
+ return;
+
+ auto selectionHostRange = makeRangeSelectingNodeContents(*selectionHost);
+ if (numberOfCharactersInSelectionHost != characterCount(selectionHostRange, iteratorOptions)) {
+ // FIXME: We don't attempt to restore the selection if the replaced element contains a different
+ // character count than the content that replaces it, since this codepath is currently only used
+ // to replace a single non-text element with another. If this is used to replace text content in
+ // the future, we should adjust the `rangeToRestore` to fit the newly inserted content.
+ return;
+ }
+
+ // The node replacement may have orphaned the original selection range; in this case, try to restore
+ // the original selected character range.
+ auto newSelectionRange = resolveCharacterRange(selectionHostRange, *rangeToRestore, iteratorOptions);
+ frame->selection().setSelection(newSelectionRange, restoreSelectionOptions);
}
void WebPage::replaceSelectionWithPasteboardData(const Vector<String>& types, const IPC::DataReference& data)
{
- for (auto& type : types)
- WebPasteboardOverrides::sharedPasteboardOverrides().addOverride(replaceSelectionPasteboardName(), type, { data });
-
+ OverridePasteboardForSelectionReplacement overridePasteboard { types, data };
readSelectionFromPasteboard(replaceSelectionPasteboardName(), [](bool) { });
-
- for (auto& type : types)
- WebPasteboardOverrides::sharedPasteboardOverrides().removeOverride(replaceSelectionPasteboardName(), type);
}
void WebPage::readSelectionFromPasteboard(const String& pasteboardName, CompletionHandler<void(bool&&)>&& completionHandler)
Modified: trunk/Source/WebKitLegacy/mac/ChangeLog (290577 => 290578)
--- trunk/Source/WebKitLegacy/mac/ChangeLog 2022-02-28 03:03:39 UTC (rev 290577)
+++ trunk/Source/WebKitLegacy/mac/ChangeLog 2022-02-28 03:24:13 UTC (rev 290578)
@@ -1,3 +1,17 @@
+2022-02-27 Wenson Hsieh <wenson_hs...@apple.com>
+
+ Invoking "Markup Image" should preserve the existing selection range
+ https://bugs.webkit.org/show_bug.cgi?id=237242
+
+ Reviewed by Darin Adler.
+
+ See WebKit and WebCore ChangeLogs for more detail.
+
+ * Misc/WebSharingServicePickerController.mm:
+ (-[WebSharingServicePickerController didShareImageData:confirmDataIsValidTIFFData:]):
+
+ Adjust this call site to pass in a reference instead of a pointer. Also deploy RefPtr in some adjacent code.
+
2022-02-27 Chris Dumez <cdu...@apple.com>
Omit template parameter for SetForScope<> variables
Modified: trunk/Source/WebKitLegacy/mac/Misc/WebSharingServicePickerController.mm (290577 => 290578)
--- trunk/Source/WebKitLegacy/mac/Misc/WebSharingServicePickerController.mm 2022-02-28 03:03:39 UTC (rev 290577)
+++ trunk/Source/WebKitLegacy/mac/Misc/WebSharingServicePickerController.mm 2022-02-28 03:24:13 UTC (rev 290578)
@@ -141,9 +141,9 @@
[pasteboard declareTypes:@[ NSPasteboardTypeTIFF ] owner:nil];
[pasteboard setData:data forType:NSPasteboardTypeTIFF];
- if (auto* node = page->contextMenuController().context().hitTestResult().innerNode()) {
- if (auto* frame = node->document().frame())
- frame->editor().replaceNodeFromPasteboard(node, serviceControlsPasteboardName);
+ if (RefPtr node = page->contextMenuController().context().hitTestResult().innerNode()) {
+ if (RefPtr frame = node->document().frame())
+ frame->editor().replaceNodeFromPasteboard(*node, serviceControlsPasteboardName);
}
[self clear];
