Closed Bug 877606 Opened 6 years ago Closed 6 years ago

Port GTK2 to GTK3 - widget clipping

Categories

(Core :: Widget: Gtk, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: stransky, Assigned: stransky)

References

Details

Attachments

(3 files, 2 obsolete files)

+++ 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
Note: another widget which needs clipping is gtk_render_arrow(). It draws a black frame around menu without it.
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.
Whiteboard: [leave open for remaining patches][check linux try build before requesting checkin] → [leave open for remaining patches]
Attached patch minimal clipping patch (obsolete) — Splinter Review
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 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.
Attachment #770127 - Flags: feedback?(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?
Attached patch clipping v.2 (obsolete) — Splinter Review
Clip with the drawingRect.
Attachment #770127 - Attachment is obsolete: true
Attachment #770127 - Flags: feedback?(karlt)
Attachment #775615 - Flags: review?(karlt)
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.
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.
Attached file 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.
Attached file a diff
This is a cairo trace diff which is added to normal widget rendering and causes the problem.
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.
Attachment #775615 - Flags: review?(karlt)
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.
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.
A workaround for the Gnome bug.
Attachment #775615 - Attachment is obsolete: true
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.
Attachment #779713 - Flags: review+
Keywords: checkin-needed
Whiteboard: [leave open for remaining patches]
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/429c2a431ba8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Blocks: 1034064
No longer blocks: 1034064
You need to log in before you can comment on or make changes to this bug.