Attachment 373628 Details for Bug 199565 – Patch for landing

[patch] Patch for landing

bug-199565-20190708085038.patch (text/plain), 10.78 KB, created by Wenson Hsieh on 2019-07-08 08:50:39 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Wenson Hsieh

Created: 2019-07-08 08:50:39 PDT

Size: 10.78 KB

patch

obsolete

>Subversion Revision: 247162
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index d87dbc82fe93bbba7d2934af0a4a86abda92cb82..e584998c6ec50a9f6b4fbabb302dc5756e051c18 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,60 @@
>+2019-07-07  Wenson Hsieh  <wenson_hsieh@apple.com>
>+
>+        Unable to paste from Notes into Excel 365 spreadsheet
>+        https://bugs.webkit.org/show_bug.cgi?id=199565
>+        <rdar://problem/43615497>
>+
>+        Reviewed by Chris Dumez.
>+
>+        When pasting into Microsoft Excel 365, the copied data is all inserted into a single cell, even when copying a
>+        table. To understand why this happens, we first need to understand how Excel's logic for handling paste works.
>+        When tapping on the "Paste" button, Excel performs and expects the following:
>+
>+        1.  Before triggering programmatic paste, move focus into a hidden contenteditable area specifically intended to
>+            capture pasted content.
>+        2.  Run a promise that resolves immediately; the promise callback restores focus to the originally focused
>+            element prior to (1).
>+        3.  Invoke programmatic paste using `document.execCommand("Paste")`.
>+        4.  The callback scheduled in step (2) then runs, restoring focus to the main editable element representing a
>+            table cell.
>+
>+        However, what ends up happening is this:
>+
>+        Steps (1)-(3): same as before.
>+        4.  We (WebKit) create a temporary Page for the purposes of sanitizing copied web content before exposing it to
>+            the paste handler. This involves creating and loading a document; when this is finished, we call into
>+            Document::finishedParsing which flushes the microtask queue.
>+        5.  This causes us to immediately run the microtask enqueued in step (2), which restores focus to the previously
>+            focused element (importantly, this is not the element that was focused in step (1)).
>+        6.  The paste commences, and inserts the sanitized fragment into the originally focused element rather than the
>+            content editable area intended to capture pasted content.
>+
>+        Excel's script then gets confused, and does not end up using their special paste logic to handle the paste. The
>+        pasted content is instead just inserted as plain text in a cell. To address this, we simply prevent document
>+        load in the Page for web content sanitization from triggering a microtask checkpoint; this allows any scheduled
>+        main thread microtasks to be deferred until the next turn of the runloop.
>+
>+        Test: editing/pasteboard/paste-does-not-fire-promises-while-sanitizing-web-content.html
>+
>+        * dom/Document.cpp:
>+        (WebCore::Document::finishedParsing):
>+
>+        Don't immediately dispatch microtasks when we finish document parsing, in the case where the page is intended
>+        only for web content sanitization, since this may end up executing script in the original document. As explained
>+        above, this causes compatibility issues when pasting in Excel.
>+
>+        * editing/markup.cpp:
>+        (WebCore::createPageForSanitizingWebContent):
>+
>+        When creating a page for sanitizing web content, mark it as such.
>+
>+        * page/Page.h:
>+
>+        Add a new flag to indicate that a Page is only intended for sanitizing web content.
>+
>+        (WebCore::Page::setIsForSanitizingWebContent):
>+        (WebCore::Page::isForSanitizingWebContent const):
>+
> 2019-07-05  Ryan Haddad  <ryanhaddad@apple.com>
> 
>         Unreviewed, rolling out r247123.
>diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp
>index 78e59127508d98f97539858761b4dd3649e2aad2..bd12169fcde519f009c7fea00fe5c07b125a2839 100644
>--- a/Source/WebCore/dom/Document.cpp
>+++ b/Source/WebCore/dom/Document.cpp
>@@ -5670,8 +5670,10 @@ void Document::finishedParsing()
>     if (!m_documentTiming.domContentLoadedEventStart)
>         m_documentTiming.domContentLoadedEventStart = MonotonicTime::now();
> 
>-    // FIXME: Schdule a task to fire DOMContentLoaded event instead. See webkit.org/b/82931
>-    MicrotaskQueue::mainThreadQueue().performMicrotaskCheckpoint();
>+    if (!page() || !page()->isForSanitizingWebContent()) {
>+        // FIXME: Schedule a task to fire DOMContentLoaded event instead. See webkit.org/b/82931
>+        MicrotaskQueue::mainThreadQueue().performMicrotaskCheckpoint();
>+    }
> 
>     dispatchEvent(Event::create(eventNames().DOMContentLoadedEvent, Event::CanBubble::Yes, Event::IsCancelable::No));
> 
>diff --git a/Source/WebCore/editing/markup.cpp b/Source/WebCore/editing/markup.cpp
>index 0f7cea703e5fcb8c26e15ac03d041e0140ad6c91..203cc47aa6d6f541487f54a4669832f3b5df575e 100644
>--- a/Source/WebCore/editing/markup.cpp
>+++ b/Source/WebCore/editing/markup.cpp
>@@ -179,6 +179,7 @@ std::unique_ptr<Page> createPageForSanitizingWebContent()
>     auto pageConfiguration = pageConfigurationWithEmptyClients();
>     
>     auto page = std::make_unique<Page>(WTFMove(pageConfiguration));
>+    page->setIsForSanitizingWebContent();
>     page->settings().setMediaEnabled(false);
>     page->settings().setScriptEnabled(false);
>     page->settings().setPluginsEnabled(false);
>diff --git a/Source/WebCore/page/Page.h b/Source/WebCore/page/Page.h
>index 2fab6e2e523c9553fb9bf84cbe0c4c9c41620593..ef0e830931f25dfef92d048b4d5de02fc3f0d376 100644
>--- a/Source/WebCore/page/Page.h
>+++ b/Source/WebCore/page/Page.h
>@@ -653,6 +653,9 @@ public:
>     void clearTrigger() { m_testTrigger = nullptr; }
>     bool expectsWheelEventTriggers() const { return !!m_testTrigger; }
> 
>+    void setIsForSanitizingWebContent() { m_isForSanitizingWebContent = true; }
>+    bool isForSanitizingWebContent() const { return m_isForSanitizingWebContent; }
>+
> #if ENABLE(VIDEO)
>     bool allowsMediaDocumentInlinePlayback() const { return m_allowsMediaDocumentInlinePlayback; }
>     WEBCORE_EXPORT void setAllowsMediaDocumentInlinePlayback(bool);
>@@ -991,6 +994,8 @@ private:
>     bool m_mediaPlaybackIsSuspended { false };
>     bool m_mediaBufferingIsSuspended { false };
>     bool m_inUpdateRendering { false };
>+
>+    bool m_isForSanitizingWebContent { false };
> };
> 
> inline PageGroup& Page::group()
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 53fe5808e8e3842f997eb74a06d51c7d70fa0b83..e174552a511871bcc50b16b6bb95fe969033e4aa 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,17 @@
>+2019-07-07  Wenson Hsieh  <wenson_hsieh@apple.com>
>+
>+        Unable to paste from Notes into Excel 365 spreadsheet
>+        https://bugs.webkit.org/show_bug.cgi?id=199565
>+        <rdar://problem/43615497>
>+
>+        Reviewed by Chris Dumez.
>+
>+        Add a test to verify that promises scheduled right before a programmatic paste resolve in the middle of the
>+        paste, while creating a document for web content sanitization. See WebCore ChangeLog for more details.
>+
>+        * editing/pasteboard/paste-does-not-fire-promises-while-sanitizing-web-content-expected.txt: Added.
>+        * editing/pasteboard/paste-does-not-fire-promises-while-sanitizing-web-content.html: Added.
>+
> 2019-07-05  Wenson Hsieh  <wenson_hsieh@apple.com>
> 
>         Click events on outer page are not being dispatched correctly after touch-zooming within an iframe
>diff --git a/LayoutTests/editing/pasteboard/paste-does-not-fire-promises-while-sanitizing-web-content-expected.txt b/LayoutTests/editing/pasteboard/paste-does-not-fire-promises-while-sanitizing-web-content-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..d1801a7f478d0a68e7f57ad21543a7e30bbd4519
>--- /dev/null
>+++ b/LayoutTests/editing/pasteboard/paste-does-not-fire-promises-while-sanitizing-web-content-expected.txt
>@@ -0,0 +1,13 @@
>+Click here to copy
>+ 
>+Verifies that a promise scheduled right before a programmatically triggered paste does not resolve during the paste. To test manually, tap the frame to copy some text, and then tap the editable area to paste. The promise should run after the paste event handler.
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+PASS Focused editor.
>+PASS Handled paste.
>+PASS Focused textarea.
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>diff --git a/LayoutTests/editing/pasteboard/paste-does-not-fire-promises-while-sanitizing-web-content.html b/LayoutTests/editing/pasteboard/paste-does-not-fire-promises-while-sanitizing-web-content.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..08f8272bc02736c2b2116fde6d59bc9777c51f1d
>--- /dev/null
>+++ b/LayoutTests/editing/pasteboard/paste-does-not-fire-promises-while-sanitizing-web-content.html
>@@ -0,0 +1,83 @@
>+<!DOCTYPE html> 
>+<html>
>+<meta name="viewport" content="width=device-width, initial-scale=1, user-scalable=no">
>+<head>
>+<script src="../../resources/js-test.js"></script>
>+<script src="../../resources/ui-helper.js"></script>
>+<style>
>+body {
>+    margin: 0;
>+}
>+
>+#copy {
>+    width: 100%;
>+    height: 50px;
>+    border: 1px dashed black;
>+}
>+
>+#editor {
>+    width: 100%;
>+    height: 100px;
>+    border: 1px dashed tomato;
>+    text-align: center;
>+}
>+</style>
>+</head>
>+<body>
>+<div id="editor" contenteditable></div>
>+<iframe id="copy" src="data:text/html,<div id='copy' style='font-size: 40px; text-align: center;'>Click here to copy</div>
>+    <script>
>+    copy.addEventListener('mousedown', event => {
>+        getSelection().selectAllChildren(copy);
>+        document.execCommand('Copy');
>+        getSelection().removeAllRanges();
>+        event.preventDefault();
>+    });
>+    </script>"></iframe>
>+<textarea></textarea>
>+<div id="description"></div>
>+<div id="console"></div>
>+<script>
>+jsTestIsAsync = true;
>+
>+description("Verifies that a promise scheduled right before a programmatically triggered paste does not resolve during the paste. To test manually, tap the frame to copy some text, and then tap the editable area to paste. The promise should run after the paste event handler.");
>+
>+textarea = document.querySelector("textarea");
>+frame = document.querySelector("iframe");
>+editor = document.getElementById("editor");
>+progress = 0;
>+
>+function checkDone() {
>+    if (++progress == 3)
>+        finishJSTest();
>+}
>+
>+addEventListener("load", async () => {
>+    if (!window.testRunner)
>+        return;
>+
>+    await UIHelper.activateElement(frame);
>+    await UIHelper.activateElement(editor);
>+    checkDone();
>+});
>+
>+editor.addEventListener("mousedown", event => {
>+    editor.focus();
>+    Promise.resolve().then(() => textarea.focus());
>+    document.execCommand("Paste");
>+    event.preventDefault();
>+});
>+
>+editor.addEventListener("paste", () => {
>+    testPassed("Handled paste.");
>+    checkDone();
>+});
>+
>+editor.addEventListener("focus", () => testPassed("Focused editor."));
>+textarea.addEventListener("focus", () => {
>+    testPassed("Focused textarea.");
>+    checkDone();
>+});
>+</script>
>+</body>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 199565: 373609 | 373610 | 373624 | 373628