Created attachment 187145 [details] [review]
shadow: add a GtkShadow struct to represent CSS shadows

And hook it up in the CSS parser

Created attachment 187146 [details] [review]
image: look at the text-shadow property when rendering symbolic icons

And add a shadow in case there's one.

Created attachment 187147 [details] [review]
themingengine: use the text-shadow property to render text shadow

This also avoids hardcoding a shadow for insensitive text in the engine,
as it can now be applied from the theme directly.

Review of attachment 187145 [details] [review]:

Should document shadows and the text-shadow property in gtkcssprovider.c.

Review of attachment 187146 [details] [review]:

Looks ok

Review of attachment 187147 [details] [review]:

::: gtk/gtkthemingengine.c
@@ +2443,3 @@
   else
     cairo_move_to (cr, x, y);
 

Any reason why the shadow drawing code moved above the matrix setup here ?
Doesn't that have the potential to cause the shadow to be misrendered ?

Do you plan to actually implement the blur radius ?

(In reply to comment #7)
> Do you plan to actually implement the blur radius ?

That would probably be nice, but not top-priority.

I plan to work a little more on this; the goal is to have both text-shadow and box-shadow CSS3 properties supported in GTK+.

The box-shadow shadow description is different from the text-shadow [1], so I will do the following:
- move the current GtkShadow to GtkTextShadow
- add a GtkBoxShadow struct, with box-shadow semantics and support for parsing it
- implement drawing support for it in GtkThemingEngine's gtk_render_frame()
- investigate implementing the radius for both text-shadow and box-shadow at this point

[1] http://www.w3.org/TR/css3-background/#the-box-shadow

Created attachment 187387 [details] [review]
cssprovider: stop parsing the color literal name when we find a space

Created attachment 187388 [details] [review]
cssprovider: stop parsing the unit string if we find a comma

So that we don't warning out when parsing list values.

Created attachment 187389 [details] [review]
textshadow: add a GtkTextShadow struct and style property

And hook it up in the CSS parser.
It mimics the semantic of text-shadow in CSS3.

Created attachment 187390 [details] [review]
image: look at the text-shadow property when rendering symbolic icons

And add a shadow in case there's one.

Created attachment 187391 [details] [review]
themingengine: use the text-shadow property to render text shadow

This also avoids hardcoding a shadow for insensitive text in the engine,
as it can now be applied from the theme directly.

Created attachment 187392 [details] [review]
boxshadow: add a GtkBoxShadow struct to represent CSS box shadows

And hook it up in the CSS parser.

Created attachment 187393 [details] [review]
themingengine: render inset box shadows

Here's a new patchset that implements my last proposal from comment #8

Some notes:
- the first two patches are fixes in the CSS parser required to make parsing of the new properties work correctly.
- the box-shadow property is currently ignored from the theming engine if it's not inset (i.e. it's correctly parsed, but not rendered). After talking with Benjamin on IRC, it turns out supporting that in a clean way would be a lot of work in GTK+, as we would need to draw outside of the clip region of the widget itself. So, we either would need to somehow take into account the value of the shadow style property when computing the widget region to invalidate on redraw, or we need a clean way to signal which regions should be invalidated from the theme itself. I think it's fine to document the property as working correctly only in the inset case for now, and think about a more general solution for this problem in the meanwhile.

(In reply to comment #6)

> Any reason why the shadow drawing code moved above the matrix setup here ?
> Doesn't that have the potential to cause the shadow to be misrendered ?

Apparently the code currently in git master is wrong too - it sets the matrix only for the normal text case and always calls cairo_move_to() for the insensitive shadow. Anyway, I fixed this with now.


Another missing thing: rendering support for multiple box-shadows is implemented with this patchset, but I didn't implement rendering for multiple text-shadows.

Review of attachment 187387 [details] [review]:

::: gtk/gtkcssprovider.c
@@ +2193,3 @@
+          while ((g_ascii_isalnum (*end)) &&
+                 (*end != '\0') &&
+                 (*end != ' '))

I think this is equivalent to just g_ascii_isalnum (*end) ?

Review of attachment 187389 [details] [review]:

::: gtk/gtkcssprovider.c
@@ +515,3 @@
+ * <literallayout>text-shadow: @horizontal_offset @vertical_offset [@blur_radius] [@color]</literallayout>
+ * </para>
+ * </refsect2>

Should document that blur radius is currently ignored.

@@ +700,3 @@
+ *         <entry>shadow list (see above)</entry>
+ *         <entry>#GtkTextShadow</entry>
+ *         <entry><literallayout>text-shadow: 1 1 0 blue, -4 -4 red;</literallayout></entry>

What is the semantics of having more than one shadow in the list ? Should be said somewhere

Review of attachment 187388 [details] [review]:

Looks fine, assuming this doesn't break our parser tests - in fact, we should make sure our parser tests cover this case

Review of attachment 187388 [details] [review]:

Looks fine, assuming this doesn't break our parser tests - in fact, we should make sure our parser tests cover this case

Review of attachment 187390 [details] [review]:

::: gtk/gtkimage.c
@@ +1853,3 @@
+
+                  gtk_text_shadow_unref (text_shadow);
+                }

