Closed Bug 553757 Opened 14 years ago Closed 10 years ago

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

Categories

(MailNews Core :: Account Manager, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 33.0

People

(Reporter: BenB, Assigned: neil)

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
Product: Thunderbird → MailNews Core
Attached patch Possible fix (obsolete) — Splinter Review
(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)
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.  ;-)
Attached patch Possible patchSplinter Review
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 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+
Assignee: rsx11m.pub → nobody
Attachment #8424493 - Attachment is obsolete: true
Attachment #8424493 - Flags: review?(neil)
Assignee: nobody → neil
(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.
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...
I'd love to but nobody on IRC will admit to being a suitable reviewer.
Either of mconley, mnyromyr, bwinton, or IanN maybe?
(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).
Attachment #8424503 - Flags: review?(mconley)
Attachment #8424503 - Flags: review?(iann_bugzilla)
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.
(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.
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" ?
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+
Pushed comm-central changeset 6af58bc18eed.
Status: NEW → RESOLVED
Closed: 10 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.