GNOME Bugzilla – Bug 731952
Replace hardcoded colors with theme colors
Last modified: 2016-03-31 13:22:07 UTC
The style until now was first introduced in e37e4da0. I don't see any difference between the mockup given in that commit log and the button after the patch. However when the window is unfocused the customize button greys out like the others and does not break consistency anymore.
Created attachment 278821 [details] [review] wizard: Use default style for Customize button The previous style was useless and looked weird when window was unfocused.
Review of attachment 278821 [details] [review]: Commit log seems to suggest you are changing the style but you are only removing a style class. Also the button now looks like this here w/o its style: http://static.fi/~zeenix/tmp/boxes-customize-button-style.png
Review of attachment 278821 [details] [review]: I could change the commit log... It would be good to know the reason why it looks that way on your system; I would not expect that behaviour and it does not occur on my system.
(In reply to comment #3) > It would be good to know the reason why it looks that way on your system; I > would not expect that behaviour and it does not occur on my system. I updated gtk+ and icon theme and now it doesn't have that affect. OTOH it now does what I was expecting: the color of text just becomes darker and not helping the fact that button looks pressed when not in focus.
Created attachment 278839 [details] Screencast of w/ and w/o the style class in question
Anyway we shouldn't use hardcoded colors. In the end we just end up with something like contacts with the dark theme: http://i.imgur.com/Iz9U4MQ.png Why does the button look pressed? GTK bug?
(In reply to comment #6) > Anyway we shouldn't use hardcoded colors. Putting it in the theme means 'no hardcoding'. What do you suggest? > In the end we just end up with > something like contacts with the dark theme: > http://i.imgur.com/Iz9U4MQ.png ?? How come we end up looking like that? > Why does the button look pressed? GTK bug? I don't know. I'm not even sure if its a bug or intended way of showing 'not in focus'.
(In reply to comment #7) > (In reply to comment #6) > > Anyway we shouldn't use hardcoded colors. > > Putting it in the theme means 'no hardcoding'. What do you suggest? We have .boxes-property-name-label { color: #bebebe; } .boxes-machine-name-label { color: white; } and all those things. These are definitely hard coded colors. Quote from [0]: "Be wary that your color choices might not be good for everyone and there should be a good way to override them. Whenever possible, pull your colors from the GNOME theme. Your colors should not be hard-coded, and must be override-able. Yes, your interface may not appear in colors that are in your brand book or evoke the emotion you wish to convey in your interface – but consider that the ability for someone to be able to use your software or not be able to use it at all also affects your brand as well." And I agree strongly. I cant use contacts :( > > In the end we just end up with > > something like contacts with the dark theme: > > http://i.imgur.com/Iz9U4MQ.png > > ?? How come we end up looking like that? 1. Start gnome-tweak-tool 2. Activate "Global dark theme" 3. Start contacts Tested on several archlinux installations as well as on fedora. > > Why does the button look pressed? GTK bug? > > I don't know. I'm not even sure if its a bug or intended way of showing 'not in > focus'. Shouldn't it be just an ordinary button like all the others? Why should it differ? [0] http://mairin.wordpress.com/2010/02/23/painless-accessibility-tips-for-gnome-designers-and-developers/
(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > Anyway we shouldn't use hardcoded colors. > > > > Putting it in the theme means 'no hardcoding'. What do you suggest? > > We have > .boxes-property-name-label { > color: #bebebe; > } > > .boxes-machine-name-label { > color: white; > } > > and all those things. These are definitely hard coded colors. Again, whats a non-hardcoded color? > > > > In the end we just end up with > > > something like contacts with the dark theme: > > > http://i.imgur.com/Iz9U4MQ.png > > > > ?? How come we end up looking like that? > > 1. Start gnome-tweak-tool > 2. Activate "Global dark theme" > 3. Start contacts 1. No, how do *we* end up looking like that, not Contacts? 2. We are not interested in supporting all scenerios and envorinments. You are supposed to use the default theme and any tweaking you do, you are most likely on your own and will have many issues in many different software. > > > Why does the button look pressed? GTK bug? > > > > I don't know. I'm not even sure if its a bug or intended way of showing 'not in > > focus'. > > Shouldn't it be just an ordinary button like all the others? Why should it > differ? Because we have a different background and you'll see that the same is set for other widget too in the theme. Having said all that, I agree that it would be nice to have this coming from the default theme (which is now part of gtk+) but the defaults are not what we need here.
(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > (In reply to comment #6) > > > > Anyway we shouldn't use hardcoded colors. > > > > > > Putting it in the theme means 'no hardcoding'. What do you suggest? > > > > We have > > .boxes-property-name-label { > > color: #bebebe; > > } > > > > .boxes-machine-name-label { > > color: white; > > } > > > > and all those things. These are definitely hard coded colors. > > Again, whats a non-hardcoded color? Its a color provided by the theme. The problem here is that the background color is hardcoded in the first place as you'll see below. > > > > > > In the end we just end up with > > > > something like contacts with the dark theme: > > > > http://i.imgur.com/Iz9U4MQ.png > > > > > > ?? How come we end up looking like that? > > > > 1. Start gnome-tweak-tool > > 2. Activate "Global dark theme" > > 3. Start contacts > > 1. No, how do *we* end up looking like that, not Contacts? This is how Boxes looks when turning on the default accessibility theme: http://i.imgur.com/2Wqr7mX.png See also the HIG: https://developer.gnome.org/hig-book/3.2/design-color.html.en Right at the bottom: "Ensure your application is not dependent on a particular theme. Test it with different themes, especially high and low contrast accessibility themes, which use fewer colors, to ensure your application respects the settings. For example, all text should appear in the foreground color against the background color specified in the chosen theme." https://developer.gnome.org/hig-book/3.2/checks-yourself.html.en 12.1.3 "Test your application with black and white, high contrast themes and confirm that all information is still conveyed correctly." > 2. We are not interested in supporting all scenerios and envorinments. You are > supposed to use the default theme and any tweaking you do, you are most likely > on your own and will have many issues in many different software. This sounds sad for the 11% of our users with any visual disability dependent on the a11y themes. (Source: again Gnome HIG) By the way: there are whole distributions built on custom themes. (e.g. I use antergos) Thats _many_ users you abandon with that. I didn't see any gnome app having problems with the default theme of Antergos besides Boxes. (Its very near to adwaita so it breaks almost nothing. Except Boxes because of the dark background thing...)
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > (In reply to comment #7) > > > > (In reply to comment #6) > > > > > Anyway we shouldn't use hardcoded colors. > > > > > > > > Putting it in the theme means 'no hardcoding'. What do you suggest? > > > > > > We have > > > .boxes-property-name-label { > > > color: #bebebe; > > > } > > > > > > .boxes-machine-name-label { > > > color: white; > > > } > > > > > > and all those things. These are definitely hard coded colors. > > > > Again, whats a non-hardcoded color? > > Its a color provided by the theme. The problem here is that the background > color is hardcoded in the first place as you'll see below. As I said before, would be nice if the color comes from system theme. The solution is not to just drop the theme but ensure that we have something from system theme to use. > > > > ?? How come we end up looking like that? > > > > > > 1. Start gnome-tweak-tool > > > 2. Activate "Global dark theme" > > > 3. Start contacts > > > > 1. No, how do *we* end up looking like that, not Contacts? > > This is how Boxes looks when turning on the default accessibility theme: > http://i.imgur.com/2Wqr7mX.png Good, now that is the answer to my question. :) > > 2. We are not interested in supporting all scenerios and envorinments. You are > > supposed to use the default theme and any tweaking you do, you are most likely > > on your own and will have many issues in many different software. > > This sounds sad for the 11% of our users with any visual disability dependent > on the a11y themes. (Source: again Gnome HIG) Not at all. There should be system themes for both. Satisfying a11y is different from satisfying all kinds of scenerios and all possible themes.
(In reply to comment #10) > > 2. We are not interested in supporting all scenerios and envorinments. You are > > supposed to use the default theme and any tweaking you do, you are most likely > > on your own and will have many issues in many different software. > > This sounds sad for the 11% of our users with any visual disability dependent > on the a11y themes. (Source: again Gnome HIG) By the way this sentence is quite an exageration of what HIG actually says: "An estimated 11% of the world population has some sort of color-blindness. Those affected typically have trouble distinguishing between certain hues such as red and green (deuteranopia or protanopia), or blue and yellow (tritanopia)." Keeping in mind that color blindless is not affected by black on white vs white on black, I don't think 11% of ours users are affected by this. Just wanted to get the record straight.
(In reply to comment #12) > By the way this sentence is quite an exageration of what HIG actually says: "An > estimated 11% of the world population has some sort of color-blindness. Those > affected typically have trouble distinguishing between certain hues such as red > and green (deuteranopia or protanopia), or blue and yellow (tritanopia)." > > Keeping in mind that color blindless is not affected by black on white vs white > on black, I don't think 11% of ours users are affected by this. Just wanted to > get the record straight. Thanks. That is very true. Indeed I just checked Boxes' default appearance with a visual disability simulator. The good message is: all of the three mentioned visual disabilities don't seem to really affect grey shades. (Assuming the visual disabilities only shift some colors.) That did astonish me a bit indeed. I am not an expert on this but I can imagine some users having problems with low contrasts (grey shades), e.g. if you have very bad eyes in general. (With no specific color blindness.) All in all I think removing as much hardcoded colors as possible is no bad thing at all: - IMHO it increases maintainability since the application will just auto-adjust to adwaita changes, less bugs - If there are people having problems distinguishing the different shades of gray they'll also profit - We'd also have a better support for custom themes - you, zeenix, stated that it's not important but I think its neither a bad thing I am tense to hear jimmacs opinion.
<zeenix> jimmac: just wanted to remind you about bug#731952 <jimmac> zeenix, when he speaks about default does he mean suggested-action? <jimmac> zeenix, i'm quite lost on what that bug is about <zeenix> jimmac: no, he is talking about color <zeenix> .boxes-wizard-summary-customize-button { <zeenix> color: white; <zeenix> } <jimmac> zeenix, why is that? <jimmac> zeenix, why not just @theme_fg_color? <zeenix> jimmac: https://bug731952.bugzilla-attachments.gnome.org/attachment.cgi?id=278839 <zeenix> we also setting colors for a few other widgets <zeenix> lables mostly <jimmac> so in essence you don't like :backdrop being toned down in terms of contrast? <jimmac> that's what I was saying to lapo with the recent redesign <jimmac> we should just be flattening :backdrop now <zeenix> jimmac: no clue what :backdrop is <jimmac> unfocused window state <zeenix> ah <zeenix> jimmac: ok, so i guess I'll just take the patch to remove the custom CSS then
Review of attachment 278821 [details] [review]: Based on last comment, I'll take it but can you please remove other custom colors just above this one?
Review of attachment 278821 [details] [review]: And yes, the log still needs improvement. :)
*** Bug 734923 has been marked as a duplicate of this bug. ***
Created attachment 283664 [details] [review] gtk-style: Remove .boxes-machine-name-label This class was introduced in ea48c29e and its usage was removed.
Created attachment 283665 [details] [review] gtk-style: Remove hardcoded color for .boxes-wizard-summary-prop-value-label The inherited value looks almost the same and provides better maintainability due to less hardcoded colors and less code in general.
Created attachment 283667 [details] [review] gtk-style: Remove hardcoded color for .boxes-wizard-summary-prop-value-label The inherited value looks almost the same and provides better maintainability due to less hardcoded colors and less code in general.
Created attachment 283668 [details] [review] gtk-style: Remove .boxes-wizard-summary-prop-value-label The inherited value looks almost the same and provides better maintainability due to less hardcoded colors and less code in general.
Created attachment 283669 [details] [review] gtk-style: Remove .boxes-wizard-summary-customize-button The inherited value looks almost the same and provides better maintainability due to less hardcoded colors and less code in general.
Created attachment 283670 [details] [review] gtk-style: Use theme color for boxes-wizard-current-page-label The theme_fg_color looks almost the same and provides better maintainability due to less hardcoded colors and less code in general.
Created attachment 283671 [details] [review] gtk-style: Use theme color for .boxes-property-name-label The theme_unfocused_fg_color looks almost the same and provides better maintainability due to less hardcoded colors and less code in general.
Created attachment 283672 [details] [review] gtk-style: Use theme color for .boxes-wizard-summary-prop-name-label The theme_unfocused_fg_color looks almost the same and provides better maintainability due to less hardcoded colors and less code in general.
Created attachment 283675 [details] [review] gtk-style: Use theme color for .boxes-wizard-label The theme_fg_color looks almost the same and provides better maintainability due to less hardcoded colors and less code in general.
How about changing the background color to a theme color? I have a draft of this based on the patches provided here; you can look at it here (four screenshots): http://imgur.com/h60n1Hg,lT1kvNA,W0wVOke,gYn6Esi * It adapts better to other themes, e.g. high contrast, and theme changes. * Its more consistent with other apps e.g. Nautilus which has almost the same background colors for sidebar and content as this new version.
Created attachment 283718 [details] [review] gtk-style: Replace .boxes-continue with .suggested-action The .suggested-action class is available from the theme. There is no need to maintain an extra clone of this class.
Review of attachment 283718 [details] [review]: looks good to me.
Created attachment 283725 [details] [review] util-app: Remove get_boxes_bg_color() (unused)
Created attachment 283727 [details] [review] wizard: Remove .boxes-bg (unneeded) .boxes-bg defines that the background color is transparent. This property is overwritten by the .wizard class.
Created attachment 283728 [details] [review] gtk-style: Rename .boxes-bg to .transparent-bg The new name describes way better that the class just makes the background transparent.
Created attachment 283729 [details] [review] gtk-style: Rename .wizard to .content-bg This class describes the background for any content area and should have an appropriate name.
Created attachment 283730 [details] [review] gtk-style: Remove .empty-boxes, use .content-bg Both classes do exactly the same.
Created attachment 283731 [details] [review] gtk-style: Remove .boxes-icon-view, use .content-bg Both classes do exactly the same.
Created attachment 283732 [details] [review] gtk-style: Remove .properties, use .content-bg Both classes do exactly the same.
Created attachment 283735 [details] [review] content-bg: Use theme-color instead of image This is a minor visual change while it allows Boxes to adapt to theme changes and especially makes themes like the high contrast accessibility theme work.
Created attachment 283736 [details] [review] sidebar: Use theme-color instead of image This is a minor visual change while it allows Boxes to adapt to theme changes and especially makes themes like the high contrast accessibility theme work.
Created attachment 283737 [details] [review] BoxesMiniGraph: Use theme colors This is a minor visual change while it allows Boxes to adapt to theme changes and especially makes themes like the high contrast accessibility theme work. The BoxesMiniGraph will now invert colors with the theme if needed.
The provided patches create this appearance: http://imgur.com/dSuDUj3,MPnW2we,oeoeurz,vC125RX,ZpCnRuk,VBQAlqW,0CHnXDK,MjFANVp,LsMVb9Z,8gpnWaS,uhEH4Jd,N4tmQ3P,sUs9nDl,qMyR03b (First chunk of images with adwaita, dark, second chunk with high contras a11y theme.)
Created attachment 283742 [details] [review] gtk-style: Remove .boxes-machine-name-label This class was introduced in ea48c29e and its usage was removed.
Created attachment 283743 [details] [review] gtk-style: Remove unneeded classes The inherited values look almost the same and provide better maintainability due to less hardcoded colors and less code in general.
Created attachment 283745 [details] [review] gtk-style: Replace hardcoded colors by theme colors The colors from the theme look almost the same while providing better maintainability due to less hardcoded colors and less code in general.
Created attachment 283746 [details] [review] gtk-style: Replace .boxes-continue with .suggested-action The .suggested-action class is available from the theme. There is no need to maintain an extra clone of this class.
Created attachment 283747 [details] [review] util-app: Remove get_boxes_bg_color() (unused)
Created attachment 283748 [details] [review] wizard: Remove .boxes-bg (unneeded) .boxes-bg defines that the background color is transparent. This property is overwritten by the .wizard class.
Created attachment 283749 [details] [review] gtk-style: Rename .boxes-bg to .transparent-bg The new name describes way better that the class just makes the background transparent.
Created attachment 283751 [details] [review] gtk-style: Rename .wizard to .content-bg This class describes the background for any content area and should have an appropriate name.
Created attachment 283752 [details] [review] gtk-style: Replace unneeded classes by .content-bg The .content-bg class does exactly the same as these classes so we can use this everywhere.
Created attachment 283753 [details] [review] content-bg: Use theme-color instead of image This is a minor visual change while it allows Boxes to adapt to theme changes and especially makes themes like the high contrast accessibility theme work.
Created attachment 283754 [details] [review] sidebar: Use theme-color instead of image This is a minor visual change while it allows Boxes to adapt to theme changes and especially makes themes like the high contrast accessibility theme work.
Created attachment 283755 [details] [review] BoxesMiniGraph: Use theme colors This is a minor visual change while it allows Boxes to adapt to theme changes and especially makes themes like the high contrast accessibility theme work. The BoxesMiniGraph will now invert colors with the theme if needed.
Review of attachment 283742 [details] [review]: from log it sounds like the class as added and its usage removed in the same commit. Could you make it clearer.
Review of attachment 283743 [details] [review]: The patch is about removal of hardcoding of colors so shortlog should reflect that instead of a side-effect.
Review of attachment 283745 [details] [review]: ack
Review of attachment 283746 [details] [review]: abbreviate 'with' as 'w/' to keep shortlog short. Good otherwise.
Review of attachment 283747 [details] [review]: "util-app: Remove unused get_boxes_bg_color()" would be shorter. :) Some details would be nice (when it become redundant?).
Review of attachment 283748 [details] [review]: Same comment about shortlog. A bit confused by the details as facts are mentiond but a conclusion seems missing . :)
Review of attachment 283749 [details] [review]: ack
Review of attachment 283751 [details] [review]: well its only used by wizard so i'll keep 'wizard' in name at least if rename it at all (which I think isn't needed/justified).
Review of attachment 283751 [details] [review]: Ah you later re-use it in other classes, so you might want to reflect that in the log.
Review of attachment 283752 [details] [review]: ack
Review of attachment 283753 [details] [review]: ack
Review of attachment 283754 [details] [review]: 'theme-color' -> 'theme color'. :)
Review of attachment 283755 [details] [review]: 'Use theme colors' As opposed to what? When shortlog is self-explainatory, the details sections should start with describing the change before justification for it. This actually applies to last few patches too.
Review of attachment 283747 [details] [review]: it was introduced in 2011, you really want me to git bisect through all commits since then to find the removal? or is there a better way?
Created attachment 283763 [details] [review] gtk-style: Remove .boxes-machine-name-label This class was introduced in ea48c29e and its usage was removed later.
Created attachment 283764 [details] [review] gtk-style: Remove unneeded hardcoded colors The inherited values look almost the same and provide better maintainability due to less hardcoded colors and less code in general.
Created attachment 283765 [details] [review] gtk-style: Replace hardcoded colors by theme colors The colors from the theme look almost the same while providing better maintainability due to less hardcoded colors and less code in general.
Created attachment 283766 [details] [review] gtk-style: Replace .boxes-continue by .suggested-action The .suggested-action class is available from the theme. There is no need to maintain an extra clone of this class.
Created attachment 283767 [details] [review] util-app: Remove unused get_boxes_bg_color() This function was introduced back in b60fccda and its usage was removed later.
Created attachment 283768 [details] [review] wizard: Remove unneeded .boxes-bg .boxes-bg defines that the background color is transparent. This property is overwritten by the .wizard class and therefore the use of this class is unneeded here.
Created attachment 283769 [details] [review] gtk-style: Rename .boxes-bg to .transparent-bg The new name describes way better that the class just makes the background transparent.
Review of attachment 283747 [details] [review]: You don't need to bisect. You need to use `git blame` with `git show`.
Created attachment 283770 [details] [review] gtk-style: Rename .wizard to .content-bg This class describes the background for any content area and should have an appropriate name. With the new name this class will later be used to describe other content backgrounds outside the wizard.
Created attachment 283771 [details] [review] gtk-style: Replace unneeded classes by .content-bg The .content-bg class does exactly the same as these classes so we can use this everywhere.
Created attachment 283772 [details] [review] content-bg: Use theme color instead of image This is a minor visual change while it allows Boxes to adapt to theme changes and especially makes themes like the high contrast accessibility theme work.
Created attachment 283773 [details] [review] sidebar: Use theme color instead of image This is a minor visual change while it allows Boxes to adapt to theme changes and especially makes themes like the high contrast accessibility theme work.
Created attachment 283774 [details] [review] BoxesMiniGraph: Replace hardcoded colors by theme colors The BoxesMiniGraph will now invert colors with the theme if needed. This is a minor visual change while it allows Boxes to adapt to theme changes and especially makes themes like the high contrast accessibility theme work.
Review of attachment 283768 [details] [review]: new shortlog: wizard: Drop use of .boxes-bg
Review of attachment 283763 [details] [review]: I don't think the addition of this class was important w.r.t to patch but removal, i-e we need info of when it was removed more than info of when it was added.
Review of attachment 283764 [details] [review]: ack
Created attachment 283776 [details] [review] gtk-style: Remove .boxes-machine-name-label This class was introduced in ea48c29e and its usage was removed in 2526a28c.
Created attachment 283777 [details] [review] gtk-style: Remove unneeded hardcoded colors The inherited values look almost the same and provide better maintainability due to less hardcoded colors and less code in general.
Created attachment 283778 [details] [review] gtk-style: Replace hardcoded colors by theme colors The colors from the theme look almost the same while providing better maintainability due to less hardcoded colors and less code in general.
Created attachment 283779 [details] [review] gtk-style: Replace .boxes-continue by .suggested-action The .suggested-action class is available from the theme. There is no need to maintain an extra clone of this class.
Created attachment 283780 [details] [review] util-app: Remove unused get_boxes_bg_color() This function was introduced back in b60fccda and its usage was removed in 6155dc10.
Created attachment 283781 [details] [review] wizard: Drop use of .boxes-bg .boxes-bg defines that the background color is transparent. This property is overwritten by the .wizard class and therefore the use of this class is unneeded here.
Created attachment 283782 [details] [review] gtk-style: Rename .boxes-bg to .transparent-bg The new name describes way better that the class just makes the background transparent.
Created attachment 283783 [details] [review] gtk-style: Rename .wizard to .content-bg This class describes the background for any content area and should have an appropriate name. With the new name this class will later be used to describe other content backgrounds outside the wizard.
Created attachment 283784 [details] [review] gtk-style: Replace unneeded classes by .content-bg The .content-bg class does exactly the same as these classes so we can use this everywhere.
Created attachment 283785 [details] [review] content-bg: Use theme color instead of image This is a minor visual change while it allows Boxes to adapt to theme changes and especially makes themes like the high contrast accessibility theme work.
Created attachment 283786 [details] [review] sidebar: Use theme color instead of image This is a minor visual change while it allows Boxes to adapt to theme changes and especially makes themes like the high contrast accessibility theme work.
Created attachment 283787 [details] [review] BoxesMiniGraph: Replace hardcoded colors by theme colors The BoxesMiniGraph will now invert colors with the theme if needed. This is a minor visual change while it allows Boxes to adapt to theme changes and especially makes themes like the high contrast accessibility theme work.
Attachment 283776 [details] pushed as 88d1119 - gtk-style: Remove .boxes-machine-name-label Attachment 283777 [details] pushed as 46d46e6 - gtk-style: Remove unneeded hardcoded colors Attachment 283778 [details] pushed as 2d050fa - gtk-style: Replace hardcoded colors by theme colors Attachment 283779 [details] pushed as 6113618 - gtk-style: Replace .boxes-continue by .suggested-action Attachment 283780 [details] pushed as 0b63de0 - util-app: Remove unused get_boxes_bg_color() Attachment 283781 [details] pushed as c5d46cd - wizard: Drop use of .boxes-bg Attachment 283782 [details] pushed as 407d644 - gtk-style: Rename .boxes-bg to .transparent-bg Attachment 283783 [details] pushed as cbcccb2 - gtk-style: Rename .wizard to .content-bg Attachment 283784 [details] pushed as 795f70f - gtk-style: Replace unneeded classes by .content-bg Attachment 283785 [details] pushed as b4a3531 - content-bg: Use theme color instead of image Attachment 283786 [details] pushed as 7b682cc - sidebar: Use theme color instead of image Attachment 283787 [details] pushed as 787852b - BoxesMiniGraph: Replace hardcoded colors by theme colors