Markus Stange [:mstange] Reporter
	Description • 11 years ago

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.

	Justin Dolske [:Dolske]
	Comment 1 • 11 years ago

I think this got fixed in a generic way?

	:Gijs (he/him) Assignee
	Comment 2 • 11 years ago

(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...

	:Gijs (he/him) Assignee
	Comment 3 • 11 years ago

(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.

	Dão Gottwald [:dao]
	Comment 4 • 11 years ago

(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.

	Mike de Boer [:mikedeboer]
	Comment 5 • 11 years ago

(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;`?

	:Gijs (he/him) Assignee
	Comment 6 • 11 years ago

(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.

	:Gijs (he/him) Assignee
	Comment 7 • 11 years ago

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.

	:Gijs (he/him) Assignee
	Comment 8 • 11 years ago

Now with testy goodness.

	Mike Conley (:mconley) (:⚙️)
	Comment 9 • 11 years ago

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?

	:Gijs (he/him) Assignee
	Comment 10 • 11 years ago

Fair points. I've left the addition of the area to onWidgetReset because I imagine other consumers may want it.

	Mike Conley (:mconley) (:⚙️)
	Comment 11 • 11 years ago

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!

	:Gijs (he/him) Assignee
	Comment 12 • 11 years ago

https://hg.mozilla.org/projects/ux/rev/903d51e5303e

	:Gijs (he/him) Assignee
	Comment 13 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/903d51e5303e