598921 - Reduce default addon bar height

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Paul [pwd]]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0b7pre) Gecko/20100923 Firefox/4.0b7pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0b7pre) Gecko/20100923 Firefox/4.0b7pre

Currently the height is too large and seems to grow depending on the button added added to the bar. The height should be locked.

Reproducible: Always

Comment 1

[Dietrich Ayala (:dietrich)]

Boriss, do we want the Add-on Bar's default height to match other toolbar height, or the old statusbar height?

Comment 2

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Current add-on bar vs. old status bar size

The current add-on bar in the latest nightlies seems to be 22 pixels high, where the status bar is 18 pixels high.  So there's two issues here: 1. whether to lock the height and 2. if so, how high to lock it.

The Firefox user experience team's goal - especially in the Firefox 4.0 theme refresh - is to minimize browser chrome area, thereby maximizing web content area - as much as possible.  So, aiming for the smallest area of add-on bar that provides all needed functionality is the goal

Until this point, add-on authors have been using a status bar which is 18 pixels high.  Most add-ons that use this bar display an icon no larger than 16x16 in the bar.  Some add text or widgets.  The plan for the add-on bar is to allow add-on authors a more standardized way to launch panels and overlays, but more area on the add-on bar itself wouldn't change panels which pop out.  So the question becomes: what is gained by increasing the area of the add-on bar?

At this late stage, we should only consider adding more area than the status bar itself if there is huge add-on developer demand based on functionality that would not be available to them at 18 pixels that would be available at 22 pixels.  I haven't seen evidence of this, and unless someone (please comment) has, let's stick with the 18 pixel height so add-on authors can continue to use the functionality and icons that they've built for use in the old status bar.

Comment 3

[Dietrich Ayala (:dietrich)]

Attachment: screenshot

locked to 18px, also added the proper border topping.

Comment 4

[Dietrich Ayala (:dietrich)]

