681735 - Account central is too tall to fit into a small window (600px high) because of too large fixed spaces between items / groups of items on Windows and Linux.

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[David E. Ross]

Mozilla/5.0 (Windows NT 5.1; rv:6.0) Gecko/20110812 Thunderbird/6.0
Default 6.0 theme
View > Layout > Wide View

When I select an account (not a folder or newsgroup within the account), the right-hand pane displays some maintenance options for the selected account.  That pane has much wasted vertical space, which makes it too tall.  Yes, there is a vertical scroll bar; and fixing this might not eliminate the need for that scroll bar.

Comment 1

[David E. Ross]

Attachment: Account Maintenance pane scrolled to top

Comment 2

[David E. Ross]

Attachment: Account Maintenance pane scrolled to bottom

This and the prior attachment illustrate what I mean by the "Account Maintenance pane".  

Note:  This is similar to bug #416263.  However, it affects a different display.  It might be a problem within the Default 6.0 theme, in which case please change the component.

Comment 3

[:aceman]

Yeah, there is a lot of blank space.
I have done some work in the Account central, I could look at this but need UI decision.
I think this is shared with Seamonkey too.

Comment 4

[:aceman]

Bwinton, is there any way to make the spacing between the groups (but also between individual actions) flexible? Maybe making the 'separator' elements mail/themes/gnomestripe/mail/accountCentral.css use percentage height?

Comment 5

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

Aceman: You could try adding spacers with flex attributes between the elements, if it's XUL.  If it's html, maybe set the height of elements in percentages?

Comment 6

[:aceman]

It is XUL with css, so probably both ways could work. I'll check it out.

Comment 7

[:aceman]

This must wait on bug 713277 to land as would change the msgAccountCentral.xul again.

Comment 8

[:aceman]

Attachment: patch experiment

Make <separators> flexible so that they nicely stretch as the window stretches vertically, but set max-height (instead of fixed height) on them so that they do not become too large.

The actions on a standard POP3 account now fit into a 550px high window. (The reported had 600px). I think whole TB does not target smaller window size.

Comment 9

[:aceman]

I've only done Linux and Windows theme in the patch. Seamonkey probably just needs to copy the changes to the proper files in /suite, if anybody would like to try it.

Comment 10

[Philip Chee]

I suggest taking out most or all of the thin separators and then replacing the whole group of MessagesSection.separator{1,2,3} with one <spacer flex="1"/>
Similarly for the group of AccountsSection.separator{1,2,3} separators.

Aceman suggests that there should be a max-height on the new spacers.

Comment 11

[:aceman]

I also suggest keeping the .thin separators (but change them to spacers) and make them flexible so that they get away at small window sizes. But with normal sized windows the result will be mostly the same as it is today.

Comment 12

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

