SMTP server pref dialog: "Password, transmitted insecurely" doesn't fit after switching from SSL to non-SSL

RESOLVED FIXED in Thunderbird 33.0

Status

MailNews Core
Account Manager
--
minor
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: BenB, Assigned: neil@parkwaycc.co.uk)

Tracking

Trunk
Thunderbird 33.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

4.59 KB, patch
Ian Neal
: review+
mconley
: review+
rsx11m
: feedback+
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
Context:
Bug 525238 introduced an "Authentication method" widget in the SMTP server prefs (and in incoming server prefs). One of the options changes its label "Password, transmitted insecurely" to "Normal password" based on whether SSL or STARTTLS is selected or not (this is per discussion with Bryan at the beginning of the bug). 

Bug:
When the dialog comes up with SSL selected, the dialog and dropdown is sized for "Normal password", so when you turn off SSL, it's too small to keep "Password, transmitted insecurely".

Implementation:
Find some way/trick to make the dialog come up large enough to keep either of the labels.

Comment 1

4 years ago
I ran into this while working on bug 1009233, without finding a solution though. I had the idea to temporarily set the longer of the two strings and then enforce a reflow with something like this in smtpEditOverlay.js, function initSmtpSettings(server):

    setLabelFromStringBundle("authMethod-password-cleartext", "authPasswordCleartextInsecurely");
    var dummy = window.width;
    sslChanged(false);

but that didn't do the job, apparently the reflow either isn't initiated or hasn't enough effect to actually expand the box to the maximum width in the window's layout. The other option I've tried is adding another "authMethod-password-dummy" menuitem which receives the authPasswordCleartextInsecurely label and is subsequently hidden, but that's not working either.

Thus, the trick may be to keep the long label around long enough until the dimensions of the dialog window are determined, then hide it again or change the label if connection security is available, or determine the width explicitly and the use a sizeTo() method on the menupopup to resize it.

The "cheap" solution might be to just enforce a minimum width on smtp.authMethodPopup as a style, corresponding to the authPasswordCleartextInsecurely length.
Component: Account Manager → Account Manager
Product: Thunderbird → MailNews Core

Comment 2

4 years ago
Created attachment 8424493 [details] [diff] [review]
Possible fix

(In reply to rsx11m from comment #1)
> Thus, the trick may be to keep the long label around long enough until the
> dimensions of the dialog window are determined, then hide it again or change
> the label if connection security is available, or determine the width
> explicitly and then use a sizeTo() method on the menupopup to resize it.

So, this is not the "nicest" fix but works. Since all clientWidth calculations on the menulist, its menupopup, or the menuitems directly didn't yield useful results as they apparently aren't set to their final values before everything is properly rendered, I'm going with a dummy label which is filled with the (longer) string for insecure connections. In a second step, the clientWidth of that label is determined and then set as the minWidth of the menupopup element. After that, the dummy label is hidden to avoid that it's visible once the dialog is rendered.

To be l10n-safe, I've also tested putting both labels in a <vbox> and then measuring the vbox width to consider the maximum length, but this gives me 12px more than needed and lets the menu over-stretch by a bit.

Judging from http://mxr.mozilla.org/l10n-central/search?string=authPasswordCleartext it appears the the secure version is not longer than the insecure version of this label, thus the patch as posted here should be safe in all current locales.
Assignee: ben.bucksch → rsx11m.pub
Attachment #8424493 - Flags: review?(neil)

Comment 3

4 years ago
Oops, apologies to Ben - I didn't see that you had assigned this bug yourself already (I need to watch out for these things...). If my patch doesn't work out, feel free to take the bug back.  ;-)
(Assignee)

Comment 4

4 years ago
Created attachment 8424503 [details] [diff] [review]
Possible patch

Well, this is my take on a fix: I load up the menulist with the long string, size the dialog, then update the menulist as necessary.

While I was there I fixed a bug whereby the default selections for a new server weren't being set in particular the port was defaulting to 1. (This required accepting a port of 0 in the UI as the numberbox does not allow a blank port.)
Attachment #8424503 - Flags: feedback?(rsx11m.pub)

Comment 5

4 years ago
Comment on attachment 8424503 [details] [diff] [review]
Possible patch

Works for me and is certainly nicer than my hack.  :-)

>+    sizeToContent();

That was probably the function that I was looking for to get the correct maximum size...

>               <textbox id="smtp.port"
>                        type="number"
>-                       min="1"
>+                       min="0"
>                        max="65535"

I don't think that "0" is a valid port number?
Attachment #8424503 - Flags: feedback?(rsx11m.pub) → feedback+

Updated

4 years ago
Assignee: rsx11m.pub → nobody

Updated

4 years ago
Attachment #8424493 - Attachment is obsolete: true
Attachment #8424493 - Flags: review?(neil)

Updated

4 years ago
Assignee: nobody → neil
(Assignee)

Comment 6

4 years ago
(In reply to rsx11m from comment #5)
> I don't think that "0" is a valid port number?

The code looks for an unset value to determine whether to update it to the default value. This fails because the numberbox validates the value against its range. I had to add an out-of-range value to provide the possibility for the automatic default value code to kick in.

Comment 7

4 years ago
Neil, are you going to flag your patch for review? I can imagine that Thunderbird might be interested to take this fix for the upcoming 31.x releases...
(Assignee)

Comment 8

4 years ago
I'd love to but nobody on IRC will admit to being a suitable reviewer.

Comment 9

4 years ago
Either of mconley, mnyromyr, bwinton, or IanN maybe?

Comment 10

4 years ago
(where technically only mnyromyr is listed as module peer for mailnews/ afaik, but several combinations of the others have done reviews for the accountmanager files in the past).
(Assignee)

Updated

4 years ago
Attachment #8424503 - Flags: review?(mconley)
Attachment #8424503 - Flags: review?(iann_bugzilla)
(Reporter)

Comment 11

4 years ago
Good idea. Looks good to me.
r=BenB, if 1. below is fixed.

2 tiny comments:
1. You remove the comment "//authMethod-password-cleartext already set in sslChanged()"
   This comment is very important to understand the code.
   Please keep it and adapt it to the new situation.
2. I'd use
server.port || 0;
to be sure.
(Assignee)

Comment 12

4 years ago
(In reply to Ben Bucksch from comment #11)
> 1. You remove the comment "//authMethod-password-cleartext already set in
> sslChanged()"
>    This comment is very important to understand the code.
>    Please keep it and adapt it to the new situation.
Yes, I'd have to adapt it somehow.

> 2. I'd use
> server.port || 0;
> to be sure.
port is a... well, it's actually an int32_t attribute for some reason, although in .idl you're supposed to use long. So 0 is the only falsy value anyway.

Comment 13

4 years ago
Will this allow the user to set the port to 0 and close AM? I think I went over the account manager and specifically fixed these number fields to only allow proper ranges. So I do not like this change much. Maybe you can set some attribute on the textbox to indicate the value is "uninitialized" ?

Updated

4 years ago
Attachment #8424503 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 8424503 [details] [diff] [review]
Possible patch

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

Sorry for the delay - looks fine to me.
Attachment #8424503 - Flags: review?(mconley) → review+
(Assignee)

Comment 15

4 years ago
Pushed comm-central changeset 6af58bc18eed.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 33.0
You need to log in before you can comment on or make changes to this bug.