Attachment 356939 Details for Bug 192543 – Tentative fix (unsure if this is really needed)

[patch] Tentative fix (unsure if this is really needed)

bug-192543-20181209183336.patch (text/plain), 11.57 KB, created by Wenson Hsieh on 2018-12-09 18:33:36 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Wenson Hsieh

Created: 2018-12-09 18:33:36 PST

Size: 11.57 KB

patch

obsolete

>Subversion Revision: 239022
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index ca19b58a5020607f1bb6eaaad762c87424891fe8..7f9dcc76fe9faa8a9caabd121b9bda9a5f181931 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,36 @@
>+2018-12-09  Wenson Hsieh  <wenson_hsieh@apple.com>
>+
>+        [Cocoa] -_webView:didInsertAttachment:withSource: should be invoked by the time the UI process is notified of page load completion
>+        https://bugs.webkit.org/show_bug.cgi?id=192543
>+        <rdar://problem/46578484>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Currently, all attachment insertion and removal updates are coalesced until the end of the runloop before
>+        notifying the client, using the editor UI update timer. This allows us to avoid sending redundant removal and
>+        insertion updates when applying edit commands which involve temporarily removing or reparenting portions of the
>+        DOM (examples of this include paragraph merging, indentation, list insertion, and block style changes).
>+
>+        However, this is confusing for clients which may want to know when all attachment elements in the document have
>+        been acknowledged in the UI process via attachment update delegate callbacks (i.e.
>+        -_webView:didInsertAttachment:withSource:); furthermore, it's confusing when clients wish to manipulate
>+        attachment elements using DOM API and wish to run native code when the UI process reflects the changes made as
>+        a result of DOM manipulation. While this is possible by posting a script message back to the UI process on a
>+        zero-delay timer after modifying the DOM, this approach is cumbersome and shouldn't be necessary.
>+
>+        Instead, WebKit can limit coalescing attachment updates to only when an edit command is executing, rather than
>+        always coalescing updates. To do this, we add a flag on Editor to indicate when attachments updates should be
>+        deferred (i.e. coalesced when we next dispatch an editor UI update). This flag is temporarily set to `true` when
>+        executing edit commands, including typing and style changes.
>+
>+        Test: WKAttachmentTests.AttachmentUpdatesWhenRemovingAndAppending
>+
>+        * editing/Editor.cpp:
>+        (WebCore::Editor::applyStyle):
>+        (WebCore::Editor::applyParagraphStyle):
>+        (WebCore::Editor::willApplyEditing const):
>+        (WebCore::Editor::appliedEditing):
>+
> 2018-12-07  Wenson Hsieh  <wenson_hsieh@apple.com>
> 
>         [iOS] Caret is obscured by finger when dragging over an editable element
>diff --git a/Source/WebCore/editing/Editor.cpp b/Source/WebCore/editing/Editor.cpp
>index 69c16a94dd9dbd83a09d0e6df33ca83d8b32f954..f2ff8691762f92fcdbb6a7bc48de909ba1806814 100644
>--- a/Source/WebCore/editing/Editor.cpp
>+++ b/Source/WebCore/editing/Editor.cpp
>@@ -925,6 +925,10 @@ void Editor::applyStyle(RefPtr<EditingStyle>&& style, EditAction editingAction,
>     if (element && !dispatchBeforeInputEvent(*element, inputTypeName, inputEventData))
>         return;
> 
>+#if ENABLE(ATTACHMENT_ELEMENT)
>+    m_shouldDeferAttachmentUpdates = true;
>+#endif
>+
>     Ref<EditingStyle> styleToApply = colorFilterMode == ColorFilterMode::InvertColor ? style->inverseTransformColorIfNeeded(*element) : style.releaseNonNull();
> 
>     switch (selectionType) {
>@@ -939,6 +943,11 @@ void Editor::applyStyle(RefPtr<EditingStyle>&& style, EditAction editingAction,
>     }
> 
>     client()->didApplyStyle();
>+
>+#if ENABLE(ATTACHMENT_ELEMENT)
>+    m_shouldDeferAttachmentUpdates = false;
>+#endif
>+
>     if (element)
>         dispatchInputEvent(*element, inputTypeName, inputEventData);
> }
>@@ -963,8 +972,17 @@ void Editor::applyParagraphStyle(StyleProperties* style, EditAction editingActio
>     if (element && !dispatchBeforeInputEvent(*element, inputTypeName, inputEventData))
>         return;
> 
>+#if ENABLE(ATTACHMENT_ELEMENT)
>+    m_shouldDeferAttachmentUpdates = true;
>+#endif
>+
>     ApplyStyleCommand::create(document(), EditingStyle::create(style).ptr(), editingAction, ApplyStyleCommand::ForceBlockProperties)->apply();
>     client()->didApplyStyle();
>+
>+#if ENABLE(ATTACHMENT_ELEMENT)
>+    m_shouldDeferAttachmentUpdates = false;
>+#endif
>+
>     if (element)
>         dispatchInputEvent(*element, inputTypeName, inputEventData);
> }
>@@ -1071,8 +1089,15 @@ bool Editor::willApplyEditing(CompositeEditCommand& command, Vector<RefPtr<Stati
>     if (!composition)
>         return true;
> 
>-    return dispatchBeforeInputEvents(composition->startingRootEditableElement(), composition->endingRootEditableElement(), command.inputEventTypeName(),
>-        command.inputEventData(), command.inputEventDataTransfer(), targetRanges, command.isBeforeInputEventCancelable() ? Event::IsCancelable::Yes : Event::IsCancelable::No);
>+    if (!dispatchBeforeInputEvents(composition->startingRootEditableElement(), composition->endingRootEditableElement(), command.inputEventTypeName(), command.inputEventData(), command.inputEventDataTransfer(), targetRanges, command.isBeforeInputEventCancelable() ? Event::IsCancelable::Yes : Event::IsCancelable::No))
>+        return false;
>+
>+#if ENABLE(ATTACHMENT_ELEMENT)
>+    if (command.isTypingCommand() || command.isTopLevelCommand())
>+        m_shouldDeferAttachmentUpdates = true;
>+#endif
>+
>+    return true;
> }
> 
> void Editor::appliedEditing(CompositeEditCommand& command)
>@@ -1119,11 +1144,23 @@ void Editor::appliedEditing(CompositeEditCommand& command)
>         }
>         respondToChangedContents(newSelection);
>     }
>+
>+#if ENABLE(ATTACHMENT_ELEMENT)
>+    if (command.isTypingCommand() || command.isTopLevelCommand())
>+        m_shouldDeferAttachmentUpdates = false;
>+#endif
> }
> 
> bool Editor::willUnapplyEditing(const EditCommandComposition& composition) const
> {
>-    return dispatchBeforeInputEvents(composition.startingRootEditableElement(), composition.endingRootEditableElement(), "historyUndo");
>+    if (!dispatchBeforeInputEvents(composition.startingRootEditableElement(), composition.endingRootEditableElement(), "historyUndo"))
>+        return false;
>+
>+#if ENABLE(ATTACHMENT_ELEMENT)
>+    m_shouldDeferAttachmentUpdates = true;
>+#endif
>+
>+    return true;
> }
> 
> void Editor::unappliedEditing(EditCommandComposition& composition)
>@@ -1144,11 +1181,21 @@ void Editor::unappliedEditing(EditCommandComposition& composition)
>     if (auto* client = this->client())
>         client->registerRedoStep(composition);
>     respondToChangedContents(newSelection);
>+
>+#if ENABLE(ATTACHMENT_ELEMENT)
>+    m_shouldDeferAttachmentUpdates = false;
>+#endif
> }
> 
> bool Editor::willReapplyEditing(const EditCommandComposition& composition) const
> {
>-    return dispatchBeforeInputEvents(composition.startingRootEditableElement(), composition.endingRootEditableElement(), "historyRedo");
>+    if (!dispatchBeforeInputEvents(composition.startingRootEditableElement(), composition.endingRootEditableElement(), "historyRedo"))
>+        return false;
>+
>+#if ENABLE(ATTACHMENT_ELEMENT)
>+    m_shouldDeferAttachmentUpdates = true;
>+#endif
>+    return true;
> }
> 
> void Editor::reappliedEditing(EditCommandComposition& composition)
>@@ -1167,6 +1214,10 @@ void Editor::reappliedEditing(EditCommandComposition& composition)
>     if (auto* client = this->client())
>         client->registerUndoStep(composition);
>     respondToChangedContents(newSelection);
>+
>+#if ENABLE(ATTACHMENT_ELEMENT)
>+    m_shouldDeferAttachmentUpdates = false;
>+#endif
> }
> 
> Editor::Editor(Frame& frame)
>@@ -4054,7 +4105,11 @@ void Editor::didInsertAttachmentElement(HTMLAttachmentElement& attachment)
> 
>     if (!m_removedAttachmentIdentifiers.take(identifier))
>         m_insertedAttachmentIdentifiers.add(identifier);
>-    scheduleEditorUIUpdate();
>+
>+    if (m_shouldDeferAttachmentUpdates)
>+        scheduleEditorUIUpdate();
>+    else
>+        notifyClientOfAttachmentUpdates();
> }
> 
> void Editor::didRemoveAttachmentElement(HTMLAttachmentElement& attachment)
>@@ -4065,7 +4120,11 @@ void Editor::didRemoveAttachmentElement(HTMLAttachmentElement& attachment)
> 
>     if (!m_insertedAttachmentIdentifiers.take(identifier))
>         m_removedAttachmentIdentifiers.add(identifier);
>-    scheduleEditorUIUpdate();
>+
>+    if (m_shouldDeferAttachmentUpdates)
>+        scheduleEditorUIUpdate();
>+    else
>+        notifyClientOfAttachmentUpdates();
> }
> 
> void Editor::notifyClientOfAttachmentUpdates()
>diff --git a/Source/WebCore/editing/Editor.h b/Source/WebCore/editing/Editor.h
>index 87b684f903a33fddb66304cf13486f9c84045bd4..95c696d88997a38046dcc41dcad0a481859df317 100644
>--- a/Source/WebCore/editing/Editor.h
>+++ b/Source/WebCore/editing/Editor.h
>@@ -604,6 +604,7 @@ private:
> #if ENABLE(ATTACHMENT_ELEMENT)
>     HashSet<String> m_insertedAttachmentIdentifiers;
>     HashSet<String> m_removedAttachmentIdentifiers;
>+    mutable bool m_shouldDeferAttachmentUpdates;
> #endif
> 
>     VisibleSelection m_mark;
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 2fdf3bdac50dda2f27bce636af26849e1668dbf3..42ce0e62c3bb32907027e1b7866ff8516b9befdc 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,19 @@
>+2018-12-09  Wenson Hsieh  <wenson_hsieh@apple.com>
>+
>+        [Cocoa] -_webView:didInsertAttachment:withSource: should be invoked by the time the UI process is notified of page load completion
>+        https://bugs.webkit.org/show_bug.cgi?id=192543
>+        <rdar://problem/46578484>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add a test to verify that when invoking DOM API from the UI process to add or remove attachment elements,
>+        changes to these attachment elements are noted in the UI process by the time the script completion handlers are
>+        called. Existing API tests verify that we continue to avoid thrashing attachment updates when executing certain
>+        editing commands, such as style changes and paragraph merging.
>+
>+        * TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm:
>+        (TestWebKitAPI::TEST):
>+
> 2018-12-07  Wenson Hsieh  <wenson_hsieh@apple.com>
> 
>         [iOS] Caret is obscured by finger when dragging over an editable element
>diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm
>index 94472adf90e162a4e665d21a672b0d650748dc4d..06932d0dd693ee618fd6ba09275980bc9791bb58 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm
>+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm
>@@ -745,6 +745,30 @@ TEST(WKAttachmentTests, AttachmentUpdatesWhenCuttingAndPasting)
>     EXPECT_FALSE([webView hasAttribute:@"webkitattachmentid" forQuerySelector:@"attachment"]);
> }
> 
>+TEST(WKAttachmentTests, AttachmentUpdatesWhenRemovingAndAppending)
>+{
>+    auto webView = webViewForTestingAttachments();
>+    [webView synchronouslyInsertAttachmentWithFilename:@"foo.txt" contentType:@"text/plain" data:testHTMLData()];
>+
>+    __block bool doneEvaluatingScript = false;
>+    [webView evaluateJavaScript:@"window.attachment = document.querySelector('attachment'); attachment.remove(); attachment.uniqueIdentifier;" completionHandler:^(NSString *uniqueIdentifier, NSError *error) {
>+        EXPECT_TRUE(!error);
>+        EXPECT_FALSE([webView _attachmentForIdentifier:uniqueIdentifier].connected);
>+        doneEvaluatingScript = true;
>+    }];
>+
>+    Util::run(&doneEvaluatingScript);
>+
>+    doneEvaluatingScript = false;
>+    [webView evaluateJavaScript:@"document.body.appendChild(attachment); attachment.uniqueIdentifier;" completionHandler:^(NSString *uniqueIdentifier, NSError *error) {
>+        EXPECT_TRUE(!error);
>+        EXPECT_TRUE([webView _attachmentForIdentifier:uniqueIdentifier].connected);
>+        doneEvaluatingScript = true;
>+    }];
>+
>+    Util::run(&doneEvaluatingScript);
>+}
>+
> TEST(WKAttachmentTests, AttachmentDataForEmptyFile)
> {
>     auto webView = webViewForTestingAttachments();

Actions: View | Formatted Diff | Diff

Attachments on bug 192543: 356939