326872 - Make tab site icons (favicons) the same size as in the location bar

Status: RESOLVED FIXED

(Camino Graveyard :: Tabbed Browsing, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Right now the favicons in the tab bar are 1-2px smaller in each dimension than the ones in the location bar.  This leads to blurry favicons, exacerbating bug 310630, and also rendering some favicons that are legible at 16px (location bar) illegible in the tab (bug 310630 comment 18).

This will apparently require increasing the height of the tab bar by a pixel or so, but I think it would be a UE win.  

Sam's got some other tabs work tentively targeted at 1.2, so putting this there, too, to start.

Comment 1

[Desmond Elliott]

Not currently working on this, freeing up

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Mass un-setting milestone per 1.6 roadmap.

Filter on RemoveRedonkulousBuglist to remove bugspam.

Developers: if you have a patch in hand for one of these bugs, you may pull the bug back to 1.6 *at that point*.

Comment 3

[Christopher Henderson]

Attachment: One line fix

Quick fix that makes the tab icons 16x16 (thanks, Pixie.app). The vertical padding is razor-thin now, so I would value others' comments. If we want taller tabs, I can do that as well; it would involve increasing the tab bar height in BrowserWindow.nib. The tab item images (backgrounds and divider) will have to be modified too, or the drawRect method changed to tile the images vertically, though I think changing the images would be the better option.

Note: this doesn't fix Bug 310630, as the BBC favicon is now blurry in both the tab bar and location bar.

Comment 4

[Christopher Henderson]

Attachment: Two Line Fix

Modifies the bottom padding so that the distances between the tops of the icon and text label are the same as before the patch. It's only a one pixel difference, but people notice. The bigger icons are now right up against the bottom of the tab, so we may need to bump the tab height up a couple of pixels.

Comment 5

[Christopher Henderson]

Attachment: Before the fix

Screenshot of the current behaviour. Note the blurriness of the icons.

Comment 6

[Christopher Henderson]

Attachment: After the one line fix

No longer blurry, but the text is ever so slightly above centre.

Comment 7

[Christopher Henderson]

Attachment: After the two line fix

Icon not blurry, text nicely centred, but the icon is scraping the bottom of the tab. I can centre the text and have a gap below the icon by using the one line patch and adding the following line to TruncatingTextAndImageCell.mm:129

textRect.origin.y -= 1.0;

which is quite hacky.

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Neither of the second two screenshots (particularly the bg tabs) look as good as the original one in terms of the overall appearance of the tab widget; I think we really have to bite the bullet and increase the tab bar height if we want to fix this.

Comment 9

[Christopher Henderson]

The only difference between the original one and the one after the two line fix is a single pixel vertical gap below the icon. To make the padding the same as the original, we'd have to increase the tab height by one pixel in the nib and the tiffs. That's doable, but before I do that we should decide whether we want even taller tabs. Right now they are 22 px, and Firefox's are ~24 px. Safari's are even shorter than ours, but they don't have icons.

Comment 10

[Christopher Henderson]

Attachment: 24 pixel high tabs

Screenshot of what 24 pixel high tabs look like. Involved editing 6 tiffs and tweaking a few constants in the source code. I like it. Gives a bit more room for the favicons.

Comment 11

[Christopher Henderson]

Comment on attachment 346639 [details] [diff] [review]
Two Line Fix

Removing review, as the fix will probably involve more than these two lines.

Comment 12

[Christopher Henderson]

I should point out that having 24 pixel high tabs means that background tabs have the same layout as bookmark bar entries. By that I mean that there is a 2 pixel gap between the icon and the top and bottom of the tab, and the tab height is 20 pixels. The extra 4 pixels are in the beveled strip between the tab bar and browser view.

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

[12:45pm] pinkerton: i say ship it

Also from the meeting, a nib will need some changing here, so this needs to be in before b1.

Comment 14

[Christopher Henderson]

Attachment: Fix v1.0

Code part of the fix. Changes a couple of constants, and nudges the text label a bit so it looks nice and centred.

Full fix requires this patch, the new BrowserWindow.nib, and the 6 replacement tab tiffs.

Comment 15

[Christopher Henderson]

Attachment: BrowserWindow.nib

BrowserWindow.nib, modified with the latest IB 2.5 on 10.5. Only change is that the height of the tab bar view was increased to 24px.

Comment 16

[Christopher Henderson]

Attachment: Tab images

The new tab images, increased to 24px high.

Comment 17

[Jeff Dlouhy]

Attachment: Tab Bar Vs. Bookmark Bar Vs. Predator

Here is a screenshot that shows the 24px tab bar and the 22px high bookmark bar. I believe there is beauty in symmetry and the tab bar should stay at 22px and not enlarged to 24px.

Also, should the overflow tab arrows be adjusted to be vertically centered?

Comment 18

[Jeff Dlouhy]

Comment on attachment 349908 [details] [diff] [review]
Fix v1.0

I believe the overflow arrows need to be adjusted as well to be vertically centered for the new 24px height.

The code and images are fine for the most part. I addressed my other concerns in attachment 350270 [details].

r-

Comment 19

[Christopher Henderson]

Attachment: Fix v1.1

Vertically centres the overflow buttons.

The tabs are the same height as the bookmark bar; a 16x16 favicon has 2 pixels of 'whitespace' above and below it on both. The extra height comes from the divider between the tabs and the content view.

Personally, I see no reason to have the two bars the same height - they are functionally different and needn't be constrained by each other. I don't have a 22px high main toolbar, either.

> I believe there is beauty in symmetry

Some people don't even have a 22px bookmark bar (one with 2+ rows), so there is no symmetry for them.

Comment 20

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(We need to remember to file a bug against me on updating the tab theme docs for 2.0 when this lands.)

Comment 21

[Jeff Dlouhy]

Comment on attachment 351215 [details] [diff] [review]
Fix v1.1

I'll give in.

:-P

r=me.

Comment 22

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 351215 [details] [diff] [review]
Fix v1.1

sr=pink.

+  textRect.origin.y -= 1.0;

maybe a comment on why this is necessary is warranted?

Comment 23

[Christopher Henderson]

Attachment: Patch, for checkin

That line now has an explanatory comment.

Thanks for the sr, Mike.

Comment 24

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Landed on cvs trunk!

hendy, can you please do comment 20 (Product Site component, me as owner)?