Changeset 265091 in webkit

Timestamp:

Jul 30, 2020 11:25:47 AM (4 years ago)

Author:

timothy_horton@apple.com

Message:

Web content gets stuck in an inactive state (no cursor updates or text insertion caret) when activating a tab with a thumbnail visible
​https://bugs.webkit.org/show_bug.cgi?id=214962
<rdar://problem/65670984>

Reviewed by Wenson Hsieh.

Source/WebCore:

New API test: WebKit.WKThumbnailViewResetsViewStateWhenUnparented

• testing/Internals.cpp:

(WebCore::Internals::isPageActive const):

• testing/Internals.h:
• testing/Internals.idl:

Add a "view window is active" getter.

Source/WebKit:

• UIProcess/Cocoa/WebViewImpl.mm:

(WebKit::WebViewImpl::setThumbnailView):
When WKThumbnailView is unparented, invalidate all activity state bits.
We do this because many of the functions that compute activity state bits
use the WKThumbnailView's window while it is parented. When it is unparented,
and we switch back to using the WKWebView's window, we mustn't get stuck
with activity state bits from WKThumbnailView's window.

This was particularly problematic in the case of the WKThumbnailViews
used for tab hover previews, because that window is not key, so we'd get
stuck with the inactive-window state, resulting in a lack of cursor updates,
a hidden text caret, the wrong selection color, etc.

• UIProcess/mac/PageClientImplMac.mm:

(WebKit::PageClientImpl::isViewWindowActive):
While writing the API test for this, I noticed that isViewWindowActive
returns YES if there is no key window AND WKWebView is unparented
([NSApp keyWindow] == activeWindow() == nil).
This seems completely insane (how can the window be active if it doesn't
exist?), and breaks the test, so ensure that we only say YES if we actually
have a window.

Tools:

• TestWebKitAPI/Tests/WebKit/WKThumbnailView.mm:

(TestWebKitAPI::TEST):
Add a test ensuring that we go back to the active-window state after
unparenting a WKThumbnailView that was installed in an inactive window.

(-[WKThumbnailViewDelegate webView:didFinishNavigation:]): Deleted.

• TestWebKitAPI/mac/OffscreenWindow.h:
• TestWebKitAPI/mac/OffscreenWindow.mm:

(-[OffscreenWindow initWithSize:]):
(-[OffscreenWindow initWithSize:isKeyWindow:]):
(-[OffscreenWindow isKeyWindow]):

Location:

trunk

Files:

• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/testing/Internals.cpp (modified) (1 diff)
• Source/WebCore/testing/Internals.h (modified) (1 diff)
• Source/WebCore/testing/Internals.idl (modified) (1 diff)
• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm (modified) (1 diff)
• Source/WebKit/UIProcess/mac/PageClientImplMac.mm (modified) (1 diff)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKit/WKThumbnailView.mm (modified) (5 diffs)
• Tools/TestWebKitAPI/mac/OffscreenWindow.h (modified) (1 diff)
• Tools/TestWebKitAPI/mac/OffscreenWindow.mm (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2020-07-30  Tim Horton  <timothy_horton@apple.com>
+
+        Web content gets stuck in an inactive state (no cursor updates or text insertion caret) when activating a tab with a thumbnail visible
+        https://bugs.webkit.org/show_bug.cgi?id=214962
+        <rdar://problem/65670984>
+
+        Reviewed by Wenson Hsieh.
+
+        New API test: WebKit.WKThumbnailViewResetsViewStateWhenUnparented
+
+        * testing/Internals.cpp:
+        (WebCore::Internals::isPageActive const):
+        * testing/Internals.h:
+        * testing/Internals.idl:
+        Add a "view window is active" getter.
+
 2020-07-30  Antoine Quint  <graouts@webkit.org>
 

trunk/Source/WebCore/testing/Internals.cpp

@@ -5172 +5172 @@
 }
 
+bool Internals::isPageActive() const
+{
+    auto* document = contextDocument();
+    if (!document || !document->page())
+        return false;
+    auto& page = *document->page();
+    return page.activityState().contains(ActivityState::WindowIsActive);
+}
+
 #if ENABLE(WEB_RTC)
 void Internals::setH264HardwareEncoderAllowed(bool allowed)

trunk/Source/WebCore/testing/Internals.h

@@ -773 +773 @@
     void setPageVisibility(bool isVisible);
     void setPageIsFocusedAndActive(bool);
-
+    bool isPageActive() const;
 
 #if ENABLE(WEB_RTC)

trunk/Source/WebCore/testing/Internals.idl

@@ -797 +797 @@
     void setPageVisibility(boolean isVisible);
     void setPageIsFocusedAndActive(boolean isFocused);
