Created attachment 180864 [details] [review]
theme-node-drawing: Rename render_background_with_border()

As more cases are added where we pick the slower cairo based
fallback path, use a more generic name.

Created attachment 180865 [details] [review]
st-shadow: Parse the 'inset' keyword

The box-shadow property in the CSS3 draft[0] supports the optional
'inset' keyword for inner shadows cast onto the background. Add
support for the keyword to the shadow parsing code.

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

Created attachment 180866 [details] [review]
st-theme-node: Add support for inset box-shadows

Implement inset box-shadows as in the CSS3 draft[0]. As the shadow
should appear beneath the node's border, we pick the slow cairo based
rendering path (though a cogl based path could be added in case the
node has a solid background with no borders).

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

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

Review of attachment 180864 [details] [review]:

Looks fine ('prerender[ed]' isn't *that* descriptive, but it is more generic)

Review of attachment 180865 [details] [review]:

A few comments

::: src/st/st-theme-node.c
@@ +2711,3 @@
         }
+      else if (term->type == TERM_IDENT &&
+               strcmp (term->content.str->stryng->str, "inset") == 0)

Hmm, so the parsing here is quite a bit loser than allowed by the spec which is:

 <shadow> = inset? && [ <length>{2,4} && <color>? ]

So inset has to be first or last (and colors can't be mixed with lengths.... as is allowed with the existing code) It's probably not worth making the code more complicated, but I like having a comment which describes what it should be and mentions that the code is more permissive.

@@ +2975,3 @@
+      g_warning ("The text-shadow property does not support inset shadows");
+      st_shadow_unref (result);
+      result = NULL;

You left node->text_shadow set but un'refed it which will lead to a crash.

Review of attachment 180866 [details] [review]:

Various simplification suggestions below. Overall seems like a reasonable addition. I think it's a little incoherent to support inset box shadows without the CSS ability to have *multiple* box shadows - since whether an element has a inset shadow is pretty much independent from whether it has a non-inset shadow, but since we don't *need* both for anything at the moment, let's not worry about it.

::: src/st/st-private.c
@@ +639,3 @@
 cairo_pattern_t *
+_st_create_inset_shadow_cairo_pattern (StShadow        *shadow_spec,
+                                       cairo_pattern_t *src_pattern)

Think you need to describe what this function does. (and to me, it doesn't create an inset shadow really, it creates an inverted shadow)

@@ +677,3 @@
+    }
+
+  surface_out = cairo_surface_reference (surface_in);

Works in one of the branches above, and not in the other

@@ +697,3 @@
+
+  /* Fill the surface with a solid black color, to account areas not covered
+   * by the blurred image when applying the shadow offsets

This comment seems to be inaccurate - "the area not covered by the blurred image" is inherently covered by the fact that you are using the equality "blur(inverse(area)) == inverse(blur(area))". The reason you are filling with black is that you need it to invert using DEST_OUT.

@@ +703,3 @@
+
+  /* Paint the inverse of the blurred surface on top */
+  cairo_set_operator (cr, CAIRO_OPERATOR_DEST_OUT);

Rather than drawing back to another surface with Cairo, since we have a bunch of A8 pixels in system memory, I'd really suggest just doing the easy thing:

 for (j = 0; j < height; j++)
   {
     const char *p = pixels + rowstride * j;
     for (i = 0; i < width; i++, p++)
       *p = 255 - *p;
   }

And then this function becomes a tiny variant of _st_create_shadow_cairo_pattern() and you can avoid the cut and paste.

::: src/st/st-theme-node-drawing.c
@@ +618,3 @@
+{
+  /* Stamp the shadow pattern out in the appropriate color
+   * in a new layer

OK, obviously, this code is just moving, but it looks very overcomplicated to me

@@ +622,3 @@
+  cairo_push_group (cr);
+  cairo_set_operator (cr, CAIRO_OPERATOR_CLEAR);
+  cairo_paint (cr);

You clear the entire new group temporary surface (and groups start off as clear anyways)

@@ +630,3 @@
+                         shadow_spec->color.blue / 255.0,
+                         shadow_spec->color.alpha / 255.0);
+  cairo_paint (cr);

Then replace the entire surface contents with this color

@@ +635,3 @@
+
+  cairo_set_source (cr, pattern);
+  cairo_paint (cr);

Then you use pattern effectively as a mask to multiply the color

@@ +658,3 @@
+      cairo_append_path (cr, outline_path);
+      cairo_clip (cr);
+    }

And then you take exactly what you had and paint it clipped.

/* ... set clip... */
cairo_set_source_rgba(cr, ...);
cairo_set_mask (cr, pattern);
cairo_paint(cr);

should have the same affect as all the above code.

@@ +767,3 @@
+                   - (width + shadow_spec->spread) / 2,
+                   - (height + shadow_spec->spread) / 2);
+  cairo_append_path (temp_cr, outline_path);

I think this is wrong - you want the interior path not the exterior path - So say we have

 XXXXX
 X---X
 X---X
 XXXXX

When drawing the inset shadow, what you want to shadow is

XXXXXXX
XXXXXXX
XX---XX
XX---XX
XXXXXXX
XXXXXXX

Your _st_create_inset_shadow_cairo_pattern blurs the inverse of the surface passed in, so what you want to pass in is


  XXX
  XXX

Not:

 XXXXX
 XXXXX
 XXXXX
 XXXXX


Suggest making sure we have a test case that would reveal this problem - something, say, with a big wide border wider than the shadow blur radius.

@@ +1173,2 @@
   box_shadow_spec = st_theme_node_get_box_shadow (node);
+  has_inset_box_shadow = (box_shadow_spec && box_shadow_spec->inset);

parens unneeded

Created attachment 181025 [details] [review]
st-shadow: Parse the 'inset' keyword

(In reply to comment #6)
> Hmm, so the parsing here is quite a bit loser than allowed by the spec

Yeah, if I recall correctly, the original commit message mentioned that with regard to the color ...


> is allowed with the existing code) It's probably not worth making the code more
> complicated, but I like having a comment which describes what it should be and
> mentions that the code is more permissive.

Done.


> @@ +2975,3 @@
> You left node->text_shadow set but un'refed it which will lead to a crash.

Gah, the new block was meant to go before that ...

Created attachment 181027 [details] [review]
st-theme-node: Add support for inset box-shadows

(In reply to comment #7)
> I think it's a little incoherent to support inset box shadows without
> the CSS ability to have *multiple* box shadows - since whether an element has a
> inset shadow is pretty much independent from whether it has a non-inset shadow,
> but since we don't *need* both for anything at the moment, let's not worry
> about it.

*nods*


> @@ +639,3 @@
>  cairo_pattern_t *
> +_st_create_inset_shadow_cairo_pattern (StShadow        *shadow_spec,
> +                                       cairo_pattern_t *src_pattern)
> 
> Think you need to describe what this function does. (and to me, it doesn't
> create an inset shadow really, it creates an inverted shadow)

With the changes suggested below the differences between create_shadow_pattern and create_inset_shadow_pattern got so small that I used conditionals rather than a separate function.


> +  /* Paint the inverse of the blurred surface on top */
> +  cairo_set_operator (cr, CAIRO_OPERATOR_DEST_OUT);
> 
> Rather than drawing back to another surface with Cairo, since we have a bunch
> of A8 pixels in system memory, I'd really suggest just doing the easy thing [...]

I actually did invert the pixels manually at first, the actual reason for the additional surface was to apply the offset/spread transformations. I now figured that those can really be applied before the blur, so the blurred surface is no longer necessary.


> ::: src/st/st-theme-node-drawing.c
> @@ +618,3 @@
> OK, obviously, this code is just moving, but it looks very overcomplicated to
> me

Yup.


> /* ... set clip... */
> cairo_set_source_rgba(cr, ...);
> cairo_set_mask (cr, pattern);
> cairo_paint(cr);

Works nicely, thanks!


> @@ +767,3 @@
> I think this is wrong - you want the interior path not the exterior path

Hmm, right - if there is an interior path we should use that ...


> Suggest making sure we have a test case that would reveal this problem -
> something, say, with a big wide border wider than the shadow blur radius.

Done in a follow-up commit.


> @@ +1173,2 @@
> parens unneeded

Fixed.

Created attachment 181029 [details] [review]
tests: Add a box-shadow test

The original support for CSS based shadows has been extended with
support for an optional spread radius and the 'inset' keyword,
so with the additional complexity a dedicated test case looks
appropriate.

The "normal" box-shadows are missing the corners, I suspect clutter broke it (again) ...

Review of attachment 181025 [details] [review]:

Looks good

Review of attachment 181027 [details] [review]:

Looks good, only minor comments.

::: src/st/st-private.c
@@ +682,3 @@
 
+  /* Invert pixels for inset shadows */
+  if (shadow_spec->inset)

I think it might be better to pass in an extra 'invert' parameter to the function. The reason I say this is that an inset shadow isn't *inherently* an inverted shadow - e.g., if we were doing a inset shadow of a background image we wouldn't want to shadow the inverse of the background image, we'd want to shadow the background image + the area outside the padding box. So the caller has to know they are drawing an inset shadow and pass in a different pattern. If not, then this behavior where shadow_spec->inset causes an inverse shadow to be created should be called out in the function docs.

@@ +711,3 @@
+    {
+      /* For inset shadows, offsets and spread radius have already been
+       * applied to the original patter, so all left to do is shift the

'patter' => 'pattern'

@@ +720,3 @@
+      cairo_pattern_set_matrix (dst_pattern, &shadow_matrix);
+
+      return dst_pattern;

Hmm, this is another place where you deviate quite a lot for inset and non-inset patterns in how the caller has to use this. That probably would encourage commenting rather than adding a separate boolean for 'offset and spread' - if there are a bunch of flags and we only use a couple of combinations of the flag then that's overgeneralization.

::: src/st/st-theme-node-drawing.c
@@ +725,3 @@
+   * should be drawn as if everything outside the outline was opaque,
+   * we use a temporary surface to draw the background as a solid shape,
+   * which is inversed when creating the shadow pattern

inversed => inverted

@@ +735,3 @@
+  cairo_scale (temp_cr,
+               width / (width + 2 * shadow_spec->spread),
+               height / (height + 2 * shadow_spec->spread));

Obviously slightly different in end effect than what is done for non-inverted shadows where we scale the blurred image rather than blur a scaled image. This seems to be closer to the intent of the CSS spec (though somewhat different in detail), think it's fine.

Review of attachment 181029 [details] [review]:

Test case seems fine - in addition to the corner problems, there seem to be issues with the alignment of offset shadows - top/bottom/left/right aren't the same when they should be, sharp shadows have aa pixels at the edges

Created attachment 181055 [details] [review]
st-theme-node: Add support for inset box-shadows

(In reply to comment #12)
> I think it might be better to pass in an extra 'invert' parameter to the
> function.

OK.


> @@ +711,3 @@
> +    {
> +      /* For inset shadows, offsets and spread radius have already been
> +       * applied to the original patter, so all left to do is shift the
> 
> 'patter' => 'pattern'

Fixed.


> @@ +720,3 @@
> Hmm, this is another place where you deviate quite a lot for inset and
> non-inset patterns in how the caller has to use this. That probably would
> encourage commenting

Other than the existing comment? Should I add a gtk-doc comment and document that behavior there as well?


> ::: src/st/st-theme-node-drawing.c
> inversed => inverted

Done.


In reply to comment #13)
> Test case seems fine - in addition to the corner problems, there seem to be
> issues with the alignment of offset shadows - top/bottom/left/right aren't the
> same when they should be, sharp shadows have aa pixels at the edges

Fixed as well.

(In reply to comment #14)

> > @@ +720,3 @@
> > Hmm, this is another place where you deviate quite a lot for inset and
> > non-inset patterns in how the caller has to use this. That probably would
> > encourage commenting
> 
> Other than the existing comment? Should I add a gtk-doc comment and document
> that behavior there as well?

I think unexpected things about the "contract" of a function rather than implemenatation details should be documented at the top of the function - comments buried inside the body of a function shouldn't be necessary to understand how it is used - I don't care if it's gtk-doc formatted or not, though it gives a certain standard structure so you don't have to think about it.

(In reply to comment #14)
 
> In reply to comment #13)
> > Test case seems fine - in addition to the corner problems, there seem to be
> > issues with the alignment of offset shadows - top/bottom/left/right aren't the
> > same when they should be, sharp shadows have aa pixels at the edges
> 
> Fixed as well.

what did you change? (so I can just eyeball that part)

(In reply to comment #15)
> what did you change? (so I can just eyeball that part)

In paint_inset_box_shadow_to_cairo_context():

 - Added

  /* Downscaling to account for the spread radius triggers
   * antialiasing - it doesn't matter much except for hard
   * shadows, where we want sharp edges
   */
  if (shadow_spec->spread > 0 && shadow_spec->blur == 0)
    cairo_set_antialias (temp_cr, CAIRO_ANTIALIAS_NONE);

 - Changed the translation to a single

  cairo_translate (temp_cr,
                   shadow_spec->xoffset + shadow_spec->spread,
                   shadow_spec->yoffset + shadow_spec->spread);

(In reply to comment #16)
> (In reply to comment #15)
> > what did you change? (so I can just eyeball that part)
> 
> In paint_inset_box_shadow_to_cairo_context():
> 
>  - Added
> 
>   /* Downscaling to account for the spread radius triggers
>    * antialiasing - it doesn't matter much except for hard
>    * shadows, where we want sharp edges
>    */
>   if (shadow_spec->spread > 0 && shadow_spec->blur == 0)
>     cairo_set_antialias (temp_cr, CAIRO_ANTIALIAS_NONE);

And this will give you jaggies for sharp shadows with rounded corners. It seems to me that the problem is likely wrong computations rather than antialiasing. I'll see if I can figure it out.

Created attachment 181095 [details] [review]
Inset shadows, my attempt

This is an interdiff to the next-to-last version of the shadow patch which
I think gets things working close to as expect them to work. Major things
in here:

 * The way that the spreading was done wasn't going to take the edges
   of a rounded rectangle and bring them to another rounded rectangle -
   we were doing the scaling based on shrinking down an overall
   rectangle rather than the boundary path, which resulted in a rather
   random shrunk boundary path. Compute the shrink and the resulting surface
   size from the boundary path.

 * Ordering of the inversion was wrong - We need to invert the blur,
   not blur the inverted source or we don't handle the edges right -
   the "source" we are passing in conceptually is infinite before
   it is inverted.

 * There can be visible areas of the shadow which are outside the
   surface we create - so fill those areas with solid color.

Also:

 * Add doc comment to _st_create_shadow_cairo_pattern() describing
   different behavior depending on whether inset or not.

 * Fix a couple of types noted in my last review

Note that it's a easier to see things if you make the border radius
for the test case bigger - say 10 rather than 4.

Attachment 180864 [details] pushed as 794c986 - theme-node-drawing: Rename render_background_with_border()
Attachment 181025 [details] pushed as 2b90be7 - st-shadow: Parse the 'inset' keyword
Attachment 181029 [details] pushed as 51bfbca - tests: Add a box-shadow test
Attachment 181055 [details] pushed as a3a6650 - st-theme-node: Add support for inset box-shadows

Pushed attachment 181055 [details] [review] merged with attachment 181095 [details] [review]