toolbar buttons should have a minimum width

RESOLVED FIXED

Status

()

Firefox
General
--
trivial
RESOLVED FIXED
14 years ago
12 years ago

People

(Reporter: itsayellow@gmail.com, Assigned: Kevin Gerich)

Tracking

({fixed-aviary1.0})

unspecified
x86
Windows 2000
fixed-aviary1.0
Points:
---
Bug Flags:
blocking-aviary1.0 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments, 6 obsolete attachments)

21.48 KB, text/plain
Details
564 bytes, patch
Details | Diff | Splinter Review
763 bytes, patch
Details | Diff | Splinter Review
23.19 KB, image/png
Details
52.38 KB, image/png
Details
26.87 KB, image/png
Details
426 bytes, patch
Details | Diff | Splinter Review
277.56 KB, application/octet-stream
Details
8.48 KB, image/jpeg
Details
22.37 KB, image/png
Details
4.26 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

14 years ago
User-Agent:       
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6) Gecko/20040120 Firebird/0.7+

I like using the default firebird theme in its 'full' state, with icons and text.

Another caveat:  I'm using "Large Fonts" on Windows2k.  So my fonts probably
take up some more pixels than the default screen fonts.  This is a very common
choice, especially for laptops with small screens and very high resolution.

What I notice is that because the word "Forward" is longer than any other word,
the "Forward" button is bigger (wider) than any other button.  For similar
reasons, the "Back" button is smaller than any other button.

The length of the word on the button doesn't seem like a good way to determine
their size, and it makes the UI look a little messy IMHO that all the buttons
have a different width based on the word length.  Also, the "Back" button to me
is one of the most important buttons I use, and it shouldn't be the smallest
from a functional point of view.

I would propose adding "min-width" tags to the default theme for the buttons.  I
did this myself, adding "min-width: 50px;" to skin\classic\browser\browser.css
for the blocks ".toolbarbutton-1" and ".toolbarbutton-menubutton-stack" blocks
at lines 150 and 162.  This makes icons+text look great to me, but it has the
side-effect of keeping the min-width even when you are viewing icons-only, which
I realize is a waste of space and undesirable.  I'm sure a more experienced
themer could get the same result, but make no minimum width for icons-only.

I'm not sure how font size is determined in the theme, but if it is variable
based on the OS screen fonts, I suppose that a min-width based on pixels might
be problematic.  If there was some way to base it on the width of the word
"Forward" that would be ideal.

Reproducible: Always
Steps to Reproduce:
1. Use default theme
2. Choose "icons+text" from "Customize..." for the theme
3. (maybe) use "Large Screen Fonts" in Windows2k

Actual Results:  
All major navigation toolbar buttons have different widths, based on the length
of their word label.

Expected Results:  
Standardized widths of navigation buttons.
-> arvid, for consideration
Assignee: blake → arvid
(Reporter)

Comment 2

14 years ago
I should mention that I'm also using the small icons.

Comment 3

14 years ago
Confirming for consideration
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: default firebird theme would look better if nav buttons had more standard widths (w/ text on buttons) → toolbar buttons should have a minimum width
(Reporter)

Comment 4

14 years ago
Created attachment 141789 [details]
\chrome\classic\skin\classic\browser\browser.css

Sorry this isn't a patch, I'm still a little new.
browser.css which correctly implements min-width only when text is showing on
toolbar buttons.
(Reporter)

Comment 5

14 years ago
I attached a version of browser.css (from \chrome\classic\skin\classic\browser)
which implements a minimum width only if text is showing on the toolbar buttons.

It uses 3.7em, which will scale with screen font, and is not quite as long as
'Forward' word length, but slightly longer than 'Reload' word length, so most
buttons except 'Forward' have a uniform width.

For 'icons' mode with no text showing, behavior should be the same as it used to be.
(Reporter)

Comment 6

14 years ago
Sorry, it appears that for some reason depressed buttons in the attachment from
comment #4 always have the minimum width, even when they are icons-only.  I
guess it's not quite right.

