Closed Bug 598921 Opened 9 years ago Closed 9 years ago

Reduce default addon bar height

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 4.0b10
Tracking Status
blocking2.0 --- final+

People

(Reporter: pwd.mozilla, Assigned: dao)

References

Details

(Whiteboard: [addon bar][hardblocker][fx4-fixed-bugday])

Attachments

(4 files, 10 obsolete files)

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
Blocks: 574688
Version: unspecified → Trunk
Boriss, do we want the Add-on Bar's default height to match other toolbar height, or the old statusbar height?
Keywords: uiwanted
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.
Assignee: nobody → dietrich
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: uiwanted
Attached image screenshot (obsolete) —
locked to 18px, also added the proper border topping.
Attachment #479770 - Flags: ui-review?(jboriss)
(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).
(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.
Attached patch v1 wip: linux, windows (obsolete) — Splinter Review
* 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
Attached patch v1 (obsolete) — Splinter Review
tested with a status bar add-on installed, and toolbarbuttons from the palette, on mac/win/linux.
Attachment #480869 - Attachment is obsolete: true
Attachment #481016 - Flags: review?(dao)
Bah, didn't test the toolbar-primary class removal on Windows yet. will do that in the am.
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?
(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.
(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.
Attached patch wip - linux updated (obsolete) — Splinter Review
Attachment #481016 - Attachment is obsolete: true
Attachment #481016 - Flags: review?(dao)
Attached patch wip - linux, mac updated (obsolete) — Splinter Review
Attachment #481240 - Attachment is obsolete: true
Attached patch v2 (obsolete) — Splinter Review
updated, comments fixed, tested mac/linux/win each with built-in toolbarbuttons, a statusbar addon and a jetpack addon (toolbaritem).
Attachment #481256 - Attachment is obsolete: true
Attachment #481265 - Flags: review?(dao)
> 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.
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?
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.
blocking2.0: --- → ?
Whiteboard: [needs review dao]
Component: General → Theme
QA Contact: general → theme
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.
Attachment #481265 - Flags: review?(dao) → review-
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.
(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?
(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.
(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.
blocking2.0: ? → final+
Whiteboard: [needs review dao] → [needs new patch]
Attached patch v3 (obsolete) — Splinter Review
Only addresses the max-height issue, and works with scenarios in comment #18.
Attachment #481265 - Attachment is obsolete: true
Attachment #487861 - Flags: review?(dao)
Whiteboard: [needs new patch] → [needs review dao]
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?
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!
Whiteboard: [needs review dao] → [needs new patch]
Attachment #487861 - Flags: review?(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.
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.
If you just want to set a max-height in this bug, comment 21 seems to apply.
Summary: Lock Addon Toolbar height at 18px → Addon bar usually shouldn't be taller than 18px
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 on attachment 487861 [details] [diff] [review]
v3

This doesn't seem to address comment 18 at all, screenshot forthcoming.
Attachment #487861 - Flags: review-
(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.
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
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
sorry! i had more tabs opened and I filled in the wrong bug.
sorry again for that.
Whiteboard: [needs new patch] → [needs new patch][addon bar]
I could probably take this as a followup to bug 598920.
That would be awesome, thanks Dao!
Assignee: dietrich → dao
Depends on: 598920
Duplicate of this bug: 618278
If Bug 618278 is a dupe of this, can we elevate the priority on this bug please?
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?
(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.
Summary: Addon bar usually shouldn't be taller than 18px → Reduce default addon bar height
Dao, when do you think you'll be able to fix this? If you don't have the time, I could take it back.
Attached patch patch (obsolete) — Splinter Review
Attachment #479770 - Attachment is obsolete: true
Attachment #487861 - Attachment is obsolete: true
Attachment #488149 - Attachment is obsolete: true
Attachment #479770 - Flags: ui-review?(jboriss)
Attached patch patch (obsolete) — Splinter Review
Attachment #502684 - Attachment is obsolete: true
Attached patch patchSplinter Review
Actually, I don't think 18px is a reasonable min-height on Windows. Changed to 20px.
Attachment #502689 - Attachment is obsolete: true
Attachment #502691 - Flags: review?(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.
Attachment #502691 - Flags: review?(dietrich) → review+
Whiteboard: [needs new patch][addon bar] → [addon bar]
Whiteboard: [addon bar] → [addon bar][hardblocker]
http://hg.mozilla.org/mozilla-central/rev/ff599017349f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b10
Attached image Empty Add-on Bar
An empty Add-on Bar is fine, 20 px high as planned.
Attached image 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.
(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?
(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?
(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.
(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.
Depends on: 625385
(In reply to comment #53)
> Yes please.

Bug 625385.
Blocks: 616363
Status: RESOLVED → VERIFIED
Whiteboard: [addon bar][hardblocker] → [addon bar][hardblocker][fx4-fixed-bugday]
Verified with Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre
You need to log in before you can comment on or make changes to this bug.