Bug 210206 – [GTK] MiniBrowser opens new windows too small causing failures on some WPT tests

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Alberto Lopez Perez]

Some WPT tests (when executed with the WPT runner via WebDriver) open new browser windows via JavaScript invoking window.open(...) and then run the test on this new window.
And there are issues when the test itself calls document.elementFromPoint() on some coordinates that are ourside of the window size.

MiniBrowser its opening new windows by default with a size of 100x100, which its too small and causes failures
For example on the WPT test: css/css-flexbox/hittest-overlapping-relative.html

[Carlos Alberto Lopez Perez]

Created attachment 395850 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Michael Catanzaro]

Comment on attachment 395850 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitWindowProperties.cpp:47
> + * sizes bigger than 100x100. And if no size its specified then this value

If no size is specified, then

[Carlos Garcia Campos]

Comment on attachment 395850 [details]
Patch

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

> Tools/MiniBrowser/gtk/BrowserWindow.c:323
> +     * The issue is that 100x100 its too small and cause failures on some

is too small

> Tools/MiniBrowser/gtk/BrowserWindow.c:328
> +    if (geometry.width > 100 && geometry.height > 100)

Maybe we could do this only when web view is controlled by automation?

[Carlos Alberto Lopez Perez]

(In reply to Carlos Garcia Campos from comment #4)
> Comment on attachment 395850 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395850&action=review
> 
> > Tools/MiniBrowser/gtk/BrowserWindow.c:323
> > +     * The issue is that 100x100 its too small and cause failures on some
> 
> is too small
> 
> > Tools/MiniBrowser/gtk/BrowserWindow.c:328
> > +    if (geometry.width > 100 && geometry.height > 100)
> 
> Maybe we could do this only when web view is controlled by automation?

I think it makes sense to do it in any case.

Just try a basic example of window.open with the minibrowser and check how bad it looks the new window it opens: (you almost can't see it). 

Ex: https://www.w3schools.com/jsref/tryit.asp?filename=tryjsref_win_open

[Carlos Garcia Campos]

(In reply to Carlos Alberto Lopez Perez from comment #5)
> (In reply to Carlos Garcia Campos from comment #4)
> > Comment on attachment 395850 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=395850&action=review
> > 
> > > Tools/MiniBrowser/gtk/BrowserWindow.c:323
> > > +     * The issue is that 100x100 its too small and cause failures on some
> > 
> > is too small
> > 
> > > Tools/MiniBrowser/gtk/BrowserWindow.c:328
> > > +    if (geometry.width > 100 && geometry.height > 100)
> > 
> > Maybe we could do this only when web view is controlled by automation?
> 
> I think it makes sense to do it in any case.
> 
> Just try a basic example of window.open with the minibrowser and check how
> bad it looks the new window it opens: (you almost can't see it). 
> 
> Ex: https://www.w3schools.com/jsref/tryit.asp?filename=tryjsref_win_open

Then I would not change this in MiniBrowser. The spec says 100 is the minimum but not the default, so we can change that behavior and use a different default size, for example parent window size.

[Carlos Alberto Lopez Perez]

(In reply to Carlos Garcia Campos from comment #6)
> (In reply to Carlos Alberto Lopez Perez from comment #5)
> > (In reply to Carlos Garcia Campos from comment #4)
> > > Comment on attachment 395850 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=395850&action=review
> > > 
> > > > Tools/MiniBrowser/gtk/BrowserWindow.c:323
> > > > +     * The issue is that 100x100 its too small and cause failures on some
> > > 
> > > is too small
> > > 
> > > > Tools/MiniBrowser/gtk/BrowserWindow.c:328
> > > > +    if (geometry.width > 100 && geometry.height > 100)
> > > 
> > > Maybe we could do this only when web view is controlled by automation?
> > 
> > I think it makes sense to do it in any case.
> > 
> > Just try a basic example of window.open with the minibrowser and check how
> > bad it looks the new window it opens: (you almost can't see it). 
> > 
> > Ex: https://www.w3schools.com/jsref/tryit.asp?filename=tryjsref_win_open
> 
> Then I would not change this in MiniBrowser. The spec says 100 is the
> minimum but not the default, so we can change that behavior and use a
> different default size, for example parent window size.


Its there a spec for this minimum/default size? I can't find any reference at https://html.spec.whatwg.org/multipage/window-object.html#dom-open

What Firefox and Chrome seem to do its to open new windows without a specific size on new tabs.

[Carlos Garcia Campos]

I saw it in https://www.w3schools.com/jsref/met_win_open.asp also included in https://developer.mozilla.org/en-US/docs/Web/API/Window/open#Window_features

[Carlos Alberto Lopez Perez]

Created attachment 396003 [details]
Patch

v2: set the default requested window size to the one of the old window

[Carlos Garcia Campos]

Comment on attachment 396003 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        The size of the new window its not specified, and we were defaulting

is not specified

> Source/WebCore/ChangeLog:12
> +        to the minimum of 100x100 which its just too small for some tests

which is

> Source/WebCore/loader/FrameLoader.cpp:-4133
> -    // Zero width and height mean using default size, not minumum one.

So, WebCore is indeed doing the right thing here, if not size is specified the default one is used. We are failing to provide a default size when windowRect is called before the window is shown. We can fix it in GLib UI client doing something like this in windowFrame():

        GdkRectangle geometry = { 0, 0, 0, 0 };
        GtkWidget* window = gtk_widget_get_toplevel(GTK_WIDGET(m_webView));
        if (WebCore::widgetIsOnscreenToplevelWindow(window) && gtk_widget_get_visible(window)) {
            gtk_window_get_position(GTK_WINDOW(window), &geometry.x, &geometry.y);
            gtk_window_get_size(GTK_WINDOW(window), &geometry.width, &geometry.height);
        } else {
            GdkRectangle defaultGeometry;
            webkit_window_properties_get_geometry(webkit_web_view_get_window_properties(m_webView), &defaultGeometry);
            if ((!defaultGeometry.width || !defaultGeometry.height) && WebCore::widgetIsOnscreenToplevelWindow(window)) {
                int defaultWidth, defaultHeight;
                gtk_window_get_default_size(GTK_WINDOW(window), &defaultWidth, &defaultHeight);
                if (!defaultGeometry.width && defaultWidth != -1)
                    geometry.width = defaultWidth;
                if (!defaultGeometry.height && defaultHeight != -1)
                    geometry.height = defaultHeight;
            }
        }
        completionHandler(WebCore::FloatRect(geometry));

Note that we will still get 100x100 when the window doesn't have a default size set (it's not the case of MiniBrowser), but we don't have a way to get the previous window here, so I think we can just assume that apps should handle that case by setting their own minimum size if 100x100 is too small.

> Source/WebCore/loader/FrameLoader.cpp:4141
> +    else
> +        windowRect.setWidth(oldWindowRect.width());

This is changing the behavior for other ports, I think we should handle this in WebKit layer, but if we end up doing it here, it should be under plartform ifdefs, or ar least only done when windowRect.width() == 0.

> Source/WebCore/loader/FrameLoader.cpp:4145
> +    else
> +        windowRect.setHeight(oldWindowRect.height());

Ditto.

[Carlos Alberto Lopez Perez]

(In reply to Carlos Garcia Campos from comment #10)
> Comment on attachment 396003 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396003&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        The size of the new window its not specified, and we were defaulting
> 
> is not specified
> 
> > Source/WebCore/ChangeLog:12
> > +        to the minimum of 100x100 which its just too small for some tests
> 
> which is
> 
> > Source/WebCore/loader/FrameLoader.cpp:-4133
> > -    // Zero width and height mean using default size, not minumum one.
> 
> So, WebCore is indeed doing the right thing here, if not size is specified
> the default one is used. We are failing to provide a default size when
> windowRect is called before the window is shown. We can fix it in GLib UI
> client doing something like this in windowFrame():
> 
>         GdkRectangle geometry = { 0, 0, 0, 0 };
>         GtkWidget* window = gtk_widget_get_toplevel(GTK_WIDGET(m_webView));
>         if (WebCore::widgetIsOnscreenToplevelWindow(window) &&
> gtk_widget_get_visible(window)) {
>             gtk_window_get_position(GTK_WINDOW(window), &geometry.x,
> &geometry.y);
>             gtk_window_get_size(GTK_WINDOW(window), &geometry.width,
> &geometry.height);
>         } else {
>             GdkRectangle defaultGeometry;
>            
> webkit_window_properties_get_geometry(webkit_web_view_get_window_properties(m
> _webView), &defaultGeometry);
>             if ((!defaultGeometry.width || !defaultGeometry.height) &&
> WebCore::widgetIsOnscreenToplevelWindow(window)) {
>                 int defaultWidth, defaultHeight;
>                 gtk_window_get_default_size(GTK_WINDOW(window),
> &defaultWidth, &defaultHeight);
>                 if (!defaultGeometry.width && defaultWidth != -1)
>                     geometry.width = defaultWidth;
>                 if (!defaultGeometry.height && defaultHeight != -1)
>                     geometry.height = defaultHeight;
>             }
>         }
>         completionHandler(WebCore::FloatRect(geometry));
> 

Good point and thanks for the hints! :)

> Note that we will still get 100x100 when the window doesn't have a default
> size set (it's not the case of MiniBrowser), but we don't have a way to get
> the previous window here, so I think we can just assume that apps should
> handle that case by setting their own minimum size if 100x100 is too small.
> 

I see.
Maybe we can try to do both things? use the default window size if set, and if there is no default window size, then use the size of the previous window.

> > Source/WebCore/loader/FrameLoader.cpp:4141
> > +    else
> > +        windowRect.setWidth(oldWindowRect.width());
> 
> This is changing the behavior for other ports, I think we should handle this
> in WebKit layer, but if we end up doing it here, it should be under
> plartform ifdefs, or ar least only done when windowRect.width() == 0.
> 
> > Source/WebCore/loader/FrameLoader.cpp:4145
> > +    else
> > +        windowRect.setHeight(oldWindowRect.height());
> 
> Ditto.

Ok.

[Carlos Alberto Lopez Perez]

Created attachment 396452 [details]
Patch

[Carlos Alberto Lopez Perez]

Created attachment 396454 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 396454 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        The size of the new window is not specified, and we were failimg

failimg -> failing

> Source/WebCore/loader/FrameLoader.cpp:4129
> +#if PLATFORM(GTK) || PLATFORM(WPE)

I'm not sure this works in WPE. It's unfortunate we have to do this here, but we don't have access to the previous window from the API layer.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestUIClient.cpp:370
> +#if PLATFORM(GTK)
> +        if (m_shouldCreateWebViewsInNewWindowsAutomatically) {
> +            g_assert_null(m_parentWindow);
> +            m_parentWindow = gtk_window_new(GTK_WINDOW_TOPLEVEL);
> +            gtk_window_set_default_size(GTK_WINDOW(m_parentWindow), m_DefaultGeometryNewWindows.width, m_DefaultGeometryNewWindows.height);
> +            gtk_container_add(GTK_CONTAINER(m_parentWindow), GTK_WIDGET(newWebView));
> +            gtk_widget_show(GTK_WIDGET(newWebView));
> +            gtk_widget_show(m_parentWindow);
> +        }
> +#endif

Maybe we can just change showInWindowAndWaitUntilMapped() to set the default size, I don't think that will affect other tests. Ok, I see now that we need it for this particular test. Then I would add a parameter ShouldSetDefaultSize or something similar.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestUIClient.cpp:705
> +    // if no size specified for window.open(), then new windows open with the default window size.

If

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestUIClient.cpp:715
> +    // if no size specified for window.open(), and new windows are not set to a specific default size with gtk_window_set_default_size()

If

[Carlos Alberto Lopez Perez]

(In reply to Carlos Garcia Campos from comment #14)
> Comment on attachment 396454 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396454&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        The size of the new window is not specified, and we were failimg
> 
> failimg -> failing
> 
> > Source/WebCore/loader/FrameLoader.cpp:4129
> > +#if PLATFORM(GTK) || PLATFORM(WPE)
> 
> I'm not sure this works in WPE. It's unfortunate we have to do this here,
> but we don't have access to the previous window from the API layer.
> 

I tested with some printfs and it seems WPE always reports the size of the view backend as the default window size. I'm not 100% sure if you can have a view backend without a specified size, but I suspect its not possible. So I guess this its not needed for WPE.

> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestUIClient.cpp:370
> > +#if PLATFORM(GTK)
> > +        if (m_shouldCreateWebViewsInNewWindowsAutomatically) {
> > +            g_assert_null(m_parentWindow);
> > +            m_parentWindow = gtk_window_new(GTK_WINDOW_TOPLEVEL);
> > +            gtk_window_set_default_size(GTK_WINDOW(m_parentWindow), m_DefaultGeometryNewWindows.width, m_DefaultGeometryNewWindows.height);
> > +            gtk_container_add(GTK_CONTAINER(m_parentWindow), GTK_WIDGET(newWebView));
> > +            gtk_widget_show(GTK_WIDGET(newWebView));
> > +            gtk_widget_show(m_parentWindow);
> > +        }
> > +#endif
> 
> Maybe we can just change showInWindowAndWaitUntilMapped() to set the default
> size, I don't think that will affect other tests. Ok, I see now that we need
> it for this particular test. Then I would add a parameter
> ShouldSetDefaultSize or something similar.
> 

I tried but I couldn't manage to set a default size from the function showInWindowAndWaitUntilMapped() that webkit_window_properties_get_geometry() would report.

As far as I can see the main issue with showInWindowAndWaitUntilMapped() its that its not called on the "create" signal when the webview its created, but its called manually by the test after that.
So it works when you care about setting a size for the current test window but it doesn't work for setting an automatic/default size for a new window that opens.

For this test to work I need to set the default size of the window when the new webview its created in the signal handler for the "create" signal, so when the signal "ready-to-show" triggers the window already has a default size that webkit_window_properties_get_geometry() can query.

[Carlos Alberto Lopez Perez]

Committed r260176: <https://trac.webkit.org/changeset/260176>