(In reply to comment #2)
> At this late stage, we should only consider adding more area than the status
> bar itself if there is huge add-on developer demand based on functionality that
> would not be available to them at 18 pixels that would be available at 22
> pixels.

The plan so far was to default to small size (18px) and offer a "big icon" option, just like the navigation bar does (bug 600232).

Comment 5

[Paul [pwd]]

(In reply to comment #4)
> (In reply to comment #2)
> > At this late stage, we should only consider adding more area than the status
> > bar itself if there is huge add-on developer demand based on functionality that
> > would not be available to them at 18 pixels that would be available at 22
> > pixels.
> 
> The plan so far was to default to small size (18px) and offer a "big icon"
> option, just like the navigation bar does (bug 600232).

I think bug 600232 became invalid somewhat when locking this toolbar became the goal. For example, I used big buttons but don't see the use of a sizeable Addon bar. An idea is that once bug 598991 is fixed, we can have a right click option to display in the Navigation bar. But that doesn't solve the option of offering uses size/location on installation. Which is, the best place to offer such an option for the less tech-savvy user.

Comment 6

[Dietrich Ayala (:dietrich)]

Attachment: v1 wip: linux, windows

* locks height to 18px
* adds top border
* removes button appearance on windows
* fixes vertical centering of addon bar contents

todo:
* mac changes
* top border on windows not showing up

Comment 7

[Dietrich Ayala (:dietrich)]

Attachment: v1

tested with a status bar add-on installed, and toolbarbuttons from the palette, on mac/win/linux.

Comment 8

[Dietrich Ayala (:dietrich)]

Bah, didn't test the toolbar-primary class removal on Windows yet. will do that in the am.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 481016 [details] [diff] [review]
v1

>+#addon-bar {
>+  max-height: 18px;

Are you sure this actually prevents larger items from increasing the bar height? And would we even want this, i.e. cut off or squeeze items? I don't think the status bar had a maximum width, and I don't think there was a problem with that.

>+  -moz-appearance: none;
>+  border-top: 3px solid ThreeDShadow;
>+  padding-top: 1px;
>+}

Is 3px a typo?

>+#addon-bar > toolbarbutton, #addon-bar > toolbaritem {
>+  -moz-appearance: none;
>+  background-image: none;
>+  box-shadow: none;
>+  border: 0;
>+}

None of these properties are relevant for #addon-bar > toolbaritem, are they?

Comment 10

[Paul [pwd]]

(In reply to comment #9)
> Comment on attachment 481016 [details] [diff] [review]
> v1
> 
> >+#addon-bar {
> >+  max-height: 18px;
> 
> Are you sure this actually prevents larger items from increasing the bar
> height? And would we even want this, i.e. cut off or squeeze items? I don't
> think the status bar had a maximum width, and I don't think there was a problem
> with that.

The bug was created as to force a standardized minimal height which is 18px. Anything attempting to increase the height of the Addon bar should be prevented from doing so. Hence _locking_ at 18px.

Comment 11

[Paul [pwd]]

(In reply to comment #6)
> * removes button appearance on windows

The Addon Bar buttons should have some button like appearance. Just a minimal one. We're not trying to recreate the Status bar here. The options of buttons available to toolbars are fruitful and provide a ton of usability, thus bug 598920 which goes on to explain that some styling should be provided, just not the default button styling of the navigation bar.

Comment 12

[Dietrich Ayala (:dietrich)]

Attachment: wip - linux updated

Comment 13

[Dietrich Ayala (:dietrich)]

Attachment: wip - linux, mac updated

Comment 14

[Dietrich Ayala (:dietrich)]

Attachment: v2

updated, comments fixed, tested mac/linux/win each with built-in toolbarbuttons, a statusbar addon and a jetpack addon (toolbaritem).

Comment 15

[Dietrich Ayala (:dietrich)]

> Are you sure this actually prevents larger items from increasing the bar
> height? And would we even want this, i.e. cut off or squeeze items? I don't
> think the status bar had a maximum width, and I don't think there was a problem
> with that.

1. confirmed it locks item size (squeezes, unless the item specifies overflow)

2. i think we want to ensure a consistent height here so that users get a consistent experience. i'd like to build in support for "big icon mode" on the add-on bar, so that there's an option for a larger (also consistent) view.

(In reply to comment #11)
> (In reply to comment #6)
> > * removes button appearance on windows
> 
> The Addon Bar buttons should have some button like appearance. Just a minimal
> one. We're not trying to recreate the Status bar here. The options of buttons
> available to toolbars are fruitful and provide a ton of usability, thus bug
> 598920 which goes on to explain that some styling should be provided, just not
> the default button styling of the navigation bar.

The buttons looked really out of place here, when decorated as buttons. Especially amidst non-button-y statusbar and toolbaritem add-ons. Until we get an agreement in that bug on a consistent styling across all items, we should remove the button-ness.

Comment 16

[Paul [pwd]]

The argument for the removal of the Status-Bar shim has been all but marked NEW. Arguments for keeping it haven't evolved and as such the bug is sitting there without a good reason to keep it for the upcoming release. The thread in the google group has also reached the same conclusion in that it should be removed in that there's a failure to provide a real reason to keep it.

As for button styling can't we simply remove the rounded corners, take the padding and margins down to 1px and remove the drop-shadow?

Comment 17

[Dietrich Ayala (:dietrich)]

Dao, when do you have cycles to review this?

Drivers: requesting blocking due to regressing the size of the bar. Also provides better appearance and position of add-ons on the bar. Takes responsibility for proper vertical centering and positioning in a diverse add-on bar ecosystem off of the add-on dev's plate.

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 481265 [details] [diff] [review]
v2

I tried dropping the bookmarks toolbar item and the location bar on the add-on bar and the result seems pretty much unacceptable. You may argue that the add-on bar isn't the place for these widgets, but 1) users will do it anyway and 2) add-ons will create widgets which won't fit the limit either.

Comment 19

[Dietrich Ayala (:dietrich)]

Great, thanks for finding that out. Can we get these changes reviewed and landed, for the common case? I'll file a followup for better supporting the oversized items.

Comment 20

[Paul [pwd]]

(In reply to comment #18)
> Comment on attachment 481265 [details] [diff] [review]
> v2
> 
> I tried dropping the bookmarks toolbar item and the location bar on the add-on
> bar and the result seems pretty much unacceptable. You may argue that the
> add-on bar isn't the place for these widgets, but 1) users will do it anyway
> and 2) add-ons will create widgets which won't fit the limit either.

In regards to the latter, is that really the concern here? The Addon developers managed with a Status bar locked at 18px after all. In regards to the first problem, could you present a screenshot please? Will these problems not be sorted out once we've managed to sort out the button styling issues?

Comment 21

[Dão Gottwald [:dao]]

(In reply to comment #19)
> Great, thanks for finding that out. Can we get these changes reviewed and
> landed, for the common case? I'll file a followup for better supporting the
> oversized items.

Small icons usually are 16px tall already (and win/pinstripe actually enforce this for the toolbarbutton-1 class), so I don't see what problem the max-height would solve for the common case.

Comment 22

[Dietrich Ayala (:dietrich)]

(In reply to comment #21)
> (In reply to comment #19)
> > Great, thanks for finding that out. Can we get these changes reviewed and
> > landed, for the common case? I'll file a followup for better supporting the
> > oversized items.
> 
> Small icons usually are 16px tall already (and win/pinstripe actually enforce
> this for the toolbarbutton-1 class), so I don't see what problem the max-height
> would solve for the common case.

I was referring to the fact that the addon bar is currently >18px tall. Which is not in the summary of this bug. My fault for conflating this bug and fixing a bunch of unrelated issues in the patch. I'll make a new patch.

Comment 23

[Dietrich Ayala (:dietrich)]

Attachment: v3

Only addresses the max-height issue, and works with scenarios in comment #18.

Comment 24

[Paul [pwd]]

I asked about this in another bug. But I have a proposal that affects this bug. It would lock the Addon Toolbar height at 23px rather than 18px. But it does allow for button styling, one tweaked from the navigation bar to fit the smaller addon bar. It also allows for making the Addon bar transparent on systems that support it. Would I be best filing a new bug for the proposal? I'm not sure how one would go about something like that?

Comment 25

[Dietrich Ayala (:dietrich)]

Bah, test fails on Windows, where it's 19px for some reason. Canceling review.

(In reply to comment #24)
> systems that support it. Would I be best filing a new bug for the proposal? I'm
> not sure how one would go about something like that?

Yep, please file a new bug. Thanks!

Comment 26

[Dão Gottwald [:dao]]

Can you please re-summarize this bug to reflect the current plan?

I was under the impression that you'd prepare a patch that does everything the previous patch did except for setting a max-height, but you seem to be doing the opposite.

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #19)
> > > Great, thanks for finding that out. Can we get these changes reviewed and
> > > landed, for the common case? I'll file a followup for better supporting the
> > > oversized items.
> > 
> > Small icons usually are 16px tall already (and win/pinstripe actually enforce
> > this for the toolbarbutton-1 class), so I don't see what problem the max-height
> > would solve for the common case.
> 
> I was referring to the fact that the addon bar is currently >18px tall. Which
> is not in the summary of this bug. My fault for conflating this bug and fixing
> a bunch of unrelated issues in the patch. I'll make a new patch.

Comment 27

[Dietrich Ayala (:dietrich)]

Per the end of that comment thread, this bug now only covers the scenario described in the summary: Lock the bar at 18px. It should not grow or shrink with content, and should be the same height on all tier 1 platforms.

Any other visual or behavioural changes should be filed as separate bugs.

Comment 28

[Dão Gottwald [:dao]]

If you just want to set a max-height in this bug, comment 21 seems to apply.

Comment 29

[Paul [pwd]]

When I proposed the bug, it was to lock the addon bar at a specific height. Whether it be 18px, 23px or 150px. The height should be locked. Locking the toolbar height and getting the buttons to conform with a simple min-height requirement is no big deal. The only real issue comes with making sure we can the Awesome Bar conform. Which is where you learn that the current approach is simply unrealistic. 18px is too restrictive and allows no wiggle. 23px is far more realistic. That said, there still needs to be some tweaking that will allow the awesome bar to shrink.

But in short, the scope of this bug is to lock the toolbar height, not "lock but there may be a few exceptions" as the renaming of the bug would now suggest.

Comment 30

[Dão Gottwald [:dao]]

Comment on attachment 487861 [details] [diff] [review]
v3

This doesn't seem to address comment 18 at all, screenshot forthcoming.

Comment 31

[Dão Gottwald [:dao]]

Attachment: screenshot of items that can't deal with 18px

Comment 32

[Dão Gottwald [:dao]]

(In reply to comment #29)
> But in short, the scope of this bug is to lock the toolbar height, not "lock
> but there may be a few exceptions" as the renaming of the bug would now
> suggest.

I understand and disagree. We haven't "locked" the height of any toolbar before, including the status bar, and there's no good reason to should start doing it now. All we need to do is to make sure standard items (e.g. toolbarbuttons) keep the toolbar at a reasonable height.

Comment 33

[DRC]

An "add tab button" like that I think is too intrusive and could confuse the user trying to move that button around like normal tabs.

I suggest a related usability feature: a different new tab button for newly created groups.
For example: when I create a new group dragging the mouse in a empty area I have an empty group that will be used either for dragging other tabs in or because I'm going to start another workflow; in the latter case I usually create a new tab inside the empty group; in this situation the + button is too small so I suggest that double clicking (or single clicking) the empty group should zoom(with the default animation) the group with a newly created empty tab.

This because in many laptops pointing the small plus button in the left bottom corner is sometimes difficult.
Maybe the mockup of the new group should have a big plus sign button (grey on grey with no borders) in the center of the rectangle and not a small one on the left-bottom corner.
This big center button should fallback to the default one if I drag some tab from an old group to the new empty one.

I hope that this motivation is sufficiently clear and why I'm filling this bug/suggestion.

thanks!

FF 4.0b7

Comment 34

[Csaba Kozák [:WonderCsabo]]

How does that come to this bug?

If you wanna suggest a new feature, do it in the newsgroups:
http://groups.google.com/group/mozilla.dev.apps.firefox

or file a new bug on Bugzilla:
https://bugzilla.mozilla.org/enter_bug.cgi

Comment 35

[DRC]

sorry! i had more tabs opened and I filled in the wrong bug.
sorry again for that.

Comment 36

[Dão Gottwald [:dao]]

I could probably take this as a followup to bug 598920.

Comment 37

[Dietrich Ayala (:dietrich)]

That would be awesome, thanks Dao!

Comment 39

[Paul [pwd]]

If Bug 618278 is a dupe of this, can we elevate the priority on this bug please?

Comment 40

[Ian Nartowicz]

How does this work if you select "icons and text" with the height locked?  Currently my browser shows toolbar buttons overlapping the content area and the text overlapping outside the browser window completely.  I may not have a completely standard setup, but something needs to be done if users are allowed to customise any toolbar palette item they want onto a fixed height toolbar.  Perhaps bug 598920 will force text to always be hidden for the addon bar?

Comment 41

[Paul [pwd]]

(In reply to comment #40)
> How does this work if you select "icons and text" with the height locked? 
> Currently my browser shows toolbar buttons overlapping the content area and the
> text overlapping outside the browser window completely.  I may not have a
> completely standard setup, but something needs to be done if users are allowed
> to customise any toolbar palette item they want onto a fixed height toolbar. 
> Perhaps bug 598920 will force text to always be hidden for the addon bar?

The height isn't locked. Also I think Dietrich said that he was considering removing the ability to show text in the addon bar anyway. Either way, it's not fixed as 18px.

Comment 42

[Dietrich Ayala (:dietrich)]

Dao, when do you think you'll be able to fix this? If you don't have the time, I could take it back.

Comment 44

[Dão Gottwald [:dao]]

Attachment: patch

Comment 45

[Dão Gottwald [:dao]]

Attachment: patch

Comment 46

[Dão Gottwald [:dao]]

Attachment: patch

Actually, I don't think 18px is a reasonable min-height on Windows. Changed to 20px.

Comment 47

[Dietrich Ayala (:dietrich)]

Comment on attachment 502691 [details] [diff] [review]
patch

sounds fine to me. lets land it w/ 20px and see what the feedback is like.

Comment 48

[Dão Gottwald [:dao]]

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

Comment 49

[Sid]

Attachment: Empty Add-on Bar

An empty Add-on Bar is fine, 20 px high as planned.

Comment 50

[Sid]

Attachment: Icon added and hovered

But when I add any icon on Add-on Bar, the height is suddenly 23 px. And when I hover the icon, its top border is "leaking" over the Add-on Bar into the content area.

Comment 51

[rob64rock]

(In reply to comment #50)
> Created attachment 503484 [details]
> Icon added and hovered
> 
> But when I add any icon on Add-on Bar, the height is suddenly 23 px. And when I
> hover the icon, its top border is "leaking" over the Add-on Bar into the
> content area.

Not seeing any Toolbar buttons hover background leaking into the web page content area on my screen nor in your screenshot. Or I'm missing something else you're trying to point out?

Comment 52

[Sid]

(In reply to comment #51)
Top border of hovered icon is 1 px higher than top border of toolbar in my screenshot. But nevermind, after several browser restarts it somehow resolved itself.

An issue with changing toolbar height remains though. Should I post it as a new bug blocking this one?

Comment 53

[Dão Gottwald [:dao]]

(In reply to comment #52)
> An issue with changing toolbar height remains though. Should I post it as a new
> bug blocking this one?

Yes please.

Comment 54

[Csaba Kozák [:WonderCsabo]]

(In reply to comment #50)
> Created attachment 503484 [details]
> Icon added and hovered
> 
> But when I add any icon on Add-on Bar, the height is suddenly 23 px. And when I
> hover the icon, its top border is "leaking" over the Add-on Bar into the
> content area.

I only see the height change, but i guess it's intended.

Comment 55

[Sid]

(In reply to comment #53)
> Yes please.

Bug 625385.

Comment 56

[Aleksei Perepelkin [:chaos]]

Verified with Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre