(In reply to comment #0)
> We need to make sure the app-symbolic icon is used and colored properly when HC
> theme is selected.

I'm not sure we want this in gnome-shell - we currently get a GIcon from the AppInfo, tell GTK+ to look up the corresponding GtkIconInfo and ask GTK+ to load it. That's fairly generic, so I tend to think that GTK is a better place to implement this. In particular, we have no knowledge at all about which icon theme is currently in use ("the one used by GTK"), so just special-casing the HighContrast theme would already be hacky ...

Would be good to have symbolic app icons for gtk3-demo, gtk3-widget-factory and gtk3-icon-browser to test this.

Created attachment 291724 [details] [review]
gtk3demo, widget factory: provide a symbolic variant of the app icon

- necessary for HC theme

I wasn't able to find any fullcolor icon for gtk3-icon-browser. Does that mean we need one as well?

yes, icon-browser has no icon yet

Turns out these symbolic icons just work, for GTK+ about dialogs.
This is because we have 

* {
  -gtk-icon-style: symbolic; //force symbolic style icons
}

In the HighContrast theme. This gets translated into passing GTK_ICON_LOOKUP_FORCE_SYMBOLIC when loading with GtkIconTheme.

This stuff is hidden pretty deeply in the gtkcssimage machinery. Not sure how easy it is to get at that from the shell.

Created attachment 291757 [details] [review]
icontheme: Pick up default lookup flags from theme

(In reply to comment #6)
> This gets translated into passing
> GTK_ICON_LOOKUP_FORCE_SYMBOLIC when loading with GtkIconTheme.

That happens in GtkIconHelper though, so we don't get the magic in the shell :-(


> This stuff is hidden pretty deeply in the gtkcssimage machinery. Not sure how
> easy it is to get at that from the shell.

I don't think we can, unless we use something like gtk_css_provider_to_string() and parse the -gtk-icon-style property outselves. The most convenient for us would be GtkIconTheme taking care of this automatically. I'm attaching a POC patch, though the mix-up of gtk- and icon theme is questionable. An alternative would be to allow querying the -gtk-icon-style property via gtk_style_context_get_property() (it's registered without a query_value function, so afaics that's currently not possible) and do the create-fake-style-context dance in the shell (that's still mixing gtk- and icon theme, but at least it's not in the general purpose toolkit - on the other hand, it's exposing an internal property publicly) ...

Comments/thoughts?

Created attachment 291759 [details] [review]
app: Use StIcon as icon_texture for GIcons

Themes - namely the HighContrast one - may now request symbolic
icons rather than fullcolor ones. In order to have recoloring
work as expected in that case, we will need a theme node to pick
up colors from - using an StIcon instead of manually loading a
texture from the cache gives us that for free, so do that.


Independent from where the FORCE_SYMBOLIC magic should happen, we'll still need something like this on the shell side to pick up the correct colors.

(In reply to comment #8)
> Independent from where the FORCE_SYMBOLIC magic should happen ...

We could also make this decidedly non-magical by adding FORCE_SYMBOLIC to the lookup flags if GtkSettings:gtk-theme-name equals "HighContrast".

(In reply to comment #9)
> (In reply to comment #8)
> > Independent from where the FORCE_SYMBOLIC magic should happen ...
> 
> We could also make this decidedly non-magical by adding FORCE_SYMBOLIC to the
> lookup flags if GtkSettings:gtk-theme-name equals "HighContrast".

Lo-tech, but that might be the best

(In reply to comment #10)
> (In reply to comment #9)
> > We could also make this decidedly non-magical by adding FORCE_SYMBOLIC to the
> > lookup flags if GtkSettings:gtk-theme-name equals "HighContrast".
> 
> Lo-tech, but that might be the best

OK, let's move this back to the shell then. I'll attach a patch set to implement a slightly more general variation of this approach.

Created attachment 291783 [details] [review]
main: Slightly refactor loading of the default stylesheet

We will soon add support a -high-contrast theme variant when GTK+'s
HighContrast theme is used. Prepare for this by moving stuff around
a bit.

Created attachment 291784 [details] [review]
main: Add support for -high-contrast theme variants

While the default Shell style is fairly decent with regard to
accessibility requirements, having the ability to tweak certain
aspects where the regular style works less well is still useful.
For this purpose, try to load a -high-contrast theme variant of
the default stylesheet when a high-contrast theme is requested
(as determined by the GTK+ theme name).

Created attachment 291785 [details] [review]
st-texture-cache: Remove load_gicon_with_colors()

The split between st_texture_cache_load_gicon() and load_gicon_with_colors()
no longer makes any sense, so just move the code into the public method.

Created attachment 291786 [details] [review]
st-theme-node: Add support for -st-icon-style property

GTK+ added support for a -gtk-icon-style property in themes to
enforce a particular icon style. Do the same for shell themes
with an -st-icon-style property, with the same set of possible
values as the GTK+ variant:
  'requested' - use symbolic or fullcolor icon depending on the
                icon name (default)
  'regular'   - enforce fullcolor icons
  'symbolic'  - enforce symbolic icons

Created attachment 291787 [details] [review]
app: Use StIcon as icon_texture for GIcons

Themes - namely the HighContrast one - may now request symbolic
icons rather than fullcolor ones. In order to have recoloring
work as expected in that case, we will need a theme node to pick
up colors from - using an StIcon instead of manually loading a
texture from the cache gives us that for free, so do that.

Created attachment 291788 [details] [review]
app: Respect icon-style for faded icon texture

Just like regular application icons, the faded icon texture used
in the app menu should follow the theme's icon style setting.

Created attachment 291789 [details] [review]
theme: Add high-contrast variant

For now, simply enforce symbolic icons.

Created attachment 291790 [details] [review]
classic: Install high-contrast theme variant

The classic style is decidedly lower contrast than the default
style, so the high-contrast variant could prove really useful
here. However for now, just override the default icon style as
in the default session.

Review of attachment 291783 [details] [review]:

::: js/ui/main.js
@@ +77,3 @@
 function _sessionUpdated() {
+    if (sessionMode.isPrimary)
+        _loadDefaultStylesheet();

Seems unrelated.

@@ +230,3 @@
     let stylesheet;
 
     stylesheet = Gio.File.new_for_uri('resource:///org/gnome/shell/theme/' + sessionMode.stylesheetName);

name isn't actually used

Review of attachment 291784 [details] [review]:

::: js/ui/main.js
@@ +249,3 @@
+
+    if (Gtk.Settings.get_default().gtk_theme_name == 'HighContrast')
+        stylesheet = _getStylesheet(name.replace('.css', '-high-contrast.css'));

A comment here would be nice.

Review of attachment 291785 [details] [review]:

Yay.

Created attachment 291791 [details] [review]
main: Slightly refactor loading of the default stylesheet

We will soon add support a -high-contrast theme variant when GTK+'s
HighContrast theme is used. Prepare for this by moving stuff around
a bit.

Review of attachment 291786 [details] [review]:

Heh, I wrote a very similar patch for bug #739864.

::: src/st/st-theme-node.c
@@ +2415,3 @@
+              if (strcmp (term->content.str->stryng->str, "requested") == 0)
+                {
+                  return ST_ICON_STYLE_REQUESTED;

Remove the braces around this.

@@ +2424,3 @@
+                {
+                  return ST_ICON_STYLE_SYMBOLIC;
+                }

else g_warning("Unknown -st-icon-style"); ?

Review of attachment 291787 [details] [review]:

I think you mean "Use StIcon as icon_texture for ShellApps"

Review of attachment 291788 [details] [review]:

A bit frustrating that we can't merge this with the TextureCache, but oh well.

Review of attachment 291789 [details] [review]:

OK.

Review of attachment 291790 [details] [review]:

OK.

Review of attachment 291791 [details] [review]:

I think this patch might be better squashed with the one that adds -high-contrast.css. It's a bit weird why some things were changed here and some in the next patch. That said, treat it as ACN if you disagree.

(In reply to comment #25)
> I think you mean "Use StIcon as icon_texture for ShellApps"

No, I meant "for apps with GAppInfo" - window-backed apps still get a plain texture, StIcon is only used when we have a GIcon. I'll try to clarify a bit (without exceeding the line limit).

Attachment 291784 [details] pushed as f4cc332 - main: Add support for -high-contrast theme variants
Attachment 291785 [details] pushed as deddac8 - st-texture-cache: Remove load_gicon_with_colors()
Attachment 291786 [details] pushed as 2940ef0 - st-theme-node: Add support for -st-icon-style property
Attachment 291788 [details] pushed as cad56c8 - app: Respect icon-style for faded icon texture
Attachment 291789 [details] pushed as 4eb0a67 - theme: Add high-contrast variant

Attachment 291790 [details] pushed as 5ba4e68 - classic: Install high-contrast theme variant

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