541052 - (ToolboxMaxHeight) Toolbox does not expand vertically anymore

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, defect)

Description

[Axel Grude [:realRaven]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.7) Gecko/20091221 Firefox/3.5.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.7) Gecko/20100111 Lightning/1.0b1 Thunderbird/3.0.1

Some change in  TB 3.0.1 prohibit vertical expansion of toolbars. The Extension QuickFolders creates a new toolbar on which folders can be dragged; this creates toolbarbuttons that look like tabs - they wrap to the next line (max-height is set to 25em) expanding the toolbar vertically. Since the upgrade to 3.0.1 the toolbar does not expand (vertically) anymore. The box containing the folders has these rules:
#QuickFolders-FoldersBox { display:  -moz-inline-box; float:right; max-height:25em; }
in the XUL file it is:
<box id="QuickFolders-FoldersBox" flex="10">
tried using various themes, same effect.


Reproducible: Always

Steps to Reproduce:
1. Install the QuickFolders extension
2. Drag Folders (from the folder tree) onto the toolbar, causing the folders to wrap to the next line
3. Make Thunderbird narrower so that right-most folder buttons will be hidden.
Actual Results:  
Folder Tabs to the right disappear

Expected Results:  
Folder Tabs wrap to the next line and the toolbar increases in height.

Comment 1

[Axel Grude [:realRaven]]

