Bug 63451 – [GTK] Embedded GtkWidgets are not drawn

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Dan Vrátil]

Created attachment 98716 [details]
A simple test case

Embedded widgets that are created in 'create-plugin-widget' signal handler are not drawn in WebKitGTK+. In the webview there is an empty space allocated with proper dimensions, but the widget is not displayed and it does not respond to any events (clicking, moving mouse over it...).

When you connect to widget's 'notify' signal, the widget reports it's being correctly resized and 'visibile' is set to TRUE, so WebKit does know about it, it just has some problems displaying/drawing it.

[Peter Hatina]

The issue seems to be caused by not placing embedded widget into any container. If the test case gets updated in this way, everything works - instead of returning only a simple label from the "create_plugin_widget", return a top-level window containing the widget and this window will be shown.

[Danilo de Paula]

(In reply to comment #1)
> The issue seems to be caused by not placing embedded widget into any container. If the test case gets updated in this way, everything works - instead of returning only a simple label from the "create_plugin_widget", return a top-level window containing the widget and this window will be shown.

I tried that approach, however I've  only got a new top-level window, and not a embedded widget as expected.

[Danilo de Paula]

Source/WebCore/platform/gtk/GtkPluginWidget.cpp::void GtkPluginWidget::paint(GraphicsContext* context, const IntRect& rect)

At least in my tests, if (!context->gdkExposeEvent()) is executed, and returning, to the widget will never get painted... I'm investigating, but if you guys find any other information, that would be cool.

[Gustavo Noronha (kov)]

Created attachment 108488 [details]
WIP patch

This is a simple patch I started writing to make the code gtk3-compliant (i.e. not use gdkExposeEvent) and test this issue. Haven't gotten the widget to show up yet, though.

[Dan Vrátil]

Created attachment 125816 [details]
Fix

Proposed patch that fixes this issue by adding the plugin widget to 
WebView container and forcing these widgets to be drawn as the very last.

Additionally, when GtkPluginWidget is destroyed it also removes the child
widget from the WebView container. Otherwise the widgets remain displayed
even when a new page is loaded in the webview.

This functionality is quite important for the Evolution WebKit port. Is it
possible to estimate when this could get to a stable release?

[WebKit Review Bot]

Attachment 125816 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/platform/gtk/GtkPluginWidge..." exit_code: 1

Source/WebCore/platform/gtk/GtkPluginWidget.cpp:46:  Declaration has space between type name and * in GtkWidget *parent  [whitespace/declaration] [3]
Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:486:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 125816 [details]
Fix

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

Great work! If you attach a proper ChangeLog to this patch and fix the nits below, we can commit it directly from the bug. See http://www.webkit.org/coding/contributing.html. I think you can skip writing tests for this change...just delete that line from the ChangeLog.

Do you mind testing what happens when you remove the contents of the GtkPluginWidget::paint method entirely? If the real expose event is drawing the child widgets, perhaps we don't have to make fake expose events at all.

> Source/WebCore/platform/gtk/GtkPluginWidget.cpp:49
> +    GtkWidget *parent;
> +
> +    parent = gtk_widget_get_parent(platformWidget());
> +    gtk_container_remove(GTK_CONTAINER(parent), platformWidget());

This should just be one line:

gtk_container_remove(GTK_CONTAINER(gtk_widget_get_praent(platformWidget()), platformWidget());

>> Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:486
>> +        gtk_container_add (GTK_CONTAINER (getViewFromFrame(m_frame)), gtkWidget);
> 
> Extra space before ( in function call  [whitespace/parens] [4]

Please fix the style issue here.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:664
> +    /* Force child widgets to be drawn as last */

Might want to expand this comment a bit:

// Chaining up to the parent forces child widgets to be drawn.

[Martin Robinson]

Oh, by the way, if we commit this patch I'll merge into the stable branch for the next stable release.

[Dan Vrátil]

Created attachment 126030 [details]
Patch

[WebKit Review Bot]

Attachment 126030 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/gtk/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/gtk/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dan Vrátil]

Hi,

indeed the paint() method in GtkPluginWidget is not needed anymore.

Hopefully it's all OK now.

[Dan Vrátil]

Created attachment 126032 [details]
Fixed patch

Sorry, another try.

[Philippe Normand]

Comment on attachment 126032 [details]
Fixed patch

Attachment 126032 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11449322

[Dan Vrátil]

Created attachment 126053 [details]
Fixed patch

[Martin Robinson]

Comment on attachment 126053 [details]
Fixed patch

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

> Source/WebKit/gtk/webkit/webkitwebview.cpp:672
> +    /* Chaining up to the parent forces child widgets to be drawn. */
> +    GTK_WIDGET_CLASS(webkit_web_view_parent_class)->draw(widget, cr);
> +

You removed the ::paint method for GTK+ 2.x, but you did not chain up in webkit_web_view_expose. We use C++ style comments usually. :) No worries though. I will fix those two minor issues and land this patch. Thank you for taking the time to investigate this issue!

[Martin Robinson]

Committed r107249: <http://trac.webkit.org/changeset/107249>