+    boolean isPageActive();
 
     [Conditional=WEB_RTC] void setH264HardwareEncoderAllowed(boolean allowed);

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2020-07-30  Tim Horton  <timothy_horton@apple.com>
+
+        Web content gets stuck in an inactive state (no cursor updates or text insertion caret) when activating a tab with a thumbnail visible
+        https://bugs.webkit.org/show_bug.cgi?id=214962
+        <rdar://problem/65670984>
+
+        Reviewed by Wenson Hsieh.
+
+        * UIProcess/Cocoa/WebViewImpl.mm:
+        (WebKit::WebViewImpl::setThumbnailView):
+        When WKThumbnailView is unparented, invalidate all activity state bits.
+        We do this because many of the functions that compute activity state bits
+        use the WKThumbnailView's window while it is parented. When it is unparented,
+        and we switch back to using the WKWebView's window, we mustn't get stuck
+        with activity state bits from WKThumbnailView's window.
+
+        This was particularly problematic in the case of the WKThumbnailViews
+        used for tab hover previews, because that window is not key, so we'd get
+        stuck with the inactive-window state, resulting in a lack of cursor updates,
+        a hidden text caret, the wrong selection color, etc.
+
+        * UIProcess/mac/PageClientImplMac.mm:
+        (WebKit::PageClientImpl::isViewWindowActive):
+        While writing the API test for this, I noticed that isViewWindowActive
+        returns YES if there is no key window AND WKWebView is unparented
+        ([NSApp keyWindow] == activeWindow() == nil).
+        This seems completely insane (how can the window be active if it doesn't
+        exist?), and breaks the test, so ensure that we only say YES if we actually
+        have a window.
+
 2020-07-30  Peng Liu  <peng.liu6@apple.com>
 

trunk/Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm

@@ -3855 +3855 @@
     if (thumbnailView)
         updateThumbnailViewLayer();
-    else
+    else {
         setAcceleratedCompositingRootLayer(m_rootLayer.get());
+        m_page->activityStateDidChange(WebCore::ActivityState::allFlags());
+    }
 }
 

trunk/Source/WebKit/UIProcess/mac/PageClientImplMac.mm

@@ -161 +161 @@
     ASSERT(hasProcessPrivilege(ProcessPrivilege::CanCommunicateWithWindowServer));
     NSWindow *activeViewWindow = activeWindow();
-    return activeViewWindow.isKeyWindow || [NSApp keyWindow] == activeViewWindow;
+    return activeViewWindow.isKeyWindow || (activeViewWindow && [NSApp keyWindow] == activeViewWindow);
 }
 

trunk/Tools/ChangeLog

@@ +1 @@
+2020-07-30  Tim Horton  <timothy_horton@apple.com>
+
+        Web content gets stuck in an inactive state (no cursor updates or text insertion caret) when activating a tab with a thumbnail visible
+        https://bugs.webkit.org/show_bug.cgi?id=214962
+        <rdar://problem/65670984>
+
+        Reviewed by Wenson Hsieh.
+
+        * TestWebKitAPI/Tests/WebKit/WKThumbnailView.mm:
+        (TestWebKitAPI::TEST):
+        Add a test ensuring that we go back to the active-window state after
+        unparenting a WKThumbnailView that was installed in an inactive window.
+
+        (-[WKThumbnailViewDelegate webView:didFinishNavigation:]): Deleted.
+        * TestWebKitAPI/mac/OffscreenWindow.h:
+        * TestWebKitAPI/mac/OffscreenWindow.mm:
+        (-[OffscreenWindow initWithSize:]):
+        (-[OffscreenWindow initWithSize:isKeyWindow:]):
+        (-[OffscreenWindow isKeyWindow]):
+
 2020-07-30  Jonathan Bedard  <jbedard@apple.com>
 

trunk/Tools/TestWebKitAPI/Tests/WebKit/WKThumbnailView.mm

@@ -29 +29 @@
 
 #import "JavaScriptTest.h"
+#import "OffscreenWindow.h"
 #import "PlatformUtilities.h"
 #import "PlatformWebView.h"
 #import "TestWKWebView.h"
+#import "WKWebViewConfigurationExtras.h"
 #import <WebKit/WKViewPrivate.h>
 #import <WebKit/_WKThumbnailView.h>
@@ -58 +60 @@
 
     [super observeValueForKeyPath:keyPath ofObject:object change:change context:context];
-}
-
-@end
-
-
-@interface WKThumbnailViewDelegate : NSObject <WKNavigationDelegate>
-@end
-
-@implementation WKThumbnailViewDelegate
-
-- (void)webView:(WKWebView *)webView didFinishNavigation:(WKNavigation *)navigation
-{
-    didFinishLoad = true;
 }
 
@@ -200 +189 @@
     auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 800, 600)]);
     WKPageSetCustomBackingScaleFactor([webView _pageForTesting], 1);
