User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008043007 Minefield/3.0pre Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008043007 Minefield/3.0pre On Windows XP most icons appear lighter/glossier on hover. However some icons change to a darker state on hover, which is inconsistent. These are: Go Search Cut Copy Paste Additionally, the copy icon hover state is too subtle and barely appears to change at all. Reproducible: Always Steps to Reproduce: 1. 2. 3.

In the Windows OS, icon hovers are indicated by an increase in saturation (which makes things seem slightly darker), not by a lightening of the color (which actually desaturates the icon). To that end, the icons that you refer to are actually consistent with the system's hover behavior; it's the other icons that are not.

Using the guidelines provided by Microsoft, I created new hover states for the Winstripe-XP icons and packaged them up into an extension (to make it easier to try them out). The glassy back/forward buttons should get lighter and glossier, as that's the appropriate thing to do for that sort of surface, but for the iconic buttons, the glossiness doesn't work and ends up making the images look washed-out and over-exposed.

Also note icons in Error Console: * All * Clear * Errors * Messages * Warnings

(In reply to comment #5) > Also note icons in Error Console: > As noted in comment 1 and the URL, it's the lighter state that is incorrect and inconsistent with the Windows toolbar guidelines, so the icons in the error console are correct.

Given that this affects the main toolbar, I think it qualifies as high visibility.

I just want to note that I haven't forgotten about this bug. I hope to have everything fixed by RC1 of 3.1.

This bug's priority relative to the set of other polish bugs is: P1 - Polish issue that appears in the main window, or is something that the user may encounter several times a day. crap, missed 3.5

Stephen, can you add this to the list of icon work to fix up for Firefox 3.6?

(In reply to comment #10) > Stephen, can you add this to the list of icon work to fix up for Firefox 3.6? Yes, looking into this now.

Adjusting the summary since "lighter" and "darker" is a little confusing since the two Photoshop blending options in play here are actually a combination of: -inner shadow at 17% opacity (darker) -white overlay at 36% opacity (lighter) >Yes, looking into this now. What happened was that the icons produced by Dave Brasgalla uses the settings above, while the icons produced by David Lanham used different values. You might want to leverage the column showing the icon's creator in the icon inventory to track down the ones we need to update. Also note that this only effects XP, since Vista doesn't use alternate states for icons.

This Photoshop file shows the blending options used by Dave Brasgalla. Note that the layer is called "hit" even though it is actually being used for hover+hit as per the XP guidelines. Also, fwiw this bug shows how dealing with images in a distributed development environment is really hard. Details of construction get obfuscated away when the final file is rendered, there is no ability to inspect or show blame, etc.

Also note that in the icon inventory there is a column called "stateType" in this case the effected icons have a stateType of Toolbar, which have the states of "Normal; Hover (XP only); Disabled" The icons that have a stateType of "Image Button" are not effected, these actually press in on hit (like the back and forward buttons, or the bookmark star).

I corrected all the icons with a hover state to be more saturated/darker. This includes: - Toolbar Icons - Small Toolbar Icons - LocationBar Feed Icon - Find Bar Icons - LocationBar Star Icon I left the Back/Forward buttons highlighted on hover since they are a special case. More like the Start button. I also found some icons that probably should have hover states but don't: - Bookmarks Organizer - Page Info - Options I am not sure if these were omitted for a reason?

>I also found some icons that probably should have hover states but don't: >- Bookmarks Organizer >- Page Info >- Options If I remember correctly all of the 32x32 prefpane icons (options window, page info, addons manager) weren't getting alternate states since we had a background color change for hover and hit and felt that would be enough feedback, and the alternate icon states would get too visually noisy. Examples: http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser/preferences/Options.png http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser/pageInfo.png However, anything on the toolbar in the library window is still effected by this problem (like the organize button, etc.): http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser/places/libraryToolbar.png

(In reply to comment #16) > However, anything on the toolbar in the library window is still effected by > this problem (like the organize button, etc.): > > http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser/places/libraryToolbar.png I can't find anywhere that libraryToolbar.png is used. Currently the toolbar icons are using individual images and do not have hover/active states. http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser/places/organize.png http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser/places/view.png http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser/places/importAndBackup.png Could probably remove libraryToolbar.png if it isn't in use and make individual hover state icons.

looks like unused and redundant from what i see. not sure if someone intended to use it instead of single icons. it was committed with bug 429689 Actually Reed asked about why duplicating icons, but got no answer... for sure it's unused.

Patch to remove unused files and add hover states for the Library toolbar.

Comment on attachment 406538 [details] [diff] [review] Add Hover States to Library Toolbar. >- skin/classic/browser/places/libraryToolbar.png (places/libraryToolbar.png) > skin/classic/browser/places/starred48.png (places/starred48.png) > skin/classic/browser/places/unstarred48.png (places/unstarred48.png) > skin/classic/browser/places/tag.png (places/tag.png) > skin/classic/browser/places/history.png (places/history.png) > skin/classic/browser/places/allBookmarks.png (places/allBookmarks.png) > skin/classic/browser/places/unsortedBookmarks.png (places/unsortedBookmarks.png) > skin/classic/browser/places/importAndBackup.png (places/importAndBackup.png) >+ skin/classic/browser/places/importAndBackup-hover.png (places/importAndBackup-hover.png) > skin/classic/browser/places/organize.png (places/organize.png) >+ skin/classic/browser/places/organize-hover.png (places/organize-hover.png) > skin/classic/browser/places/view.png (places/view.png) >+ skin/classic/browser/places/view-hover.png (places/view-hover.png) We should use libraryToolbar.png instead of separate files, so that the different states don't need to be loaded while the user interacts with the UI.

Comment on attachment 405319 [details] Corrected Hover State with the layers flattened out into a png I wasn't able to inspect the blending options to confirm values (image reviews are like doing a code review on compiled code), but it looked fine and Stephen of course knows what he is doing.

Use libraryToolbar.png instead of the individual files. Remove the individual icons.

Stephen, the last row in Toolbar.png equals the second one, right? The third row for the bookmarks and history icons seems to be identical to the first one, too. Can we get rid of that stuff? You'd have to update browser.css for that. Let me know if you prefer me to take this.

(In reply to comment #25) > Stephen, the last row in Toolbar.png equals the second one, right? The third > row for the bookmarks and history icons seems to be identical to the first one, > too. Can we get rid of that stuff? You'd have to update browser.css for that. > Let me know if you prefer me to take this. I noticed that and wasn't sure if there was a reason for the duplication. I will remove the duplicates from the image and update browser.css.

>reason for the duplication During Firefox 3 we decided to only do image updates to avoid any last minute regressions since the theme was coming in late (and in this case hover and hit are identical). Otherwise no reason for the duplication.

Removed Duplicate Images from Toolbar*.png

Merged the Active State with the Hover State for the Navigation Bar.

Comment on attachment 413446 [details] [diff] [review] Add Hover States to Library Toolbar - 03 > #back-button:not([disabled="true"]):hover, >-#back-button[buttonover="true"] { >+#back-button[buttonover="true"], #back-button[buttonover="true"] can be removed, it doesn't do anything. Same for #forward-button[buttonover="true"]. >+#back-button:not([disabled="true"]):hover:active { Isn't that already covered by #back-button:not([disabled="true"]):hover? Same applies to all the other buttons.

(In reply to comment #30) > >+#back-button:not([disabled="true"]):hover:active { > > Isn't that already covered by #back-button:not([disabled="true"]):hover? Same > applies to all the other buttons. hover:active is for when it is pressed.

There's a flaw between the full screen icons and the back icons in Toolbar-small.png. (In reply to comment #31) > (In reply to comment #30) > > >+#back-button:not([disabled="true"]):hover:active { > > > > Isn't that already covered by #back-button:not([disabled="true"]):hover? Same > > applies to all the other buttons. > > hover:active is for when it is pressed. Yes, but :hover:active doesn't apply anywhere where :hover wouldn't already apply.

Comment on attachment 413473 [details] [diff] [review] Add Hover States to Library Toolbar - 04 >+#history-button:not([checked="true"]):hover, >+#history-button[checked="true"] { > -moz-image-region: rect(24px 168px 48px 144px); > } That's more or less equivalent to this: #history-button:hover, #history-button[checked="true"] { -moz-image-region: rect(24px 168px 48px 144px); } So please remove :not([checked="true"]) here and elsewhere. >+#organizeButton, #viewMenu, #maintenanceButton { nit: line break after , Have you found the glitch in Toolbar-small.png? Looks good otherwise, I'll test this tomorrow and finish the review.

> Have you found the glitch in Toolbar-small.png? Good catch. I copied one row of pixels too many.

> So please remove :not([checked="true"]) here and elsewhere. Yeah I was a little unsure of that, it seemed pretty specific/intentional. Thanks Dao!

Comment on attachment 413506 [details] [diff] [review] Correct Hover States on Toolbar Icons - 05 fullscreen-button also needs [checked="true"], just like history-button.

> fullscreen-button also needs [checked="true"], just like history-button. Do I need a new review?

no new review needed, but I'm setting it anyway

Comment on attachment 413671 [details] [diff] [review] patch with binary file changes a192=beltzner

These bugs landed after b4 was cut. Moving flag out.