Jetpack makes statusbar grow taller in proportion to number of statusbarpanels

RESOLVED FIXED in 0.7

Status

P1
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: myk, Assigned: myk)

Tracking

unspecified

Details

Attachments

(1 attachment)

(Assignee)

Description

9 years ago
Per a conversation in the discussion forum <http://groups.google.com/group/mozilla-labs-jetpack/browse_thread/thread/cf116b3e1155db97#> and the following screenshots, the more statusbarpanels there are in the statusbar, the taller the statusbar is when Jetpack is installed:

http://img403.yfrog.com/i/fx355xp.png/
http://img403.yfrog.com/i/fx36b4xp.png/

It's not clear whether the height is dependent on all statusbarpanels or just those that Jetpack creates.  Obviously, the height of the statusbar should not depend on the number of statusbarpanels it contains.
Quick test-case:
for (var i=0; i<50; i++) jetpack.statusBar.append({width:0});
(Assignee)

Comment 2

9 years ago
(In reply to comment #1)
> Quick test-case:
> for (var i=0; i<50; i++) jetpack.statusBar.append({width:0});

Nice reduced testcase!

I see this on Mac too, although I don't see it on Linux.  I suspect the bug is here:

http://hg.mozilla.org/labs/jetpack/file/178751f17f55/extension/content/js/status-bar-panel.js#l111

That line sets the height of the statusbarpanel's iframe to the height of the statusbar's box object.  But the height of the statusbar's box object includes padding and borders, if any <https://developer.mozilla.org/en/XUL_Tutorial/Box_Objects#Retrieving_Position_and_Size>, so doing this on an OS where the statusbar has some padding or border will cause the statusbar to become a bit taller in order to accommodate both the iframe and its padding/border.

I suspect the fix is as simple as setting the height of the iframe to 100% or setting its container's XUL flex/align appropriately.  Looking...
(Assignee)

Comment 3

9 years ago
Created attachment 415549 [details] [diff] [review]
patch v1: fixes problem

Setting the height of the iframe to 100% fixes the problem, although it makes the iframes only 10px tall on all three primary OSes.  Setting |-moz-box-align: stretch| on the statusbarpanel fixes that, however, making the iframes stretch to the heights of the panels.

The statusbarpanels themselves are 18px, 16px, and 22px tall on Windows (XP, default theme), Mac OS X, and Linux (Ubuntu 9.10, New Wave theme), respectively.

Perhaps we should choose a specific height that we guarantee all statusbarpanels will be, something that is a compromise between the goals of minimizing statusbar height (in order to maximize content area), fitting into OS themes, and providing enough space for jetpacks to do interesting things.  For example, 16px.

That'd also make it easier for jetpack authors to design statusbarpanels, knowing they'll be a certain fixed height on all three primary platforms.

For OSes where the statusbar is taller than the fixed height, we would center the iframe within the statusbarpanel to make it look nice.

Atul: what do you think?
Attachment #415549 - Flags: review?(avarma)
Comment on attachment 415549 [details] [diff] [review]
patch v1: fixes problem

(In reply to comment #2)
> > for (var i=0; i<50; i++) jetpack.statusBar.append({width:0});
> Nice reduced testcase!
Indeed! A neat test :)
Attachment #415549 - Flags: review?(avarma) → review+
Re: guaranteeing that statusbar items will always be say.. 16px high.

Aza, what are the plans for down the line in a world without status bars? jetpack.statusBar.append would probably just automagically migrate to the new place to show statusBar-like items, but that will most likely be non-16px..?

Comment 6

9 years ago
Ed: If they are all 16px height then it makes it much easier to migrate them to a new interface as we know they will all be uniform in some way. They will all be thin and potentially long.

I'm in favor of the 16px concept.
(Assignee)

Comment 7

9 years ago
Fixed by changeset http://hg.mozilla.org/labs/jetpack/rev/e732ed306b96.  Filed bug 535383 on the 16px issue.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.