-    auto delegate = adoptNS([[WKThumbnailViewDelegate alloc] init]);
-    [webView setNavigationDelegate:delegate.get()];
-    [webView  loadRequest:[NSURLRequest requestWithURL:[[NSBundle mainBundle] URLForResource:@"lots-of-text" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]]];
-    Util::run(&didFinishLoad);
+    [webView synchronouslyLoadTestPageNamed:@"lots-of-text"];
 
     RetainPtr<_WKThumbnailView> thumbnailView = adoptNS([[_WKThumbnailView alloc] initWithFrame:NSMakeRect(0, 0, 100, 100) fromWKWebView:webView.get()]);
@@ -242 +228 @@
     auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 800, 600)]);
     WKPageSetCustomBackingScaleFactor([webView _pageForTesting], 1);
-    auto delegate = adoptNS([[WKThumbnailViewDelegate alloc] init]);
-    [webView setNavigationDelegate:delegate.get()];
-    [webView  loadRequest:[NSURLRequest requestWithURL:[[NSBundle mainBundle] URLForResource:@"lots-of-text" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]]];
-    Util::run(&didFinishLoad);
+    [webView synchronouslyLoadTestPageNamed:@"lots-of-text"];
 
     auto thumbnailView = adoptNS([[_WKThumbnailView alloc] initWithFrame:NSMakeRect(0, 0, 100, 100) fromWKWebView:webView.get()]);
@@ -286 +269 @@
 }
 
+TEST(WebKit, WKThumbnailViewResetsViewStateWhenUnparented)
+{
+    WKWebViewConfiguration *configuration = [WKWebViewConfiguration _test_configurationWithTestPlugInClassName:@"WebProcessPlugInWithInternals" configureJSCForTesting:YES];
+    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 800, 600) configuration:configuration]);
+    [webView removeFromSuperview];
+    [webView synchronouslyLoadTestPageNamed:@"lots-of-text"];
+
+    auto thumbnailView = adoptNS([[_WKThumbnailView alloc] initWithFrame:NSMakeRect(0, 0, 100, 100) fromWKWebView:webView.get()]);
+
+    auto observer = adoptNS([[SnapshotSizeObserver alloc] init]);
+
+    [thumbnailView addObserver:observer.get() forKeyPath:@"snapshotSize" options:NSKeyValueObservingOptionNew context:snapshotSizeChangeKVOContext];
+
+    RetainPtr<NSWindow> otherWindow = adoptNS([[OffscreenWindow alloc] initWithSize:CGSizeMake(100, 100) isKeyWindow:NO]);
+    [[otherWindow contentView] addSubview:thumbnailView.get()];
+    [otherWindow orderFront:nil];
+    [[webView hostWindow] makeKeyAndOrderFront:nil];
+    Util::run(&didTakeSnapshot);
+    didTakeSnapshot = false;
+    EXPECT_FALSE([thumbnailView layer].contents == nil);
+    [webView waitForNextPresentationUpdate];
+    EXPECT_FALSE([[webView objectByEvaluatingJavaScript:@"window.internals.isPageActive()"] boolValue]);
+
+    [webView addToTestWindow];
+    [webView waitForNextPresentationUpdate];
+    EXPECT_FALSE([[webView objectByEvaluatingJavaScript:@"window.internals.isPageActive()"] boolValue]);
+
+    [thumbnailView removeFromSuperview];
+    [webView waitForNextPresentationUpdate];
+    EXPECT_TRUE([[webView objectByEvaluatingJavaScript:@"window.internals.isPageActive()"] boolValue]);
+
+    [thumbnailView removeObserver:observer.get() forKeyPath:@"snapshotSize" context:snapshotSizeChangeKVOContext];
+}
+
+
 } // namespace TestWebKitAPI
 
-#endif
+#endif // PLATFORM(MAC)

trunk/Tools/TestWebKitAPI/mac/OffscreenWindow.h

@@ -35 +35 @@
 
 - (instancetype)initWithSize:(CGSize)size;
+- (instancetype)initWithSize:(CGSize)size isKeyWindow:(BOOL)isKeyWindow;
 
 @end

trunk/Tools/TestWebKitAPI/mac/OffscreenWindow.mm

@@ -27 +27 @@
 #import "OffscreenWindow.h"
 
-@implementation OffscreenWindow
+@implementation OffscreenWindow {
+    BOOL _isKeyWindow;
+}
 
 - (instancetype)initWithSize:(CGSize)size
 {
+    return [self initWithSize:size isKeyWindow:YES];
+}
+
+- (instancetype)initWithSize:(CGSize)size isKeyWindow:(BOOL)isKeyWindow
+{
+    _isKeyWindow = isKeyWindow;
+
     NSRect rect = NSMakeRect(0, 0, size.width, size.height);
     NSRect windowRect = NSOffsetRect(rect, -10000, [(NSScreen *)[[NSScreen screens] objectAtIndex:0] frame].size.height - rect.size.height + 10000);
@@ -47 +56 @@
 - (BOOL)isKeyWindow
 {
-    return YES;
+    return _isKeyWindow;
 }