Created attachment 183403 [details] [review]
Style expanded summary-sources

When items in the message tray are expanded there
is no feedback from the summary-source. Add a style
to summary-sources which matches them to their
counterpart action in the panel.

Created attachment 183478 [details] [review]
Style-expanded-summary-sources

Fixed author and contact information.

Created attachment 183564 [details] [review]
message-tray: Add :expanded pseudo class to summary buttons

In order to make it more obvious to users that the entire summary
item is clickable (rather than just its icon), it makes sense to
style expanded summary items differently. Add an :expanded pseudo
class to make this possible.

Created attachment 183567 [details] [review]
message-tray: Add :expanded pseudo class to summary buttons

Add an :expanded pseudo class which allows styling summary buttons
differently while the corresponding notication is popped out.

Looking over this again quickly, Florian, I think to get the effect perfect there has to be 2 pseudo classes.

When we've got a notification expanded but the summary-source is collapsed to its smallest state we run into the problem of the aspect for the background glow needing to be the narrow version. So we need to know

a) am I (where I is the summary-source) expanded?
b) is my summary-notification expanded/showing?


So we should if possible have something like:
.summary-source-button:title_expanded
.summary-source-button:notification_expanded {
    border-image: url("source-button-border.svg") 10 10 0 1;
}
.summary-source-button:title_expanded {
    background-image: url("panel-button-highlight-wide.svg");
}
.summary-source-button:notification_expanded {
    background-image: url("panel-button-highlight-narrow.svg");
}

Where title_expanded supersedes notification_expanded (so that we always show the correct-aspect graphic). Would this be possible?

Forgot to Note: b would have to be true for _expanded to be true, as it is now, just that now we can also know if the summary-title itself is showing or not.

Created attachment 183892 [details] [review]
MessageTray: style summary sources

Make summary sources look more clickable and highlight them when selected.
Highlighting the fully expanded summary source when selected matches the
highlighting in the top bar items and teaches the user that any part of the
expanded summary source can be clicked.

Based on the initial patches by Florian Müllner and Jonathan Strander.

----------------------

I've merged the three relevant patches in this one since there were some
changes I was testing and ended up with a patch that was easier to attach
than describe :).

1) It seems that handling border-image "none" is not implemented in
st-theme-node.c . It needs to be implemented similarly to how it is implemented
for setting background-image to "none". This patch depends on that functionality
being there.

2) I removed keeping the summary item text gray when it is not hovered over
because the only time it is visible is when you hover to the left of the
left-most item, and I think it is odd and distracting.

3) The highlight of just the icon was asymmetric because, for example, in LTR
locales it was getting 4px padding on the left and 16px padding on the right.
I moved the symmetric padding to summary-source, so that we can just highlight
summary-source when only the icon is showing. It's better to highlight
summary-source-button when the title is showing because that indicates the
full area that is clickable and titles might have some padding on the right
anyway because we use the length of the longest title for all of them.
Indicating the full area that is clickable when only the icon is showing is not
important because as soon as you hover over it, it expands to show the title.

4) I had to set y_fill to true for summary-source-button so that the bottom
border for summary-source would still be on the bottom of the screen. Then I had
to set y_fill to false when adding source-title-bin and explicitly set its
height to be the same as the height of the icon so that the second line of the
title that is wrapped as the title expands is not visible. (This is a known
Clutter bug that Pango.EllipsizeMode.NONE we set is ignored.)

5) I used "selected" pseudo class to mean that the summary notification is
popped up and "expanded" pseudo class to mean that the title is showing. I moved
adding "selected" pseudo class to _showSummaryBoxPointer() from
_onSummaryBoxPointerExpanded() because it needs to apply both when the summary
notification is shown and when the right click menu is shown.

6) As suggested by Jonathan in his last two comments, I used
background-image: url("panel-button-highlight-narrow.svg");
for when just the icon is being shown. The highlight is a little
more noticeable that way.

