444837 - slow path in cairo_draw_with_xlib taken for NS_THEME_SCROLLBAR_TRACK_VERTICAL in nsListControlFrame

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Karl Tomlinson (:karlt)]

The slow path in cairo_draw_with_xlib is sometimes taken for
NS_THEME_SCROLLBAR_TRACK_VERTICAL in nsListControlFrame.  This happens, for
example, when viewing Bugzilla reports at certain Zoom levels.

It happens because the clip is within the cairo_draw_with_xlib drawing rect
and nsNativeThemeGTK::DrawWidgetBackground does not use
DRAW_SUPPORTS_CLIP_RECT.

The clip rect comes from a nsDisplayClip for the nsListControlFrame.  This
uses nsIRenderingContext::SetClipRect which (correctly) rounds the rect bounds
to pixels.

However the drawing rect for the scrollbar track comes from DrawWidgetBackground rounding the top-left and (incorrectly) rounding the width and height (where bz suspected a problem).


I wonder whether some themes really don't support clipping or whether that's
just due to gtk2drawing messing with the cliprect?

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/gtk2/gtk2drawing.c&rev=1.106&mark=1216-1217,1225-1234,2195-2196,2221#1212

which looks like it would be unnecessary now that we have
GetExtraSizeForWidget of nsNativeThemeGtk.cpp (if properly accounted for
there).

The gtk_paint_box "area" parameter is described as "clip rectangle, or NULL if
the output should not be clipped", which I guess is not completely clear on
whether the clip is mandatory.

Bug 333250 comment 4 seems to indicate that it really is Clearlooks ignoring
the clip.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: rounding changes

This modifies nsNativeThemeGTK::DrawWidgetBackground so that rather than
rounding the width of the drawing rect, the drawing rect is constrained to the
widget rect, the bounds of which are rounded to pixels.

The drawing rect is rounded to GDK pixels to ensure that the widget position
does not depend on the dirty rect.  (Previously the widget position included
offsets from snapping the dirty rect to device pixels and from rounding the
dirty rect top left.  These would have usually cancelled out but occasionally
they would accumulate because of different rounding in gfxPoint::Round and
NSToIntRound when the dirty rect top left is -ve, which can happen thanks to
GetExtraSizeForWidget.)  The dirty/drawing rect is rounded out when less than
the widget rect to avoid possible issues like bug 430450.

This now uses nsPresContext::AppUnitsToGfxUnits(nsRect) for transforming to
gfxRect.  Strangely enough, DrawWidgetBackground now looks pretty similar to
how nsPluginInstanceOwner::Paint would look with attachment 328811 [details] [diff] [review], except
that DrawWidgetBackground retains the trick of working in device coords when
snapped, which avoids rounding issues.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How is this related to the patch in bug 440950?

Comment 3

[Karl Tomlinson (:karlt)]

(In reply to comment #2)

There are many similarities, but notable differences.
(Sorry that I didn't spot your patch, Eli.)

Like you say in bug 440950 comment 3, attachment 326079 [details] [diff] [review] rounds widths and
heights, which would be inconsistent with clipping (and so wouldn't fix this
bug).

It looks like attachment 326079 [details] [diff] [review] removes the "widget rect depends on
dirty rect" issue when snapping, but

  gdk_rect = ConvertGfxToGdkRect(gfx_rect - gfx_clip.TopLeft());

looks like it still has a similar issue when not snapping.	

The attachment here RoundOuts the dirty rect, and Eli seems to agree in bug
440950 comment 0 that the non-snapped clip/dirty rect is not quite right.

This patch also fixes bug 440950, if the issue in testcase attachment 271058 [details] is that the background becomes visible between buttons on hover/click.

Comment 4

[Karl Tomlinson (:karlt)]

(In reply to comment #3)
> This patch also fixes bug 440950, if the issue in testcase attachment 271058 [details] is
> that the background becomes visible between buttons on hover/click.

Oh.  I also fixes the lines on scrolling.

Comment 5

[Eli Friedman]

Comment on attachment 329147 [details] [diff] [review]
rounding changes

Patch generally looks fine... pretty similar to what I came up with.

>+  // The margin should be applied to the widget rect rather than the dirty
>+  // rect but nsCSSRendering::PaintBackgroundWithSC has already intersected
>+  // the dirty rect with the uninflated widget rect.
>+  if (GetExtraSizeForWidget(aWidgetType, &extraSize)) {
>+    drawingRect.Inflate(extraSize);
>+  }

It seems like you looked into this a bit more than I did... if the widget really paints outside of where layout expects it to, won't that cause invalidation bugs?

Comment 6

[Karl Tomlinson (:karlt)]

(In reply to comment #5)
> if the widget really paints outside of where layout expects it to,
> won't that cause invalidation bugs?

It doesn't paint outside of where layout expects it to (its overflow area) but it paints more than what needs painting.  I filed bug 444870 on this.

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: rounding changes and aClipRect -> aDirtyRect

Added renaming of aClipRect to aDirtyRect for other nsITheme::DrawWidgetBackground implementations also.

The callers pass a dirty rect (intersected with the widget rect), and it is really a dirty rect that is passed to the native drawing methods.
The clip region is on the rendering context (cairo_t).
The native theme apis take a clip rect, but not a region, so the dirty rect gets passed to these (kind of as an optimization).

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  PRBool snapXY = ctx->UserToDevicePixelSnapped(rect);

Do you want to pass PR_TRUE here to allow snapping in the presence of scaling?

+  nsIntRect widgetRect(0, 0, NS_lround(rect.Width()), NS_lround(rect.Height()));

Seems to me what we should do here is Round "rect" and use the resulting width and height, so that the pixels covered by the widget are the same pixels as if, for example, we were drawing an image. I guess this is similar to the comment I had on Eli's patch.

Otherwise looks good.

Comment 9

[Karl Tomlinson (:karlt)]

(In reply to comment #8)
> +  PRBool snapXY = ctx->UserToDevicePixelSnapped(rect);
> 
> Do you want to pass PR_TRUE here to allow snapping in the presence of scaling?

I don't know.  That would essentially mean asking GTK to do the scaling, and I
don't know whether or not we want that.  Also snapping only when there is no
scaling is more consistent with other uses of UserToDevicePixelSnapped and
therefore other elements.  Best to keep current behavior for now and make that decision separately.

> 
> +  nsIntRect widgetRect(0, 0, NS_lround(rect.Width()),
> NS_lround(rect.Height()));
> 
> Seems to me what we should do here is Round "rect" and use the resulting
> width and height, so that the pixels covered by the widget are the same
> pixels as if, for example, we were drawing an image. I guess this is similar
> to the comment I had on Eli's patch.

I agree with Eli's comment in bug 440950 comment 2 for the widget rect.
AFAICT we only round regular CSS images and rectangles when
UserToDevicePixelSnapped succeeds.  For complex transformations, we could
modify the scales in the transformation matrix so as to retain subpixel
accuracy of frame bounds, but that is more complexity than I want to add
right now.

Comment 10

[Karl Tomlinson (:karlt)]

We should fix bug 445932, before doing the RoundOut here to avoid creating more issues like bug 444870 where the drawing rect extends beyond the surface.

Comment 11

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/index.cgi/rev/62cd4027f7d1