Comment 7

14 years ago
Created attachment 141984 [details] [diff] [review]
patch making toolbar buttons the same width in icons and text mode

pretty much the same approach as Matt's attempt, but used the width of the "New
Window" button as a reference, and added a tweak to make menu-buttons (back and
forward) work properly.

Comment 8

14 years ago
(In reply to comment #7)
> Created an attachment (id=141984)
> patch making toolbar buttons the same width in icons and text mode

hmm.  seems to be some slight rendering weirdness from this patch, in
combination with the patch for dropmarker hover states (bug 226274 comment #4).
probably pixel rounding errors (bug 63336).

sometimes the edge between menu buttons (forward, back, etc) and their
dropmarkers either gets an extra pixel or looses a pixel.  probably due to the
use of "em" units in this patch.  not sure if there's currently any way around it.

Comment 9

14 years ago
Created attachment 142024 [details] [diff] [review]
fixed patch from comment #7 to include the go button
Attachment #141984 - Attachment is obsolete: true

Comment 10

14 years ago
Created attachment 142652 [details] [diff] [review]
patch updated to work around pixel rounding errors

update of patch from comment #9.
makes all toolbar buttons, including the go button, a uniform width based on
the button with the widest label: "New Window."  basically like IE's "Show text
labels" mode.  now uses 'ex' values that round to even pixels as outlined in:
http://kb.mozillazine.org/index.phtml?title=Dev_:_Theme_Development_:_em_vs_ex
unlike the previous patches, this prevents pixel rounding errors on Windows
systems using default font sizes (which is probably the majority of users), and
probably most other combinations of OSs and font sizes.

Updated

14 years ago
Attachment #142024 - Attachment is obsolete: true

Comment 11

14 years ago
Created attachment 142653 [details] [diff] [review]
alternate of comment #10 patch includes uniform width for text mode

the patch in comment #10 only affects buttons in the full "Icons and Text"
mode.  this alternate patch includes a uniform button width for toolbar buttons
in "Text" only mode.  it also uses the same evenly rounding 'ex' unit technique
as that patch, for compatibility with the dropmarker hover states patch in bug
226274 comment #4.
(Reporter)

Comment 12

14 years ago
Awesome work on the theme!

Now, please pardon me for bringing up an annoying detail: :)

Should the minimum width be based on the "New Window" button?  This is a long
text string, and most people don't have it visible on their toolbar anyway.  I'm
wondering if it would be ok for a few long buttons to be wider than the other
buttons, while ensuring that MOST buttons are of uniform width.

Comment 13

14 years ago
i don't know.  that's a decision that might need to be made by someone "higher
up."  i chose the "New Window" button, because it it the biggest, and therefore
would cause all buttons to have the same minimum width.  this ensures a
consistent, uniform appearance.  but i understand your point - it is a bit wide
for many buttons.  but what width to use if not this one?  you might want to try
to get someone that makes judgements about the look and feel of the ui to check
out this bug.  the patch will be simple to update, we just need to figure out
what to change it to, if it needs to be changed at all.
(Reporter)

Comment 14

14 years ago
I use 6.8ex for my minimum widths: it's pretty tight, but still keeps buttons
like 'Back' and 'Stop' from turning into slivers.

Comment 15

14 years ago
Bug 60553 proposes a true under-the-hood fix.

Updated

14 years ago
Assignee: arvid → webmail
Flags: blocking-aviary1.0+

Comment 16

14 years ago
bug 60553 is basically calling for what IE does which is to dynamically size the
buttons to all be the same size and the width of the widest button. I think that
short of that (and we're probably not going to get that for 1.0) we need
something that gets most of the buttons to a uniform size. Bookmarks and New
Window can be a bit fatter and folks probably won't notice or mind. Right now, I
think if we can find a hack that doesn't make the buttons too wide for the
common case (the default toolbar configuration) while still accomodating
customization, that's what we should go with. 

Can one of you on windows XP (and in "classic" mode or on win2K if possible)
post screenshots of the latest patch with the default toolbar config and with a
"full" toolbar that contains all of the icons available in the customize pallet?
That would be a big help. 

Comment 17

14 years ago
Created attachment 158281 [details]
current Firefox state as compared to IE

Comment 18

14 years ago
Having all the buttons the same size (like IE) would make the toolbar look more
natural.
*** Bug 236961 has been marked as a duplicate of this bug. ***
Not a "blocker"
Flags: blocking-aviary1.0+ → blocking-aviary1.0-

Comment 21

14 years ago
Created attachment 161503 [details] [diff] [review]
Patch for browser, bookmarks manager and help

An up-to-date patch.  This patch only affects icons+text mode; no changes are
made to text-only mode or the default icons-only mode.

Min-width is set to 9.5ex for most toolbar buttons.  This makes the buttons
look just wider than square with the default Windows font (8pt Tahoma) and is
wide enough to cover everything in the browser except Downloads and Bookmarks,
which are *very* slightly wider, and New Window, which is noticably wider.  The
Go button is exempt since it's not a normal toolbar button, but I added some
padding on the right to make it seem less squashed up.

Almost all icons in the bookmarks manager are wider than 9.5ex anyway, but at
least the change does add some (IMO necessary) padding to Move and Delete. 
There's no check for icons+text mode here, since none of the other style rules
seem to have it and I couldn't find any way to select another mode in the
bookmarks manager.

The back and forward inner buttons have a slightly smaller min-width of 9ex,
since the additional dropmarker sections make them appear wider than the other
buttons anyway.  I think the effect balances out well.

I also took the liberty of copying over the toolbarbutton margin and padding
settings from browser.css to the other two files for consistency.  In
particular, the margin-right: 0 fix for toolbarbutton icons is pretty much a
necessity for icons+text mode, otherwise the icons look quite badly misaligned.


Who would be a good person to ask for review?

Comment 22

14 years ago
Created attachment 161504 [details]
Full set of browser buttons after patch

The full set of browser buttons with this patch applied, on an IE-like toolbar
configuration, under both Luna and Classic.

Comment 23

14 years ago
Created attachment 161506 [details]
Default toolbar config after patch (except changed to icons+text mode)

I didn't take a screenshot in the default icons-only mode since that mode is
unaffected.

Comment 24

14 years ago
(In reply to comment #21)
> Created an attachment (id=161503)
> Patch for browser, bookmarks manager and help

Looks pretty good.  But something needs to be done for text only mode as well. 
And it might be good to leave Go Button tweaks to bug 235277.

Some nitpicks:
Native inner menubuttons are the same width as normal ones - not including the
dropmarker, so in the context of this patch, they would be 9.5ex.  I assume you
made them 9ex to allow room for the dropmarkers, but in that case, why not just
leave the .toolbarbutton-menubutton-buttons alone, so the outer toolbarbuttons
are 9.5ex like all the rest?  For uniformity, either the inner menubutton, or
the whole outer button including the dropmarker should be the same as the rest.
 At the current value, neither is the case.

Is there some place more upstream that we could define this width so that it
doesn't have to be repeated for each app?  global/toolbarbutton.css or perhaps
global/browser.css.  Hmm, lemme play with this a little, i'm getting a few ideas...

Comment 25

14 years ago
Created attachment 161553 [details] [diff] [review]
patch: different approach, same effect

This patch does it all from a single selector in global/toolbarbutton.css.

-It's defined globably, so it filters down to browser, bookmark manager, help,
etc.
-Instead of setting a minimum outer-button width, it sets the width of the
label which sizes the button from the inside.  Regular toolbarbuttons and inner
.toolbarbutton-menubutton-buttons are the same width.
-It works for both Icons+Text and Text Only (though that could easily be
changed with a seperate rule using a different width for Text Only).
-It only affects children of toolbars, so, for example, toolbar bookmarks and
tab close aren't affected.
-It only affects toolbarbuttons with a "label" attribute.
-It doesn't affect Icons Only mode, since labels are display: none'd.

In my quick testing, it seems to play nice with the standard kit.  Extensions
and such might be a different story.  And it was a late-night quickie, so there
may be something obvious i'm overlooking.

Might not be the way we wanna go with this, but it's an option to consider.

Comment 26

14 years ago
(In reply to comment #25)
> Created an attachment (id=161553)
> patch: different approach, same effect

A side-effect - it doesn't hold during Customize, so the buttons shrink back to
their "real" size.  The selector breaks because toolbarbuttons are put in a
palette wrapper during customize.  They return to the fixed size after exiting
customize.  Probably fixable with more CSS selector tom-foolery.

Comment 27

14 years ago
(In reply to comment #26)
> A side-effect - it doesn't hold during Customize, so the buttons shrink back to
> their "real" size.  The selector breaks because toolbarbuttons are put in a
> palette wrapper during customize.  They return to the fixed size after exiting
> customize.  Probably fixable with more CSS selector tom-foolery.

You can get around that by using the descendant selector instead of the child
selector between the toolbar and toolbarbutton, or adding another identical rule
with an extra "> toolbarpaletteitem" before the toolbarbutton.  (If you use the
former approach, you can then simplify the second selector to a child instead of
a descendant).

I like the idea of just changing toolbarbutton.css better, but I couldn't
actually get the method you used to work on normal toolbarbuttons on Luna - DOMi
reports that the toolbarbutton text does indeed have the correct min-width of
9ex, but its computed width was still the same value as you get from
shrinkwrapping the button.  What it DID affect was the find toolbar, and it did
so most strangely - hovering the buttons showed that the icon was actually
pushed to the left, slightly outside of the hover area.

Switching to Windows Classic, this does actually seem to work - it does
improperly widen the find toolbar buttons still, but the toolbar buttons get
their minimum width and the find toolbar icons are within the hover area. 
Perhaps a bug in Windows native theming.  The toolbar buttons look wider than
with my patch, too - presumably because there's padding etc. on elements
containing the text - so perhaps a smaller value would be wise if the text is
going to have min-width rather than the button.

I think if this global approach is going to be used, it does need to check that
the toolbar is [mode="full"] or [mode="text"] and probably set the width on the
button rather than the text, since giving the text min-width seems buggy at
best.  Going back to using .toolbarbutton-1 and .toolbarbutton-menu-button would
probably work.

I'd be happy to just make another version of my own patch with 9.5ex for
.toolbarbutton-menubutton-button if that's generally preferred.  I realise the
standard is to make the inner button the same width as other buttons, but the
back button just seemed a bit too fat that way.  Perhaps it wouldn't on Windows
Classic where there's no visible hover state on the dropmarker (though that's a
bug in itself).  As for just setting the outer button min-width and ignoring
.toolbarbutton-menubutton-button, I found that this caused the inner button to
shrinkwrap and, hence, the back button ended up with a much larger dropmarker
(to fill the remaining space) than the forward button, which looks very strange
indeed (again probably less so on Windows Classic, with its lack of dropmarker
hover).

Comment 28

14 years ago
Created attachment 161564 [details]
Standalone installable Winstripe with my original changes applied

Forgot to attach this yesterday.  This is my test version of Winstripe,
modified into a separately installable theme so changes are easily comparable
with the original.

This includes the patch from attachment 161503 [details] [diff] [review], and is probably the fastest way
to test the changes out - just save it and drag it onto the themes window.

Comment 29

14 years ago
(In reply to comment #27)
> Going back to using .toolbarbutton-1 and .toolbarbutton-menu-button would
> probably work.

That's exactly why it won't work - the Bookmark Manager buttons won't be covered
by that.  And making a seperate rules just for BM kinda defeats the purpose of
this way of doing it.

I'll keep tweaking on it, and see if i can get something more solid.  I've
already worked around the customize problem, but there's still a few more things
to straighten out.  I did overlook the Find Toolbar on the first try, and i'm
not easily able to test on XP/Luna either.  If it's not gonna work out, we'll
just go with the regular methods we started with.

Comment 30

14 years ago
Created attachment 162107 [details] [diff] [review]
Improved patch

This uses the same method as my previous patch, since I wasn't personally able
to find a way to make the combined method suggested by miahz work.  It fixes a
few problems with the original:

+ Should actually apply cleanly
+ The dropmarkers on the back and forward buttons in Help were being pushed to
the right, because margin-right: 2px was incorrectly applied to the inner
button.  (This is how it's applied in browser.css, but it's later overriden by
an identical selector.)
+ Small icons + text mode wasn't affected.  Fixed by removing a redundant
second definition of min-width: 0px for small icons mode (it's defined above
for all modes, and then overriden for icons+text)
+ I caved in and used 9.5ex for inner buttons as well as everything else. 
There's only a 3px size growth compared to my original screenshots, so they're
still pretty valid for comparison purposes.

I didn't include text-only mode since, after testing it a bit, I personally
feel it's better as it is.  Essentially, I think these are the reasons
min-width is needed for icons+text:

a) Toolbar icons that are taller than they are wide look strange and have an
uncomfortable hit area
b) Icon spacing is visibly quite uneven otherwise

Neither of these really apply to text-only mode.  The toolbar buttons in
text-only mode seem more akin to menubar buttons, which certainly don't have a
minimum width, and IMO would feel pretty weird if they did.  And after all, the
initial description only really makes mention of icons+text mode.
Attachment #161503 - Attachment is obsolete: true
Attachment #161564 - Attachment is obsolete: true

Updated

14 years ago
Attachment #161503 - Flags: review?(bugs)

Updated

14 years ago
Attachment #162107 - Flags: review?(bugs)

Comment 31

14 years ago
Created attachment 162108 [details]
Installable Winstripe with above patch applied

Updated installable Winstripe for quick testing/comparison.
(Assignee)

Comment 32

14 years ago
Neil, I'm looking at your latest patch. It looks good, except for the Go button
which grows in size when you click on it and pushes the URLbar to the right.
Please fix that and I'd be glad to land this tonight. Anyone have a comment on
Neil's patch?
(Assignee)

Comment 33

14 years ago
Created attachment 162223 [details]
home button with 1 pixel on the right cut off

One little thing I noticed is that whatever button is immediately to the left
of the URLbar has one pixel shaved off when you mouse over it. See screen shot.
Any ideas?

The Go Button problem is unrelated to the patch. But please incorporate this
change:

#go-button {
  padding: 2px 3px;
}

#go-button:hover:active {
  padding: 3px 2px 1px 4px;
}

Comment 34

14 years ago
I haven't been able to reproduce the 1px cutoff issue so far on Luna or Classic.
 Perhaps it's triggered by some specific aspect of that toolbar configuration. 
Does it happen with the default-like config from attachment 161506 [details]?  If not, a
screenshot of the whole toolbar config might help me investigate.  If so, I'm
rather stumped.  Is it definitely a consequence of this patch?  What happens if
you put a button like New Window there, which is wider than the given min-width
anyway?  If that's still cut off, I just have no idea what added rule could be
causing it.

I played with the Go button a little, and in the spirit of the patch
unsquishifying the labels, I'd like to suggest following alternative defs:

#go-button {
  padding: 2px 5px 2px 3px;
}