Using the DOM inspector, I found out that if you have multiple (user defined) toolbars e.g. Tag Toolbar there are pushed so far down the mail-toolbox, that they become invisible. This can be easily tested by clicking on the toolbars in the DOM-Node Tree while the "Blink selected Elements" option is enabled. So this is likely going to affect a lot of extensions that create toolbars which are displayed within the mail-toolbox.`As a workaround, I have changed overflow-y from hidden to visible, (so I can scroll into the toolbars) but that doesn't change the height restriction.

Comment 2

[Axel Grude [:realRaven]]

The bug can be worked around by adding this rule set to userchrome.css:
#mail-toolbox { overflow-x:visible !important; overflow-y:visible !important;}

the "offending rules" of setting overflows to hidden are in 
chrome://messenger/content/messenger.css

Comment 3

[Andreas Nilsson (:andreasn)]

Can you do a patch or diff of that?
Let me know if you need help setting up a build environment (it's so easy even I managed to do it :).

Comment 4

[Axel Grude [:realRaven]]

Shouldn't the Fix from Bug 536245 rolled back for this? I am only an extension editor at the moment, so I haven't ever built TB.

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #4)
> Shouldn't the Fix from Bug 536245 rolled back for this? I am only an extension
> editor at the moment, so I haven't ever built TB.

Not necessarily. AFAIK a fix was suggested on the newsgroups that would mean that bug 536245 can stay in and toolbars still work for your extension.

Comment 6

[Axel Grude [:realRaven]]

IMHO - the original bug proposed to make overflow hidden for the
toolbox container - causing some bad side effects for user created toolbars. 

Instead, we should only hide the overflow for the actual
(first) toolbar (mail-bar3); if I add these rules, I can fix both issues, from Bug 541052 and Bug 536245. Note that overflow-y does not need to be specified.

#mail-toolbox { /* this part merely undoes the rules from Bug 536245 */
  overflow-x:visible !important;
  overflow-y:visible !important;
}
#mail-bar3 {
    overflow-x:hidden !important;
}
I will add an attachment with a screenshot.

Comment 7

[Axel Grude [:realRaven]]

Attachment: TB after fix

For this test, I have added buttons to the toolbar and switched it to "Icons
beside Text".
After applying the userChrome rules from my previous comment:

1. the mail toolbar is cut off at the right, as expected
2. the quickfolders toolbar can expand (as designed) by floating its buttons
down
3. the other screen elements are not cut off (e.g. scrollbars of message pane
are visible)

Can somebody more versed in patching TB create a patch? I believe the overflow
rules that were added in Bug 536245 for #mail-toolbox have to be removed.

Comment 8

[Philip Chee]

> Created an attachment (id=423003)
> TB after fix
Aaargh! My eyes hurt!

Comment 9

[Axel Grude [:realRaven]]

ROFL! 8P

I just like Plashtic (Mostly Crystal)! I know that example was a bit extreme ... teehee... but you get the point ;}

Comment 10

[Blake Winton (:bwinton) (:☕️)]

Attachment: A patch to change the overflow rules for the toolbar.

It would be really nice if we could drive this in by Monday's code freeze.  (And nicer still if I didn't have to work on that Monday, cause it's Family Day. :)

Screenshot coming up.

Comment 11

[Blake Winton (:bwinton) (:☕️)]

Attachment: A screenshot of the previous patch.

There you go, Bryan.  Lemme know what you think.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

First, let me state that while I asked you to look at this bug (and very much appreciate that you have), I'm also very unwilling to ask you to spend your own weekend or holiday time on it, and I think you shouldn't do it (same goes from Bryan regarding ui-review).  (It's not like this release is a security firedrill, after all).  

I'm going to steal the review from philor, and mark this r+, since I have no idea whether he's likely to see this or be around this weekend.  I really dislike that we haven't had time to write a test here, but I think shipping without one is slightly better than slipping to 3.0.3.

Bryan, if you get time to offer ui-r today and decide on ui-r+, please add the checkin-needed keyword to this bug (for the reader following along at home, Bryan's out all of next week, so today will be the last chance).  Since Standard8 is gone for the weekend and out on Monday, a=dmose for landing on both trunk and branch assuming ui-r+ is granted.

All that said, I think slipping this fix to 3.0.3 wouldn't be the end of the world.  If it doesn't make it, life will go on.

Comment 13

[Axel Grude [:realRaven]]

Hi Blake, thanks for looking at this on a Saturday!
Sorry that patch doesn't work it causes a scrollbar to appear - for some reason the toolbar area does not expand vertically for this.

But  what about this:
#mail-toolbox {
  overflow-x:visible !important;
  overflow-y:visible !important;
}
#mail-bar3 {
  overflow-x:hidden !important;
}

This will restrict the mail-bar3 width without impacting on the toolbox as a whole! I don't know how to turn this into a patch [I am only writing extensions, so I currently haven't got the know how to build TB myself], but if you like me to test against the original bug to see that it cures the original problem please give me a shout.

Comment 14

[Axel Grude [:realRaven]]

Attachment: Using #mail-bar3 {overflow-x:hidden !important;}

screen behavior after applying the rules from my previous comment.
It also seems to fix the behavior of  Bug 536245. Of course there might be other build in toolbars in TB3 that need the same rule as

#mail-bar3 {overflow-x:hidden !important;}

It also is important to keep 
#mail-toolbox { overflow-x:visible; overflow-y:visible;}

because the Users won't like the scrollbar - expectation is that the any toolbar areas (Menus / tab bars) contain usable controls are always visible.

Comment 15

[Axel Grude [:realRaven]]

(In reply to comment #14)
There is a typo in the screen shot, it should be 
"cropping starts from the right"
and not
"cropping starts right to left".
That obviously doesn't make sense.
Oh and yes, I would love if somebody could turn that into a patch, if it doesn't go into 3.0.3 that's no major issue, as long as I can tell the users it is going to be fixed eventually.

In the meantime, is it possible (and desirable) to include these css rules in my extension's css files, until the fix made its way into the trunk?

Comment 16

[Blake Winton (:bwinton) (:☕️)]

Attachment: [checked-in] A patch that's more like Axel's.

(In reply to comment #14)
> Of course there might be
> other build in toolbars in TB3 that need the same rule as
> #mail-bar3 {overflow-x:hidden !important;}

Yeah, it's the other toolbars that may need this that's my main problem with the patch.

Having said that, I tried it with the QuickFolders, and with a custom toobar, and couldn't see anything wrong, so maybe it won't be too bad.  And we can suggest to extension developers that they add a similar rule if we run into problems.

> It also is important to keep 
> #mail-toolbox { overflow-x:visible; overflow-y:visible;}
> because the Users won't like the scrollbar - expectation is that the any
> toolbar areas (Menus / tab bars) contain usable controls are always visible.

Yeah, I agree, but as it turns out, that's the default, so we don't need to do anything other than revert the previous change if we want this in core.

Later,
Blake.

Comment 17

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 426829 [details] [diff] [review]
[checked-in] A patch that's more like Axel's.

looks good to me

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 426829 [details] [diff] [review]
[checked-in] A patch that's more like Axel's.

r=dmose, conditional on adding some wiki-content to MDC so that authors have some source of info to know about what they should be doing with their toolbars.

Comment 19

[Blake Winton (:bwinton) (:☕️)]

Pushed to comm-1.9.1 as http://hg.mozilla.org/releases/comm-1.9.1/rev/d291fafce578

(And I'm waiting until comm-central is a little less burny to push there.)

Comment 20

[Blake Winton (:bwinton) (:☕️)]

And finally pushed to comm-central as
http://hg.mozilla.org/comm-central/rev/97a3ccf97728

Comment 21

[Axel Grude [:realRaven]]

Fixed, as of Thunderbird 3.0.3 :)