455524 - [Linux] location bar is higher than the search bar, star and rss icons aren't vertically centered

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P2)

Description

[mmc]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.9.0.1) Gecko/2008072310 Red Hat/3.0.1-3.el4 Firefox/3.0.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.9.0.1) Gecko/2008072310 Red Hat/3.0.1-3.el4 Firefox/3.0.1

The star and rss icons are not vertically centered, but they are closer to the top border. I can see it only in linux (windows is fine).

Reproducible: Always

Steps to Reproduce:
Have a look at the attached screenshot.

Comment 1

[mmc]

Attachment: screenshot: comparison of current bar (top) and proposed (bottom)

The screenshot shows that the icons are not vertically centered (and also the star hits the right border). Ideally they should be moved 1px down and 1px to the left.

Comment 2

[Justin Dolske [:Dolske]]

Hmm. I think part of the problem here is that while the feed and star icons are 16x16 images, the bottom row of pixels is blank in both images. [The feed icon also only uses 15 pixels of width (left column is blank). The star icon is a full 16 pixels wide.] The go button is positioned correctly, but it's a 13x13 pixel image with CSS padding applied.

Can we just change the images, so their contents are 1 pixel lower? Not sure if these images are used elsewhere, or if we're trying to be consistent in how icons are padded within the image. [The alternative is to use CSS to move the existing icons into place.]

Alex?

Comment 3

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

>Can we just change the images, so their contents are 1 pixel lower? Not sure if
>these images are used elsewhere

Sure, star and go shouldn't appear anywhere else.  The Web feed icon is also specific to the location bar (this is just the glyph version).

Comment 4

[David Dahl :ddahl]

I found the star png ( browser/themes/gnomestripe/browser/places/starPage.png ) and would like to attempt to replace it. Is there anything I should be aware of when working on something like this? Can I just open it in the Gimp and edit it?

Comment 5

[Daniel Holbert [:dholbert]]

FWIW, you also want to hit these icons:
  browser/themes/gnomestripe/browser/page-livemarks.png
  browser/themes/gnomestripe/browser/places/pageStarred.png