#go-button:hover:active {
  padding: 3px 4px 1px 4px;
}

i.e. an extra 2px on the right.  I've verified that this does still prevent the
button from growing in its pressed state; it also looks more balanced, IMO,
since the button already has some padding/margin on its left but the text label
hasn't got any noticable amount on its right.  If you'd still rather stick with
the definitions you gave before, though, let me know and I'll include those in
the next patch version instead.
(Assignee)

Comment 35

14 years ago
Strange. I can reproduce the clipped icon border effect .. only on certain icons
like New Tab, Home, Cut, Copy, Stop regardless of where they are in the toolbar.
I did this with a fresh profile. However I was not able to reproduce it on
another machine. I'll do a test on another machine when I get home in an hour or
two.

Let's go with your padding values.
(Assignee)

Comment 36

14 years ago
Created attachment 162231 [details]
clipped icon in classic, new profile

Comment 37

14 years ago
I wonder if this could have anything to do with ex units and rounding errors at
certain font sizes.  It seems an odd thing to be caused by it, but perhaps next
time you are (or anyone else here who tests and happens to experience this is)
on a machine/profile where the problem is occuring, it might be worth changing
the min-width to a pixel value just to see if it helps matters.

Comment 38

14 years ago
(In reply to comment #37)
> I wonder if this could have anything to do with ex units and rounding errors at
> certain font sizes.

I bet that's exactly what it is.  I'll take a look at your newest patch and see
i can tweak these last few things out.

Comment 39

14 years ago
Created attachment 162244 [details] [diff] [review]
Updated patch

Same as previous, except:

+ Made against current file revisions
+ Go button fixed
+ Don't add margin-right: 2px to full/outer buttons in help/bookmarks, per
Stephen Horlander's recent checkin that removed the def from browser.css.

Still unable to reproduce the clipping issue here - miahz, this patch is a
better starting point to work from if you want to take a look.

The line endings seem strange in the current browser.css (and in some recent
PNG checkins, which I ended up hex editing to fix locally, but that's another
issue).  If the browser part of the patch looks strange, that's why.
Attachment #162107 - Attachment is obsolete: true

Updated

14 years ago
Attachment #162107 - Flags: review?(bugs)
(Assignee)

Comment 40

14 years ago
We only have a few hours until the tree is closed for RC1. Miazh, are you seeing
the same clipping that I am?

Comment 41

14 years ago
If you want a stopgap for RC1, we could use 57px for now, which is the computed
value of 9.5ex at Windows' default 8pt font size.  It's just as effective as
9.5ex for the default case, and while it's not as useful for larger font sizes,
it doesn't make matters any worse.

That's assuming that ex units are indeed the problem - has anyone been able to
confirm that a px value solves it?  I've been trying really hard to reproduce it
here so that I can test, but I guess I'm just really (un)lucky.
(Assignee)

Comment 42

14 years ago
I reproduced the clipping on my home machine. It wasn't apparent in the default
toolbar but I saw it I put a few other buttons on there. I put this in my
useChrome.css and it doesn't seem to be clipping.

toolbar[mode="full"] .toolbarbutton-1,
toolbar[mode="full"] .toolbarbutton-menubutton-button {
  min-width: 57px !important;
}

Oh and about the line endings in browser.css, I think I screwed them up when I
did a check in last night :/

Comment 43

14 years ago
Created attachment 162255 [details] [diff] [review]
57px version to avoid clipping

I suspect this is the best we can get before RC1.
Attachment #162244 - Attachment is obsolete: true
(Assignee)

Comment 44

14 years ago
checked in on branch! thanks Neil!
Keywords: fixed-aviary1.0

Comment 45

14 years ago
Sorry guys - didn't know we were looking at a tight deadline.  And though i
couldn't reproduce the visual rounding glitch, i can confirm that the computed
widths were fractional at non-8-sized fonts.  Using whole 9 or 10ex widths
instead of 9.5ex seems to circumvent this in all the font sizes i tried.  We'll
get that in the next round i guess.

Comment 46

14 years ago
I wondered if whole ex units might help, but I didn't want to risk it with only
a few hours to test, in case it caused problems in some scenario we couldn't
catch.  Definitely worth a look in the long term.

Comment 47

13 years ago
Fixed on trunk by the branch landing.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Comment 48

13 years ago
Text only mode was not fixed, if i'm not mistaken.
You need to log in before you can comment on or make changes to this bug.