Closed Bug 251492 Opened 20 years ago Closed 20 years ago

[gnome]selected item text wrong color in high-contrast themes

Categories

(Core Graveyard :: GFX: Gtk, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ginnchen+exoracle, Assigned: ginnchen+exoracle)

References

Details

Attachments

(6 files, 1 obsolete file)

Use Mozilla Classic theme with gnome high-contrast theme,
Try profile-manager's list or Prefs left tree,
select one item, and click somewhere to lose focus.
Now you can't tell which item is selected.
More general test cases,
http://www.mozilla.org/projects/ui/accessibility/unix/testcase/nsIAccExt/XUL/nsIAccessibleTestTreeNode_1.xul
http://www.mozilla.org/projects/ui/accessibility/unix/testcase/nsIAccExt/XUL/nsIAccessibleTestListboxNode_1.xul
Select some items in the tree or listbox, click url bar to lose focus.
User can't tell which items are selected in HighContrast theme.
Status: NEW → ASSIGNED
Attached patch patchSplinter Review
Add 2 colors for ACTIVE_STATE.
Change Highlight color from fg/bg to text/base, bacause text/base is more
general for text foreground/background color.
Apply patch to Firefox gnomestripe
diff with winstripe listbox.css
diff with winstripe tree.css
Attachment #153264 - Flags: superreview?(roc)
Attachment #153264 - Flags: review?(bryner)
Attachment #153265 - Flags: superreview?(roc)
Attachment #153265 - Flags: review?(bryner)
Attachment #153266 - Flags: superreview?(roc)
Attachment #153266 - Flags: review?(bryner)
Attachment #153267 - Attachment description: firfox gnomestripe tree.css → firefox gnomestripe tree.css
Attachment #153267 - Flags: superreview?(roc)
Attachment #153267 - Flags: review?(bryner)
I don't understand why you're making selected tree cells the color from
GTK_STATE_ACTIVE instead of GTK_STATE_SELECTED.
GTK_STATE_ACTIVE is similar to css:selected
GTK_STATE_SELECTED is similar to css:selected:focus

I'm using GTK_STATE_ACTIVE, because it really works despite its literal.

From http://www.ajgenius.us/gnome/gnome2-gtk2-themes.html

base[state] Sets the color used for the background of among other things, entry
and Tree/List Widgets.

text[state] Sets the color used for foreground of widgets using base for the
background color. 

GTK_STATE_ACTIVE
A variant of the NORMAL color used when the widget is in the GTK_STATE_ACTIVE
state, and also for the trough of a ScrollBar, tabs of a NoteBook other than the
current tab and similar areas. Frequently, this should be a darker variant of
the NORMAL color.

GTK_STATE_SELECTED
A color used to highlight data selected by the user. for instance, the selected
items in a list widget, and the selection in an editable widget.
> the selected items in a list widget

