Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Topbar icons and text are controlled by wrong variable #42954

Closed
nickvergessen opened this issue Jan 19, 2024 · 28 comments · Fixed by #42977
Closed

Topbar icons and text are controlled by wrong variable #42954

nickvergessen opened this issue Jan 19, 2024 · 28 comments · Fixed by #42977
Labels
1. to develop Accepted and waiting to be taken care of 28-feedback bug design Design, UI, UX, etc. feature: theming technical debt

Comments

@nickvergessen
Copy link
Member

How to use GitHub

  • Please use the 👍 reaction to show that you are interested into the same feature.
  • Please don't comment if you have no relevant information to add. It's just extra noise for everyone subscribed to this issue.
  • Subscribe to receive notifications on status change and new comments.

Steps

  • Use default background image
  • Change theming color to the yellow option
Default blue Option yellow
💥 Text and all icons on the right side have the wrong color (based on primary and not if background is bright/dark, but that fact is used by the app icons in top left with var(--background-image-invert-if-bright) vs. var(--color-primary-text)
Bildschirmfoto vom 2024-01-19 09-24-58 Bildschirmfoto vom 2024-01-19 09-24-50
Default blue Option yellow
💥 "More apps" icon has wrong color 💥 "More apps" icon has wrong color
Bildschirmfoto vom 2024-01-19 09-25-12 Bildschirmfoto vom 2024-01-19 09-25-22
@nickvergessen nickvergessen added bug 1. to develop Accepted and waiting to be taken care of feature: theming labels Jan 19, 2024
@nickvergessen nickvergessen added this to the Nextcloud 28.0.2 milestone Jan 19, 2024
@JuliaKirschenheuter
Copy link
Contributor

cc @szaimen

@JuliaKirschenheuter
Copy link
Contributor

probably #42620 is a problem

@SystemKeeper

This comment was marked as resolved.

@nickvergessen

This comment was marked as resolved.

@szaimen

This comment was marked as resolved.

@szaimen
Copy link
Contributor

szaimen commented Jan 19, 2024

probably #42620 is a problem

No, the fix works correctly for the default themes which are more important then custom primary colors imho.

In order to fix all the cases correclty, we would need to have another UI setting that allows to set the color (black or white) for the header icons independently and not calculate it from any other value. See #35482 (comment)

@susnux
Copy link
Contributor

susnux commented Jan 19, 2024

I think to make the color background image dependent it would work to use:

color: white;
filter: var(--background-image-invert-if-bright);

Again this will not cover all cases especially for background images with shapes in different color.

@szaimen
Copy link
Contributor

szaimen commented Jan 19, 2024

Again this will not cover all cases especially for background images with shapes in different color.

yes, that is why I pointed at #35482 (comment)

@susnux
Copy link
Contributor

susnux commented Jan 19, 2024

Yes either each icon needs individual colors, or the background need a backdrop (see the comment below), or we simply ignore images that are not single color in the top area.

@ShGKme
Copy link
Contributor

ShGKme commented Jan 19, 2024

but we actually do have a color associated with each pre-defined background. Which can be used for defining elements on a background like the app menu.

public const SHIPPED_BACKGROUNDS = [
'hannah-maclean-soft-floral.jpg' => [
'attribution' => 'Soft floral (Hannah MacLean, CC0)',
'description' => 'Abstract background picture in yellow and white color whith a flower on it',
'attribution_url' => 'https://stocksnap.io/photo/soft-floral-XOYWCCW5PA',
'theming' => self::THEMING_MODE_DARK,
'primary_color' => '#9f652f',
],
'ted-moravec-morning-fog.jpg' => [
'attribution' => 'Morning fog (Ted Moravec, Public Domain)',
'description' => 'Background picture of a forest shrouded in fog',
'attribution_url' => 'https://flickr.com/photos/tmoravec/52392410261',
'theming' => self::THEMING_MODE_DARK,
'primary_color' => '#114c3b',
],

Then it is used in getColorPrimary.

Cannot we then have separated background color and primary (aka elements) color?

@szaimen
Copy link
Contributor

szaimen commented Jan 19, 2024

Cannot we then have separated background color and primary (aka elements) color?

The problem is rather with custom primary colors. This also is taken into account for the color of the header icons which it should probably not be. However custom backgrounds are still broken because you cannot control the color of the header icons manually currently...

@ShGKme
Copy link
Contributor

ShGKme commented Jan 19, 2024

The problem is rather with custom primary colors. This also is taken into account for the color of the header icons which it should probably not be. However custom backgrounds are still broken because you cannot control the color of the header icons manually currently...

Yes, but at least we may make app icons and text rely only on the background color, not on primary elements color.

@szaimen
Copy link
Contributor

szaimen commented Jan 19, 2024

Yes, but at least we may make app icons and text rely only on the background color, not on primary elements color.

Indeed that sounds better

@susnux
Copy link
Contributor

susnux commented Jan 19, 2024

I think we finally need to escape the primary-color-mixup-background-color rabbit hole.

The text color on the app menu should be independent from any primary color. Primary is only for accents, while the background color might sometimes be related but it is not always.

@ShGKme
Copy link
Contributor

ShGKme commented Jan 19, 2024

The text color on the app menu should be independent from any primary color. Primary is only for accents, while the background color might sometimes be related but it is not always.

We may rename it or create a new variable, but anyway we have a color that represents the background color and store it together with the background image in presets...

@susnux
Copy link
Contributor

susnux commented Jan 19, 2024

Meaning:

  • primary-element is what the user chose but made accessible
  • primary is a bad name but should be the color to use on the background (-image, -color)
    • maybe rename to color-background-text or similar

@ShGKme
Copy link
Contributor

ShGKme commented Jan 19, 2024

escape the primary-color-mixup-background-color rabbit hole

Having a good and full description of all the colors, variables, meanings we have and should have would be definitely helpful 😀

Taking into account user settings, custom colors, default color (Nextcloud Blue) and etc.

@susnux
Copy link
Contributor

susnux commented Jan 19, 2024

Maybe I find some time to look into it, but I think a proper solution contains real separation between background-color and primary-color.

Because:

but we actually do have a color associated with each pre-defined background.

This color is just a primary color that probably looks good on that background but e.g. the bright image has some dark yellow set as primary which then causes primary-text to be white -> white text on white image -> bad!

@nursoda
Copy link

nursoda commented Feb 15, 2024

Since milestone 28.0.2 is mentioned above: It's not fixed in 28.0.2 – counting on 28.0.3 – it's NOT fixed in 28.0.3 RC1.

@nursoda

This comment was marked as off-topic.

@susnux
Copy link
Contributor

susnux commented Feb 15, 2024

@nursoda this is unrelated - happens because of the icon SVG of the dashboard app.

@nursoda

This comment was marked as off-topic.

@ObiWahn
Copy link

ObiWahn commented Feb 17, 2024

screenshot_2024-02-17_11:02:11

please fix this, I have to deal with a lot of non technical users and they seem to care much more about this than about any other functional bug.

@escoand
Copy link
Contributor

escoand commented Feb 17, 2024

Duplicate of #43298

@susnux
Copy link
Contributor

susnux commented Feb 17, 2024

No this one is a different issue, this one is about the wrong color of the app bar icons when you select a different primary color.

In the example using the default clouds background image + yellow primary color will set black text of the app menu entries -> wrong color for the background image.

@susnux susnux reopened this Feb 17, 2024
@susnux
Copy link
Contributor

susnux commented Feb 17, 2024

@ObiWahn looks fine on the screenshot, but you have set up a too bright background color which then causes the text color to switch to black.
The text color will always try to be at least 4.5:1 color contrast to the background color. This is needed for accessibility.

Try to set the background color to #00679e which is the default, this will then use white text and icons on the app navigation.

@susnux susnux removed this from the Nextcloud 28.0.3 milestone Feb 17, 2024
@ObiWahn
Copy link

ObiWahn commented Feb 18, 2024

Thanks @susnux, everything should be stock, I do not even know how to modify the themes.

After some testing:

  • I have currently no background at all selected index.php/settings/user/theming
  • And the current color is in the first col of the third row (afaik it was never modified).
  • Switching to the 2nd col has the wanted effect
  • I can not find any input textbox for a color code

Question:

  • Is there a way make the slightly darker blue the default for all users?

@susnux
Copy link
Contributor

susnux commented Feb 18, 2024

Is there a way make the slightly darker blue the default for all users?

https://docs.nextcloud.com/server/latest/admin_manual/configuration_server/theming.html#theming

And the current color is in the first col of the third row (afaik it was never modified).

Yes we needed to adjust the colors for accessibility and it seems we need to adjust the color picker defaults.
Once #42977 is merged this will be finally fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1. to develop Accepted and waiting to be taken care of 28-feedback bug design Design, UI, UX, etc. feature: theming technical debt
Projects
None yet
Development

Successfully merging a pull request may close this issue.