422559 - Toolbar alignment issues on Windows

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

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

This is a bug to track various padding and allignment issues with the main window of the Windows theme.

Comment 1

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

Attachment: Keyhole alignment with different windows themes

This shows how the keyhole is currently aligned with the three windows XP themes (olive is the same as blue).

Comment 2

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

Attachment: Toolbar padding suggestion

I think ideally the padding on the left side of the keyhole should match the
padding above and below the keyhole.  On the right side of the browser the
padding to the right of the search bar should match the padding above and below
the search bar.  This is because it is visually very easy to compare nearby
padding and difficult to compare the padding on the opposite sides of the
browser window.  I would also make sure the padding between the location bar
and search bar is the same the same as the distance between the search bar and
the right side of the window, which is also the same as the padding above and
below it.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Comparision between different sets of toolbar buttons

Happens for each theme not only Aero.

Alex, your ideas are good but it misses one point. The padding visually also depends on which buttons are placed to the left and right side of an element. I attached a collection of various possibilities. If you have another toolbar button the distance between both elements looks fine. Further there is a difference between big and small icons. So the general idea to have the same left/right padding as the top/bottom padding doesn't work well for elements which doesn't change their size between these two modes, e.g. the location or search bar. Even the padding between the location bar and search bar doesn't fit the right padding to the window border.

Having a check to the Navigation Toolbar under OS X, I have to say that this one looks much better as the current style under Windows. Perhaps this is a subjective feeling, but for me it's not a clean way.

Comment 4

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

>The padding visually also depends on which buttons are placed to the left and
>right side of an element.

I personally think we should set up the padding to look best for the default case (large icons, no modifications to the icon order).

Comment 5

[Dão Gottwald [:dao]]

As I wrote in bug 415329 comment 29 and the surrounding comments, I think we shouldn't enlarge the toolbar but fix the keyhole.

Specifically, I don't think I see the point of the keyhole's strong background bevel. If we can make that lighter and smaller (more like the location bar, which is expected to mirror the curve), we could easily reduce the radius of the back button by one pixel, and it would still stand out.

With regards to the relation between horizontal and vertical padding, I don't think it needs to be 1:1. Often things looks more harmonic if the horizontal padding is slightly bigger. But in the special case of the keyhole, the relation doesn't fully work anyway due to the lack of straight borders.

Comment 6

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

Here are the specific alignment changes I would like:

-36 pixel tall navigation toolbar
-2 pixels of padding above, below and to the left of the keyhole
-7 pixels of space above and below a 22 pixel tall navigation bar and search bar
-7 pixels in between the location bar and search bar
-7 pixels of padding from the right end of the location bar to the window frame

Comment 7

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

I think Alex has the right approach in comment 6, based on what I've seen in the bug. The OSX themers delivered a bunch of different ratios for the back/forward button, and we found that as you reduce it in size the difference ends up looking less intentional and meaningful and more like a mistake.

Dao: can we put together a patch that implements the recommendations in comment 6, get it in, and see if it needs further tweaking? The quicker we can do that the quicker we can turn around a modification to the back button if it proves necessary.

(As a sidenote, generally speaking I trust Alex to have agonized over these changes and considered all feedback, and to have reviewed things with me before making strong recommendations as he's done in comment 6)

Comment 8

[Dão Gottwald [:dao]]

I haven't seen anybody responding to this in the other bug, so I'll repeat:

We're not in a good shape when it comes to giving the web content as much space as possible. As I've already agreed with Alex, there won't be serious complaints about making the navigation toolbar 2 px higher, because hardly anyone will notice (hence "see if it needs further tweaking" doesn't apply.) But it does mean that we'll look slightly more bloated in comparisons such as <http://www.istartedsomething.com/20080303/internet-explorer-8-interface-not-fat/>, and rightfully so. If anything, we should try to win a pixel here and there.

The interesting question seems to be whether or not it's possible to make the keyhole slightly less enormous. I don't know the answer to that, but I think it should be possible. I'll try to make mockup based on the current keyhole on XP, but regardless of whether that'll will fail or succeed, it would be nice if those who are actually working on the Windows icons could give it a serious shot.

Comment 9

