Bug 183143 - [GTK] testRunner.setWindowIsKey() has no effect on the web process side in WebKit2
Summary: [GTK] testRunner.setWindowIsKey() has no effect on the web process side in We...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Claudio Saavedra
URL:
Keywords: InRadar, LayoutTestFailure, PlatformOnly
Depends on:
Blocks:
 
Reported: 2018-02-26 13:11 PST by Daniel Bates
Modified: 2018-03-05 07:05 PST (History)
7 users (show)

See Also:


Attachments
Patch (5.27 KB, patch)
2018-03-02 04:45 PST, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch (5.39 KB, patch)
2018-03-02 05:22 PST, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch for landing (5.12 KB, patch)
2018-03-02 05:31 PST, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch for landing (5.19 KB, patch)
2018-03-05 06:08 PST, Claudio Saavedra
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2018-02-26 13:11:51 PST
The GTK implementation of testRunner.setWindowIsKey() only updates a boolean flag and does not change the activation state of the window. We need to implement so as to be able to test page activation issues Once we implement such support we may be able to unskip some or all of the following tests:

fast/dom/Window/window-focus-self.html
fast/events/blur-focus-window-should-blur-focus-element.html
fast/selectors/querySelector-window-inactive.html
fast/selectors/selection-window-inactive.html
scrollbars/corner-resizer-window-inactive.html
Comment 1 Michael Catanzaro 2018-02-27 15:43:37 PST
Thanks for reporting a bug so that we know about the problem.

Implementing setWindowIsKey to focus the window should be easy; we just need to call gtk_window_present(). *This function is broken on Wayland* so developers and test expectations will need to take this into account.
 
Claudio pointed out one problem: there is no inverse function. To un-focus the window, we'll need to create a second window and give that window focus. We might have to keep it around forever, too, to prevent the original window from regaining focus. That's quite non-ideal, but I don't see a better way.
Comment 2 Michael Catanzaro 2018-02-27 15:44:46 PST
(In reply to Michael Catanzaro from comment #1)
> Thanks for reporting a bug so that we know about the problem.
> 
> Implementing setWindowIsKey to focus the window should be easy; we just need
> to call gtk_window_present(). *This function is broken on Wayland* so
> developers and test expectations will need to take this into account.

To be clear, the failure expectations should move from platform/gtk/TestExpectations to platform/gtk-wayland/TestExpectations.
Comment 3 Claudio Saavedra 2018-03-02 02:03:44 PST
I've been toying with having a secondary window for this purpose and at least 4 of those tests are fixed that way. The one that is not fixed seems to be related to some selector getting the focus instead of the window, so I still need to investigate that a bit further.
Comment 4 Claudio Saavedra 2018-03-02 04:04:44 PST
These two tests, now broken, would also pass.

fast/selectors/text-field-selection-text-shadow.html
fast/selectors/text-field-selection-stroke-color.html
Comment 5 Claudio Saavedra 2018-03-02 04:45:54 PST
Created attachment 334891 [details]
Patch
Comment 6 Ms2ger (he/him; ⌚ UTC+1/+2) 2018-03-02 05:03:44 PST
Comment on attachment 334891 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=334891&action=review

> LayoutTests/platform/gtk/TestExpectations:2386
>  webkit.org/b/183143 fast/events/blur-focus-window-should-blur-focus-element.html

Please file a new bug for this one?
Comment 7 Carlos Garcia Campos 2018-03-02 05:03:52 PST
Comment on attachment 334891 [details]
Patch

Could we create the second window on demand?
Comment 8 Claudio Saavedra 2018-03-02 05:22:41 PST
Created attachment 334894 [details]
Patch
Comment 9 Carlos Garcia Campos 2018-03-02 05:26:50 PST
Comment on attachment 334894 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=334894&action=review

> Tools/WebKitTestRunner/PlatformWebView.h:118
> +    GtkWidget *m_otherWindow;

{ nullptr };

> Tools/WebKitTestRunner/gtk/PlatformWebViewGtk.cpp:45
> +    , m_otherWindow(nullptr)

Adn we don't need this here.
Comment 10 Claudio Saavedra 2018-03-02 05:31:26 PST
Created attachment 334896 [details]
Patch for landing
Comment 11 Claudio Saavedra 2018-03-02 05:32:23 PST
(In reply to Ms2ger from comment #6)
> Comment on attachment 334891 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334891&action=review
> 
> > LayoutTests/platform/gtk/TestExpectations:2386
> >  webkit.org/b/183143 fast/events/blur-focus-window-should-blur-focus-element.html
> 
> Please file a new bug for this one?

I was thinking to keep this one open since I don't understand yet well what is going on in that test that is causing it to fail.
Comment 12 Michael Catanzaro 2018-03-02 08:13:58 PST
Comment on attachment 334896 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=334896&action=review

> Tools/WebKitTestRunner/PlatformWebView.h:118
> +    GtkWidget *m_otherWindow { nullptr };

Nit: GtkWidget* m_otherWindow

> LayoutTests/platform/gtk/TestExpectations:2386
>  webkit.org/b/183143 fast/events/blur-focus-window-should-blur-focus-element.html

Please report a new bug for this, and move it to the normal failures section of the expectations.

Also: I'm pretty sure all these tests will still be broken in Wayland, as mentioned above, so the expectations should probably be moved to the Wayland expectations?
Comment 13 Michael Catanzaro 2018-03-02 08:24:31 PST
Good job btw: I was too scared to try using a spare GtkWindow, but seems it worked out fine.
Comment 14 Claudio Saavedra 2018-03-05 06:08:35 PST
Created attachment 334999 [details]
Patch for landing
Comment 15 WebKit Commit Bot 2018-03-05 06:44:25 PST
Comment on attachment 334999 [details]
Patch for landing

Clearing flags on attachment: 334999

Committed r229283: <https://trac.webkit.org/changeset/229283>
Comment 16 WebKit Commit Bot 2018-03-05 06:44:27 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Radar WebKit Bug Importer 2018-03-05 06:45:23 PST
<rdar://problem/38138918>
Comment 18 Claudio Saavedra 2018-03-05 07:05:19 PST
(In reply to Michael Catanzaro from comment #12)
> 
> Please report a new bug for this, and move it to the normal failures section
> of the expectations.

https://bugs.webkit.org/show_bug.cgi?id=183328