866635 - refresh layout in the SMTP manager pane in the Account manager

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[:aceman]

Attachment: screenshot showing the problems.

I propose to fix some layout problems in the SMTP manager pane in the Account manager:

1. in the bottom summary block, the text fields do not align vertically with their labels. It may be because the labels ase <label> tags, while the values are a readonly <textbox>. Not sure if the textbox is still needed so I propose to just convert it to label.

2. There is empty space below the summary block, so I propose to extend the SMTP server list to more rows. Maybe we could make it dependent on the number of servers defined.

Comment 1

[:aceman]

Attachment: patch

Comment 2

[rsx11m]

You are patching mailnews/ but only the CSS rules for the Thunderbird themes.
Thus, SeaMonkey's themes don't need to be changed, or didn't you check those?

Comment 3

[:aceman]

Attachment: patch v2

Ding... Thanks for the reminder:)

Comment 4

[rsx11m]

Comment on attachment 743200 [details] [diff] [review]
patch v2

You are welcome. :-) 

>+listitem[default="true"],
> #identitiesList > listitem:first-child,
> treechildren::-moz-tree-cell-text(isDefaultServer-true) {
>   font-weight: bold;
> }

As another observation, the listitem[default="true"] rule looks very general here, I'm having a hard time trying to figure out what you are addressing here. Maybe make the rule a bit more specific?

Comment 5

[:aceman]

For me, I would just make it [default="true"], but that also hits default buttons ;) We have a bug 793819 to sort this out, if it should be very specific, or expose this style application wide.

Comment 6

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

Comment on attachment 743200 [details] [diff] [review]
patch v2

The 'rows="10"' seems to not let the box expand to the size you're hoping, so ui-r-, but I do like the text alignment changes, and if you can get the flex working, this should get a ui-r+.

Comment 7

[:aceman]

Do you have enough (10+) SMTP servers defined?

Comment 8

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

No, I only have two, but from a user's point of view, I don't understand why that should affect the flexing…  (I'm also going to claim without evidence that it is a very small percentage of our users who have 10 or more smtp servers.  ;)

Comment 9

[:aceman]

Yes, it looks like if there are less rows, the listbox does not expand to full 10 rows height.

chrome://global/content/xul.css contains listbox[rows] { height: auto; }
It then probably sets the height in a different way.

Comment 10

[Ian Neal]

Comment on attachment 743200 [details] [diff] [review]
patch v2

I'll wait for the new patch...

Comment 11

[:aceman]

bwinton, is it a problem if the listbox is not exactly 10 rows, depending on the contents of it?

Comment 12

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

Not really, no.  The ideal case in my mind is one of the following.  Either the list box only shows as many rows as you have servers (possibly plus one, so that you get the idea that you can add more), or the list box takes up all the extra space on the pane.

Comment 13

[:aceman]

Attachment: patch v3

OK, so what about this?

Comment 14

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

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

This patch doesn't seem to do either of the things I thought would be ideal…

https://dl.dropboxusercontent.com/u/2301433/Screenshots/LotsOfAccounts.png
https://dl.dropboxusercontent.com/u/2301433/Screenshots/FewAccounts.png

Are you seeing the same behaviour that I am when you run the patch?

Comment 15

[:aceman]

What is wrong in the screenshots? I'd say the list should not be smaller than the columns if buttons alongside it. But try to add more servers to let it grow until the limit (20 rows).

Comment 16

[:aceman]

Attachment: patch v4

But try with this one.

Comment 17

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

The server info should be at the bottom of the dialog, and the list of servers should take up the rest of the space.

Comment 18

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

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

(By "space" in my previous comment, I meant "height"…)

And since you didn't really know what I was asking for, I'm going to clear out the ui-r request.  If you think this patch actually does that, then feel free to re-set it, and I'll give it look as soon as I can.

Thanks,
Blake.

Comment 19

[:aceman]

That is in conflict with your comment 12, where you want the listbox to grow according to the number of servers, not just fill up the height.

Comment 20

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

(In reply to Blake Winton (:bwinton) from comment #12)
> Not really, no.  The ideal case in my mind is one of the following.  Either
> the list box only shows as many rows as you have servers (possibly plus one,
> so that you get the idea that you can add more), or the list box takes up
> all the extra space on the pane.

I see no conflict with at least half of my comment.  ;)  And the patch I tested didn't grow according to the number of servers+1, it had a fixed height.  (I tried to get a picture of the scrollbar, but couldn't due to it disappearing on OS X when you're not scrolling.)

So, which direction is the patch taking?  Should it grow according to the number of servers, or take up the extra space?

Thanks,
Blake.

Comment 21

[:aceman]

It does not grow dynamically when you add servers.
Only when you open the smtp pane, the listbox is resized to rows = MIN(number of servers+1, 20) and stays there until you leave the pane.

Comment 22

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

Right, so that doesn't sound like what I was asking for.  I'm not entirely sure what info you need from me to move forward, though.  I think I presented the two options fairly clearly…  I don't particularly mind which one gets picked.

Comment 23

[:aceman]

Then I probably didn't understand comment 12 and anything clarifying it would help me :)

If you mean dynamically expanding the listbox when a new server is added, then I do not like that much (it happens nowhere else).

Comment 24

[:aceman]

Josiah, could you help me here on what bwinton might have wanted?

Comment 25

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

Okay, since I'm still being cc-ed on this bug, I'll try and describe what I'm hoping for…

You can choose one of two behaviours.

1) (I don't think you like this, but it's an option.)
   The list box is as big as the number of servers you have plus one,
   and grows dynamically when you add servers, up to the size of the dialog.

