Closed Bug 431627 Opened 13 years ago Closed 12 years ago

Toolbar icons should have consistent photoshop blending settings

Categories

(Firefox :: Theme, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3.7a1
Tracking Status
status1.9.2 --- beta5-fixed

People

(Reporter: 427384, Assigned: shorlander)

References

()

Details

(Keywords: polish, Whiteboard: [polish-easy] [polish-visual] [polish-p1])

Attachments

(4 files, 10 obsolete files)

99.69 KB, application/x-xpinstall
Details
155.60 KB, application/x-photoshop
Details
156.64 KB, application/x-zip-compressed
Details
225.68 KB, patch
dao
: review+
beltzner
: approval1.9.2+
Details | Diff | Splinter Review
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: unspecified → Trunk
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.
Duplicate of this bug: 433126
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.
Duplicate of this bug: 433894
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.
Summary: Some icons have darker state on hover → Some icons have darker state on hover (the lighter state is incorrect)
Assignee: nobody → faaborg
Keywords: polish
Whiteboard: [polish-easy] [polish-visual]
Given that this affects the main toolbar, I think it qualifies as high visibility.
Whiteboard: [polish-easy] [polish-visual] → [polish-easy] [polish-visual] [polish-high-visibility]
Summary: Some icons have darker state on hover (the lighter state is incorrect) → Toolbar icons should have darker state on hover (the lighter state is incorrect)
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
Whiteboard: [polish-easy] [polish-visual] [polish-high-visibility] → [polish-easy] [polish-visual] [polish-p1]
Flags: blocking-firefox3.6?
Stephen, can you add this to the list of icon work to fix up for Firefox 3.6?
Assignee: faaborg → stephen
Flags: wanted-firefox3.6+
Flags: blocking-firefox3.6?
Flags: blocking-firefox3.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.
Summary: Toolbar icons should have darker state on hover (the lighter state is incorrect) → Toolbar icons should have consistent photoshop blending settings
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).
Attached file Corrected Hover State (obsolete) —
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.
Attachment #406538 - Attachment is patch: true
Attachment #406538 - Attachment mime type: application/octet-stream → text/plain
Attachment #405319 - Flags: ui-review? → ui-review?(faaborg)
Attachment #406538 - Flags: review? → review?(dao)
Attachment #406538 - Flags: review?(dao) → review-
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.
Attachment #405319 - Flags: ui-review?(faaborg) → ui-review+
Use libraryToolbar.png instead of the individual files. Remove the individual icons.
Attachment #406538 - Attachment is obsolete: true
Attachment #412294 - Flags: review?
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.
Attachment #412294 - Flags: review?(dao)
(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.
Attached file Corrected Hover States - 04 (obsolete) —
Removed Duplicate Images from Toolbar*.png
Attachment #412292 - Attachment is obsolete: true
Merged the Active State with the Hover State for the Navigation Bar.
Attachment #412294 - Attachment is obsolete: true
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.
Attachment #413445 - Attachment is obsolete: true
> So please remove :not([checked="true"]) here and elsewhere.

Yeah I was a little unsure of that, it seemed pretty specific/intentional.

Thanks Dao!
Attachment #413473 - Attachment is obsolete: true
Attachment #413506 - Flags: review?(dao)
Attachment #413473 - Flags: review?(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.
Attachment #413506 - Flags: review?(dao) → review+
> fullscreen-button also needs [checked="true"], just like history-button.

Do I need a new review?
Attachment #413506 - Attachment is obsolete: true
no new review needed, but I'm setting it anyway
Attachment #413624 - Attachment is obsolete: true
Attachment #413671 - Flags: review+
Attachment #413671 - Flags: approval1.9.2?
Comment on attachment 413671 [details] [diff] [review]
patch with binary file changes

a192=beltzner
Attachment #413671 - Flags: approval1.9.2? → approval1.9.2+
http://hg.mozilla.org/mozilla-central/rev/a65dd3cd9634
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a1
These bugs landed after b4 was cut. Moving flag out.
You need to log in before you can comment on or make changes to this bug.