Looks like all but the first shadow in the list are ignored ?

Review of attachment 187391 [details] [review]:

::: gtk/gtkthemingengine.c
@@ +2459,1 @@
 

Same here - only first element ?

Review of attachment 187392 [details] [review]:

I wonder if this is similar enough to the text shadow case to use a single type for both ?

Review of attachment 187392 [details] [review]:

::: gtk/gtkcssprovider.c
@@ +517,3 @@
+ * The horizontal and vertical offsets can be either positive or negative,
+ * defining respectively a right/left or down/up distance for the shadow.
+ * The optional blur radius must have a positive value.

s/positive/non-negative/ ?

@@ +531,3 @@
+ * The spread defines an additional distance radius the shadow should
+ * spread in all directions, and must have a positive value.
+ * The optional blur radius must have a positive value.

We really need some examples like the ones shown in the CSS3 draft for shadow semantics.
Look at tests/styleexamples.c for how I produced the existing example images.

@@ +2918,3 @@
+      inset = g_str_has_prefix (str, "inset");
+
+      if (inset)

will this choke on inset(255,0,0) ? shouldn't it ?

Review of attachment 187392 [details] [review]:

::: gtk/gtkcssprovider.c
@@ +531,3 @@
+ * The spread defines an additional distance radius the shadow should
+ * spread in all directions, and must have a positive value.
+ * The optional blur radius must have a positive value.

And we need to document the current limitations here, such as:
- only inset shadows are supported
- only the first shadow in the list gets rendered

Review of attachment 187393 [details] [review]:

::: gtk/gtkthemingengine.c
@@ +1523,3 @@
+  _cairo_uneven_frame (cr, border_radius, x, y, width, height,
+                       &border, junction);
+}

One thing that occurs to me here is that we draw symbolic icon shadows in core gtk while text and box shadows seem to be the responsibility of the theme. That seems a bit inconsistent, and also hard on themes. Should we have some support api for this ?

@@ +2142,3 @@
+
+          gdk_cairo_set_source_rgba (cr, &(element->color));
+          cairo_fill (cr);

Oh, for box shadows we use the entire list!

Comment on attachment 187387 [details] [review]
cssprovider: stop parsing the color literal name when we find a space

This is obsolete with the CSS rewrite.

Comment on attachment 187388 [details] [review]
cssprovider: stop parsing the unit string if we find a comma

Ditto.

Created attachment 188173 [details] [review]
shadow: add a GtkShadow private type

This will be used as a base both for parsing text-shadow and box-shadow
properties. The type is private, as there's no real use in exporting
this in a public API.

Created attachment 188174 [details] [review]
css: add a parser for GTK_TYPE_SHADOW properties

Created attachment 188175 [details] [review]
styleproperties: add a text-shadow style property

Created attachment 188176 [details] [review]
themingengine: use the text-shadow property to render text shadow

This also avoids hardcoding a shadow for insensitive text in the engine,
as it can now be applied from the theme directly.

https://bugzilla.gnome.org/show_bug.cgi?id=649314

Created attachment 188177 [details] [review]
image: look at the text-shadow property when rendering symbolic icons

And add a shadow in case there's one.

[ Let's use this bug for "text-shadow" only for now, I'll get to "box-shadow" again soon if this gets in ]

Ok, I took a different approach at this now that the CSS parser branch has been merged.

Some comments that go together with the patchset.
After talking with Benjamin we agreed it's not optimal to expose this kind of boxed types in the public API, because they're not really useful for anybody except theming engines.
As we already support a set of internal-only properties (e.g. GtkAnimationDescription and Gtk9Slice), I think it makes sense to take the same approach for this.

