Review of attachment 153497 [details] [review]:

Looks close - I don't think it's right for all scrollviews to get this shadow, however. I think it really depends on the contents of the scrollview whether this will look good or weird. So, it probably needs to be a configurable property of the StScrollView.

::: data/theme/gnome-shell.css
@@ +42,3 @@
 }
 
+StBin#top-shadow

* To me, this isn't just any top shadow, it's a top shadow specific to the scrollview. So, it should be 

  StScrollView > .top-shadow

Or
  .scroll-view-top-shadow

(Slight preference for the first)

* I'd like to reserve use of names/ids for things that are unique across the entire stage. Since this is not unique, it should be using a style class not a name.

* Generally,

  StBin.scroll-view-top-shadow
  .scroll-view-top-shadow

Are pretty much identical, except that the latter is more readable and more performant. I don't think it's usually necessary to select both on the class of the actor and also on a style-class or actor name.

@@ +50,3 @@
+}
+
+StBin#bot-shadow

'bot' here is a random abbreviation. Spell out 'bottom'

::: src/st/st-scroll-view.c
@@ +66,3 @@
 
+  ClutterActor *top_shadow;
+  ClutterActor *bot_shadow;

As in the header, spell out bottom

@@ +534,3 @@
+
+  d = value - lower;
+  if (d > 0.1 || d < -0.1)

Hmm, I wonder if the length of the shadow should continuously decrease as you get near the end. But this is fine for now. I'd write it as:

 if (fabs (value - lower) < 0.1)

though - I think that's clearer in expressing "near" than the way you have it. - obviously they are the same thing in the end.

Just a random thought which occurred to me right now ...

Generally I don't like modifying a general purpose widget for some very specific use case - e.g., what happens, if the view is scrolled horizontally? Surely you'd want left/right shadows in that case.

So my suggestion would be a little more general: add a container actor, which covers the whole scrolled area but remains fixed, and expose it via a property (overlay or something). Then you can access that actor to create the overlaid shadows in the app menu.

That way you cover a lot more use cases than the shadows ("something above the scrolled content which is not moved by the scroll bars") with just slightly more work in the app-menu patch (add an actor for the shadow on each end).

Thoughts?

(In reply to comment #2)
> what happens, if the view is scrolled horizontally?
> Surely you'd want left/right shadows in that case.

In next patch, I will add property (disable/enable).

> add a container actor, which
> covers the whole scrolled area but remains fixed, and expose it via a property

Where it can be used? (except shadow)

Created attachment 154423 [details] [review]
[StScrollView] Implement bottom/top shadows.

Review of attachment 154423 [details] [review]:

Looks pretty good. Minor comments follow

::: js/ui/appDisplay.js
@@ +92,3 @@
         this.actor.hide();
 
+        let view = new St.ScrollView({ x_fill: true, y_fill: false, style_class: 'all-app-scroll-view', enable_vshadows: true });

This needs to be line broken - I think the limit for putting properties on the same line is about 2.

::: src/st/st-scroll-view.c
@@ +149,3 @@
+
+void
+st_scroll_view_set_vshadows (StScrollView *self,

Needs a doc coment, can borrow the text from the property. See below.

@@ +447,3 @@
+  child_box.y2 = content_box.y1 + clutter_actor_get_height (priv->top_shadow);
+  if (child_box.y2 > child_box.y1 && child_box.x2 > child_box.x1)
+    clutter_actor_allocate (priv->top_shadow, &child_box, flags);

I don't like the conditional here... in general, you should always allocate the child if it is visible and not allocate the child if it isn't visible... you can just use a MAX(0, <blah>) on the width.

@@ +610,3 @@
                                    pspec);
 
+  pspec = g_param_spec_boolean ("enable-vshadows",

Everywhere else you have vshadows, but here you have 'enable-vshadows'. The setter, the enumeration, and the property name all need to match. I'd prefer just 'vshadows' to 'enable-vshadows' for the porperty name.

@@ +612,3 @@
+  pspec = g_param_spec_boolean ("enable-vshadows",
+                                "Enable Vertical Shadows",
+                                "Enable Vertical Shadows",

Should be:

 Vertical Shadows
 Show shadows at the top and and bottom of the area unless fully scrolled to that edge

::: src/st/st-scroll-view.h
@@ +90,3 @@
 
+void          st_scroll_view_set_vshadows        (StScrollView *self,
+                                                  gboolean      value);

General convention for setters here is to name the parameter the same as the name of the setters, so 'vshadows' rather than 'value'

Created attachment 154443 [details] [review]
[StScrollView] Implement bottom/top shadows.

Review of attachment 154443 [details] [review]:

Commit message should be something like:

 Add top and bottom shadows to app browser

 Add a 'vshadow' property to StScrollView, which, when turned on,
 overlays gradient shadows on the top and bottom of the StScrollView.

 Turn this on for the StScrollView used for the app browser.

(The subject of the commit message is what the patch accomplishes. The body says how that happens.) Good to commit with the fixes noted below.

::: src/st/st-scroll-view.c
@@ +153,3 @@
+ * @vshadows: Whether to enable vertical shadows
+ *
+ * Show shadows at the top and and bottom of the area unless fully scrolled to that edge

Editorial fix:

 Sets whether to show shadows at the top and bottom of the area. Shadows
 are omitted when fully scrolled to that edge.

@@ +162,3 @@
+  StScrollViewPrivate *priv = ST_SCROLL_VIEW (self)->priv;
+
+  priv->vshadows = vshadows;

As a style thing, boolean setters should start with:

 vshadows = vshadows != FALSE;
 if (priv->vshadows == vshadows)
    return;

@@ +169,3 @@
+    return;
+
+  update_shadow_visibility (G_OBJECT (vadjust), NULL, self);

And at the end here, you need

 g_object_notify (G_OBJECT (vadjust), "vshadows")

@@ +453,3 @@
+  child_box.x2 = MAX (child_box.x1, content_box.x2 - sb_width);
+  child_box.y2 = MAX (child_box.y1, content_box.y1 + clutter_actor_get_height (priv->top_shadow));
+  clutter_actor_allocate (priv->top_shadow, &child_box, flags);

I'd think I'd surround all of this with:

 if (CLUTTER_ACTOR_IS_VISIBLE (priv->top_shadow)) {
 }

Not absolutely necessary, but I think it's good practice. Same for the bottom shadow.