[Dão Gottwald [:dao]]

And of course I won't refuse to write the requested patch if the keyhole remains this tall, but not exploring the other options would be a mistake.

Comment 10

[Dão Gottwald [:dao]]

Attachment: current layout for reference

Comment 11

[Dão Gottwald [:dao]]

Attachment: option 1: remove horizontal rules as discussed for Vista

If there's no border, the keyhole can't touch it ...
And this would be a win in the vertical space rather than a regression.

I'll try to follow up with a mockup of a smaller keyhole, but that requires more time.

Comment 12

[Dão Gottwald [:dao]]

Attachment: option 2: smaller keyhole

Comment 13

[David Weitz]

I just made a bug report, but should it be marked as duplicate. It's bug 425044.

Comment 14

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

(In reply to comment #9)
> And of course I won't refuse to write the requested patch if the keyhole
> remains this tall, but not exploring the other options would be a mistake.

It's remaining that tall - can you write the patch? :)

Comment 15

[Dão Gottwald [:dao]]

No. My objections / proposals remain entirely unanswered, which was of course not the idea behind my cited statement. I'd rather fix the keyhole icon on my own (option 2). Note that bug 420236 fixes the vertical alignment for Vista by doing option 1.

Comment 16

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

(In reply to comment #8)
> The interesting question seems to be whether or not it's possible to make the
> keyhole slightly less enormous. I don't know the answer to that, but I think it

We examined many keyhole sizes, and the design decision was made a long time ago. We've since been working with the icon designers to get all of the various states made up, and are not revisiting this decision now.

As mentioned in comment 7, making the keyhole smaller ends up reducing the visual prominence that we have explicitly tried to achieve.

The extra pixel we'd gain for the content area here, in my opinion, isn't worth reducing the size of the keyhole at this time. More content is easily available by choosing "small icons".

(In reply to comment #15)
> No. My objections / proposals remain entirely unanswered, which was of course

They are now answered. :)

> not the idea behind my cited statement. I'd rather fix the keyhole icon on my
> own (option 2). Note that bug 420236 fixes the vertical alignment for Vista by
> doing option 1.

It calls less attention to it, but the alignment is still off if you count pixels.

If you won't write the patch, please let me know, and I'll do it.

Comment 17

[Dão Gottwald [:dao]]

(In reply to comment #16)
> We examined many keyhole sizes, and the design decision was made a long time
> ago. We've since been working with the icon designers to get all of the various
> states made up, and are not revisiting this decision now.
> 
> As mentioned in comment 7, making the keyhole smaller ends up reducing the
> visual prominence that we have explicitly tried to achieve.

Comment 7 refers to OSX. If I remember correctly, the OSX keyhole was reduced after that since it made the toolbar too high.

Anyway, I tried to address that with attachment 309625 [details]. It doesn't look "less intentional and meaningful and more like a mistake" to me, but nobody else seems to have look at it.

> The extra pixel we'd gain for the content area here, in my opinion, isn't worth
> reducing the size of the keyhole at this time. More content is easily available
> by choosing "small icons".

The existence of small icons doesn't justify making the big icons, the default setup, bigger.

> > not the idea behind my cited statement. I'd rather fix the keyhole icon on my
> > own (option 2). Note that bug 420236 fixes the vertical alignment for Vista by
> > doing option 1.
> 
> It calls less attention to it, but the alignment is still off if you count
> pixels.

You can't actually count pixels in the way that Alex did if the borders aren't there.

> If you won't write the patch, please let me know,

I won't.

Comment 18

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

Comment on attachment 309569 [details]
option 1: remove horizontal rules as discussed for Vista

Fine, will you write this patch? It also has the advantage of meaning we don't need the additional override from bug 420236

Comment 19

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

(also note the other spacing issues he mentioned, which I don't think you're contesting, notably the equalization of spacing between the searchbar and locationbar and the searchbar and the end of the window, as per comment 6 - I agree with Alex that our searchbar seems pretty crammed up against the window's edge.

Comment 20

[Dão Gottwald [:dao]]

(In reply to comment #19)
> (also note the other spacing issues he mentioned, which I don't think you're
> contesting

Not at all.

Thanks for reconsidering this.

Comment 21

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

Hm, Alex just mentioned that removing the lines won't actually solve this problem, and doesn't in Vista, as when the bookmark toolbar is removed the back/forward button rubs up against the tabstrip.

Sadly, I think we need to go back to option 3, which you're unwilling to consider at all, so we're back to me writing the patch.

Comment 22

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

Mid air collision and looks like beltzner, he already mentioned my concerns about the back button touching the tabstrip when the bookmarks toolbar is off.  Additionally I'm worried that on XP this ends up making us look like maxthon, and 
really goes against the visual integration strategy.  On Vista it 
makes sense because we are trying to obscure how many bars we have 
since we lack glass support, but this objective doesn't translate to XP.

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 309569 [details]
option 1: remove horizontal rules as discussed for Vista

(In reply to comment #21)
> Hm, Alex just mentioned that removing the lines won't actually solve this
> problem, and doesn't in Vista, as when the bookmark toolbar is removed the
> back/forward button rubs up against the tabstrip.

That's solvable.

> Sadly, I think we need to go back to option 3, which you're unwilling to
> consider at all, so we're back to me writing the patch.

It's sad that we didn't discuss this a month ago.

Comment 24

[Dão Gottwald [:dao]]

Attachment: patch

Comment 25

[Dão Gottwald [:dao]]

Attachment: bookmark toolbar removed (before)

Comment 26

[Dão Gottwald [:dao]]

Attachment: bookmark toolbar removed (after)

Comment 27

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

This doesn't address the comment about the borders needing to be there on XP. Though otherwise it looks good.

Comment 28

[Dão Gottwald [:dao]]

We've dropped native OS widgets elsewhere. If it streamlines the UI (the original reason for doing it on Vista) and makes us consume less space, why not here? You reviewed attachment 309569 [details] before, so I don't buy that the borders need to be there per se.

(I think the Maxthon comparison is a fallacy. For one thing it doesn't look like Maxthon to me, based on a quick image search, and then I don't think Maxthon matters to any extent.)

Comment 29

[Kai Liu]

I think we should exclude Windows Classic from these changes to the border.  The lack of a horizontal rule for the toolbar looks much tackier on Classic (there is simply no precedent for such an appearance on Classic) than on other themes, and the vertical alignment problems that needed addressing never applied to Classic anyway...

Comment 30

[Dão Gottwald [:dao]]

IE7 doesn't have a border between the navigation toolbar and the tabbar regardless of the OS theme, so there's kind of a precedent. That's not to say that we need to follow IE7, though. We could use :-moz-system-metric(windows-default-theme) for this.

Comment 31

[Dão Gottwald [:dao]]

Attachment: patch v2

this excludes Classic

Comment 32

[Kai Liu]

(In reply to comment #30)
> IE7 doesn't have a border between the navigation toolbar and the tabbar
> regardless of the OS theme, so there's kind of a precedent.
>
Ah, yes.  I had the menu bar turned on in IE7, so I had forgotten about that.  IE7 does show the borders if either the menu bar or the links bar is turned on...

(In reply to comment #31)
> this excludes Classic
> 
Thanks. :)

Comment 33

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

(In reply to comment #28)
> We've dropped native OS widgets elsewhere. 

First, this isn't about widgets, it's about style. Second, where we have done that we've done it for very deliberate reasons, not because it would save us a single pixel of height.

> If it streamlines the UI (the original reason for doing it on Vista) and makes 
> us consume less space, why not here?

In fact, the reason for doing it on Vista was that it made the style look more Vista native.

> You reviewed attachment 309569 [details] before, so I don't buy that the borders
> need to be there per se.

Yes, and I made a mistake, as Alex pointed out, in the hopes of reaching an expedient solution. My fault, entirely.

> (I think the Maxthon comparison is a fallacy. For one thing it doesn't look
> like Maxthon to me, based on a quick image search, and then I don't think
> Maxthon matters to any extent.)

I agree that Maxthon isn't really relevant, other than to point to a browser that isn't taking as much pain and effort as we are to try and match visual styles native to the operating system.

Similarly, I don't think IE7 is a good comparison, as it looks entirely non-native on XP. If we were to take IE7's example, then we shouldn't be paying *any* attention to Classic (as it doesn't!) nor should we be styling our colours differently for XP; in fact, we would be shipping the Vista theme on XP, instead of what we're doing.

But we've decided to make Firefox 3 look like a native XP application, putting in effort that the IE7 developers chose not to employ.

Dao: I'm totally willing to entertain other options here. Like, I notice that there's 5px above entries on the PTB and 4px below. Why is that? Alex?

Ryan: could you help us out, here? We're looking for a patch that implements the specification in comment 6. Also, see my previous aside to Dao: maybe we should get rid of that?

Comment 34

[Kai Liu]

Attachment: screenshot showing the toolbox problem

1/ The problem with the bottom spacing being wrong is a result of how the containing toolbox is being rendered, not a problem with the toolbar itself.  If you look closely at the screenshot that I just attached, you'll see that if you add the bookmarks toolbar, the navigation toolbar will look fine, but the problem is still there--it had just shifted down to the bookmarks toolbar.  Similarly, if you remove both the bookmarks toolbar and the navigation toolbar, then the problem is present with the menu bar.  So this ought to fix the problem:
#navigator-toolbox:-moz-system-metric(windows-default-theme) {
  padding-bottom: 1px;
}
Or 2px if you don't think that 1px is enough overcome the visual void from fade gradient that the Luna theme applies to the edge.  And of course, we'll also have to adjust things differently for Vista.

2/ The "problem" of the toolbar being 34px in Luna but 36px in Classic is that the toolbar buttons are 34x34 in Luna, but 36x36 in Classic, and it's the toolbar buttons that are deciding the toolbar height.  This also means Luna's toolbar buttons take up less horizontal space than Classic's.  Now, if we really want to fix this "problem" (um, what's wrong w/ 34px?), all we have to do is just add an extra pixel of padding to the toolbar buttons for Luna.  That'll bump the buttons to 36px, bump the toolbar to 36px, and also eliminate the horizontal discrepancy between Luna and Classic.

3/ Alex, I don't think that the horizontal spacing should always match the vertical spacing.  Right now, both the address bar and the search box have horizontal paddings of 4 pixels on either end, which I think looks just fine.  Remember that not everyone will have the toolbar arranged as it is in default, so we have to be very, very careful about changing those margins because we cannot just assume that they will be in that layout.  If people's screenshots are any indication, many people have different layouts or have extensions that add various elements to the toolbar.  So we should keep the horizontal margins around these two boxes as-is: a symmetric 4 pixels on each side that plays well with all sorts of different toolbar layouts.  This will result in 8 pixels instead of 7 between them in default, but nobody is going to notice that (plus, an odd value like 7 is not going to be easy to do unless we make the margins asymmetric or use different margins for the two boxes--neither of which we should do, IMHO).  That having been said, I think that the start-margin for the address bar can (and should) be increased on Luna/Aero (but not on Classic once bug 428878 lands) to compensate for the curve that juts out into the margin, which is what Dao has done in his patch.  As for the search box being pressed too close to the edge of the screen, I'm not really sure if it looks that bad right now.  But if we do want to increase it, we should do so by increasing the end padding of the toolbar and not by changing the margins of the search box because, once again, we have no guarantee that the search box will always be the last element--some people remove it, some extensions add stuff after the search box, etc.

Comment 35

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

Kai, sounds like you have some ideas which can result in a patch. I'm not running XP so it's harder for me to test these things. Is there any way you could apply those changes and show screenshots that show the entire navigation toolbar, bookmarks toolbar and tabstrip?

(In reply to comment #34)
> then the problem is present with the menu bar.  So this ought to fix the
> problem:
> #navigator-toolbox:-moz-system-metric(windows-default-theme) {
>   padding-bottom: 1px;
> }

Doesn't that unbalance the amount of space between the bottom and top of the location bar? Or does that end up looking OK because of the same visual discrepancy of the white fade line? 

> gradient that the Luna theme applies to the edge.  And of course, we'll also
> have to adjust things differently for Vista.

Yeah, we can override the change in browser-aero.css; we already removed the borders there, too.

> 2/ The "problem" of the toolbar being 34px in Luna but 36px in Classic is that
> the toolbar buttons are 34x34 in Luna, but 36x36 in Classic, and it's the
> toolbar buttons that are deciding the toolbar height.  This also means Luna's
> toolbar buttons take up less horizontal space than Classic's.  Now, if we
> really want to fix this "problem" (um, what's wrong w/ 34px?), all we have to
> do is just add an extra pixel of padding to the toolbar buttons for Luna. 
> That'll bump the buttons to 36px, bump the toolbar to 36px, and also eliminate
> the horizontal discrepancy between Luna and Classic.

There's nothing wrong with 34px. The only problem is the alignment issue which makes things look, on Luna, like the keyhole is too far down within its toolbar. If the solution above addresses that, then IMO, there is no additional problem.

> 3/ Alex, I don't think that the horizontal spacing should always match the
> vertical spacing.  Right now, both the address bar and the search box have
> horizontal paddings of 4 pixels on either end, which I think looks just fine. 
> Remember that not everyone will have the toolbar arranged as it is in default,
> so we have to be very, very careful about changing those margins because we
> cannot just assume that they will be in that layout.  If people's screenshots
> are any indication, many people have different layouts or have extensions that

The lion's share of our users do not adjust their layouts, and we should be optimizing for that, I think. I'm sensitive to what you're saying here, but we shouldn't do things that cause potential oddity to the total detriment of the majority of users and to the default case.

In other words: it should look great, out of the box. :)

> with all sorts of different toolbar layouts.  This will result in 8 pixels
> instead of 7 between them in default, but nobody is going to notice that 

That makes sense to me, assuming that plus adjusting the end padding results in 8px of space at the right edge of the screen as well. Alex? Objections? Comments?

Comment 36

[Kai Liu]

(In reply to comment #35)
> Kai, sounds like you have some ideas which can result in a patch. I'm not
> running XP so it's harder for me to test these things. Is there any way you
> could apply those changes and show screenshots that show the entire navigation
> toolbar, bookmarks toolbar and tabstrip?
> 
Sure.  I'll do that later this evening.

> Doesn't that unbalance the amount of space between the bottom and top of the
> location bar? Or does that end up looking OK because of the same visual
> discrepancy of the white fade line? 
> 
No, it doesn't.  The problem right now is that in XP's theme (and apparently, in Vista's theme, too; finally got around to firing up my Vista VPC) is that the toolbox cuts off one-pixel short, and that is the cause of the downward-shift problem.  There never was an alignment problem; it was a toolbox-cutting-us-off problem all along.  The reason this is happening is that uxtheme renders everything within the content area, including any borders, whereas classic rendering places the border outside of the content area.  So what was supposed to be a free pixel of space for the content area gets taken up by the border in uxtheme rendering.  So adding this extra pixel of padding in cases where uxtheme rendering is used will correct the problem and basically solve all these vertical alignment problems that this bug seeks to address.

Comment 37

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

It sounds like we got the vertical alignment stuff figured out.  In terms of horizontal alignment:

>many people have different layouts or have extensions that
>add various elements to the toolbar.

I was going to comment that this group of people is incredibly small, but then I remembered that adblock+ adds a button to the right of the search bar.  So the percent of users is probably still small, but not incredibly small.

Can add 5 pixels of padding the padding to right of the search bar, and 2 from the window, so we get the exact 7 pixel borders when in the default layout, and matching 2 pixels on each side of the window when the user customizes their toolbar?

Comment 38

[Kai Liu]

Attachment: to clarify what I meant by "toolbox issue"

Okay, first, I want to clarify what I mean by this being a toolbox issue and also what I meant when I talked about the fade in XP (see the blue text at the very bottom of the screenshot).

(In reply to comment #37)
> Can add 5 pixels of padding the padding to right of the search bar, and 2 from
> the window, so we get the exact 7 pixel borders when in the default layout, and
> matching 2 pixels on each side of the window when the user customizes their
> toolbar?
> 

If we keep the toolbar at 34px (unless you want to increase the toolbar button size to 36x36), the top and bottom gaps will only by 6px, and so a 4+2 will get us parity between the vertical and horizontal gap.  As a bonus, if bug 428878 lands on Classic, then Classic will also have a 6px top/bottom gap.  Personally, I'd like to avoid changing the symmetric 4/4 horizontal margins on the address bar and search box if at all possible.

Comment 39

[Kai Liu]

Attachment: before and after

This is a preliminary before-and-after comparison of the patch...

* The classic view shows what it'll look like with bug 428878 landed (part of that reason being I didn't want to switch back to a clean browser.css :P ... but also because if that lands, then this will give a better picture of what things will look like)

* I didn't adjust the padding at the beginning of the toolbar.  The padding breakdown in that area is as follows:
- margins: 0px
- toolbar padding: 0px
- internal padding of b/f button: 2px (from CSS)
- whitespace from the image itself: 1px
- grand total: 3px (though it looks like 4px on Luna because some of those pixels blend in very well with the Luna toolbar background and are not as easy to see)
So if you want to reduce the 3px padding there, we'd either have to reduce the internal padding of the b/f button or adjust the image region so that we don't capture that extra column of empty pixels.

* toolbox bottom padding was set to 1px

* start margin for the address bar was increased by 2px to compensate for the jutting-out effect of the curve (not applied to Classic per bug 428878)

* end padding of the navigation toolbar changed from 0px to 2px, resulting in the top, right, and bottom gaps to all be 6px for the search box

Comment 40

[Kai Liu]

Attachment: patch v3

And the patch used to generate the screenshots in comment 39

Comment 41

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

Kai, thanks for all of your hard work here and excellent explanation. Much appreciated.

I think this is mostly right; things still look a little cramped on XP to me, though. I think that what's happening is that while we have a single pixel of padding above and below the keyhole, the visual effect of the white line at the top border makes it look like there's 2px on top and 1px on the bottom, making the bottom look cramped. By making the toolbar 36px instead of 34px, we'd have 2px of padding above and below, and I believe the result would be far less cramped looking.

Could you try a version with that set of dimensions, and thus with 7px above/below the location and search boxes, and 7 between the searchbox & locationbar, and between the rightmost entity and the edge?

Comment 42

[Kai Liu]

Attachment: 7-7-7 padding, 36x36 buttons

(In reply to comment #41)
> Could you try a version with that set of dimensions, and thus with 7px
> above/below the location and search boxes, and 7 between the searchbox &
> locationbar, and between the rightmost entity and the edge?
> 

Attaching screenshot of 7-7-7 padding with a 36px toolbar height...

Comment 43

[Kai Liu]

Attachment: patch v3.1

* Increased button size to 36x36 (the 36x36 vs. 34x34 discrepancy between Classic and Luna is the result of the same uxtheme-drawing-inside-the-box behavior).  Since toolbar height is determined by button height, the toolbar is now 36px, giving us a 7/7 top/bottom gap.

* Increased end padding for Luna from 2px to 3px so that the end gap matches the 7/7 top/bottom gap.

Comment 44

[Mike Connor [:mconnor]]

That looks pretty huge now.  I don't think we really want to go to 36x36.

Comment 45

[Kai Liu]

(In reply to comment #44)
> That looks pretty huge now.  I don't think we really want to go to 36x36.
> 

I think the options are:

1) Use 36x36 buttons (attachment 316410 [details])
   * 36px toolbar with 7-7-7 end padding
   * 2px vertical back button padding (**)

2) Use 34x34 buttons (attachment 316359 [details])
   * 34px toolbar with 6-6-6 end padding
   * 1px vertical back button padding (**)
   * [I personally like this option the most.]

3) Use 34x34 buttons but pad the toolbar vertically
   * 36px toolbar with 7-7-7 end padding
   * 2px vertical back button padding (**)
   * The buttons will take up slightly less horz. space than (1)
   * The buttons will no longer be flush with the border, which is the
     standard style that we see in Windows Explorer, IE, Firefox, etc.

4) Use 34x36 buttons
   * Same as (3), but with the problem of the buttons not being flush
     replaced by the problem of elongated non-square buttons.

(**) I am inclined with agree with Dao here: why enlarge the whole toolbar for the sake of just one button?  (Although I personally think that the button is a bit oversized by a radius of 1px, I am not trying to debate this settled matter--I'm just not entirely sure if it's a good trade-off to increase the toolbar size just to accommodate one button.)

Comment 46

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

Comment on attachment 316412 [details] [diff] [review]
patch v3.1

Firefox 2 shipped with 35px (36px including the white line) so we're talking about a pixel of difference. I think that's worth not bumping up against the bottom lines.

Thanks for everyone's attention on this, the passion and zeal has been extraordinary. Let's go with this one.

r+uir+a=beltzner (or is that firebot?)

Comment 47

[Simon Bünzli]

Comment on attachment 316412 [details] [diff] [review]
patch v3.1

>+#navigator-toolbox > #nav-bar {
>+  -moz-padding-end: 2px;
>+}

For the record (being fully aware that this won't/can't be corrected anymore):

1. This will (once again!) break Fitts'iness at the right border for those of us who happen to have buttons at the toolbar's extremes for easier accessibility.

2. With the padding only on one specific toolbar, things look slighly off in comparison to the throbber or whatever you put to the very right of the menu bar.

Comment 48

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

(In reply to comment #47)
> 1. This will (once again!) break Fitts'iness at the right border for those of
> us who happen to have buttons at the toolbar's extremes for easier
> accessibility.

Will it? I thought the target area goes all the way to the end on the keyhole. Why wouldn't we do that for buttons on the right edge, as well?

> 2. With the padding only on one specific toolbar, things look slighly off in
> comparison to the throbber or whatever you put to the very right of the menu
> bar.

Good point. Kai, can you address with your patch?

Comment 49

[Simon Bünzli]

(In reply to comment #48)
> (In reply to comment #47)
> Will it? I thought the target area goes all the way to the end on the keyhole.

The keyhole and other buttons are fine, this time it's the toolbar itself which will get a padding all those buttons just can't extend over.

Wouldn't a better fix be to just add a larger end-margin to the search box when it's a toolbar's :last-child ?

Comment 50

[Kai Liu]

(In reply to comment #49)
> Wouldn't a better fix be to just add a larger end-margin to the search box when
> it's a toolbar's :last-child ?
> 

Unfortunately, there is a problem with that: even if it is the last visible element, it is not the last child of #nav-bar; it is followed by fullscreenflex and window-controls, which are hidden elements used only in full screen mode.  If the search box is the last visible element, then I can use sibling selectors to select either fullscreenflex and window-controls, but I can't do anything with that.  Do you know of a better way?  Gecko doesn't have a reverse-sibling selector that I'm not aware of, right?

All that I could think of right now is to add a new hbox to the end of #nav-bar, call it nav-bar-end-padding, and then do stuff with that.  But this method feels clunky, so I'd prefer to avoid if there's a better way.

Comment 51

[Kai Liu]

Attachment: patch v4

Simon, could you check this patch to make sure that I'm not doing anything crazy?  And is there a better way to select for the last element (see comment 50)?

* Get rid of #nav-bar's end padding.

* If either the search box or the address bar is the last element of #nav-bar, add some spacing to the end.  Do this only if the user is using either large icons or icons+text (since the whole point is to make the horizontal spacing match the vertical spacing--the extra horizontal spacing looks bad when the vertical spacing is very small... besides, people who choose small icons do so to make as much use of the space as possible).

* Made the toolbar buttons 36x36 only in large icon mode.  The keyhole isn't used in icon+text mode (plus, the buttons are already pretty big in icon+text) or in small mode (and people who choose small probably would not appreciate the extra 2px added to the buttons)

Comment 52

[Simon Bünzli]

That would satisfy my first point.

Another idea coming to mind: What about selecting on the toolbar's currentset instead? That one doesn't seem to include the references to the full screen controls. So the following three toolbars should get end-padding:

#nav-bar:not([currentset])                     /* default configuration */
toolbar[currentset$=",searchbar-container"]    /* searchbox at end */
toolbar[currentset$=",urlbar-container"]       /* address bar at end */

With that logic, we could also fix my second gripe by adding the same end-padding to the following two toolbars as well:

#menubar:not([currentset])                     /* default configuration */
toolbar[currentset$=",throbber-box"]           /* throbber at end */

Then, you could even move urlbar, searchbar and throbber to different toolbars and you'd still get the desired padding.

Comment 53

[Kai Liu]

(In reply to comment #52)
> Another idea coming to mind: What about selecting on the toolbar's currentset
> instead? That one doesn't seem to include the references to the full screen
> controls.
>
Good idea.  I'll do that later this afternoon.

> toolbar[currentset$=",searchbar-container"]    /* searchbox at end */
> toolbar[currentset$=",urlbar-container"]       /* address bar at end */
> 
Both the address bar and the search box already have 4px margins on either side (the standard for text boxes), so there will always be some spacing.  The concern here is that with the high vertical spacing between the text boxes and the bounding borders of the nav bar (7px in Luna if we go with a 36px toolbar), the 4px gap makes it seem like the search box is jamemd up against the side.  However, if you use small icons, that 7px vertical gap drops down to 2px, and there is no longer that feeling of the search box being crammed (if anything, the default 4px looks a little too wide in this case).  This is why patch v4 checks to make sure that the extra padding isn't added in small icons mode or text-only mode.  And this is why I'm hesitant to do this for all toolbars because then there would be no heuristic to check to make sure that the toolbar is tall enough for this extra visual spacing to make sense.  Usually, when I see a screenshot of the address bar moved to another toolbar, it's to the menu bar, where there is virtually no vertical spacing between the box and the toolbar boundaries and where the extra end padding being proposed here would look terrible.

> #menubar:not([currentset])                     /* default configuration */
> toolbar[currentset$=",throbber-box"]           /* throbber at end */
> 
The menu bar has no padding of its own, and the throbber's spacing comes from its internal spacing--4px padding on the end plus a 1px transparent border for 5px.  I'm not quite sure what you are proposing; that the menu bar's end padding should be increased to match that of the nav bar?  In that case, wouldn't it have to depend on the nav bar's end padding, to account for the cases where the last item on the nav bar isn't a text box or where no extra padding is added because the nav bar isn't tall enough?

This is what I personally think should be done:
* Increase the padding for the nav-bar if the address bar or search box is the last element and if the nav-bar is of sufficient height (large icons or icons+text mode).
* Keep the existing default spacings as-is for all other cases.
* As for the throbber, with 5px end spacing for the throbber coupled with the lesser visual weight from its round shape, it doesn't look that bad with a 7px spacing in the nav-bar.  If necessary, we could try bumping the internal end spacing for the throbber to 6px...

Comment 54

[Kai Liu]

Attachment: patch v4.1

This patch is effectively the same as patch v4, except that by selecting on currentset, it allows me to avoid the kludge that I was using earlier in v4.  I also shifted the throbber to the left by 1px (by increasing its internal spacing; I did not touch the menu bar's padding) so that it matches up better with elements on the nav bar.  Aside from this 1px shift, the default appearance with this patch is identical to what's shown in attachment 316410 [details].

To summarize:
* With default settings, the end spacing here will by 7-7-7.  Throbber end spacing is 6px, but the reduced visual weight of the circular shape should allow it to appear aligned.  (7px for the throbber looks a little too much, esp. given the lack of vertical spacing around the throbber to balance that)
* Button size for large icons, no-text is now 36x36, thus pushing the toolbar height to 36px, thus giving the keyhole the desired 2px vertical spacing.  Button size under other configurations (small icon, icon+text) remain unchanged.
* No extra spacing will be added if the address bar or the search box are not the end elements of the toolbar; so any buttons that users add will remain flush with the edge.

Comment 55

[Simon Bünzli]

Comment on attachment 317411 [details] [diff] [review]
patch v4.1

Fine by me. Thanks for the quick fix!

Comment 56

[Kai Liu]

Comment on attachment 317411 [details] [diff] [review]
patch v4.1

Not sure if the approval from the obsoleted patch carries over, so requesting...

Comment 57

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

Comment on attachment 317411 [details] [diff] [review]
patch v4.1

a=beltzner, the process works! :)

Comment 58

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

Gavin: can we get a checkin here?

Comment 59

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

mozilla/browser/themes/winstripe/browser/browser-aero.css 	1.9
mozilla/browser/themes/winstripe/browser/browser.css 	1.210