Andrew Comminos [:acomminos] Assignee
	Description • 9 years ago

On GTK3, resizers do not draw correctly in two ways;

- For most themes, they aren't rendered at all due to the background-color property not being set on the style context. Since the line colours are shades of the background color, they render as invisible.
- LTR and RTL resizers are rendered the same as we don't set the junction state on the style context.

	Andrew Comminos [:acomminos] Assignee
	Comment 1 • 9 years ago

This patch corrects the drawing of resizers by setting the background color property using the view style class, and skips a broken GTK2 reftest instead of failing it (as GTK3 does the correct thing now). In another bug, we may want to add a gtk3Widget reftest property.

	Karl Tomlinson (:karlt)
	Comment 3 • 9 years ago

Comment on attachment 8621718 [details] [diff] [review]
Render resizers correctly on GTK3.

>-skip-if(B2G||Mulet) fails-if(Android) fails-if(gtkWidget) != rtl.html no-resize.html # bug 834724 # Initial mulet triage: parity with B2G/B2G Desktop
>+skip-if(B2G||Mulet) fails-if(Android) skip-if(gtkWidget) != rtl.html no-resize.html # bug 834724 # Initial mulet triage: parity with B2G/B2G Desktop

Please mark the test fails and pass as appropriate instead of skipping.
xulRuntime.widgetToolkit can be used to distinguish between gtk versions 2 and
3.

