openhab / openhab-webui

Web UIs of openHAB
Eclipse Public License 2.0
222 stars 240 forks source link

Change color of "colorless" SVG icons depending on design #1743

Closed mueller-ma closed 11 months ago

mueller-ma commented 1 year ago

Which UI are you reporting an issue for?

The problem

Let me copy the issue of @moltke69 which he opened in the Android repo (https://github.com/openhab/openhab-android/issues/3204):

If you use own SVG icons without any "color information" (like fill, stroke, ...) the used color is always "black" independent to the the design you use (light or dark). For light design this maybe ok, but for dark design, you cannot see the icon anymore.

Screenshot_20230207-182555 Screenshot_20230207-182635

Of course, it is possible to add colors in the SVG files, but if you want to be flexible (one user at home is using the light design, the other the dark design), you cannot use the same icons.

It would be great, if you can choose the default color (for colorless SVG icons) on the basis of the design or perhaps on the "dynamic colors based on the wallpaper".

Your suggestion

My solution: SVG can set the color to the keyword currentColor which can be set by CSS to color the icon. The Android app will set the color to black for the light theme and white for the dark theme (https://github.com/openhab/openhab-android/pull/3282).

Your environment

Additional information

lolodomo commented 1 year ago

In fact, I would like to use the iconcolor attribute. I have a pending PR already working for non OH sources. For OH icons source, BasicUI uses a HTML img tag to render the icon and at my knowledge it is not possible to control the image color.

mueller-ma commented 1 year ago

Using the icon color attribut isn't flexible enough, e.g. you can set the icon to black, but then all UIs must be in light mode.

lolodomo commented 1 year ago

There is probably already a default color set in CSS for icon as the colors for non OH icons are different depending on the theme. I am going to check that.

mueller-ma commented 1 year ago

For OH icons source, BasicUI uses a HTML img tag to render the icon and at my knowledge it is not possible to control the image color.

Right, it seems that the CSS is only applied, when the SVG is included in a svg tag: https://codepen.io/smashingmag/pen/MWQjEKN

In fact, I would like to use the iconcolor attribute.

What's the exact behavior of this new attribute? Tint over the icon?

lolodomo commented 1 year ago

What's the exact behavior of this new attribute? Tint over the icon?

Set the "main" color of an icon (for icons not defining a color palette). I will show a screen capture later.

lolodomo commented 1 year ago

I am really not sure we need to add anything like a new colior keyword. My feeling is that it should be handled automatically by the app. I believe it is the case for BasicUI. To be confirmed by showing screen captures.

mueller-ma commented 1 year ago

How should the app know that an icon has to be tinted? Icons from classic shouldn't, icons from oh:mdi probably should. Should all UIs maintain a list of iconsets? And what about custom icons?

I think using the official (as in not openHAB custom) attribute "currentColor" is the right way. Icons can include it to say "please color me". Please note that at least mdi from iconify includes this attribute for all icons. I haven't check other iconsets.

Can Basic UI be refactored to use "img" tags for png and "svg" tags for svg? Then the color change are ~4 lines of CSS.

lolodomo commented 1 year ago

I first understood you were talking about a new element in the sitemap syntax or one new value for an existing element. I now understand you are talking about something in the SVG file itself? I am not sure what you are talking about when you mention currentColor.

Can Basic UI be refactored to use "img" tags for png and "svg" tags for svg?

I would like but apparently the svg tag is for inline SVG only? Our icons are a server link triggering our OH icon server. I could download the SVG before building the page to make them inline but we would loose the dynamic icons depending on item state.

Then the color change are ~4 lines of CSS.

What are these lines?

mueller-ma commented 1 year ago

I'm talking about the keyword "currentColor" inside a SVG, see the codepen link above for an example.

It seems that svg inside a img tag cannot be themed by css or js. Maybe you can use object tags? https://stackoverflow.com/questions/28652648/how-to-use-external-svg-in-html#28652750

lolodomo commented 1 year ago

Here is how it looks like with different icon sources.

sitemap test7 label="Tests"
{
        Frame {
                Default item=CurrentTemp icon="temperature"
                Default item=SetpointTemp icon="iconify:wi:day-sunny-overcast"
                Selection item=TestString icon="material:favorite"
        }
}

image image

At least, it is already good for iconfy and material icons, without doing anything.

Remains to test with a OH custom SVG icon. Where can I find an example of SVG icon having only currentColor ? Edit: ok, you provided one in #1744

lolodomo commented 1 year ago

Now with the same icon either retrieved directly from iconify or from the same icon defined as custom OH icon:

<svg xmlns="http://www.w3.org/2000/svg" width="1em" height="1em" viewBox="0 0 24 24"><path fill="currentColor" d="M12 2a7 7 0 0 0-7 7c0 2.38 1.19 4.47 3 5.74V17a1 1 0 0 0 1 1h6a1 1 0 0 0 1-1v-2.26c1.81-1.27 3-3.36 3-5.74a7 7 0 0 0-7-7M9 21a1 1 0 0 0 1 1h4a1 1 0 0 0 1-1v-1H9v1Z"/></svg>
sitemap test8 label="Tests"
{
        Frame {
                Switch item=TestDimmer icon="mdi_lightbulb"
                Switch item=TestDimmer icon="if:mdi:lightbulb"
        }
}

image image So yes, color is not adjusted in case of OH custom icon, very probably because img tag is used for OH icons.

lolodomo commented 1 year ago

I see that it exists JavaScript code that "converts" a img tag to an inline svg dynamically as soon as the image source is loaded. It requires to download the icon twice ! I could investigate that solution.

mueller-ma commented 1 year ago

Sounds good. I think the double download shouldn't be an issue as icons are cached, I guess.

lolodomo commented 1 year ago

For possible future implementation in Basic UI: https://duncanlock.net/blog/2021/11/09/styleable-inline-svg-icon-sprite-system-with-caching-fallback/ https://github.com/iconfu/svg-inject Keeping a compatibility with PNG files will make things even more difficult.

dilyanpalauzov commented 1 year ago

Isn’t it sufficient to utilize the CSS color-scheme property, instead of embedding at runtime the SVG?

moltke69 commented 1 year ago

The problem exists not only in BasicUI but also in the Apps. But there a CSS wouldn't work. You have to change the icons.

mueller-ma commented 1 year ago

The main issue for Basic UI is that icons are embedded in 'img' html tags, but CSS cannot be used to style these tags. FYI: The Android app solved the issue by passing CSS to the SVG render library.

dilyanpalauzov commented 1 year ago

After trying several things, I think non-embedded svg is not going to work. Unless probably the SVGs contain `style="color-scheme: dark light" but asking the users to include it in their images is too much.

lolodomo commented 11 months ago

I believe this is now ok in Basic UI after several PRs merged in the past. If something i s still missing, reopen the issue and please explain.