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.

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

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;

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

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.

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.

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

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.

(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.

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)

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.

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.

Comment on attachment 159702 [details] [diff] [review] patch v2 revised Looks fine to me.

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

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

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