423806 - Keyhole (combined back forward) for small icons mode on windows

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Toolbar-small.png

Actually, this doesn't look anything like a keyhole, but I guess the name has stuck.  We need to update the windows theme for combined back/forward navigation controls with a unified history menu.  I'll attach the new toolbar-small.png and toolbar-small-aero.png files.

We want to get this in for beta 5, so we are unfortunately going to need a really fast turn around time.

Comment 1

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Toolbar-small-aero.png

Comment 2

[Alex Faaborg [:faaborg] (Firefox UX)]

Note that the etch in these images is visually a little messed up, but the dimensions will stay the same as these files, so you can use them for creating the patch.

Comment 3

[mcdavis941 (sporadically reading bugmail)]

(In reply to comment #0)
> Created an attachment (id=310416) [details]
> Toolbar-small.png
> 
> Actually, this doesn't look anything like a keyhole, but I guess the name has
> stuck.  We need to update the windows theme for combined back/forward
> navigation controls with a unified history menu.  I'll attach the new
> toolbar-small.png and toolbar-small-aero.png files.
> 
> We want to get this in for beta 5, so we are unfortunately going to need a
> really fast turn around time.
> 

I can take this.  I'll wrap up some other stuff I have going on and get started in about an hour.

Comment 4

[mcdavis941 (sporadically reading bugmail)]

Alex, could you tell me what's your intention for the blank space on the left side of the image files?  Will there be another version of the back and forward arrows in that spot, paralleling the layout of Toolbar.png, for use when mode is icons+text?

Comment 5

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Toolbar-small-aero.png

>Alex, could you tell me what's your intention for the blank space on the left
>side of the image files?  Will there be another version of the back and forward
>arrows in that spot, paralleling the layout of Toolbar.png, for use when mode
>is icons+text?

Ok, I forgot this mode, but seriously, who uses small icons + text anyway, that doesn't make any sense :p  Updated toolbar with 16x16 nav controls, the XP toolbar will be using aero icons until we can get some luna ones produced.

Comment 6

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Toolbar-small.png

Here is the luna one so that you can land these along with your patch.

Comment 7

[mcdavis941 (sporadically reading bugmail)]

(In reply to comment #5)
> but seriously, who uses small icons + text anyway, that
> doesn't make any sense :p  

Not me.  Small toolbar buttons with text labels are still gigantic.

Comment 8

[mcdavis941 (sporadically reading bugmail)]

Attachment: browser.css changes v1

Comment 9

[Simon Bünzli]

(In reply to comment #6)
> Created an attachment (id=310678) [details]

Shouldn't the back/forward glyphs be green for Luna, so that the action of navigating has a color association, e.g. between the toolbar and the unified back/forward popup's hover icon (or wherever else the non-unified glyphs are used)?

Comment 10

[mcdavis941 (sporadically reading bugmail)]

(In reply to comment #9)
> (In reply to comment #6)
> > Created an attachment (id=310678) [details] [details]
> 
> Shouldn't the back/forward glyphs be green for Luna, so that the action of
> navigating has a color association, e.g. between the toolbar and the unified
> back/forward popup's hover icon (or wherever else the non-unified glyphs are
> used)?

Good point but I thought he was saying those are just stand-ins until the Luna version is produced.

Comment 11

[Alex Faaborg [:faaborg] (Firefox UX)]

>Shouldn't the back/forward glyphs be green for Luna

Yeah, I just had to copy over the aero ones for now since we don't have any luna source material.  This will be corrected in a nightly after beta 5.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 310699 [details] [diff] [review]
browser.css changes v1

>Index: mozilla/browser/themes/winstripe/browser/browser.css


>-toolbar[iconsize="large"][mode="icons"] #back-button:not([disabled="true"]):active {
>+toolbar[iconsize="large"][mode="icons"] #back-button:not([disabled="true"]):hover:active {

What are all these changes for? They look unrelated.

http://gavinsharp.com/tmp/skey.png is the intended look, right? Wasn't sure whether they were supposed to be touching.

Comment 13

[mcdavis941 (sporadically reading bugmail)]

Attachment: sample showing -moz-image-regions

(In reply to comment #12)
> http://gavinsharp.com/tmp/skey.png is the intended look, right? Wasn't sure
> whether they were supposed to be touching.

Just to make sure I'm going for the right image regions, here's what I'm using.  The purple border is included in the displayed image region.

Comment 14

[mcdavis941 (sporadically reading bugmail)]

(In reply to comment #13)
> > http://gavinsharp.com/tmp/skey.png is the intended look, right?

If that's to me, then yes.  That is what I meant to do based on what I think Alex wanted.

Alex, is this right?

Comment 15

[mcdavis941 (sporadically reading bugmail)]

(In reply to comment #12)
>> >-toolbar[iconsize="large"][mode="icons"] #back-button:not([disabled="true"]):active {
> >+toolbar[iconsize="large"][mode="icons"] #back-button:not([disabled="true"]):hover:active {
> 
> What are all these changes for? They look unrelated.
> 

This is to make the active-state behavior and appearance consistent with the other toolbar buttons in the case when you click and hold on a button but then drag off before releasing.  (For anybody who hasn't seen this, click on the refresh button but don't release, and keeping the mouse down, roll off the refresh button and over the content area before releasing.  You'll see it only shows active state appearance when the mouse is actually over the button.) I had to add new rules for the small unified case, then took care of the large unified case at the same time so everybody's consistent. 

I'll file a separate bug for that if you want.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

adding in-litmus? - attachment 310737 [details] shows the appropriate -moz-regioning. Should work for inactive, active, hover and depressed states.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 310699 [details] [diff] [review]
browser.css changes v1

a=beltzner

Comment 18

[Alex Faaborg [:faaborg] (Firefox UX)]

>If that's to me, then yes.  That is what I meant to do based on what I think
>Alex wanted.

>Alex, is this right?

Sorry about the lag, looks great.

Comment 19

[Reed Loden [:reed]]

Checking in browser/themes/winstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v  <--  browser.css
new revision: 1.189; previous revision: 1.188
done

Comment 20

[Biju]

current backward/forward button (attachment 310678 [details]) is too big (24px) than other small (16px) toolbar buttons.
The user already switched from default big button because s/he wanted a small one.

Comment 21

[Dão Gottwald [:dao]]

Well, I think it should be 22px high, like the location bar. The top border looks odd, so that's the first thing that we should remove.

Comment 22

[Tony Chung [:tchung]]

Verified fix om Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008041004 Minefield/3.0pre

Comment 23

[Tony Chung [:tchung]]

errr... i mean Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008041005 Minefield/3.0pre

Comment 24

[Jesse Ruderman]

I think Biju filed bug 429062 about his complaint in comment 21.

Comment 25

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=5273 added to Litmus.