REGRESSION (r244633): Mail flashes when copying text in an email
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 23 Jun 2020 22:52:30 +0000 (22:52 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 23 Jun 2020 22:52:30 +0000 (22:52 +0000)
https://bugs.webkit.org/show_bug.cgi?id=213529
<rdar://problem/55025522>

Reviewed by Tim Horton.

Source/WebKit:

When copying a text selection in Mail conversation view, Mail uses injected bundle hooks to supply additional
metadata before writing to the pasteboard. Grabbing this data involves temporarily expanding any collapsed
blockquotes that appear in the mail message, before re-collapsing them after we finish gathering data to be
written to the pasteboard.

Prior to r244633, the size of the document while expanding collapsed content would not be propagated to the
WKWebView, since intrinsic content size changes were batched and sent ahead of the next drawing area flush.
After r244633, however, updates are now sent every time the layout size changes. This was done to ensure that
the intrinsic content size was always up to date before sending the first layout milestone to the UI process.

To preserve the intent of r244633 (with respect to the first layout milestone) as well as intrinsic content size
behavior prior to r244633 after performing layout, we reintroduce the mechanism for batching intrinsic size
updates until the next flush, but keep the out-of-band mechanism for immediately sending an up-to-date intrinsic
size to the UI process before firing the first layout milestone.

Test: WebKit.AutoLayoutBatchesUpdatesWhenInvalidatingIntrinsicContentSize

* WebProcess/WebCoreSupport/WebChromeClient.cpp:
(WebKit::WebChromeClient::intrinsicContentsSizeChanged const):
* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::updateIntrinsicContentSizeIfNeeded):

If we have a pending intrinsic content size update, clear it out here so that an out-of-band call to
updateIntrinsicContentSizeIfNeeded doesn't get overridden by an older intrinsic content size that was previously
scheduled.

(WebKit::WebPage::flushPendingIntrinsicContentSizeUpdate):

Add a helper method to send any pending intrinsic content size update.

(WebKit::WebPage::scheduleIntrinsicContentSizeUpdate):

Add a helper method to schedule an intrinsic content size update for the next tiled drawing area flush.

* WebProcess/WebPage/WebPage.h:
* WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:
(WebKit::TiledCoreAnimationDrawingArea::updateRendering):

Tools:

Add an API test to verify that temporarily changing the height of the document before restoring it to the
original value does not cause intrinsic content size to be invalidated.

* TestWebKitAPI/Tests/WebKitCocoa/AutoLayoutIntegration.mm:
(-[AutoLayoutWKWebView invalidateIntrinsicContentSize]):
(-[AutoLayoutWKWebView setInvalidatedIntrinsicContentSizeBlock:]):
(-[AutoLayoutWKWebView invalidatedIntrinsicContentSizeBlock]):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@263428 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog
Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp
Source/WebKit/WebProcess/WebPage/WebPage.cpp
Source/WebKit/WebProcess/WebPage/WebPage.h
Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/AutoLayoutIntegration.mm

index 9755589..4fbe62d 100644 (file)
@@ -1,3 +1,49 @@
+2020-06-23  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        REGRESSION (r244633): Mail flashes when copying text in an email
+        https://bugs.webkit.org/show_bug.cgi?id=213529
+        <rdar://problem/55025522>
+
+        Reviewed by Tim Horton.
+
+        When copying a text selection in Mail conversation view, Mail uses injected bundle hooks to supply additional
+        metadata before writing to the pasteboard. Grabbing this data involves temporarily expanding any collapsed
+        blockquotes that appear in the mail message, before re-collapsing them after we finish gathering data to be
+        written to the pasteboard.
+
+        Prior to r244633, the size of the document while expanding collapsed content would not be propagated to the
+        WKWebView, since intrinsic content size changes were batched and sent ahead of the next drawing area flush.
+        After r244633, however, updates are now sent every time the layout size changes. This was done to ensure that
+        the intrinsic content size was always up to date before sending the first layout milestone to the UI process.
+
+        To preserve the intent of r244633 (with respect to the first layout milestone) as well as intrinsic content size
+        behavior prior to r244633 after performing layout, we reintroduce the mechanism for batching intrinsic size
+        updates until the next flush, but keep the out-of-band mechanism for immediately sending an up-to-date intrinsic
+        size to the UI process before firing the first layout milestone.
+
+        Test: WebKit.AutoLayoutBatchesUpdatesWhenInvalidatingIntrinsicContentSize
+
+        * WebProcess/WebCoreSupport/WebChromeClient.cpp:
+        (WebKit::WebChromeClient::intrinsicContentsSizeChanged const):
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::updateIntrinsicContentSizeIfNeeded):
+
+        If we have a pending intrinsic content size update, clear it out here so that an out-of-band call to
+        updateIntrinsicContentSizeIfNeeded doesn't get overridden by an older intrinsic content size that was previously
+        scheduled.
+
+        (WebKit::WebPage::flushPendingIntrinsicContentSizeUpdate):
+
+        Add a helper method to send any pending intrinsic content size update.
+
+        (WebKit::WebPage::scheduleIntrinsicContentSizeUpdate):
+
+        Add a helper method to schedule an intrinsic content size update for the next tiled drawing area flush.
+
+        * WebProcess/WebPage/WebPage.h:
+        * WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:
+        (WebKit::TiledCoreAnimationDrawingArea::updateRendering):
+
 2020-06-23  Andy Estes  <aestes@apple.com>
 
         [Apple Pay] WebPaymentCoordinatorProxy can be destroyed without dismissing its authorization presenter