I tried reproducing this on Mac, but couldn't. :(  So it'll have to wait until I get a Windows/Linux box…  (Or you could ask mconley to ui-review it.  I'll trust his judgement on this.)

Comment 13

[:aceman]

What does it mean? What happens on Mac?

Comment 14

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

Comment on attachment 609009 [details] [diff] [review]
patch experiment

Mac gets all compressed just like you'ld expect it to.

So, under Windows Aero, I still see the scroll bar, but it looks way better.  So I'm going to say ui-r=me, but if you can find a way to stop the scrollbar from showing, that would be cool, too.

Thanks, and I apologize again for the delay in reviewing the patch.
Blake.

Comment 15

[:aceman]

I plan to make a proper patch (will all separators=>spacers revisited and cleaned up css) now after bug 713277 is out of the way. Maybe the Windows problem will go away.

It also looks like the separator.thin {} declarations weren't ever used as declarations in some core css was more specific.

Comment 16

[:aceman]

(In reply to Philip Chee from comment #10)
> I suggest taking out most or all of the thin separators and then replacing
> the whole group of MessagesSection.separator{1,2,3} with one <spacer
> flex="1"/>
> Similarly for the group of AccountsSection.separator{1,2,3} separators.
> 
> Aceman suggests that there should be a max-height on the new spacers.

If I merged the groups of three separators into one I would need a separate css rule for them (to have a max-height: 45px) and a separate rule for the thin ones (to have a max-height: 15px) as I want to preserve them (to keep the default design as it is now). So I am not sure if it is worth that or I can just leave the group (of 3) as is so that both sizes of spaces are covered with one rule. I'll produce a patch with the one rule first.

Comment 17

[:aceman]

Attachment: real patch

So, here is a better version. It does these changes to optimize the spacing:
- changes separators to spacers, removes class="thin"
- removes css referencing separators
- moves the spacer belonging to an action line above it (to that the last spacer is not below the last action on the page. It would just extend the page with padding, causing unnecessary scrollbar when window is small)
- removes spacers belonging to all the class="acctCentralTitleRow" elements. They are now spaced the same as before but thanks to previous point.
- the position and max-height of the spacers now mimic the previous state of the layout almost pixel perfect (tested on Linux).

Open question:
To mimic the big space between the last action of a group and the next group heading, there are about 8 of these small spacers needed. Is it OK to have them like that? Or is it worth to collapse them into 1 with flex="8" and add a new css rule into the theme for spacer.big {max-height: 4.5em}? Then we could then even remove the UncollapseSectionSeparators() function in msgAccountCentral.js.

Comment 18

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

Comment on attachment 618421 [details] [diff] [review]
real patch

Looks good to me on Mac and Windows!  (I haven't tested Linux due to lack of a VM, but I'll trust that you fixed the problem there, too. ;)

ui-r=me!

Comment 19

[:aceman]

Philip?

Comment 20

[Philip Chee]

Sorry will try to get back to you as soon as possible.

Comment 21

[Philip Chee]

Comment on attachment 618421 [details] [diff] [review]
real patch

> So, here is a better version. It does these changes to optimize the spacing:
> - changes separators to spacers, removes class="thin"
> - removes css referencing separators
> - moves the spacer belonging to an action line above it (to that the last spacer is not 
> below the last action on the page. It would just extend the page with padding, causing 
> unnecessary scrollbar when window is small)
> - removes spacers belonging to all the class="acctCentralTitleRow" elements. They are 
> now spaced the same as before but thanks to previous point.

You forgot to fix Lightning: lightning-newCalendar-row and lightning-newCalendar-separator.

> - the position and max-height of the spacers now mimic the previous state of the layout 
> almost pixel perfect (tested on Linux).

> Open question:
> To mimic the big space between the last action of a group and the next group heading, 
> there are about 8 of these small spacers needed. Is it OK to have them like that? Or is 
> it worth to collapse them into 1 with flex="8" and add a new css rule into the theme for 

Yes I think one spacer with flex="8" is better. Also try reducing flex to < 8 you might not need such a large flex.

> spacer.big {max-height: 4.5em}? Then we could then even remove the 

I don't think obsessing over pixel perfect matching of the previous UI state is necessary. Just set it to 4em (or 5em).

> UncollapseSectionSeparators() function in msgAccountCentral.js.
Removing code is good!

> +spacer {
> +  max-height: .55em;
0.55em Really? well if you replace this with spacer.big with an integer em this would be better.

> -      <separator id="acctCentralHeader.separator"/>
> +      <spacer id="acctCentralHeader.separator1" flex="1"/>
> +      <spacer id="acctCentralHeader.separator2" flex="1"/>

Please change all .separatorX to .spacerX as they aren't separators any more.
And try one spacer with flex="2".

Comment 22

[:aceman]

Attachment: patch v2

Fixes Philip's comments, fixes calendar item in AC and also changes the "add a new calendar" icon to a 24px high one because the current one is 32px and makes the whole row higher than the other rows (items/actions).

Comment 23

[:aceman]

Ah, the patch only fixes gnomestripe in calendar. The other themes will be updated if the change in icon is accepted by the UI reviewer.

Comment 24

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Could you give me a screenshot of this patch applied? Also, how did you get the 24px icon? Did you resize the original vector image or just resized the 32px image?

Comment 25

[:aceman]

Attachment: screenshot of the patch v2 in action

I just copied calendar-event-dialog.png from calendar/base/themes/gnomestripe/icons and named is cal-icon24.png. The picture in the image is very similar (shows a page of calendar) to the cal-icon32.png image.

Screenshot attached. Has the new icon and nicely aligned items and TB window shrunken to very small size so that it is seen how the spacing between the items shrinks too to fit.

Comment 26

[Philip Chee]

> The picture in the image is very similar (shows a page of calendar) to the
> cal-icon32.png image.
Yes but does it fit on all three platforms? I'm pretty sure it doesn't match on Pintripe/OSX.

Comment 27

[Philip Chee]

Comment on attachment 625482 [details]
screenshot of the patch v2 in action

I would prefer to see screenshots on all three tier one platforms (linux/gtk, osx, and windows aero/non-aero). Thanks.

Comment 28

[:aceman]

I can't produce the other platforms...
But the idea is to have the same icon on all of them. Of course, if you find better ones I can wire them up per platform.

Comment 29

[Philip Chee]

> But the idea is to have the same icon on all of them. Of course, if you find better
> ones I can wire them up per platform.
You can ask andreasn to make some new icons for each platform.

Comment 30

[:aceman]

Andreasn, can you help us here?

Comment 31

[:aceman]

Attachment: patch v3

Bwinton, can you try out the patch on all platforms + Win Aero?
The intent is to show the 7th icon from the toolbar-large.png image (toolbar-small.png on OS X) as the calendar icon in Account central.

Can any body try on Seamonkey?

I could only test it on Linux.

Comment 32

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 626596 [details] [diff] [review]
patch v3

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

f- since I don't think we should use the toolbar icons.

::: calendar/lightning/themes/common/suite-accountCentral.css
@@ +37,5 @@
>  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
>  
>  #lightning-newCalendar-row {
> +  list-style-image: url("chrome://calendar/skin/toolbar-large.png");
> +  -moz-image-region: rect(0px 168px 24px 144px);

IIRC then mac uses 24px icons in the toolbar, so setting this for suite might fail. I think its best to use a 24x24px version of the cal-icon32.png.

::: calendar/lightning/themes/pinstripe/accountCentral.css
@@ +34,5 @@
>   *
>   * ***** END LICENSE BLOCK ***** */
>  
>  #lightning-newCalendar-row > hbox > label {
> +  background: -moz-image-rect(url(chrome://calendar/skin/toolbar-small.png), 0, 168, 24, 144) no-repeat !important;

As mentioned, mac positions might be wrong

Comment 33

[:aceman]

Yeah, I wondered why there is only one definition for suite for all platforms, when TB had 3.

Andreasn, it seems your idea didn't work.
It seems this is a problematic topic and I'll better drop this from the patch, it is not needed for the solution of this bug.

Comment 34

[:aceman]

Attachment: patch v4

Reduced patch. This already has ui-r from bwinton. And fixes nits of Philip Chee from comment 21.

Comment 35

[Andreas Nilsson (:andreasn)]

(In reply to :aceman from comment #33)
> Yeah, I wondered why there is only one definition for suite for all
> platforms, when TB had 3.
> 
> Andreasn, it seems your idea didn't work.
> It seems this is a problematic topic and I'll better drop this from the
> patch, it is not needed for the solution of this bug.

I can cut these out as a separate image if needed. How about we do that as a followup bug?

Comment 36

[:aceman]

(In reply to Andreas Nilsson (:andreasn) from comment #35)
> I can cut these out as a separate image if needed. How about we do that as a
> followup bug?
Ah I didn't say it explicitly: I created bug 758306 for exactly this. To decide what needs to be done with the calendar icon.

The bug here stays for the spacing between the actions on the Account central, as originally intended.

Comment 37

[Philip Chee]

Comment on attachment 626907 [details] [diff] [review]
patch v4

Very sorry for the extreme delay. Everything looks good here.

Comment 38

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 626907 [details] [diff] [review]
patch v4

This patch appears to have bitrotted. aceman, can you de-bitrot it so I can give it a spin?

Comment 39

[Philip Chee]

Attachment: Patch v4.1 fix bitrot

> This patch appears to have bitrotted. aceman, can you de-bitrot it so I can
> give it a spin?

It's just the licence header change in accountCentral.xul. I unbitrotted it in order to review it so I might as well upload it for aceman.

Comment 40

[:aceman]

Yeah, thanks!

Comment 41

[:aceman]

Comment on attachment 625482 [details]
screenshot of the patch v2 in action

This screenshot shows how the spacing shrinks, but no longer represents the calendar icon used in further patches.

Comment 42

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 634055 [details] [diff] [review]
Patch v4.1 fix bitrot

This looks good to me. Thanks aceman.

Comment 43

[:aceman]

Thanks! I think this has enough reviews (bwinton, philip.chee, mconley).

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/85ab3ec4b3f1

Comment 45

[David E. Ross]

Mozilla/5.0 (Windows NT 5.1; rv:16.0) Gecko/20121005 Thunderbird/16.0

I still see this problem.

Comment 46

[:aceman]

Screenshot?

Comment 47

[David E. Ross]

Attachment: Screen shot of too-tall Account Settings window.

This is an image of an Account Settings window with the bottom off the bottom of my monitor screen.  Since the title bar is available, this one can be resized to fit the screen.

Comment 48

[David E. Ross]

Attachment: Another screen shot of a too-tall Account Settings window

This is an image of an Account Settings window with the top off the top of my monitor screen.  Since the title bar is not available, this one cannot be resized to fit the screen.

Comment 49

[:aceman]

But this bug is solely for the Account central.

You already filed bug 700039 for the Account settings window.

Comment 50

[David E. Ross]

Oops!  You are correct.  It is bug #700039.