Bug 24358 – [GTK] Scrollbars not clipped correctly

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

[Xan Lopez]

The expose handler in WebKitWebView has two branches. One coalesces all the damage regions in one big area and sends that to frame()->view()->paint(), and other sends each region individually. In both cases the clip area is passed as an IntRect in the second parameter of paint(), but there's a problem: our Scrollbar ::paint method ignores that and always uses the bounding box of all the regions in the Expose event as the clip rectangle. This works ok when we also merge the regions ourselves, but does not work when we send each one separatedly: in that case we'll use as clip rectangle a much bigger area than what we actually have to paint, and will risk painting the scrollbars outside of the visible area.

The simple fix is to change the Widget::paint method to use the clip rectangle parameter always. Unfortunately this introduces another problem: the floatingdiv.html manual test stops working, the scrollbar there is invisible again. I must say that I'm not 100% sure why, but my theory is that since we are now using the correct clip rectangle, we only send an expose event with the minimum area, instead of a much larger one, and thus we uncover a new bug: we are not applying the needed coordinate translation for our Scrollbar widgets in the ::paint method.

The first patch fixes these two problems.

The second one moves the ::paint method to ScrollbarGtk, since it's now scrollbar specific and this is the only actual widget we have anyway.

The third one changes the frameRectsChanged method to only take into account non ScrollView scrollbars, since our ScrollView scrollbars are native and won't go through this codepath (the same is done in the ::paint method).

[Xan Lopez]

Created attachment 28268 [details]
cliprect.patch

[Xan Lopez]

Created attachment 28269 [details]
movepaint.patch

[Xan Lopez]

Created attachment 28270 [details]
onlyrenderscrollbar.patch

[Holger Freyther]

Comment on attachment 28268 [details]
cliprect.patch


> --- a/WebCore/platform/gtk/WidgetGtk.cpp
> +++ b/WebCore/platform/gtk/WidgetGtk.cpp
> @@ -101,7 +101,7 @@ void Widget::hide()
>   *  2.) We assume that GTK_NO_WINDOW is set and that frameRectsChanged positioned
>   *      the widget correctly. ATM we do not honor the GraphicsContext translation.

maybe we can update the comment? I think this was meant with GraphicsContext...


>   */
> -void Widget::paint(GraphicsContext* context, const IntRect&)
> +void Widget::paint(GraphicsContext* context, const IntRect& rect)
>  {
>      if (!platformWidget())
>          return;
> @@ -114,7 +114,13 @@ void Widget::paint(GraphicsContext* context, const IntRect&)
>  
>      GdkEvent* event = gdk_event_new(GDK_EXPOSE);
>      event->expose = *context->gdkExposeEvent();
> -    event->expose.region = gtk_widget_region_intersect(widget, event->expose.region);
> +    event->expose.area = static_cast<GdkRectangle>(rect);

this is using the operator GtkRectangle()...

[Holger Freyther]

Comment on attachment 28269 [details]
movepaint.patch


> +/*
> + * Strategy to painting a Widget:
> + *  1.) do not paint if there is no GtkWidget set
> + *  2.) We assume that GTK_NO_WINDOW is set and that frameRectsChanged positioned
> + *      the widget correctly. ATM we do not honor the GraphicsContext translation.

okay, if you update the comment in the previous patch then update this as well..

[Holger Freyther]

Comment on attachment 28270 [details]
onlyrenderscrollbar.patch

> From a0e3df34a1bfd4fb3234e2459226cf485ce036f8 Mon Sep 17 00:00:00 2001
> From: Xan Lopez <[e-mail]>
> Date: Wed, 4 Mar 2009 20:23:06 +0200
> Subject: [PATCH 3/3] 2009-03-04  Xan Lopez  <[e-mail]>
> 
>         Reviewed by NOBODY (OOPS!).
> 
>         https://bugs.webkit.org/show_bug.cgi?id=24358
>         [GTK] Scrollbars not clipped correctly
> 
>         Do not take into account the case of being a ScrollView scrollbar,
>         since those are native in our case.
> 
>         * platform/gtk/ScrollbarGtk.cpp:
>         (ScrollbarGtk::frameRectsChanged):
> ---
>  WebCore/ChangeLog                     |   17 +++++++++++++++--
>  WebCore/platform/gtk/ScrollbarGtk.cpp |   16 +++-------------
>  2 files changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index c6352e7..cd3f559 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -5,8 +5,21 @@
>          https://bugs.webkit.org/show_bug.cgi?id=24358
>          [GTK] Scrollbars not clipped correctly
>  
> -	Move Widget::paint to ScrollbarGtk::paint, since it's scrollbar
> -	specific and it's our only Widget anyway.
> +        Do not take into account the case of being a ScrollView scrollbar,
> +        since those are native in our case.

^^^ oh, tabs vs. space?

for the rest, let us try it :)



>

[Xan Lopez]

Thanks, landed in r4149{0,1,2}.

[Xan Lopez]

Ok, remember what I said in the last patch about our scrollbars always being renderlayout because the other ones are native? Not true. Turns out there are scrollview scrollbars that are not native apparently. This happens in gmail for example. Seems I either didn't follow the code correctly or there's some bug somewhere, not sure. Anyway something like this fixes a new bug in gmail, where the scrollbars disappear when you scroll:

diff --git a/WebCore/platform/gtk/ScrollbarGtk.cpp b/WebCore/platform/gtk/ScrollbarGtk.cpp
index be4c1c4..0bd2dda 100644
--- a/WebCore/platform/gtk/ScrollbarGtk.cpp
+++ b/WebCore/platform/gtk/ScrollbarGtk.cpp
@@ -72,7 +72,12 @@ void ScrollbarGtk::frameRectsChanged()

     // Translate our coordinates, we are a RenderLayout scrollbar because our
     // ScrollView scrollbars are native.
-    IntPoint loc = parent()->contentsToWindow(frameRect().location());
+    IntPoint loc;
+
+    if (parent()->isScrollViewScrollbar(this))
+        loc = parent()->convertToContainingWindow(frameRect().location());
+    else
+        loc = parent()->contentsToWindow(frameRect().location());

     // Don't allow the allocation size to be negative
     IntSize sz = frameRect().size();
@@ -141,7 +146,13 @@ void ScrollbarGtk::paint(GraphicsContext* context, const IntRect& rect)
     event->expose = *context->gdkExposeEvent();
     event->expose.area = static_cast<GdkRectangle>(rect);

-    IntPoint loc = parent()->contentsToWindow(rect.location());
+    IntPoint loc;
+
+    if (parent()->isScrollViewScrollbar(this))
+        loc = parent()->convertToContainingWindow(rect.location());
+    else
+        loc = parent()->contentsToWindow(rect.location());
+
     event->expose.area.x = loc.x();
     event->expose.area.y = loc.y();


I'll try to go through this again tomorrow though. Thoughts about it welcome.

[Xan Lopez]

Adding Holger in CC, see previous comment :)

[Xan Lopez]

Created attachment 28387 [details]
subframescrollbars.patch

[Xan Lopez]

Committed in r41509, reviewed on IRC.

[Holger Freyther]

Comment on attachment 28387 [details]
subframescrollbars.patch

just for the record.