877606 - Port GTK2 to GTK3 - widget clipping

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Martin Stránský [:stransky] (ni? me)]

+++ This bug was initially created as a clone of Bug #627699 +++

GTK+ 3.0 and GNOME 3 are approaching and we should get Firefox ready for them. This one is about a possible bug in cairo - see:

https://bugzilla.gnome.org/show_bug.cgi?id=694086
https://bugzilla.mozilla.org/show_bug.cgi?id=627699#c259
https://bug627699.bugzilla.mozilla.org/attachment.cgi?id=727206

Comment 1

[Martin Stránský [:stransky] (ni? me)]

Note: another widget which needs clipping is gtk_render_arrow(). It draws a black frame around menu without it.

Comment 2

[Martin Stránský [:stransky] (ni? me)]

Hm, strange. On the same system - Fedora 18, gtk3-3.6.4-1.fc18.x86_64, system cairo cairo-1.12.14-2.fc18.x86_64 - I have two different scenarios:

A latest trunk built with gtk3 needs clipping for almost all elements - menus, combo boxes, windows... 

An old trunk (from January 2013) renders menus and other elements correctly without the clipping - only some elements need it (gtkentry, carret).

So the clipping bug looks like dependent somehow on the Firefox code.

Comment 3

[Martin Stránský [:stransky] (ni? me)]

Attachment: minimal clipping patch

Rendering of almost all widgets are broken without this patch. We may check-in better fix but if you want to run the GTK3 browser now you need this patch.

Comment 4

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 770127 [details] [diff] [review]
minimal clipping patch

Karl, do you mind if we commit this patch as a temporary workaround? The gtk3 port is completely useless without it and I'm not sure now fast we can deliver the proper fix.

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 770127 [details] [diff] [review]
minimal clipping patch

I agree this is the place to do the clipping, if we need to do so.

The clip should include the overflow from GetExtraSizeForWidget() and so should be the size of drawingRect with top-left at 0,0.

However, I'd like to find out what is causing the problem.  If it is a cairo bug, then it will affect other situations in hard to reproduce and debug ways.

I'm hoping I'll have time to build and look at this in 3 weeks time.

Does it happen even with the built-in GTK theme?

Comment 6

[Martin Stránský [:stransky] (ni? me)]

Attachment: clipping v.2

Clip with the drawingRect.

Comment 7

[Martin Stránský [:stransky] (ni? me)]

The difference is in the clipping region used by cairo. The gtk_render_background() uses _cairo_gstate_paint() cairo routine, where gstate->clip is involved.

When the code is invoked from mozilla, gstate->clip is a rect of whole surface, so no clipping is actually used.

When the code is invoked from simple gtk test code, gstate->clip is rect passed to gtk_render_background() + previous translation.

Comment 8

[Martin Stránský [:stransky] (ni? me)]

btw. It seems to be related to the cairo state/surface somehow. It does not happen on cr pased to an expose event. Only on the derived one which is used for widget rendering.

Comment 9

[Martin Stránský [:stransky] (ni? me)]

Attachment: firefox-15

Somewhere in this cairo trace log is a cairo operation which causes the clipping (set by gtk_render_background/gtk_render_frame) is ignored.

Comment 10

[Martin Stránský [:stransky] (ni? me)]

Attachment: a diff

This is a cairo trace diff which is added to normal widget rendering and causes the problem.

Comment 11

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 775615 [details] [diff] [review]
clipping v.2

The patch does not work well even as a workaround in themes where the background color is different that the widget one. So we really need to fix the bug in the right way.

Comment 12

[Martin Stránský [:stransky] (ni? me)]

There are a cairo path left - a big rectangle from previous rendering and the new clipping path is inside it.

So the clipping is applied to the bigger rectangle instead of the inner clipping path. Not sure what's the correct cairo behavior here.

Comment 13

[Martin Stránský [:stransky] (ni? me)]

Generally all gfxContext methods uses the cairo_*_preserve() variants of cairo calls - clip(), stroke(), fill(). So all constructions like:

aContext->NewPath();
aContext->Rectangle(gfxRect(r.x, r.y, r.width, r.height));
[...]
aContext->Fill(); / aContext->Clip();

produces such artifacts. We can easily clear the path by cairo_new_path() before actual widget drawing. The question is if this is something which should be fixed in gtk3 or in Firefox. I expect the gtk3 call may set up cairo properly before rendering.

Comment 14

[Martin Stránský [:stransky] (ni? me)]

Attachment: clear cairo path before widget rendering

A workaround for the Gnome bug.

Comment 15

[Karl Tomlinson (:karlt)]

Comment on attachment 779713 [details] [diff] [review]
clear cairo path before widget rendering

Thanks very much for working this out, Martin.

The way state engines like cairo_t are used comes down to conventions.
Clearing the path either before or after use works, so long as all code
behaves the same.

I think the gtk rendering code is clearing the path after use by using the
cairo_clip, cairo_fill, cairo_stroke functions without _preserve, when it
performs the last operation with the path.

As you observe gfxContext drawing methods do not consume the path.

I would have expected from the name of "cairo_new_path" that the authors were
expecting the path to be cleared before use.  A name like "cairo_clear_path"
would have suggested clearing after use.

However, the naming of cairo's path-using functions, with _preserve indicating
that the path is not consumed, suggest that the path-consuming functions
(without _preserve) are expected to be the functions normally used.  If the
path-consuming functions were not expected to be the norm, then I would have
expected them to be called cairo_fill_consume or similar.

One advantage of clearing before use is that it is easier to track down the
place where the convention is not followed, than it would be if the previous
path operation had to be traced.

However, I think it is reasonable to accept different conventions in gtk and
Gecko, and this is the appropriate way to deal with the difference in
conventions.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/429c2a431ba8

This patch was missing all the commit information. Please make sure that your future patches have it before requesting checkin.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/429c2a431ba8