GtkShadow private boxed type, which maps to the definition of the "box-shadow" property in CSS3. "text-shadow" is defined likewise, modulo the spread and inset fields, which are just be ignored by the theming engine when rendering the text shadow; this also allows for an unified parsing of both properties in GtkCssParser, so adding support for "box-shadow" will be very easy.

I also fixed the issues pointed out by Matthias in comment 22 and comment 23 and improved the documentation a bit.

Created attachment 188193 [details] [review]
css: add a parser for GTK_TYPE_SHADOW properties

Remove a printf leftover

Created attachment 188194 [details] [review]
shadow: add a GtkShadow private type

Remove the unused boxed declaration of GtkShadowElement

Keeping this private, and using a single type for text and box shadows sounds good to me. Don't really have any other comments here.

Some basic comments:
- Apart from the GtkImage modification, this stuff looks fine to me.
- Is the GtkImage patch really supposed to pretend that images are text?
- The order of commits is a bit screwed up now I guess.
- Some commits look like they contain stuff that belongs in different commits. (resolving the shadow is in the same commit as adding the style property?)
- reftests? I think the reference images should be reasonably easy to simulate with stacking GtkLabels in a GtkFixed. The only thing to get right is adding padding to the tested label, so the windows get equal size.

Some code comments:

>  static void
> +prepare_context_with_shadow (cairo_t *cr,
> +                             PangoLayout *layout,
> +                             gdouble x,
> +                             gdouble y,
> +                             GtkShadowElement *shadow)
>
This code moves the matrix computations and redoes them for every shadow element, that looks weird. You can just do rel_move_to (shadow_x, shadow_y); show_layout(), rel_move_to (-shadow_x, -shadow_y) when drawing the shadow.

Also, I'd add a function
  void _gtk_text_shadow_paint (shadow, cr, layout);
That way, you don't have to expose internals of the shadow to anything but gtkshadow.c and keep the struct declaration private to the C file.

+      _gtk_css_parser_try_double (parser, &blur);
+      _gtk_css_parser_try_double (parser, &spread);
+
Could you use code like
  if (!_gtk_css_parser_try_double (parser, &blur))
    blur = 0;
instead of initializing the value and hoping that the function does not touch the passed-in value? I don't think it's good code practice to assume that behavior. Also, it makes it clearer what the code does (someone could clean up the code by removing the unnecessary initializations and not realize try_double() can fail).

I think we should tell people that blur and spread don't work instead of silently accepting it and then not doing what the user expected to happen. So either you remove blur and spread parsing (which would use the default error - which I guess is telling the user that it expected a color value) or you can _gtk_css_parser_error() a more useful error message.

+      /* XXX: the color is optional and UA-defined if it's missing,
+       * but it doesn't really make sense for us...
+       */
+
Also, we can't cope with optional color values yet as the color parsing function assumes a color must be available and I think it throws errors if not.

+typedef struct _GtkShadowElement GtkShadowElement;
+
+struct _GtkShadowElement {
+  gint16 hoffset;
+  gint16 voffset;
+  gint16 radius;
+  gint16 spread;
+
+  gboolean inset;
+
+  GdkRGBA color;
+};
+
...
+
+GList     *_gtk_shadow_get_elements   (GtkShadow          *shadow);
+void       _gtk_shadow_get_element_at (GtkShadow          *shadow,
+                                       guint               position,
+                                       GtkShadowElement   *element);
+
With my proposed changes to shadow painting, this can all be removed from the header I guess.

+gboolean   _gtk_shadow_resolve        (GtkShadow          *shadow,
+                                       GtkStyleProperties *props);
+
This function does the wrong thing. It should return a GtkShadow * and not a gboolean. The reason is that the GtkShadow is shared all the way back to the CSS provider. So resolving the shadow's colors would be an irreversible operation, so if you later changed the color theme or did something else that modified the available colors, the shadow wouldn't update.

Created attachment 188243 [details] [review]
shadow: add a GtkShadow private type

This will be used as a base both for parsing text-shadow and box-shadow
properties. The type is private, as there's no real use in exporting
this in a public API.

Created attachment 188244 [details] [review]
css: add a parser for GTK_TYPE_SHADOW properties

Created attachment 188245 [details] [review]
styleproperties: add a "text-shadow" style property

And document its use in the CSS parser gtk-doc.

Created attachment 188246 [details] [review]
styleproperties: resolve properties of type GTK_TYPE_SHADOW

