415329 - large toolbar buttons are vertically misaligned

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Gabriele Best [:Gabri]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; it-IT; rv:1.9b3pre) Gecko/2008013104 Minefield/3.0b3pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; it-IT; rv:1.9b3pre) Gecko/2008013104 Minefield/3.0b3pre

As you can see there http://img523.imageshack.us/img523/2751/bugah0.png
new icons on toolbar are vertical-unaligned and border is incomplete.
Next, there is another little bug http://img441.imageshack.us/img441/2181/bugcs1.png

I hope you can fix those before beta3 release.
thank you.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Ria Klaassen (not reading all bugmail)]

What is your screen resolution?

The other issue you mentioned is Bug 414872.

Comment 2

[Gabriele Best [:Gabri]]

Mine is 1280x1024

Comment 3

[Ria Klaassen (not reading all bugmail)]

Attachment: reporter's screen shot

Comment 4

[mcdavis941 (sporadically reading bugmail)]

(In reply to comment #0)
> As you can see there http://img523.imageshack.us/img523/2751/bugah0.png
> new icons on toolbar are vertical-unaligned and border is incomplete.

Where did those black lines above and below the nav bar come from, in your screenshot?  Is that how Firefox normally looks on your system, or did you add those for illustration purposes?

Comment 5

[mcdavis941 (sporadically reading bugmail)]

Attachment: screenshot: alignment across toolbar buttons

The keyhole icons are using the same centerline as the other main toolbar buttons for vertical alignment.  If you look real close, you can see that there's a common centerline that goes through the exact centers of the keyhole arrows and of stop and refresh (where the edge of the glossy highlight is the center of the icon).

Comment 6

[Gabriele Best [:Gabri]]

Attachment: This patch fix navigation toolbar padding

Here the screen AFTER the patch:
http://img441.imageshack.us/img441/454/fix2uq0.png

Before the patch:
http://img145.imageshack.us/img145/3342/bugvv5.png

Comment 7

[Simon Bünzli]

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

If you look very closely at this screenshot, you'll notice that the toolbar is 35px high (the toolbar separator is a 2px gray and white line). A 32px button will thus always look slightly misaligned, although the visual impression is worsened by the fact that the white half of the horizontal toolbar separator is hardly discernible as such. We'll thus probably need a 31px keyhole (as moving the keyhole 1px up won't might not look as good).

(In reply to comment #6)
> Here the screen AFTER the patch:
> http://img441.imageshack.us/img441/454/fix2uq0.png

Now, it's everything else that is slightly misaligned. This is particularly visible with the bevel of the Refresh button and somewhat visible for the rest when you move a Windows Explorer window closer (e.g. with the vertical separators) which is what we exactly copy in appearance.

Comment 8

[Gabriele Best [:Gabri]]

Attachment: Patch v1.1

That one is a little better than the previous one.

Comment 9

[Simon Bünzli]

(In reply to comment #8)
> That one is a little better than the previous one.

It's still a hack and will still break once you move the keyhole button to the menu bar. Unless you've got good reason for the contrary (and note: we're currently as native as possible!), you should really work specifically on the keyhole and not on all the toolbars.

Comment 10

[Dão Gottwald [:dao]]

Well, if you mouse over a normal toolbar button, the bevel overlaps the toolbar's bottom border. That's not what Windows Explorer does.

Comment 11

[Simon Bünzli]

(In reply to comment #10)
For me, things are pixel-perfect, so: screenshot, please.

Comment 12

[Dão Gottwald [:dao]]

Attachment: screenshot

Comment 13

[Simon Bünzli]

(In reply to comment #12)
AFAICT (and that's not easy with that little context), the left button resides on the last toolbar while the right one does not - which happens to make a difference.

Comment 14

[Dão Gottwald [:dao]]

Right, Explorer shows the same glitch when I make that toolbar the last one.

Comment 15

[Simon Bünzli]

Alex: Have you considered making the keyhole only 31px high (cf. comment #7)?

Comment 16

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

Attachment: Everything is misaligned, not just the keyhole

>Have you considered making the keyhole only 31px high

This would solve centering the keyhole, but everything on our navigation toolbar has a default size that is even (icons at 24 pixels tall, location bar at 22 pixels tall).  If we have an odd pixel height for the toolbar as a whole, everything will be 1 pixel off center, not just the keyhole.

Comment 17

[Simon Bünzli]

(In reply to comment #16)
> everything will be 1 pixel off center, not just the keyhole.

True. OTOH that's what Explorer does as well (which is what we're aiming for to look as native as possible).

Comment 18

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

>OTOH that's what Explorer does as well (which is what we're aiming for to
>look as native as possible).

I think people will be more likely to notice that our toolbar icons are not vertically aligned then they are to notice the difference between Firefox and Explorer.

Comment 20

[Benjamin Xiao]

We shouldn't be copying Explorer's misalignments for the sake of a native feel. After all, most users associate native-ness through colors and shapes, not alignment.

Comment 21

[Simon Bünzli]

Looks like this isn't a native/non-native issue after all: For some reason we add a 1px padding above all large toolbar buttons but not below (see the URL). That explains why Explorer's toolbars are of even size while ours are odd. Guess we should correct that...

Comment 22

[Simon Bünzli]

Comment on attachment 302391 [details] [diff] [review]
Patch v1.1

Mrtb: Care to provide an updated patch (and then ask Gavin or Asaf for review)?

Comment 23

[Dão Gottwald [:dao]]

Attachment: remove the top padding + drive-by cleanup

nice finding, Simon.

Comment 24

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

Dão: while you are adjusting this, can you give us a 36 pixel tall toolbar so that they keyhole has 2 pixels of space above and below it, and also add 2 pixels of padding in between the window frame and the left side of the etch. 

Comment 25

[Simon Bünzli]

(In reply to comment #24)
> also add 2 pixels of padding in between the window frame and the left side
... without regressing bug 414842. ;-)

Comment 26

[Dão Gottwald [:dao]]

(In reply to comment #24)
> Dão: while you are adjusting this, can you give us a 36 pixel tall toolbar so
> that they keyhole has 2 pixels of space above and below it

That would be 40 px instead of 38 px, so 2 px more just for the keyhole. That seems wrong. We already have a problem with consuming too much vertical space, especially if both the personal toolbar and the tabbar are shown.
The forward button (without the keyhole's permanent outline!) looks already bigger than the normal icons. Can we put the keyhole on a diet?

Comment 27

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

Attachment: Keyhole alignment spec

>That would be 40 px instead of 38 px, so 2 px more just for the keyhole. ... Can >we put the keyhole on a diet?

Here is a clarification of the change I requested.  The toolbar height is 36 pixels (so in addition to the keyhole the location bar is also centered vertically).  The keyhole has a padding of 2 pixels above, below, and to the left of it, and the back button is now vertically aligned with the F in File, which I believe we tried to do in Firefox 2 as well.

Comment 28

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

>and the back button is now vertically aligned with the F in File

Never mind on that point, I just realized that it is currently aligned with the F in File, and when I quickly made the mockup I pushed everything two pixels in.  Giving the etch some padding is more important than lining up with the file menu though.

Comment 29

[Dão Gottwald [:dao]]

I'm not arguing against horizontal padding, I'm arguing against making the toolbar higher. If we weigh (A) a 1 pixel padding on the top and the bottom of the keyhole or (B) making the keyhole consume less space against making the toolbar 2 pixels higher, I think (A) or (B) wins as the less evil thing.

Comment 30

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

I really don't think users are going to care between a 36 pixel tall toolbar (proposed) and a 35 pixel tall toolbar (current).

Comment 31

[Dão Gottwald [:dao]]

No user will notice. However, it's something that adds to the overall picture.

For example, see Firefox 2 being featured here:
http://www.istartedsomething.com/20080303/internet-explorer-8-interface-not-fat/

Note that the 35 px (+ border) height comes from the bug that Simon discovered. It's 34 px with the attached patch.

Comment 32

[Dão Gottwald [:dao]]

(In reply to comment #26)
> The forward button (without the keyhole's permanent outline!) looks already
> bigger than the normal icons. Can we put the keyhole on a diet?

To stress this, I also think the forward button and the keyhole outline being higher than the location bar hurts the desired mirror effect.

Comment 33

[Dão Gottwald [:dao]]

Attachment: remove the top padding, fix the keyhole's padding, drive-by cleanup

Comment 34

[Asaf Romano (gone)]

Comment on attachment 307351 [details] [diff] [review]
remove the top padding, fix the keyhole's padding, drive-by cleanup

r=mano

Comment 35

[Reed Loden [:reed]]

Checking in browser/themes/gnomestripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/browser.css,v  <--  browser.css
new revision: 1.199; previous revision: 1.198
done
Checking in browser/themes/winstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v  <--  browser.css
new revision: 1.186; previous revision: 1.185
done