1594367 - the account setup incoming/outgoing columns (fields) are too narrow to see the server names and usernames properly

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[Magnus Melin [:mkmelin]]

The current account setup incoming/outgoing columns (fields) are too narrow to see the server names and usernames properly.

They need to be wide enough that most server names, and most email addresses would never get out of view.

Comment 1

[Alessandro Castellani [:aleca]]

Is this a recent regression, or the default width of the dialog has always been too narrow?

Comment 2

[Alessandro Castellani [:aleca]]

Attachment: 1594367-narrow-dialog.patch

I restructured the columns and rows of the manual edit are in order to keep consistent sizing and let only the columns with the fields and menulist grow, while the column with the labels remains as narrow as possible.

Comment 3

[Alessandro Castellani [:aleca]]

Attachment: Screenshot from 2019-11-06 11-36-33.png

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9107023 [details] [diff] [review]
1594367-narrow-dialog.patch

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

Better. Some emails will still be truncated. Personally, the longest email I have is 27 ch, but I don't have a very long name....
Not sure why we don't put the full address in the manual config username field. That should probably also be fixed, and then at the moment, only 28 ch will fit. 

So, I think we need at least still a slight increase

::: mail/themes/shared/mail/creationDialog.css
@@ +293,5 @@
>    margin-top: 10px;
>  }
>  
> +.config-label {
> +  max-width: 10em;;

extra ;

Not sure if the width is ok for all localizations. We might just want to convert the whole manual area into using html:table, since this is really tabular data.

There is also some vertical misalignment in there now, for PORT/SSL/Authentication and Username rows

Comment 5

[Alessandro Castellani [:aleca]]

All right, I will update this patch with the following edits:

• Convert the area to use html:table.
• Increase a bit more the width of the dialog.
• Populate the username field with the full name.

Comment 6

[Alessandro Castellani [:aleca]]

Magnus, this is the method that extracts the username from the email address which is then used in the manual config username field.
https://searchfox.org/comm-central/rev/2c6b74cbc1457670a7e89613baee908837d73799/mail/components/accountcreation/content/accountConfig.js#269

Do you know why it's set that way and if it's safe to remove it and directly use the full email address?

Comment 7

[Alessandro Castellani [:aleca]]

Attachment: 1594367-narrow-dialog.patch

Meanwhile, I did the conversion to html:table.
I'm asking a UI review from Richard to be sure all the elements still align properly.

Comment 8

[Magnus Melin [:mkmelin]]

(In reply to Alessandro Castellani (:aleca) from comment #6)

Magnus, this is the method that extracts the username from the email address which is then used in the manual config username field.
https://searchfox.org/comm-central/rev/2c6b74cbc1457670a7e89613baee908837d73799/mail/components/accountcreation/content/accountConfig.js#269

Do you know why it's set that way and if it's safe to remove it and directly use the full email address?

Back in the day it used to be fairly popular to have a username, and then an email address. But this is getting more rare I think.
Anyway, it should be ok to remove - this is manual config after all. For autoconfig we do use full email by default, and worst case scenario the user has to delete the @domain part from that box (so not that bad).
I think the code you reference is just for some debug logging, and I do think you can just remove that.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9108291 [details] [diff] [review]
1594367-narrow-dialog.patch

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

::: mail/components/accountcreation/content/emailWizard.xul
@@ +320,5 @@
> +
> +        <html:table class="manual-config-table">
> +          <html:tr>
> +            <html:th class="config-label">
> +              <label/>

not needed

@@ +323,5 @@
> +            <html:th class="config-label">
> +              <label/>
> +            </html:th>
> +            <html:th>
> +              <label class="column-title" value="&incomingColumn.label;"/>

you'd put the class on the <th>, and the body is then only &incomingColumn.label; (no label)

@@ +326,5 @@
> +            <html:th>
> +              <label class="column-title" value="&incomingColumn.label;"/>
> +            </html:th>
> +            <html:th>
> +              <label class="column-title" value="&outgoingColumn.label;"/>

same things here and elsewhere: just put the content as the th/td body, and get rid of the <label>

@@ +331,5 @@
> +            </html:th>
> +          </html:tr>
> +
> +          <html:tr>
> +            <html:td class="config-label">

all the things in the leftmost column should be <th>s since they represent a header value for the row

::: mail/themes/shared/mail/creationDialog.css
@@ +299,5 @@
>  
> +.manual-config-table {
> +  display: flex;
> +  flex-direction: column;
> +}

probably shouldn't need the flex stuff now when it's a table

Comment 10

[Richard Marti (:Paenglab)]

Comment on attachment 9108291 [details] [diff] [review]
1594367-narrow-dialog.patch

Looks good. Thanks.

Comment 11

[Alessandro Castellani [:aleca]]

Attachment: display-table.png

Unfortunately, I think I need to use the display: flex; for the table as the XUL elements don't behave properly inside the html:table.

As you can see from the screenshot, using the simple table doesn't let the elements expand and grow through the width of the column.

The bottom panel shows what happens if I try to force those elements with a width: 100% attribute.

Using flexbox seems to be the safest and more consistent solution for the UI.

Comment 12

[Alessandro Castellani [:aleca]]

Attachment: 1594367-narrow-dialog.patch

Patch updated, and here's a try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0253ee2e232d4f83dff08c73fcee58ec45e2773b

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9108530 [details] [diff] [review]
1594367-narrow-dialog.patch

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

Looks good. I guess there could be a better way for the flex + table, but this works so let's with it for now.

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 9108530 [details] [diff] [review]
1594367-narrow-dialog.patch

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

::: mail/components/accountcreation/content/accountConfig.js
@@ -270,5 @@
> -      if (!username) {
> -        return "undefined";
> -      }
> -      let domain = username.split("@")[1];
> -      return domain ? "(redacted)@" + domain : "(redacted)";

Why is this bit OK to remove? Autoconfig prints a lot of stuff to the console and for privacy reasons the username was changed to "redacted".

Comment 15

[Jorg K (CEST = GMT+2)]

Code was added here: https://hg.mozilla.org/comm-central/rev/1cb168d17d1b#l1.54

Comment 16

[Magnus Melin [:mkmelin]]

I don't think there's a need to protect your (maybe) email address from being shown in logs on your own computer, and likely counterproductive since that could prevent you from finding an error. If you read the original, both you and Ben already there said it wasn't necessary.

Comment 17

[Jorg K (CEST = GMT+2)]

OK, if you want to remove the "redacting", then please inline the function. A function with one call site, a confusing name and a || "undefined"; body is not needed. Besides, to avoid confusion, I suggest to make it "(empty)" or "(undefined)".

Comment 18

[Alessandro Castellani [:aleca]]

Attachment: 1594367-narrow-dialog.patch

I removed that method and launched another try run to be sure nothing gets broken: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8b50ef84554f3cec074229d69937dbd18534f80a

Comment 19

[Jorg K (CEST = GMT+2)]

(Almost) excellent, but I asked for "(empty)" or "(undefined)".

Comment 20

[Alessandro Castellani [:aleca]]

Attachment: 1594367-narrow-dialog.patch

Oopsie, sorry about that.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d72ce60b5f76
Convert the manual config area in the emailWizard to <html:table>. r=mkmelin

Comment 22

[Jorg K (CEST = GMT+2)]

You want this in TB 71 beta 4?