toolbar changes its size when adding an addon button

RESOLVED FIXED

Status

Add-on SDK
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Jorge R, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b10pre) Gecko/20110118 Firefox/4.0b10pre
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b10pre) Gecko/20110118 Firefox/4.0b10pre

When you insert some addon buttons to tab bar or url bar, its size is reduced and it doesn't look good.

Reproducible: Always

Steps to Reproduce:
1. Download this addon for example: https://addons.mozilla.org/firefox/addon/hard-blockers-counter/
2. Add the addon button on the addon bar to the tab bar or the url bar.

Actual Results:  
The size of the bar in which is the button resizes and doesn't look well.

Expected Results:  
The icon shold be resized or it should maintein its size but the tab bar or urlbar shoul have a minimun height
(Reporter)

Comment 1

6 years ago
Created attachment 504797 [details]
Urlbar showing the bug
(Reporter)

Comment 2

6 years ago
Created attachment 504799 [details]
Tab bar showing the bug
Confirmed on Windows 7

Mozilla/5.0 (Windows NT 6.1; rv:2.0b10pre) Gecko/20110118 Firefox/4.0b10pre

Comment 4

6 years ago
Confirm again. Mozilla/5.0 (Windows NT 6.1; rv:2.0b10pre) Gecko/20110118 Firefox/4.0b10pre ID:20110118030327
This is a bug in the widget used by the add-on.
Component: Toolbars → Jetpack SDK
Product: Firefox → Mozilla Labs
QA Contact: toolbars → jetpack-sdk
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
This is caused by the max-height on the toolbaritem.
This is pretty bad actually, and would very very likely be a softblocker if this was a Firefox bug.
Status: UNCONFIRMED → NEW
Ever confirmed: true
I don't see an easy way to fix that from inside of the jetpack, so there seems to be no workaround :(

Also, I had to manually define the width of the widget - without it the widget content got cropped, so it seems like there's a generic CSS styling issue with the content.

Last thing, from Limi:
Use: 'font-family: "Segoe UI", Tahoma, "Lucida Grande", sans-serif;' to match Firefox theme.
Limi reported the font issue as bug 627189.
Duplicate of this bug: 627235
OS: Mac OS X → All
Hardware: x86_64 → All
This affects the Addon Bar, too, as reported in Bug 627235 (duped in comment 10).

From that bug:
 - normal screenshot: attachment 505253 [details]
 - buggy screenshot: attachment 505254 [details]
minimized testcase: https://builder.addons.mozilla.org/addon/1000076/latest/
Hrm. Without max-height, the toolbaritem always expands to 22px, even if I restrict the iframe within it to 16px (the iframe is sized correctly, but the toolbaritem expands vertically anyway).
Is it bad that the toolbaritem height is 22px? The iframe is still 16px high, isn't it?
(In reply to comment #14)
> Is it bad that the toolbaritem height is 22px? The iframe is still 16px high,
> isn't it?

Fine on regular toolbars. The Add-on bar is 18px high (like the status bar was), and the unit test is checking that.
Removing the max-height didn't change the Add-on bar height for me on Mac.
This fixed it for me, using latest SDK source: https://github.com/mozilla/addon-sdk/pull/97.
Someone in #jetpack asked about the bug, tested this fix, confirmed that it worked for them on Win7 w/ beta 9.
Created attachment 505782 [details]
Pointer to pull request
Attachment #505782 - Flags: review?(myk)
Attachment #505782 - Flags: review?(myk) → review+
I tested the patch on macos and it does indeed seem to fix the issue with height. 

The remaining issues are:

* if you don't specify the width of the widget, it'll get cropped
* restarting the browser resets the widget positioning

Do we want to cover that in this bug or file separate ones?
(In reply to comment #20)
> The remaining issues are:
> 
> * if you don't specify the width of the widget, it'll get cropped
> * restarting the browser resets the widget positioning
> 
> Do we want to cover that in this bug or file separate ones?

One issue per bug please. Anything else and bugs become a mess.
https://github.com/mozilla/addon-sdk/commit/0f9e14ea9d9f3251cbe925e2b69facd31376be8a
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Blocks: 628022
No longer blocks: 628022
I backed this out, as it broke a test (bug 628598) and landed without approval in a frozen tree.

https://github.com/mozilla/addon-sdk/commit/044023ee9f174bdf35e36426e8855f662006c31d
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Dietrich: any thoughts on getting this working cross-platform?
It's not necessarily a platform problem, since a bunch of the reported failures in bug 628598 are on Linux, where I tested.

I'll try to spend some time looking at it this week. Alternatively, anyone else can look in the meantime :)
Comment on attachment 505782 [details]
Pointer to pull request

Further testing reveals that specifying both height and max-height makes tests pass on all platforms. re-r=myk with that change
Attachment #505782 - Flags: review+
Patch landed with change requested in review comment 26 in the interest of getting this in before the 1.0b3 freeze:

https://github.com/mozilla/addon-sdk/commit/f0766efeebe408b4f7e9eeacfdbfe43e92f50847
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Created attachment 515655 [details] [diff] [review]
Remove max-height

As Cork highlighted on irc, there is still such issues with widgets and addon-bar/toolbars.

We should not use max-height in toolbars because it resize all the toolbar due to xul box layout. So I removed max-height on toolbaritem and add a new one on the iframe. As toolbaritem has overflow:hidden, toolbaritem size is not related to iframe one. So everything should be fine now. widget tests pass on windows.
Attachment #515655 - Flags: review?(dietrich)
Created attachment 515665 [details] [diff] [review]
Remove max-height and center iframe

Same patch but align widget on center.
Attachment #515655 - Attachment is obsolete: true
Attachment #515665 - Flags: review?(dietrich)
Attachment #515655 - Flags: review?(dietrich)
Comment on attachment 515665 [details] [diff] [review]
Remove max-height and center iframe

r=me, tested fine on Mac and Linux.
Attachment #515665 - Flags: review?(dietrich) → review+
Keywords: checkin-needed
Followup patch (attachment 515665 [details] [diff] [review]) landed:

https://github.com/mozilla/addon-sdk/commit/24c8f4a67a76e0aaac7fc4ebcde62492c2f4c2ce
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.