Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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.

RESOLVED FIXED in Thunderbird 16.0

Status

MailNews Core
Account Manager
--
minor
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: David E. Ross, Assigned: aceman)

Tracking

({polish, uiwanted})

Trunk
Thunderbird 16.0
polish, uiwanted
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 4 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
Created attachment 555509 [details]
Account Maintenance pane scrolled to top
(Reporter)

Comment 2

6 years ago
Created attachment 555511 [details]
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.
Component: General → Account Manager
QA Contact: general → account-manager
Summary: Account Maintenance Pane Is Too Tall → Account central Is Too Tall
(Assignee)

Comment 3

6 years ago
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.
Assignee: nobody → acelists
Keywords: uiwanted
OS: Windows XP → All
Hardware: x86 → All
Version: unspecified → Trunk
(Assignee)

Comment 4

5 years ago
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?
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?
(Assignee)

Comment 6

5 years ago
It is XUL with css, so probably both ways could work. I'll check it out.
(Assignee)

Comment 7

5 years ago
This must wait on bug 713277 to land as would change the msgAccountCentral.xul again.
Status: NEW → ASSIGNED
Depends on: 713277
Keywords: polish
(Assignee)

Comment 8

5 years ago
Created attachment 609009 [details] [diff] [review]
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.
Attachment #609009 - Flags: ui-review?(bwinton)
(Assignee)

Updated

5 years ago
Component: Account Manager → Account Manager
Product: Thunderbird → MailNews Core
QA Contact: account-manager → account-manager
(Assignee)

Comment 9

5 years ago
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.
(Assignee)

Updated

5 years ago
Summary: Account central Is Too Tall → Account central is too tall to fix into a small window (600px high) because of too large fixed spaces between items / groups of items

Comment 10

5 years ago
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.
(Assignee)

Comment 11

5 years ago
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.
(Assignee)

Updated

5 years ago
Summary: Account central is too tall to fix into a small window (600px high) because of too large fixed spaces between items / groups of items → Account central is too tall to fit into a small window (600px high) because of too large fixed spaces between items / groups of items
Summary: Account central is too tall to fit into a small window (600px high) because of too large fixed spaces between items / groups of items → 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.
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.)
(Assignee)

Comment 13

5 years ago
What does it mean? What happens on Mac?
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.
Attachment #609009 - Flags: ui-review?(bwinton) → ui-review+
(Assignee)

Comment 15

5 years ago
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.
(Assignee)

Comment 16

5 years ago
(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.
(Assignee)

Comment 17

5 years ago
Created 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.
- 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.
Attachment #609009 - Attachment is obsolete: true
Attachment #618421 - Flags: ui-review?(bwinton)
Attachment #618421 - Flags: feedback?(philip.chee)
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!
Attachment #618421 - Flags: ui-review?(bwinton) → ui-review+
(Assignee)

Comment 19

5 years ago
Philip?

Comment 20

5 years ago
Sorry will try to get back to you as soon as possible.

Comment 21

5 years ago
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".
Attachment #618421 - Flags: feedback?(philip.chee)
(Assignee)

Comment 22

5 years ago
Created attachment 625425 [details] [diff] [review]
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).
Attachment #618421 - Attachment is obsolete: true
Attachment #625425 - Flags: ui-review?(philipp)
(Assignee)

Comment 23

5 years ago
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.
No longer depends on: 713277
Depends on: 713277
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?
(Assignee)

Updated

5 years ago
Attachment #625425 - Flags: feedback?(philip.chee)
(Assignee)

Comment 25

5 years ago
Created attachment 625482 [details]
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.
Attachment #625482 - Flags: ui-review?(philipp)
Attachment #625482 - Flags: feedback?(philip.chee)

Comment 26

5 years ago
> 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

5 years ago
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.
Attachment #625482 - Flags: feedback?(philip.chee)
(Assignee)

Comment 28

5 years ago
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

5 years ago
> 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.
(Assignee)

Comment 30

5 years ago
Andreasn, can you help us here?
(Assignee)

Comment 31

5 years ago
Created attachment 626596 [details] [diff] [review]
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.
Attachment #625425 - Attachment is obsolete: true
Attachment #625425 - Flags: ui-review?(philipp)
Attachment #625425 - Flags: feedback?(philip.chee)
Attachment #626596 - Flags: ui-review?(bwinton)
Attachment #626596 - Flags: feedback?(philipp)
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
Attachment #626596 - Flags: feedback?(philipp) → feedback-
(Assignee)

Comment 33

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 758306
(Assignee)

Comment 34

5 years ago
Created attachment 626907 [details] [diff] [review]
patch v4

Reduced patch. This already has ui-r from bwinton. And fixes nits of Philip Chee from comment 21.
Attachment #626596 - Attachment is obsolete: true
Attachment #626596 - Flags: ui-review?(bwinton)
Attachment #626907 - Flags: review?(philip.chee)
(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?
(Assignee)

Comment 36

5 years ago
(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

5 years ago
Comment on attachment 626907 [details] [diff] [review]
patch v4

Very sorry for the extreme delay. Everything looks good here.
Attachment #626907 - Flags: review?(philip.chee) → review+
(Assignee)

Updated

5 years ago
Attachment #626907 - Flags: review?(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?
Attachment #626907 - Flags: review?(mconley) → review-

Comment 39

5 years ago
Created attachment 634055 [details] [diff] [review]
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.
Attachment #634055 - Flags: review?(mconley)
(Assignee)

Comment 40

5 years ago
Yeah, thanks!
(Assignee)

Comment 41

5 years ago
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.
Attachment #625482 - Flags: ui-review?(philipp)
Comment on attachment 634055 [details] [diff] [review]
Patch v4.1 fix bitrot

This looks good to me. Thanks aceman.
Attachment #634055 - Flags: review?(mconley) → review+
(Assignee)

Comment 43

5 years ago
Thanks! I think this has enough reviews (bwinton, philip.chee, mconley).
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/85ab3ec4b3f1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 16.0
(Reporter)

Comment 45

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

I still see this problem.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 46

5 years ago
Screenshot?
(Reporter)

Comment 47

5 years ago
Created attachment 670395 [details]
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.
(Reporter)

Comment 48

5 years ago
Created attachment 670398 [details]
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.
(Assignee)

Comment 49

5 years ago
But this bug is solely for the Account central.

You already filed bug 700039 for the Account settings window.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 50

5 years ago
Oops!  You are correct.  It is bug #700039.
You need to log in before you can comment on or make changes to this bug.