Closed Bug 884492 Opened 6 years ago Closed 6 years ago

Tabbar doesn't handle tall widgets gracefully

Categories

(Firefox :: Theme, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: mstange, Assigned: Gijs)

References

Details

(Whiteboard: [Australis:P3][Australis:M9])

Attachments

(2 files, 1 obsolete file)

STR (in UX):
1. Use toolbar customization to put the fullscreen button into the tab bar.
2. Use the DOM Inspector to give the button height:80px in order to emulate a
   tall add-on button

Will addon buttons be allowed to be more than 31px in height? If not, this bug is probably invalid.
I think this got fixed in a generic way?
Flags: needinfo?(gijskruitbosch+bugs)
Whiteboard: [Australis:P3]
(In reply to Justin Dolske [:Dolske] from comment #1)
> I think this got fixed in a generic way?

If you're implying it's already fixed, no, I can still reproduce on tip, on Mac, with the STR in comment 0. I'm not sure we should support this usecase; there's a separate bug about making icons have a height set (on retina, where they have a width: set) for the add-on SDK, and fixing this I think should help here with more realistic STR than "use DOMI to mess with this thing)...

Generally speaking, I'm not sure we should be supporting people adding Really Large add-on buttons... anywhere? But then again, I guess it could depend on what we mean when we say addon buttons will not be "allowed" to be too big - AMO, or stylesheet constraints (which we currently do not have) or something else. For some of these, we would need to do work. Not marking as INVALID for now, I guess...
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #2)
> there's a separate bug about making icons have a height set (on retina,
> where they have a width: set) for the add-on SDK

Forgot: this is bug 882910.
(In reply to :Gijs Kruitbosch from comment #2)
> there's a separate bug about making icons have a height set (on retina,
> where they have a width: set) for the add-on SDK, and fixing this I think
> should help here with more realistic STR than "use DOMI to mess with this
> thing)...

That's not going to be sufficient for e.g. text fields or other random custom widgets that may be just a few pixels taller than tabs.
(In reply to Dão Gottwald [:dao] from comment #4)
> (In reply to :Gijs Kruitbosch from comment #2)
> > there's a separate bug about making icons have a height set (on retina,
> > where they have a width: set) for the add-on SDK, and fixing this I think
> > should help here with more realistic STR than "use DOMI to mess with this
> > thing)...
> 
> That's not going to be sufficient for e.g. text fields or other random
> custom widgets that may be just a few pixels taller than tabs.

In other words, all items in #navbar should have a `max-height` and `overflow-y: hidden;`?
(In reply to Mike de Boer [:mikedeboer] from comment #5)
> (In reply to Dão Gottwald [:dao] from comment #4)
> > (In reply to :Gijs Kruitbosch from comment #2)
> > > there's a separate bug about making icons have a height set (on retina,
> > > where they have a width: set) for the add-on SDK, and fixing this I think
> > > should help here with more realistic STR than "use DOMI to mess with this
> > > thing)...
> > 
> > That's not going to be sufficient for e.g. text fields or other random
> > custom widgets that may be just a few pixels taller than tabs.
> 
> In other words, all items in #navbar should have a `max-height` and
> `overflow-y: hidden;`?

I think you mean #TabsToolbar. But, that would be one solution. Also note that (AFAIK) this is OS-X only, which makes fixing it that way less attractive (plus XUL + overflow can be 'fun', and I wouldn't like to mess with the tabstrip too much at the moment, for TART reasons).

Alternatively, we could fix the titlebar gradient to be properly sized on OS X, too, and figure out what's up with the app tabs. The tabs being stretched is normal in the sense that you can also get that if you use large fonts (200%) on Windows. It doesn't look as bad on Windows, though, because we size the titlebar dynamically. Of course, tabs being stretched 2.5x is never going to look amazing, but it does work and doesn't look as horrific/messy as this. I'm not sure why the app tabs are misbehaving, that'd need to be looked into.
Assignee: nobody → gijskruitbosch+bugs
We should rerun the TabsInTitlebar._update(true) code when items get added to or removed from the menubar or tabstoolbar. When the code in bug 851652 lands, that should make this work, AFAICS.
Status: NEW → ASSIGNED
Depends on: 851652
Now with testy goodness.
Attachment #831553 - Flags: review?(mconley)
Comment on attachment 831553 [details] [diff] [review]
run TabsInTitlebar._update when nodes are added/removed in tab/menubar,

Review of attachment 831553 [details] [diff] [review]:
-----------------------------------------------------------------

Tentative r- because I've got some concerns here... let me know what you think.

::: browser/base/content/browser.js
@@ +4446,5 @@
>  #endif
>    }
>  };
>  
> +TabsInTitlebar.onWidgetReset =

If CAN_DRAW_IS_TITLEBAR is not set, then this._update is not defined, and this is going to be broken.

I know that the listener is only added if CAN_DRAW_IS_TITLEBAR is true, but it feels weird to have some functions blacked out with CAN_DRAW_IS_TITLEBAR, and some not.

I think these functions should be moved inside the TabsInTitlebar definition, wrapped in a CAN_DRAW_IN_TITLEBAR ifdef.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +400,5 @@
>        }
>  
>        this.insertWidgetBefore(node, currentNode, container, aArea);
>        if (gResetting) {
> +        this.notifyListeners("onWidgetReset", id, aArea);

Hm. We're going to be calling TabsInTitlebar._update() for every widget here while we reset. That doesn't sound like a good idea... couldn't we somehow call it (if necessary) during endBatchUpdate?
Attachment #831553 - Flags: review?(mconley) → review-
Fair points. I've left the addition of the area to onWidgetReset because I imagine other consumers may want it.
Attachment #831697 - Flags: review?(mconley)
Attachment #831553 - Attachment is obsolete: true
Comment on attachment 831697 [details] [diff] [review]
run TabsInTitlebar._update when nodes are added/removed in tab/menubar,

Review of attachment 831697 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I'll take it. Thanks Gijs!
Attachment #831697 - Flags: review?(mconley) → review+
https://hg.mozilla.org/projects/ux/rev/903d51e5303e
Whiteboard: [Australis:P3] → [Australis:P3][Australis:M9][fixed-in-ux]
https://hg.mozilla.org/mozilla-central/rev/903d51e5303e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][Australis:M9][fixed-in-ux] → [Australis:P3][Australis:M9]
Target Milestone: --- → Firefox 28
Blocks: 967917
You need to log in before you can comment on or make changes to this bug.