2) The server info always appears at the bottom of the dialog,
   and the list box takes up all the vertical space between it and the header at the top.

Does that make sense?  Should I try to mock up an image?

Comment 26

[:aceman]

Yes please, try an image for option 2, so that we can finally clear this.

Comment 27

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

https://dl.dropboxusercontent.com/u/2301433/Screenshots/SomethingLikeThis.png

Comment 28

[:aceman]

Attachment: patch v5

OK, so something like this?

Comment 29

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

Comment on attachment 8422648 [details] [diff] [review]
patch v5

I mostly like it!  It would be nice if we could tighten up the minimum space a little (when the description is really long, it gets unwieldy).  Perhaps we could have the details block scroll internally?

Other than that, ui-r=me!
Thanks!

Comment 30

[:aceman]

(In reply to Blake Winton (:bwinton) [Away until Aug 23rd at least.] from comment #29)
> I mostly like it!  It would be nice if we could tighten up the minimum space
> a little (when the description is really long, it gets unwieldy).
What do you mean with tighten up?

> Perhaps we could have the details block scroll internally?
It is a <grid> so that doesn't look easy. But we could at least make the description inputbox have multiline=true to allow more lines and have an automatic scrollbar. However, it seems it expands to more lines even if the text is short.

Comment 31

[:aceman]

Paenglab, would you have an idea what bwinton wanted in comment 29?

Comment 32

[Richard Marti (:Paenglab)]

Attachment: pane with long description

What he means is, a long description makes the whole pane grow in the width as you can see on the screenshot.

I moved the pane with the scrollbar to show how the buttons on the top are moved outside the visible area.

The serverInfoBox should always use the whole visible width but when a label in this box is too long the serverInfoBox itself should become a scrollbar or the too long label should become a line break or scroll like it does now with the cursor in the read only textbox.

Maybe add a ellipsis and show a tooltip would also work.

Comment 33

[:aceman]

Attachment: patch v6

Ok, please try this.

Comment 34

[Richard Marti (:Paenglab)]

Comment on attachment 8650658 [details] [diff] [review]
patch v6

This looks good. Only a long comment without spaces expands the whole pane. But it's unlikely such a comment is used here.

Maybe you could change the serverDetails.label to "Details of selected server". I think you ask a reviewer which is better for native English who can give his feedback for this.

Comment 35

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #34)
> Comment on attachment 8650658 [details] [diff] [review]
> patch v6
> 
> This looks good. Only a long comment without spaces expands the whole pane.
> But it's unlikely such a comment is used here.
Maybe that is why <textbox>es were used for the values. Those do not expand, but crop, but the user can drag the mouse to scroll to the hidden text (state without the patch). But then we had no wrapping on long texts even if there were spaces in long descriptions. Not sure what is better.

I could put the labels into <textbox> and leave values as they are, so that they align again (either both are textboxes, or both are labels as in the patch). But then I think the control= attribute only works on labels. I am not sure if it is used for anything though.

Comment 36

[:aceman]

Attachment: patch v7

I couldn't make the label and the textbox baseline aligned in any combination (also not label and label). I propose to drop that part from the patch. I see the non-aligned baselines only on Win XP so maybe it does not hit other systems. I think we can proceed without that part.

Comment 37

[Richard Marti (:Paenglab)]

Comment on attachment 8658840 [details] [diff] [review]
patch v7

(In reply to :aceman from comment #36)
> Created attachment 8658840 [details] [diff] [review]
> patch v7
> 
> I couldn't make the label and the textbox baseline aligned in any
> combination (also not label and label). I propose to drop that part from the
> patch. I see the non-aligned baselines only on Win XP so maybe it does not
> hit other systems. I think we can proceed without that part.

It's also non-aligned on the other Windows and OS X. I'm fine with a follow-up bug for this. Or you could add to the labels a class something like "infoLabel" and then use this CSS:

@media (-moz-os-version: windows-xp) {
  .infoLabel {
    margin-top: 0;
  }
}

@media not all and (-moz-os-version: windows-xp) {
  .infoLabel {
    margin-bottom: 1px;
  }
}

OS X needs also:
.infoLabel {
  margin-bottom: 1px;
}

I haven't checked Linux, but when you say it doesn't have this problem, then okay.

Comment 38

[:aceman]

Yeah, I didn't want to hardcode the 1px because that will break again if the definitions/css of the standard xul elements change in the future (or even get replaced). Then the difference may become more/less than 1px and he have the bug back. Therefore I tried to use the same elements in both columns. But all of the options I tried had some other drawback.

Comment 39

[Ian Neal]

(In reply to :aceman from comment #38)
> Yeah, I didn't want to hardcode the 1px because that will break again if the
> definitions/css of the standard xul elements change in the future (or even
> get replaced). Then the difference may become more/less than 1px and he have
> the bug back. Therefore I tried to use the same elements in both columns.
> But all of the options I tried had some other drawback.

Don't forget you will need a ui-r/r from an SM peer too, I would suggest Neil as he can do both at the same time.

Comment 40

[:aceman]

Comment on attachment 8658840 [details] [diff] [review]
patch v7

As you can see in the patch, I was thinking of you, but surely I can try Neil when you say so :)

Comment 41

[[e-mail]]

(In reply to aceman from comment #38)
> Yeah, I didn't want to hardcode the 1px because that will break again if the
> definitions/css of the standard xul elements change in the future (or even
> get replaced). Then the difference may become more/less than 1px and he have
> the bug back. Therefore I tried to use the same elements in both columns.
> But all of the options I tried had some other drawback.

At least on my system, labels are 13px tall on both Windows XP and Linux; their margins make the row height 16px. Textboxes on the other hand are 16px tall on Linux, but only 13px tall on Windows XP. Sadly increasing the textbox height to 16px has no visible effect, so I don't know what's going on.

If you don't need to be able to copy the values, then <label flex="1" crop="right"> should work, I would have thought (maybe even without the flex="1" but I'm not sure).

I think the SMTP summary looks lonely at the bottom of the window (particularly on SeaMonkey where there are separate action buttons instead of an action menulist); would it be possible to have have half the gap between summary and bottom than there is now instead?

Comment 42

[[e-mail]]

(Beyond the scope of this bug, but maybe we should have all the SMTP servers as child entries of the Outgoing Server entry?)

Comment 43

[:aceman]

(In reply to [e-mail] from comment #41)
> At least on my system, labels are 13px tall on both Windows XP and Linux;
> their margins make the row height 16px. Textboxes on the other hand are 16px
> tall on Linux, but only 13px tall on Windows XP. Sadly increasing the
> textbox height to 16px has no visible effect, so I don't know what's going
> on.
> 
> If you don't need to be able to copy the values, then <label flex="1"
> crop="right"> should work, I would have thought (maybe even without the
> flex="1" but I'm not sure).
Somehow none of the solutions played well with the various "pack" and "crop" (or wrap) options. At least if we want to keep the "pack=end" on the field labels. I wonder how that table actually works on LTR locales, when the direction of the values labels/textboxes is not specified (no "pack=start").

> I think the SMTP summary looks lonely at the bottom of the window
> (particularly on SeaMonkey where there are separate action buttons instead
> of an action menulist); would it be possible to have have half the gap
> between summary and bottom than there is now instead?
You mean having the same half-gap before and after the details-block? I.e. having same flex on both sides (vertically)?

(In reply to [e-mail] from comment #42)
> (Beyond the scope of this bug, but maybe we should have all the SMTP servers
> as child entries of the Outgoing Server entry?)
I am sure there is a bug for this, but it is a larger project when somebody gets too much free time :)

Comment 44

[[e-mail]]

(In reply to aceman from comment #43)
> (In reply to comment #41)
> > If you don't need to be able to copy the values, then <label flex="1"
> > crop="right"> should work, I would have thought (maybe even without the
> > flex="1" but I'm not sure).
> Somehow none of the solutions played well with the various "pack" and "crop"
> (or wrap) options. At least if we want to keep the "pack=end" on the field
> labels. I wonder how that table actually works on LTR locales, when the
> direction of the values labels/textboxes is not specified (no "pack=start").
pack="start" is the default, but I don't see how that's relevant anyway.

> > would it be possible to have have half the gap
> > between summary and bottom than there is now instead?
> You mean having the same half-gap before and after the details-block? I.e.
> having same flex on both sides (vertically)?
I'm not sure what you mean by half-gap before the details; you can make the list bigger like you did before, just while still leaving some space at the bottom.

> (In reply to comment #42)
> > (Beyond the scope of this bug, but maybe we should have all the SMTP servers
> > as child entries of the Outgoing Server entry?)
> I am sure there is a bug for this, but it is a larger project when somebody
> gets too much free time :)
Don't be silly, nobody gets that much free time ;-)

Comment 45

[:aceman]

Attachment: patch v7.1

So you mean like this?

Comment 46

[:aceman]

Thanks.

Comment 47

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/c4ee04b393f7cb4012c680983b09005a6a3b3207
Bug 866635 - refresh layout in the SMTP manager pane in the Account manager. ui-r=bwinton, ui-r=Paenglab, r=IanN, r=mkmelin