(Not being familiar with UI stuff, I don't know the answer to your other questions -- sorry)

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
> I found the star png ( browser/themes/gnomestripe/browser/places/starPage.png )
> and would like to attempt to replace it. Is there anything I should be aware of
> when working on something like this? Can I just open it in the Gimp and edit
> it?
You'll want to talk to faaborg before you do anything for sure!

Comment 7

[David Dahl :ddahl]

Attachment: fixed star and livemarks in urlbar

Here is the new star image and livemarks in mage in the url bar. they look pretty good.

Comment 8

[David Dahl :ddahl]

Attachment: Now, the livemarks image is just a little off of middle, however, it is v-aligned

So the livemarks image is now 1 px off in the select menu when subscribing to a feed. This image is vertical-aligned, and looked a pixel off before I changed the image. I adjusted the css by 2px to align it properly, screenshot is coming next...

Comment 9

[David Dahl :ddahl]

Attachment: tweaked css margin-bottom: 2px;

Comment 10

[David Dahl :ddahl]

Comment on attachment 356610 [details]
tweaked css margin-bottom: 2px;

added an id to this node and did the margin-bottom:2px; Not sure if this is the preferred way of adjusting the vertical alignment?

Comment 11

[David Dahl :ddahl]

Looks like the image is actually an image attribute of the menu itself:

<xul:menuitem id="liveBookmarksMenuItem" label="&feedLiveBookmarks;" class="menuitem-iconic" image="chrome://browser/skin/page-livemarks.png" selected="true"/>

I assume there is no way to affect the style of this image being an attr? Should I create another image to substitute here?

Comment 12

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

When editing images be careful not to add a color profile (or re-encode the image with a new profile changing the appearance), otherwise it looks like this is a simple modification of just moving the icons down a pixel.

Comment 13

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

Comment on attachment 356600 [details]
fixed star and livemarks in urlbar

we need 3 or 4 pixels of padding on the right side of the star, currently it is touching the end of the text field.

Comment 14

[David Dahl :ddahl]

Attachment: added padding to the gnomestripe theme for #star-button

I have also de-iCCP, sRGB etc the png files. I will prepare a patch if this looks good.

Comment 15

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

Attachment: 2 Pixels of padding vs. 3

Let's go for 3 pixels of space on the right of the star to match the top and bottom

Comment 16

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

Comment on attachment 356816 [details]
added padding to the gnomestripe theme for #star-button

details in (kind of OCD) comment #15

Comment 17

[David Dahl :ddahl]

Attachment: 3 px of padding

Ok. I got it. Btw: how did you produce that screenshot with pixels? On your virtual machine or native linux app? Btw: the windows css shows a single image for both bookmarked and not bookmarked with region shifting during hover. Should I be emulating that technique for linux or is that a Windows behavior only?

Comment 18

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

>how did you produce that screenshot with pixels? On your
>virtual machine or native linux app?

that was just the screen shot you attached in photoshop with a grid being displayed.

>the windows css shows a single image
>for both bookmarked and not bookmarked with region shifting during hover.

We do that as a hack so that the entire image loads, and there isn't a brief flicker as the state changes (normal, hover, hit).

Comment 19

[David Dahl :ddahl]

Attachment: css and png binary changes for linux theme

Submitting the patch to the icons and css.

Comment 20

[Dão Gottwald [:dao]]

Attachment: screenshot with patch

This doesn't look right to me, in terms of vertical alignment and horizontal padding.

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 357382 [details] [diff] [review]
css and png binary changes for linux theme

See previous comment. Also note that the patch doesn't take right-to-left chrome direction into account:

> /* Star button */
> #star-button {
>-  padding: 1px;
>+  padding: 1px 4px 1px 1px;

should be:

  padding: 1px;
  -moz-padding-end: 4px;

Comment 22

[Dão Gottwald [:dao]]

Attachment: screenshot without patch

Note that the star doesn't touch the right border.

Comment 23

[David Dahl :ddahl]

Aha! Nice. Looks like I need to learn a bit more about the -moz css attrs. Thx. Attaching a new patch.

Comment 24

[David Dahl :ddahl]

Attachment: new patch with Dao's guidance

Comment 25

[Dão Gottwald [:dao]]

(In reply to comment #24)
> Created an attachment (id=357460) [details]
> new patch with Dao's guidance

Note that -moz-padding-end instead of padding-right only makes a difference in the right-to-left case. So this still leaves us with attachment 357457 [details], which I think isn't the desired look.

Comment 26

[David Dahl :ddahl]

Attachment: Screenshot with current patch ubuntu 8.10/gnome

A screenshot from my build running on Ubuntu 8.10/Gnome, I think it is centered better.

Comment 27

[Dão Gottwald [:dao]]

Comment on attachment 357460 [details] [diff] [review]
new patch with Dao's guidance

I still see the vertical misalignment (attachment 357457 [details]). As for the horizontal padding, I don't think we should take attachment 338877 [details] as a reference -- that's not what Firefox usually looks like on Linux.

Comment 28

[David Dahl :ddahl]

Firefox can vary GREATLY in how it looks based on distro, DPI, window manager and window settings. I was just running KDE 4.1 with some custom window and font settings. I switched back to gnome to be more "normal", but it is far from the "norm", I think.

Comment 29

[David Dahl :ddahl]

Attachment: comparison of ddahl and dao's vertical alignment

Here is a comparison of our vertical alignment. I'm not sure what to do here.

Comment 30

[Dão Gottwald [:dao]]

(In reply to comment #28)
> Firefox can vary GREATLY in how it looks based on distro, DPI, window manager
> and window settings. I was just running KDE 4.1 with some custom window and
> font settings. I switched back to gnome to be more "normal", but it is far from
> the "norm", I think.

Sure, but if we can't fix it for all settings, we need to prioritize. What I'm saying is that we shouldn't optimize for a random OS theme (attachment 357184 [details]) and worsen the look for one of the most common themes (attachment 357457 [details] -- I'm running Ubuntu 8.10 as well).

Comment 31

[Dão Gottwald [:dao]]

Comment on attachment 358272 [details]
comparison of ddahl and dao's vertical alignment

Pending response to comment 30...

David: Also note that your patch makes the star's alignment inconsistent with the Go button.

Comment 32

[Dão Gottwald [:dao]]

Alex: Please see your words from comment 15. With the current approach, I see us moving away from that spec.

Comment 33

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

>Alex: Please see your words from comment 15. With the current approach, I see
>us moving away from that spec.

I noticed additional whitespace to the right side of the star, but that didn't seem like a big enough deal to give a ui-r-, the most glarring problem was the vertical alignment.

Good catch with the alignment being inconsistent with the go button.

Comment 34

[Dão Gottwald [:dao]]

(In reply to comment #33)
> >Alex: Please see your words from comment 15. With the current approach, I see
> >us moving away from that spec.
> 
> I noticed additional whitespace to the right side of the star, but that didn't
> seem like a big enough deal to give a ui-r-, the most glarring problem was the
> vertical alignment.

Yeah, and the vertical alignment is still off for me (see attachment 357458 [details] (before) and attachment 357457 [details] (after)). So we're not fixing the original problem and creating new one, although a minor one.

Comment 35

[Dão Gottwald [:dao]]

The vertical alignment looks like a rounding / pixel-snapping issue to me.

As for the horizontal padding, we should probably just increase #urlbar-icons' -moz-padding-end to 3px, to make it consistent with bug 405210 comment 28.

Comment 36

[Dão Gottwald [:dao]]

Attachment: dao's patch

So on Linux, the massive dropmarker makes the Location bar 1px higher than the Search bar. Instead of hacking around that, I propose that we revise the dropmarker.

Comment 37

[Dão Gottwald [:dao]]

Attachment: before

Comment 38

[Dão Gottwald [:dao]]

Attachment: after

Comment 39

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

Comment on attachment 363921 [details]
after

We definetly need to fix the one pixel taller problem, also I think this looks more streamlined, even if it is slightly different than native.  Tango people should weigh as well if they really disagree.

Comment 40

[Michael Monreal [:micmon]]

I have some problems figuring out what this bug is about... for me the icons seem to be positioned correctly with whatever theme I try and also my url entry is not any taller than the search entry... What themes are you testing with?

Comment 41

[Dão Gottwald [:dao]]

I'm using the Ubuntu 8.10 default theme. (I forgot the exact name and I'm on XP now.) I also used a slight variation which is available by default and where I saw the same problem.

Comment 42

[Dão Gottwald [:dao]]

... and I've seen it on different computers.

Comment 43

[Dão Gottwald [:dao]]

I've just tried all the themes that are available by default on 8.10.

No problem with: Crux, Glider
Broken: Clearlooks, Darkroom, Dunst, Glanz, High Contrast, Human, Human Clearlooks

The given patch works for all of them, although the dropdown is a little faint with Darkroom.

Comment 44

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

So this is fixing bug 397466, and what's changed since bug 397466 comment 12 is that we no longer have the location bar yellow background for SSL, which means we don't need the wrapper hack. Is that right?

Comment 45

[Dão Gottwald [:dao]]

We didn't need the wrapper hack for the yellow background, which I was trying to say in bug 397466 :)
#autocomplete-security-wrapper was used for new hacks related to the dropmaker, which aren't needed anymore with this bug.

Comment 46

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

Comment on attachment 363919 [details] [diff] [review]
dao's patch

I assume you've tested RTL behavior and appearance.

Comment 47

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/ba6065ce59ac

Comment 48

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/91c1c6fd1851

Comment 50

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090505 Minefield/3.6a1pre ID:20090505031205

and

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b5pre) Gecko/20090505 Shiretoko/3.5b5pre ID:20090505031155

Comment 52

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

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.