So shouldn't selected tree cells be using GTK_STATE_SELECTED? 
(In reply to comment #8)
> > the selected items in a list widget
> 
> So shouldn't selected tree cells be using GTK_STATE_SELECTED? 
If so, there will be no difference whether the tree or listbox is focused.
You can try gedit's pref setting tree with high contrast theme.
Click something in the tree, then click something(e.g. checkbox) on the right
side. The color of selected item in the tree will change.
OK. Then I'd like to see some comments in GTK2 nsLookAndFeel saying that you're
assuming eColor_highlight is being used for :active and :focused, and
eColor__moz_gtk2_active is being used for :active and not :focused.
(In reply to comment #10)
> OK. Then I'd like to see some comments in GTK2 nsLookAndFeel saying that you're
> assuming eColor_highlight is being used for :active and :focused, and
> eColor__moz_gtk2_active is being used for :active and not :focused.

I'll add comments.
Comment on attachment 153264 [details] [diff] [review]
patch

widget/public/nsILookAndFeel.h
>+    eColor__moz_gtk2_active,
//colour used for cell text background, selected not focus
>+    eColor__moz_gtk2_activetext,
//colour used for cell text, selected not focus
>     eColor__moz_gtk2_hovertext,
//colour used for text, hover

widget/src/gtk2/nsLookAndFeel.cpp
>+    case eColor__moz_gtk2_active:
// cell text background color, selected not focus
>+        aColor = GDK_COLOR_TO_NS_RGB(mStyle->base[GTK_STATE_ACTIVE]);
>+        break;
>+    case eColor__moz_gtk2_activetext:
// cell text color, selected not focus
>+        aColor = GDK_COLOR_TO_NS_RGB(mStyle->text[GTK_STATE_ACTIVE]);
>+        break;
>     case eColor__moz_gtk2_hovertext:
// hover text color
>         aColor = GDK_COLOR_TO_NS_RGB(mStyle->fg[GTK_STATE_PRELIGHT]);
>         break;
FYI:
in source code of gtk_cell_renderer_text_render

      if (GTK_WIDGET_HAS_FOCUS (widget))
	state = GTK_STATE_SELECTED;
      else
	state = GTK_STATE_ACTIVE;
Attachment #153264 - Flags: superreview?(roc) → superreview+
Attachment #153265 - Flags: superreview?(roc) → superreview+
Attachment #153266 - Flags: superreview?(roc) → superreview+
Attachment #153267 - Flags: superreview?(roc) → superreview+
Attachment #153264 - Flags: review?(bryner) → review+
Attachment #153265 - Flags: review?(bryner) → review+
Attachment #153266 - Flags: review?(bryner) → review+
Attachment #153267 - Flags: review?(bryner) → review+
Fixed.

Checking in content/shared/public/nsCSSKeywordList.h;
/cvsroot/mozilla/content/shared/public/nsCSSKeywordList.h,v  <--  nsCSSKeywordList.h
new revision: 3.61; previous revision: 3.60
done
Checking in content/shared/src/nsCSSProps.cpp;
/cvsroot/mozilla/content/shared/src/nsCSSProps.cpp,v  <--  nsCSSProps.cpp
new revision: 3.107; previous revision: 3.106
done
Checking in themes/classic/jar.mn;
/cvsroot/mozilla/themes/classic/jar.mn,v  <--  jar.mn
new revision: 1.123; previous revision: 1.122
done
Checking in themes/classic/global/win/tree.css;
/cvsroot/mozilla/themes/classic/global/win/tree.css,v  <--  tree.css
new revision: 1.53; previous revision: 1.52
done
Checking in themes/classic/global/win/listbox.css;
/cvsroot/mozilla/themes/classic/global/win/listbox.css,v  <--  listbox.css
new revision: 1.7; previous revision: 1.6
done
Checking in widget/public/nsILookAndFeel.h;
/cvsroot/mozilla/widget/public/nsILookAndFeel.h,v  <--  nsILookAndFeel.h
new revision: 1.41; previous revision: 1.40
done
Checking in widget/src/gtk2/nsLookAndFeel.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsLookAndFeel.cpp,v  <--  nsLookAndFeel.cpp
new revision: 1.14; previous revision: 1.13
done
Checking in toolkit/themes/gnomestripe/global/jar.mn;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/jar.mn,v  <--  jar.mn
new revision: 1.5; previous revision: 1.4
done
RCS file: /cvsroot/mozilla/toolkit/themes/gnomestripe/global/tree.css,v
done
Checking in toolkit/themes/gnomestripe/global/tree.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/tree.css,v  <--  tree.css
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/themes/gnomestripe/global/listbox.css,v
done
Checking in toolkit/themes/gnomestripe/global/listbox.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/listbox.css,v  <--  listbox.css
initial revision: 1.1
done
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
I really don't like this patch.  In the past, when we've created new colors,
we've created them without ifdefs and set them to appropriate values on all
platforms.  I don't even know what -moz-gtk2-active means.  And if it never
differs from highlight, why add a new color?

I think this patch should be backed out.
(In reply to comment #15)
> And if it never
> differs from highlight, why add a new color?

Never mind that bit.

I still think this should be backed out.  I'd rather see us use a standardizable
name and rather see us have a better understanding of what it is the new color
means.
David, this patch isn't small, can we file a new bug to fix the problem instead
of backing it out?
This is not the first time we introduce eColor__moz_gtk2_*.
I agree with you to use standardizable names.
I really hate to use #ifdef in .css, too.
I agree to file a bug to remove eColor__moz_gtk2_*.
The first use of -moz-gtk* is obviously broken because there's no
foreground/background pair.  In both cases, new values and properties shouldn't
have been added without CSS module owner/peer approval.

And there's really little of the patch that would be preserved in a correct patch.
I noticed the no foreground/background pair problem after it was checked in.
Because the background is rendered by -moz-appearance, I was not going to fix this.

In this patch, I added to 2 colors, we should preserve them. Just need
standardize their names.

I suggest to standardize all these colors in one bug. And get CSS module
owner/peer approval.
Patch has been backed out. Reopen. Ginn, please work with David to make a better
solution.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
David, the colors I need are
menuhovertext/menuhoverbackground, 
buttonhovertext/buttonhoverbackground(radiobutton, checkbox may share this)
celltextselected/celltextselectedbackground

Would you please give keywords for them?

What's your suggestion to deal with other platforms(win,mac,gtk,xlib)?
Add #ifdef to .css? Add #ifdef to nsCSSProps? Or add code to every nsLookAndFeel?

Thanks
(In reply to comment #22)
> David, the colors I need are

How about:

> menuhovertext/menuhoverbackground, 

MenuHoverText, MenuHover

> buttonhovertext/buttonhoverbackground(radiobutton, checkbox may share this)

ButtonHoverText, ButtonHoverFace

> celltextselected/celltextselectedbackground

CellHighlightText, CellHighlight


> What's your suggestion to deal with other platforms(win,mac,gtk,xlib)?
> Add #ifdef to .css? Add #ifdef to nsCSSProps? Or add code to every nsLookAndFeel?

Add code to every nsLookAndFeel (matching the colors on that paltform for what
we currently use).  This will allow the other platforms to improve the values as
necessary.
Flags: blocking-aviary1.0?
Attached patch patch v2 (obsolete) — Splinter Review
Fixed bad CSS color name in bug 237535.
Fixed both bug 244955 and bug 251492.
Add 3 pairs of CSS Color name as dbaron's suggestion
Attachment #159574 - Flags: superreview?(dbaron)
Comment on attachment 159574 [details] [diff] [review]
patch v2

All the new values need -moz- prefixes, as all of our extensions do.  (I
could have been clearer about this in comment 23, I guess.)

ButtonHoverFace is never used in the themes, but it probably should be.
Likewise for MenuHover.

For non-GTK2, it seems like you should be using what's currently highlight /
highlighttext for menuhover / menuhovertext, not what's currently menu /
menutext.
Attachment #159574 - Flags: superreview?(dbaron) → superreview-
(In reply to comment #25)
> (From update of attachment 159574 [details] [diff] [review])
> All the new values need -moz- prefixes, as all of our extensions do.  (I
> could have been clearer about this in comment 23, I guess.)
I'll change the names.

> ButtonHoverFace is never used in the themes, but it probably should be.
> Likewise for MenuHover.
For button, there's "-moz-appearance: button"
MenuHoverText only used for GTK2, and there's "-moz-appearance: menuitem"
"-moz-appearance" is higher than "backgroundcolor".
I think we needn't use ButtonHoverFace or MenuHover now.
But we may need them in the future.

> For non-GTK2, it seems like you should be using what's currently highlight /
> highlighttext for menuhover / menuhovertext, not what's currently menu /
> menutext.
It's my mistake. I'll change it.
Attached patch patch v2 revisedSplinter Review
1.add -moz- prefixes
2.For non-GTK2, use highlight/highlighttext for menuhover/menuhovertext
3.For radio.css, checkbox.css, narrow the changes affect GTK2 only.
(For non-GTK2, we don't want to use buttonhovertext for radio and checkbox
indicator or label)
4.For menu, button, toolbarbutton, add menuhover and buttonhoverface as
background-color. (although it may be ignored because of -moz-appearance, I
think it's a backup)
Attachment #159574 - Attachment is obsolete: true
Attachment #159702 - Flags: superreview?(dbaron)
dbaron, can you have a look an lets decide if this should go on the avairy branch.
Comment on attachment 159702 [details] [diff] [review]
patch v2 revised

>+CSS_KEY(buttonhovertext, buttonhovertext)
This is unused, and should be removed

I didn't review the theme changes in great detail -- there's a chance some of
them could be a little risky.  I think this probably shouldn't go on the aviary
branch -- at least not without closer review of the theme changes.
Attachment #159702 - Flags: superreview?(dbaron) → superreview+
Comment on attachment 159702 [details] [diff] [review]
patch v2 revised

In nsCSSKeywordList.h,
+CSS_KEY(buttonhovertext, buttonhovertext)
It's a mistake. should be cleaned.

Thank you, dbaron.
Attachment #159702 - Flags: review?(bryner)
Flags: blocking-aviary1.0? → blocking-aviary1.0-
Comment on attachment 159702 [details] [diff] [review]
patch v2 revised

Looks fine to me.
Attachment #159702 - Flags: review?(bryner) → review+
Checking in content/shared/public/nsCSSKeywordList.h;
/cvsroot/mozilla/content/shared/public/nsCSSKeywordList.h,v  <--  nsCSSKeywordList.h
new revision: 3.67; previous revision: 3.66
done
Checking in content/shared/src/nsCSSProps.cpp;
/cvsroot/mozilla/content/shared/src/nsCSSProps.cpp,v  <--  nsCSSProps.cpp
new revision: 3.118; previous revision: 3.117
done
Checking in themes/classic/jar.mn;
/cvsroot/mozilla/themes/classic/jar.mn,v  <--  jar.mn
new revision: 1.126; previous revision: 1.125
done
Checking in themes/classic/global/unix/button.css;
/cvsroot/mozilla/themes/classic/global/unix/button.css,v  <--  button.css
new revision: 1.14; previous revision: 1.13
done
Checking in themes/classic/global/unix/checkbox.css;
/cvsroot/mozilla/themes/classic/global/unix/checkbox.css,v  <--  checkbox.css
new revision: 1.13; previous revision: 1.12
done
Checking in themes/classic/global/unix/radio.css;
/cvsroot/mozilla/themes/classic/global/unix/radio.css,v  <--  radio.css
new revision: 1.12; previous revision: 1.11
done
Checking in themes/classic/global/unix/toolbarbutton.css;
/cvsroot/mozilla/themes/classic/global/unix/toolbarbutton.css,v  <-- 
toolbarbutton.css
new revision: 1.7; previous revision: 1.6
done
Checking in themes/classic/global/win/listbox.css;
/cvsroot/mozilla/themes/classic/global/win/listbox.css,v  <--  listbox.css
new revision: 1.9; previous revision: 1.8
done
Checking in themes/classic/global/win/menu.css;
/cvsroot/mozilla/themes/classic/global/win/menu.css,v  <--  menu.css
new revision: 1.54; previous revision: 1.53
done
Checking in themes/classic/global/win/tree.css;
/cvsroot/mozilla/themes/classic/global/win/tree.css,v  <--  tree.css
new revision: 1.55; previous revision: 1.54
done
Checking in toolkit/themes/gnomestripe/global/button.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/button.css,v  <--  button.css
new revision: 1.5; previous revision: 1.4
done
Checking in toolkit/themes/gnomestripe/global/checkbox.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/checkbox.css,v  <--  checkbox.css
new revision: 1.2; previous revision: 1.1
done
Checking in toolkit/themes/gnomestripe/global/menu.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/menu.css,v  <--  menu.css
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/themes/gnomestripe/global/radio.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/radio.css,v  <-- 
radio.cssnew revision: 1.2; previous revision: 1.1
done
Checking in toolkit/themes/gnomestripe/global/toolbarbutton.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/toolbarbutton.css,v  <-- 
toolbarbutton.css
new revision: 1.5; previous revision: 1.4
done
Checking in toolkit/themes/winstripe/global/listbox.css;
/cvsroot/mozilla/toolkit/themes/winstripe/global/listbox.css,v  <--  listbox.css
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/themes/winstripe/global/tree.css;
/cvsroot/mozilla/toolkit/themes/winstripe/global/tree.css,v  <--  tree.css
new revision: 1.4; previous revision: 1.3
done
Checking in widget/public/nsILookAndFeel.h;
/cvsroot/mozilla/widget/public/nsILookAndFeel.h,v  <--  nsILookAndFeel.h
new revision: 1.45; previous revision: 1.44
done
Checking in widget/src/beos/nsLookAndFeel.cpp;
/cvsroot/mozilla/widget/src/beos/nsLookAndFeel.cpp,v  <--  nsLookAndFeel.cpp
new revision: 1.27; previous revision: 1.26
done
Checking in widget/src/gtk/nsLookAndFeel.cpp;
/cvsroot/mozilla/widget/src/gtk/nsLookAndFeel.cpp,v  <--  nsLookAndFeel.cpp
new revision: 1.63; previous revision: 1.62
done
Checking in widget/src/gtk2/nsLookAndFeel.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsLookAndFeel.cpp,v  <--  nsLookAndFeel.cpp
new revision: 1.19; previous revision: 1.18
done
Checking in widget/src/gtk2/nsLookAndFeel.h;
/cvsroot/mozilla/widget/src/gtk2/nsLookAndFeel.h,v  <--  nsLookAndFeel.h
new revision: 1.7; previous revision: 1.6
done
Checking in widget/src/mac/nsLookAndFeel.cpp;
/cvsroot/mozilla/widget/src/mac/nsLookAndFeel.cpp,v  <--  nsLookAndFeel.cpp
new revision: 1.58; previous revision: 1.57
done
Checking in widget/src/os2/nsLookAndFeel.cpp;
/cvsroot/mozilla/widget/src/os2/nsLookAndFeel.cpp,v  <--  nsLookAndFeel.cpp
new revision: 1.36; previous revision: 1.35
done
Checking in widget/src/photon/nsLookAndFeel.cpp;
/cvsroot/mozilla/widget/src/photon/nsLookAndFeel.cpp,v  <-- 
nsLookAndFeel.cppnew revision: 1.32; previous revision: 1.31
done
Checking in widget/src/windows/nsLookAndFeel.cpp;
/cvsroot/mozilla/widget/src/windows/nsLookAndFeel.cpp,v  <--  nsLookAndFeel.cpp
new revision: 1.48; previous revision: 1.47
done
Checking in widget/src/xlib/nsLookAndFeel.cpp;
/cvsroot/mozilla/widget/src/xlib/nsLookAndFeel.cpp,v  <--  nsLookAndFeel.cpp
new revision: 1.29; previous revision: 1.28
done
Status: REOPENED → RESOLVED
Closed: 20 years ago20 years ago
Resolution: --- → FIXED
With all these new colour codes does that mean we can remove the GTK2 ifdefs in
global/unix/checkbox.css global/unix/radio.css and global/win/menu.css?
(In reply to comment #33)
> With all these new colour codes does that mean we can remove the GTK2 ifdefs in
> global/unix/checkbox.css global/unix/radio.css and global/win/menu.css?

Maybe we can remove the GTK2 ifdefs in global/unix/checkbox.css
global/unix/radio.css. But I didn't do a test with GTK or Xlib toolkit yet.

For global/win/menu.css, we can remove some GTK2 ifdefs, but not all of them.
Because there's no implementation of -moz-appearance: menuitem for other toolkits
The toolkit part needs to be relanded:

Error: Expected color but found '-moz-gtk2-hovertext'.  Error in parsing value
for property 'color'.  Declaration dropped.
Source File: chrome://global/skin/toolbarbutton.css
Line: 55
Keywords: aviary-landing
fix the wrong "Landing the Aviary Branch (Toolkit sections excluding
toolkit/content)." by ben%bengoodger.com 2004-11-30 14:54
Checking in button.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/button.css,v  <--  button.css
new revision: 1.7; previous revision: 1.6
done
Checking in checkbox.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/checkbox.css,v  <--  checkbox.css
new revision: 1.4; previous revision: 1.3
done
Checking in menu.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/menu.css,v  <--  menu.css
new revision: 1.5; previous revision: 1.4
done
Checking in radio.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/radio.css,v  <--  radio.css
new revision: 1.4; previous revision: 1.3
done
Checking in toolbarbutton.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/toolbarbutton.css,v  <-- 
toolbarbutton.css
new revision: 1.7; previous revision: 1.6
done
Keywords: aviary-landing
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: