Bug 17154 – [GTK] Widget size negotiation

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

[Alp Toker]

When not packed into a GtkScrolledWindow, WebKitWebView should request the full size of its content and expand/shrink as needed. This is useful for integrating bits of Web content into traditional GTK+ applications.

So, in this case, it should be made to behave like GtkTextView:

int
main (int argc, char *argv[])
{
    gtk_init (&argc, &argv);

    GtkWidget *web_view = WEBKIT_WEB_VIEW (webkit_web_view_new ());
    webkit_web_view_set_editable (web_view, TRUE);

    GtkWidget *window = gtk_window_new (GTK_WINDOW_TOPLEVEL);
    gtk_window_set_title (window, "WebView");
    gtk_container_add (GTK_CONTAINER (window), GTK_WIDGET (web_view));

    gchar *uri = (gchar *) (argc > 1 ? argv[1] : "http://www.google.com/");
    webkit_web_view_open (web_view, uri);

    gtk_widget_show_all (window);

    gtk_main ();
}

[Alp Toker]

Created attachment 18870 [details]
Proof of concept

This patch adds support for basic size requests based on the HTML content. It only grows and never shrinks unfortunately, and the patch is full of debugging comments but you get the idea.

[Mark Rowe (bdash)]

What would the behaviour be if a new URL were loaded, or the content size changed (via JavaScript, for example).

[Alp Toker]

(In reply to comment #2)
> What would the behaviour be if a new URL were loaded, or the content size
> changed (via JavaScript, for example).
>

The widget requests the size in this case and it's usually allocated. You can see this if you navigate to google.com using the demo code in the bug report, then click on google maps. The whole GTK+ window will expand to accommodate Google maps.

The main use case for this is trusted content that needs to be packed in the UI directly, and perhaps things similar to dashboard widgets.

I couldn't get it working well enough to justify getting this in just yet. Any thoughts from GTK+ packing experts or WebCore hackers welcome. Does Mac support anything like this?

[Alp Toker]

Improved comparative test-case:

/* gcc -Wall `pkg-config --cflags --libs webkit-1.0` size-test.c */

/* This manual test constructs two top level windows for size request and
 * look-and-feel comparisons between the native GTK+ TextView and WebKit's
 * WebView.
 */

#include <gtk/gtk.h>
#include <webkit/webkit.h>

int
main (int argc, char *argv[])
{
    gtk_init (&argc, &argv);

    GtkWidget *view;
    GtkWidget *window;

    // Construct a Window with a WebView
    view = webkit_web_view_new ();
    webkit_web_view_set_editable (WEBKIT_WEB_VIEW (view), TRUE);
    window = gtk_window_new (GTK_WINDOW_TOPLEVEL);
    gtk_window_set_title (GTK_WINDOW (window), "WebView");
    gtk_container_add (GTK_CONTAINER (window), GTK_WIDGET (view));
    gchar *uri = (gchar *) (argc > 1 ? argv[1] : "about:blank");
    webkit_web_view_open (WEBKIT_WEB_VIEW (view), uri);
    gtk_widget_show_all (window);

    // Construct a Window with a TextView
    view = gtk_text_view_new ();
    window = gtk_window_new (GTK_WINDOW_TOPLEVEL);
    gtk_window_set_title (GTK_WINDOW (window), "TextView");
    gtk_container_add (GTK_CONTAINER (window), GTK_WIDGET (view));
    gtk_widget_show_all (window);

    gtk_main ();
    return 0;
}

[Gustavo Noronha (kov)]

Created attachment 33044 [details]
Implement proper size request for the widget

 WebKit/gtk/ChangeLog                          |   15 +++++++++++++++
 WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp |   12 +++++++++---
 WebKit/gtk/webkit/webkitwebview.cpp           |   13 +++++++++++++
 3 files changed, 37 insertions(+), 3 deletions(-)

[Gustavo Noronha (kov)]

Created attachment 33045 [details]
Implement proper size request for the widget

 WebKit/gtk/ChangeLog                          |   15 +++++++++++++++
 WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp |   12 +++++++++---
 WebKit/gtk/webkit/webkitwebview.cpp           |   13 +++++++++++++
 3 files changed, 37 insertions(+), 3 deletions(-)

[Gustavo Noronha (kov)]

Comment on attachment 33045 [details]
Implement proper size request for the widget

The Anjal project needs this and is currently solving the problem with a somewhat hackish API addition. I think we should provide a real solution. This works well on my tests.

[Holger Freyther]

Comment on attachment 33045 [details]
Implement proper size request for the widget


> +    // We need to queue a resize request only if the size changed,
> +    // otherwise we get into an infinite loop!
> +    GtkWidget* widget = GTK_WIDGET(m_webView);
> +    if (GTK_WIDGET_REALIZED(widget) &&
> +        (widget->requisition.height != size.height()) &&
> +        (widget->requisition.width != size.width()))
> +        gtk_widget_queue_resize(widget);

cool, do we need to force a redraw here too? otherwise you could add no_redraw to it.


> +static void webkit_web_view_size_request(GtkWidget* widget, GtkRequisition* requisition)
> +{
> +    WebKitWebView* web_view = WEBKIT_WEB_VIEW(widget);
> +    Frame* coreFrame = core(webkit_web_view_get_main_frame(web_view));
> +    if (!coreFrame)
> +        return;
> +
> +    FrameView* view = coreFrame->view();
> +    requisition->width = view->contentsWidth();
> +    requisition->height = view->contentsHeight();

I think you should check for the "view" as well.

[Gustavo Noronha (kov)]

Created attachment 33074 [details]
Implement proper size request for the widget

 WebKit/gtk/ChangeLog                          |   15 +++++++++++++++
 WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp |   12 +++++++++---
 WebKit/gtk/webkit/webkitwebview.cpp           |   16 ++++++++++++++++
 3 files changed, 40 insertions(+), 3 deletions(-)

[Gustavo Noronha (kov)]

Comment on attachment 33074 [details]
Implement proper size request for the widget

Here we go.

[Gustavo Noronha (kov)]

Landed as r46123.

[Martin Robinson]

(In reply to comment #11)
> Landed as r46123.

Sadly, I think this may be a misfeature. I appreciate the use-case, but perhaps there is another way to crack the egg. Here are a few of the issues this has caused:

1. When you pack the widget into a container, like an HBox of VBox it's impossible to ever make the view smaller. You'll be able to resize it to be larger, but never smaller, since the widget will never decrease its requisition size.

2. If you simply comment out the code in ChromeClient.cpp, you can now resize the window freely, but resize performance is abysmal.

Proposal: Revert this patch. Gustavo, can you confirm that either packing the WebKitWebView into a container or overriding the "size-request" signal fulfills the Anjal usecase. It should now be possible to query document properties via the GDom API. As it stands this patch breaks packing the widget into containers other than GtkScrolledWindow completely.

[Gustavo Noronha (kov)]

(In reply to comment #12)

> Proposal: Revert this patch. Gustavo, can you confirm that either packing the WebKitWebView into a container or overriding the "size-request" signal fulfills the Anjal usecase. It should now be possible to query document properties via the GDom API. As it stands this patch breaks packing the widget into containers other than GtkScrolledWindow completely.

I am against because I believe the way it's now is the way it's supposed to work in GTK+. I believe if we need to enforce something else for a different use case, then that use case is the one that should override the signal. Can you perhaps elaborate on why we would pack the widget on a box instead of on a ScrollView? Perhaps looking at the use case can help us figure out solutions.  In other news, with GTK+ 3 the size negoatiation story changed to be more like clutter's, so I guess there's some revision we need to do anyway?

[Martin Robinson]

(In reply to comment #13)
> (In reply to comment #12)

> I am against because I believe the way it's now is the way it's supposed to work in GTK+. I believe if we need to enforce something else for a different use case, then that use case is the one that should override the signal. Can you perhaps elaborate on why we would pack the widget on a box instead of on a ScrollView? Perhaps looking at the use case can help us figure out solutions.  In other news, with GTK+ 3 the size negoatiation story changed to be more like clutter's, so I guess there's some revision we need to do anyway?

My objection is that a user should be able to pack a widget in any of the containers that GTK+ provides and have it play nice. Right now a WebView widget can only grow in anything other than a GtkScrolledWindow. That seems wrong, but I may just misundertand GTK+ here. I also think requesting thousands of pixels during size-request might be causing some performance issues in certain containers. Other ports do not try to grow their widget as large the document content, instead they let the widget itself determine the size of the viewport. Perhaps I'm wrong about the entire thing though.

For GTK+ 3 we are setting both the minimum and natural sizes to the size of the DOM document.

Usecase: Imagine wanting to pack a WebView (sans GtkScrolledWindow) with a fixed size into some container along with some toolbars. Seems like a pretty reasonable usecase. Right now you can't do it without manually overriding the size-request signal.

Honestly though, my real usecase is allowing WebKit to draw and control the main frame scrollbars itself. I'm was very close to having this when I bumped into this bug. I would like to support having main frame scrollbars with and without the GtkScrolledWindow wrapper. This is the only way we will remove the scrollbar race conditions, which plague many of our tests and turn our bots red.

[Gustavo Noronha (kov)]

(In reply to comment #14)
> My objection is that a user should be able to pack a widget in any of the containers that GTK+ provides and have it play nice. Right now a WebView widget can only grow in anything other than a GtkScrolledWindow. That seems wrong, but I may just misundertand GTK+ here. I also think requesting thousands of pixels during size-request might be causing some performance issues in certain containers. Other ports do not try to grow their widget as large the document content, instead they let the widget itself determine the size of the viewport. Perhaps I'm wrong about the entire thing though.
> 

I think the other ports do it like that because they pretty much rely on WebCore painting the scrollbars. The idea of having WebKitWebView be a similar to GtkTextView from the beginning, and act as a regular GTK+ widget as much as possible is what led to this decision.

> For GTK+ 3 we are setting both the minimum and natural sizes to the size of the DOM document.

Makes sense to me to have minimum be 1x1 on the GTK+3 case, fwiw. GTK+3 is much more amenable to the requirements of a widget like ours =)

> Usecase: Imagine wanting to pack a WebView (sans GtkScrolledWindow) with a fixed size into some container along with some toolbars. Seems like a pretty reasonable usecase. Right now you can't do it without manually overriding the size-request signal.
> 

Makes sense. I'd argue though that the main use case has been packing it inside a ScrolledWindow, so that should be the default one to be supported.

> Honestly though, my real usecase is allowing WebKit to draw and control the main frame scrollbars itself. I'm was very close to having this when I bumped into this bug. I would like to support having main frame scrollbars with and without the GtkScrolledWindow wrapper. This is the only way we will remove the scrollbar race conditions, which plague many of our tests and turn our bots red.

YES! I think it should be the default mode for Web browsers, like I already said, letting WebCore draw the scrollbars would make web compatibility much better. Just to document what we discussed on IRC: if we can make it a second widget it'd be perfect in my book. It should be simple given we can make it a subclass of WebView, so there would not be a need to change too much, but I could also agree to making it a setting.