index 22f9c46..5aff6d9 100644 (file)
@@ -592,7 +592,7 @@ PlatformPageClient WebChromeClient::platformPageClient() const
 
 void WebChromeClient::intrinsicContentsSizeChanged(const IntSize& size) const
 {
-    m_page.updateIntrinsicContentSizeIfNeeded(size);
+    m_page.scheduleIntrinsicContentSizeUpdate(size);
 }
 
 void WebChromeClient::contentsSizeChanged(Frame& frame, const IntSize& size) const
index 770910b..422646d 100644 (file)
@@ -6556,6 +6556,7 @@ void WebPage::removeAllUserContent()
 
 void WebPage::updateIntrinsicContentSizeIfNeeded(const WebCore::IntSize& size)
 {
+    m_pendingIntrinsicContentSize = WTF::nullopt;
     if (!minimumSizeForAutoLayout().width() && !sizeToContentAutoSizeMaximumSize().width() && !sizeToContentAutoSizeMaximumSize().height())
         return;
     ASSERT(mainFrameView());
@@ -6567,6 +6568,22 @@ void WebPage::updateIntrinsicContentSizeIfNeeded(const WebCore::IntSize& size)
     send(Messages::WebPageProxy::DidChangeIntrinsicContentSize(size));
 }
 
+void WebPage::flushPendingIntrinsicContentSizeUpdate()
+{
+    if (auto pendingSize = std::exchange(m_pendingIntrinsicContentSize, WTF::nullopt))
+        updateIntrinsicContentSizeIfNeeded(*pendingSize);
+}
+
+void WebPage::scheduleIntrinsicContentSizeUpdate(const IntSize& size)
+{
+    if (!minimumSizeForAutoLayout().width() && !sizeToContentAutoSizeMaximumSize().width() && !sizeToContentAutoSizeMaximumSize().height())
+        return;
+    ASSERT(mainFrameView());
+    ASSERT(mainFrameView()->isFixedWidthAutoSizeEnabled() || mainFrameView()->isSizeToContentAutoSizeEnabled());
+    ASSERT(!mainFrameView()->needsLayout());
+    m_pendingIntrinsicContentSize = size;
+}
+
 void WebPage::dispatchDidReachLayoutMilestone(OptionSet<WebCore::LayoutMilestone> milestones)
 {
     RefPtr<API::Object> userData;
index 4dabe09..6a7f688 100644 (file)
@@ -1283,7 +1283,10 @@ public:
 
     WebPageProxyIdentifier webPageProxyIdentifier() const { return m_webPageProxyIdentifier; }
 
+    void scheduleIntrinsicContentSizeUpdate(const WebCore::IntSize&);
+    void flushPendingIntrinsicContentSizeUpdate();
     void updateIntrinsicContentSizeIfNeeded(const WebCore::IntSize&);
+
     void scheduleFullEditorStateUpdate();
     bool isThrottleable() const;
 
@@ -2105,6 +2108,7 @@ private:
     WeakPtr<WebRemoteObjectRegistry> m_remoteObjectRegistry;
 #endif
     WebPageProxyIdentifier m_webPageProxyIdentifier;
+    Optional<WebCore::IntSize> m_pendingIntrinsicContentSize;
     WebCore::IntSize m_lastSentIntrinsicContentSize;
 #if HAVE(VISIBILITY_PROPAGATION_VIEW)
     std::unique_ptr<LayerHostingContext> m_contextForVisibilityPropagation;
index 6b5af67..bc55f0e 100644 (file)
@@ -458,6 +458,7 @@ void TiledCoreAnimationDrawingArea::updateRendering(UpdateRenderingType flushTyp
 
         m_webPage.updateRendering();
         m_webPage.flushPendingEditorStateUpdate();
+        m_webPage.flushPendingIntrinsicContentSizeUpdate();
 
         if (m_pendingRootLayer) {
             setRootCompositingLayer(m_pendingRootLayer.get());
index 5b3c248..6d20bfd 100644 (file)
@@ -1,3 +1,19 @@
+2020-06-23  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        REGRESSION (r244633): Mail flashes when copying text in an email
+        https://bugs.webkit.org/show_bug.cgi?id=213529
+        <rdar://problem/55025522>
+
+        Reviewed by Tim Horton.
+
+        Add an API test to verify that temporarily changing the height of the document before restoring it to the
+        original value does not cause intrinsic content size to be invalidated.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/AutoLayoutIntegration.mm:
+        (-[AutoLayoutWKWebView invalidateIntrinsicContentSize]):
+        (-[AutoLayoutWKWebView setInvalidatedIntrinsicContentSizeBlock:]):
+        (-[AutoLayoutWKWebView invalidatedIntrinsicContentSizeBlock]):
+
 2020-06-23  Diego Pino Garcia  <dpino@igalia.com>
 
         [ews] Allow archiving 'jsc' product
index deb0168..36d454e 100644 (file)
@@ -28,6 +28,7 @@
 #import "PlatformUtilities.h"
 #import "TestNavigationDelegate.h"
 #import <WebKit/WKWebViewPrivate.h>
+#import <wtf/BlockPtr.h>
 #import <wtf/RetainPtr.h>
 
 #if PLATFORM(MAC)
@@ -38,9 +39,12 @@ static bool didEvaluateJavaScript;
 @interface AutoLayoutWKWebView : WKWebView
 @property (nonatomic) BOOL expectingIntrinsicContentSizeChange;
 @property (nonatomic) NSSize expectedIntrinsicContentSize;
+@property (nonatomic) dispatch_block_t invalidatedIntrinsicContentSizeBlock;
 @end
 
-@implementation AutoLayoutWKWebView
+@implementation AutoLayoutWKWebView {
+    BlockPtr<void()> _invalidatedIntrinsicContentSizeBlock;
+}
 
 - (instancetype)initWithFrame:(NSRect)frame configuration:(WKWebViewConfiguration *)configuration
 {
@@ -110,6 +114,9 @@ static bool didEvaluateJavaScript;
 
 - (void)invalidateIntrinsicContentSize
 {
+    if (_invalidatedIntrinsicContentSizeBlock)
+        _invalidatedIntrinsicContentSizeBlock();
+
     if (!_expectingIntrinsicContentSizeChange)
         return;
 
@@ -117,6 +124,16 @@ static bool didEvaluateJavaScript;
     didInvalidateIntrinsicContentSize = true;
 }
 
+- (void)setInvalidatedIntrinsicContentSizeBlock:(dispatch_block_t)block
+{
+    _invalidatedIntrinsicContentSizeBlock = makeBlockPtr(block);
+}
+
+- (dispatch_block_t)invalidatedIntrinsicContentSizeBlock
+{
+    return _invalidatedIntrinsicContentSizeBlock.get();
+}
+
 @end
 
 TEST(WebKit, AutoLayoutIntegration)
@@ -209,4 +226,48 @@ TEST(WebKit, AutoLayoutRenderingProgressRelativeOrdering)
     [webView setNavigationDelegate:nil];
 }
 
+TEST(WebKit, AutoLayoutBatchesUpdatesWhenInvalidatingIntrinsicContentSize)
+{
+    auto webView = adoptNS([[AutoLayoutWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 1000, 1000)]);
+    [webView _setMinimumLayoutWidth:100];
+
+    auto navigationDelegate = adoptNS([[TestNavigationDelegate alloc] init]);
+    [webView setNavigationDelegate:navigationDelegate.get()];
+
+    __block bool didFinishNavigation = false;
+    didInvalidateIntrinsicContentSize = false;
+    [navigationDelegate setDidFinishNavigation:^(WKWebView *, WKNavigation *) {
+        didFinishNavigation = true;
+    }];
+
+    [webView setExpectingIntrinsicContentSizeChange:YES];
+    [webView loadHTMLString:@"<body style='margin: 0; height: 400px;'></body>" baseURL:nil];
+    TestWebKitAPI::Util::run(&didInvalidateIntrinsicContentSize);
+    TestWebKitAPI::Util::run(&didFinishNavigation);
+
+    NSString *script = @"document.body.style.height = '800px';"
+        "document.scrollingElement.scrollTop;"
+        "document.body.style.height = '400px';"
+        "document.scrollingElement.scrollTop;";
+
+    __block int intrinsicSizeInvalidationCount = 0;
+    [webView setInvalidatedIntrinsicContentSizeBlock:^{
+        ++intrinsicSizeInvalidationCount;
+    }];
+
+    __block bool doneEvaluatingScript = false;
+    [webView evaluateJavaScript:script completionHandler:^(id, NSError *) {
+        doneEvaluatingScript = true;
+    }];
+    TestWebKitAPI::Util::run(&doneEvaluatingScript);
+
+    __block bool doneWaitingForNextPresentationUpdate = false;
+    [webView _doAfterNextPresentationUpdate:^{
+        doneWaitingForNextPresentationUpdate = true;
+    }];
+    TestWebKitAPI::Util::run(&doneWaitingForNextPresentationUpdate);
+
+    EXPECT_EQ(0, intrinsicSizeInvalidationCount);
+}
+
 #endif