Created attachment 188247 [details] [review]
shadow: add a method to paint the shadow for a PangoLayout

Created attachment 188248 [details] [review]
themingengine: use the text-shadow property to render text shadow

This also avoids hardcoding a shadow for insensitive text in the engine,
as it can now be applied from the theme directly.

Created attachment 188249 [details] [review]
shadow: add a method to paint the shadow for symbolic icons

Created attachment 188250 [details] [review]
image: look at the text-shadow property when rendering symbolic icons

And add a shadow in case there's one.

New patchset attached, which addresses most of the points raised here.
Comments below.

(In reply to comment #39)
> - Is the GtkImage patch really supposed to pretend that images are text?

Yeah; note that this is done only for symbolic icons, which are monochromatic.

> - The order of commits is a bit screwed up now I guess.

No, that was just because I didn't reattach the whole patchset to the bug.

> - Some commits look like they contain stuff that belongs in different commits.
> (resolving the shadow is in the same commit as adding the style property?)

Fixed now.

> - reftests? I think the reference images should be reasonably easy to simulate
> with stacking GtkLabels in a GtkFixed. The only thing to get right is adding
> padding to the tested label, so the windows get equal size.

Yeah, I will add those.

> This code moves the matrix computations and redoes them for every shadow
> element, that looks weird. You can just do rel_move_to (shadow_x, shadow_y);
> show_layout(), rel_move_to (-shadow_x, -shadow_y) when drawing the shadow.

Right, fixed now.

> Also, I'd add a function
>   void _gtk_text_shadow_paint (shadow, cr, layout);
> That way, you don't have to expose internals of the shadow to anything but
> gtkshadow.c and keep the struct declaration private to the C file.

Good idea. I added such functions both for a pango layout and a symbolic icon now.

> Could you use code like
>   if (!_gtk_css_parser_try_double (parser, &blur))
>     blur = 0;
> instead of initializing the value and hoping that the function does not touch
> the passed-in value?

Fixed.

> I think we should tell people that blur and spread don't work instead of
> silently accepting it and then not doing what the user expected to happen. So
> either you remove blur and spread parsing (which would use the default error -
> which I guess is telling the user that it expected a color value) or you can
> _gtk_css_parser_error() a more useful error message.

No, I don't think this is a good idea.
The parser here is supposed to parse a full GtkShadow property, which will also be used for box-shadow. It's up to the theming engine to ignore values which are not relevant for the current type (e.g. inset or spread are not defined for text-shadow), but the CSS parser doesn't have to know - doing two different parsing for text-shadow and box-shadow would complicate the code a lot, for no real gain.

The fact that radius is currently ignored for text-shadow is documented in the GtkCssProvider documentation, so I think it's fine.

> +      /* XXX: the color is optional and UA-defined if it's missing,
> +       * but it doesn't really make sense for us...
> +       */
> +
> Also, we can't cope with optional color values yet as the color parsing
> function assumes a color must be available and I think it throws errors if not.

Yeah, that's why I return there with the error set by the color parser if it fails.

> With my proposed changes to shadow painting, this can all be removed from the
> header I guess.

Done.

> +gboolean   _gtk_shadow_resolve        (GtkShadow          *shadow,
> +                                       GtkStyleProperties *props);
> +
> This function does the wrong thing. It should return a GtkShadow * and not a
> gboolean. The reason is that the GtkShadow is shared all the way back to the
> CSS provider.

Yeah, I fixed it now in this new patchset.

I pushed to master the layout part of this patchset, after another round of review from Benjamin on IRC.
The icon part would require more work, as we want to make it work generally for all icons (and possibly e.g. spinners, arrows and separators too) at the theming engine level, instead of hijacking text-shadow for this and special-casing the code in GtkImage.

Attachment 188243 [details] pushed as fcc78be - shadow: add a GtkShadow private type
Attachment 188244 [details] pushed as 5b62532 - css: add a parser for GTK_TYPE_SHADOW properties
Attachment 188245 [details] pushed as 016f540 - styleproperties: add a "text-shadow" style property
Attachment 188246 [details] pushed as c3f3e4a - styleproperties: resolve properties of type GTK_TYPE_SHADOW
Attachment 188247 [details] pushed as 73e0070 - shadow: add a method to paint the shadow for a PangoLayout
Attachment 188248 [details] pushed as 8c65d91 - themingengine: use the text-shadow property to render text shadow

*** Bug 356032 has been marked as a duplicate of this bug. ***

This is now fixed in master.