I also noticed that this patch causes the following errors:

(lt-gnome-shell-real:19000): Cogl-glx-CRITICAL **: cogl_object_unref: assertion `object != NULL' failed

(lt-gnome-shell-real:19000): St-CRITICAL **: _st_paint_shadow_with_opacity: assertion `shadow_material != COGL_INVALID_HANDLE' failed

This happens due to just one line that specifies the text-shadow for summary-source-button .

Indeed, this is because Cogl won't allocate a 0x0 texture, and we don't check the return value for errors.

(In reply to comment #7)
> 1) It seems that handling border-image "none" is not implemented in
> st-theme-node.c . It needs to be implemented similarly to how it is implemented
> for setting background-image to "none". This patch depends on that
> functionality
> being there.

This turned out to be easy. Will attach a patch.

Created attachment 183906 [details] [review]
StThemeNode: support border-image: none

Treat border-image: none as a valid specification that overwrites any
previously specified border image.

"source-button-border.svg" wasn't included in your patch Marina.

Created attachment 183911 [details] [review]
Add source-button-border

To make summary-sources match other UI in Shell
add a border-image.




----

There. Feel free to squash it as part of the overall patch.

Created attachment 183912 [details] [review]
Add source-button-border

Woops. This is the real patch. The other was from an older revision of this bug's codebase.

Review of attachment 183906 [details] [review]:

Looks correct.

Comment on attachment 183906 [details] [review]
StThemeNode: support border-image: none

Attachment 183906 [details] pushed as adbc1d9 - StThemeNode: support border-image: none

Review of attachment 183892 [details] [review]:

Some general observations:
 - :expanded is set when the item starts expanding
   and unset when it finished contracting
   (notable as the wide background image behaves
    ugly while expanding)

 - not sure about :selected - maybe :active as in the panel?
   (no strong opinion though, your call)

Other than that, looks fine to me.
(Oh, and I think it makes sense merging the other patch, we
 usually add images when we start using them)

::: data/theme/gnome-shell.css
@@ +972,3 @@
     background-gradient-direction: vertical;
     background-gradient-start: rgba(0,0,0,0.01);
+    background-gradient-end: rgba(0,0,0,0.82);

Unrelated, but probably not worth splitting out to a separate commit.

@@ +1200,3 @@
+
+.source-title-bin {
+    height: 24px;

Why is this necessary? I compared screenshots with and without that class, and I can't spot any difference.

Review of attachment 183912 [details] [review]:

The image needs to be added to data/Makefile.am.

Review of attachment 183892 [details] [review]:

Another issue I just noticed:

(lt-gnome-shell-real:14302): Cogl-glx-CRITICAL **: cogl_object_unref: assertion `object != NULL' failed

(lt-gnome-shell-real:14302): St-CRITICAL **: _st_paint_shadow_with_opacity: assertion `shadow_material != COGL_INVALID_HANDLE' failed
Window manager warning: Log level 16: value "-nan" of type `gfloat' is invalid or out of range for property `y' of type `gfloat'

(repeated ad infinitum)

My guess is that this is caused by text-shadows for labels with a width of 0.

Created attachment 184026 [details] [review]
MessageTray: style summary sources

Make summary sources look more clickable and highlight them when selected.
Highlighting the fully expanded summary source when selected matches the
highlighting in the top bar items and teaches the user that any part of the
expanded summary source can be clicked.

Based on the initial patches by Florian Müllner and Jonathan Strander.

Pseudo class 'active' is set on buttons when they are pressed, so it
was really not a good idea to replace 'selected' with that :). That's
what caused flickering of the tray icon items.

Created attachment 184027 [details] [review]
MessageTray: style summary sources

small fix

Created attachment 184028 [details] [review]
MessageTray: style summary sources

Add y_fill: false when adding an icon.

Review of attachment 184028 [details] [review]:

Looks good!