>+    switch (direction) {
>+    case GTK_TEXT_DIR_LTR:
>+      sides = GTK_JUNCTION_CORNER_BOTTOMRIGHT;
>+      break;
>+    case GTK_TEXT_DIR_RTL:
>+      sides = GTK_JUNCTION_CORNER_BOTTOMLEFT;
>+      break;
>+    default:
>+      sides = GTK_JUNCTION_NONE;
>+      break;

Should the default case warn?

>     style = gtk_widget_get_style_context(gStatusbarWidget);
>     gtk_style_context_save(style);
>+    gtk_style_context_set_junction_sides(style, sides);
>     gtk_style_context_add_class(style, GTK_STYLE_CLASS_GRIP);
>     gtk_style_context_set_state(style, GetStateFlagsFromGtkWidgetState(state));
>+    // Add GTK_STYLE_CLASS_VIEW to set 'background-color'. This is necessary
>+    // as gtk_render_handle derives the handle colour from the background.
>+    gtk_style_context_add_class(style, GTK_STYLE_CLASS_VIEW);
> 
>     gtk_render_handle(style, cr, rect->x, rect->y, rect->width, rect->height);

Can you identify the GTK code that is being mimicked here please?

I wonder whether we should have a different widget here, but I haven't found
a GTK widget that uses render_handle for resize grips?

We may also need to work around changes to the resize code in GTK versions
3.14 and 3.16.

	Andrew Comminos [:acomminos] Assignee
	Comment 4 • 9 years ago

(In reply to Karl Tomlinson (ni?:karlt back June 30) from comment #3)
> Comment on attachment 8621718 [details] [diff] [review]
> Render resizers correctly on GTK3.
> 
> >-skip-if(B2G||Mulet) fails-if(Android) fails-if(gtkWidget) != rtl.html no-resize.html # bug 834724 # Initial mulet triage: parity with B2G/B2G Desktop
> >+skip-if(B2G||Mulet) fails-if(Android) skip-if(gtkWidget) != rtl.html no-resize.html # bug 834724 # Initial mulet triage: parity with B2G/B2G Desktop
> 
> Please mark the test fails and pass as appropriate instead of skipping.
> xulRuntime.widgetToolkit can be used to distinguish between gtk versions 2
> and
> 3.

Will do.

> >+    switch (direction) {
> >+    case GTK_TEXT_DIR_LTR:
> >+      sides = GTK_JUNCTION_CORNER_BOTTOMRIGHT;
> >+      break;
> >+    case GTK_TEXT_DIR_RTL:
> >+      sides = GTK_JUNCTION_CORNER_BOTTOMLEFT;
> >+      break;
> >+    default:
> >+      sides = GTK_JUNCTION_NONE;
> >+      break;
> 
> Should the default case warn?

Sure, I agree that would be better.

> >     style = gtk_widget_get_style_context(gStatusbarWidget);
> >     gtk_style_context_save(style);
> >+    gtk_style_context_set_junction_sides(style, sides);
> >     gtk_style_context_add_class(style, GTK_STYLE_CLASS_GRIP);
> >     gtk_style_context_set_state(style, GetStateFlagsFromGtkWidgetState(state));
> >+    // Add GTK_STYLE_CLASS_VIEW to set 'background-color'. This is necessary
> >+    // as gtk_render_handle derives the handle colour from the background.
> >+    gtk_style_context_add_class(style, GTK_STYLE_CLASS_VIEW);
> > 
> >     gtk_render_handle(style, cr, rect->x, rect->y, rect->width, rect->height);
> 
> Can you identify the GTK code that is being mimicked here please?
> 
> I wonder whether we should have a different widget here, but I haven't found
> a GTK widget that uses render_handle for resize grips?
> 
> We may also need to work around changes to the resize code in GTK versions
> 3.14 and 3.16.

The code is based off of the implementation of gtk_theming_engine_render_handle in gtkthemingengine.c (https://github.com/GNOME/gtk/blob/gtk-3-4/gtk/gtkthemingengine.c#L2403). It calculates the colours to use for the handle based on the background colour, which is not set in the style context for the status bar.

	Andrew Comminos [:acomminos] Assignee
	Comment 5 • 9 years ago

It appears that the reason for the resizer not drawing correctly on try is due to Ambiance's use of the unico theme engine (unico_engine_render_handle) which doesn't appear to respect the text direction at all. Perhaps we should transform the cairo context for RTL resizers to workaround this, and always tell GTK we want LTR.

	Andrew Comminos [:acomminos] Assignee
	Comment 6 • 9 years ago

This patch works around unico not rendering RTL resizers on the try server.

It does not correct the rendering of resizers when background-color is unset, but that can be looked into separately. Unico doesn't depend on the background color anyway.

	Karl Tomlinson (:karlt)
	Comment 7 • 9 years ago

(In reply to Andrew Comminos [:acomminos] from comment #4)
> The code is based off of the implementation of
> gtk_theming_engine_render_handle in gtkthemingengine.c
> (https://github.com/GNOME/gtk/blob/gtk-3-4/gtk/gtkthemingengine.c#L2403). It
> calculates the colours to use for the handle based on the background colour,
> which is not set in the style context for the status bar.

What I'm concerned about is that GTK_STYLE_CLASS_VIEW seems like a theme-specific tweak to pick up the background color from a particular css rule in particular theme.

I'd like to know what kind of style context we need so that we'll get an appropriate background from all themes (if possible).

If we can generate the same style context that GTK uses when it draws the resizer, then themes that work with GTK will work with Gecko.

If recent GTK no longer draw resizers, then perhaps the best we can do is look at earlier versions.  Or perhaps picking a widget that we expect to always have a background color (GtkWindow) may be an option.

	Andrew Comminos [:acomminos] Assignee
	Comment 8 • 9 years ago

(In reply to Karl Tomlinson (ni?:karlt) from comment #7)
> (In reply to Andrew Comminos [:acomminos] from comment #4)
> > The code is based off of the implementation of
> > gtk_theming_engine_render_handle in gtkthemingengine.c
> > (https://github.com/GNOME/gtk/blob/gtk-3-4/gtk/gtkthemingengine.c#L2403). It
> > calculates the colours to use for the handle based on the background colour,
> > which is not set in the style context for the status bar.
> 
> What I'm concerned about is that GTK_STYLE_CLASS_VIEW seems like a
> theme-specific tweak to pick up the background color from a particular css
> rule in particular theme.

Yes, it sort of is. GTK doesn't render resizers in textboxes at all, so we don't have a nice reference to use.

> I'd like to know what kind of style context we need so that we'll get an
> appropriate background from all themes (if possible).
> If we can generate the same style context that GTK uses when it draws the
> resizer, then themes that work with GTK will work with Gecko.

Rendering a resizer in a GtkWindow's style context should work fine- that's what was done in GTK3 before window resizers were deprecated. The unfortunate side-effect of this is that the resizer is rendered with a square, opaque background that does not blend with the text area (that of the window background). Applying GTK_STYLE_CLASS_VIEW gave it a lighter background.

Code from GTK 3.2, where context is the style context for GtkWidget:
> gtk_style_context_add_class (context, GTK_STYLE_CLASS_GRIP);
> gtk_style_context_set_junction_sides (context, get_grip_junction (widget));
> gtk_render_handle (context, cr, 0, 0, rect.width, rect.height);

> If recent GTK no longer draw resizers, then perhaps the best we can do is
> look at earlier versions.  Or perhaps picking a widget that we expect to
> always have a background color (GtkWindow) may be an option.

GTK 3.16 actually allows CSS themes to provide images for the resizer grip- so our resizers should actually work fine there.

Going to flag the unico patch for checkin now to cleanup test failures on try.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=82d3d33ac362

	Pulsebot
	Comment 9 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/273949681959

	Andrew Comminos [:acomminos] Assignee
	Comment 10 • 9 years ago

Here's a tiny patch to make the RTL failure only apply to GTK2.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 11 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/273949681959

	Karl Tomlinson (:karlt)
	Comment 12 • 9 years ago

(In reply to Andrew Comminos [:acomminos] from comment #8)
> Rendering a resizer in a GtkWindow's style context should work fine- that's
> what was done in GTK3 before window resizers were deprecated. The
> unfortunate side-effect of this is that the resizer is rendered with a
> square, opaque background that does not blend with the text area (that of
> the window background). Applying GTK_STYLE_CLASS_VIEW gave it a lighter
> background.

Ah, so gtk_render_handle() draws the background as well as the resizer?
Is that with unico or Adwaita or both?

If the resizer draws the background then I guess we don't want the native resizer if the textarea is not natively themed.

AFAIK Gecko only uses resizers in textareas.  Providing the style context of a textarea would then seem sensible to pick up the right background when we do want a native resizer.  If doing that then the widget should be an entry to be consistent with the style class, because themes may use the widget type to select the background.

	Andrew Comminos [:acomminos] Assignee
	Comment 13 • 9 years ago

(In reply to Karl Tomlinson (ni?:karlt) from comment #12)
> (In reply to Andrew Comminos [:acomminos] from comment #8)
> > Rendering a resizer in a GtkWindow's style context should work fine- that's
> > what was done in GTK3 before window resizers were deprecated. The
> > unfortunate side-effect of this is that the resizer is rendered with a
> > square, opaque background that does not blend with the text area (that of
> > the window background). Applying GTK_STYLE_CLASS_VIEW gave it a lighter
> > background.
> 
> Ah, so gtk_render_handle() draws the background as well as the resizer?
> Is that with unico or Adwaita or both?

Yes, that's with GTK3's default theme engine. Adwaita before 3.16 draws the background behind the resizer.

> AFAIK Gecko only uses resizers in textareas.  Providing the style context of
> a textarea would then seem sensible to pick up the right background when we
> do want a native resizer.  If doing that then the widget should be an entry
> to be consistent with the style class, because themes may use the widget
> type to select the background.

Using an entry's style context causes the full entry background to be rendered, including the border stroke and gradient. I don't think that'll quite work.

	Pulsebot
	Comment 14 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/35f290323695

	Carsten Book [:Tomcat]
	Comment 15 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/35f290323695