308062 - Trust CA dialogue: "trust this certif for" options text is cut off

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

In the new Trust a new CA dialogue, the text of the "Trust this certificate
for:" options is cut off by default on the right.  The dialogue should be wide
enough by default.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: illustration of the issue

Comment 2

[Jasper]

I'd say it's save and UI&layout wise good to just shift that whole block of
checkboxes to the left so that the gap is filled.

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I tend to agree with Jasper; that block looks odd sitting out there, since
there's not another block in the left-hand column there (but it is lined up with
all the other right-hand column blocks).

The same thing appears in the certificate viewing window (where it's not cut
off), so if we move the entire block, we should also do so in certificate
viewing window, too.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Simon, is this easy to fix or should we push this off?

Comment 5

[Simon Fraser [no longer active]]

Should just be a nib window resize.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I imagine this is a problem in all of the certificate sheets.  I tried to fix it, but I somehow broke the nib trying to resize the CertificateView to match the resized scrollbox; I need to enroll in ".nib editing 102" before I can fix this one myself.

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

smfr, will you be able to make these changes for 1.0, if it's just resizing the nib windows?

Comment 8

[Samuel Sidler (old account; do not CC)]

-> 1.1

Comment 9

[Sean Murphy]

Attachment: Trust CA Dialog Patch

To keep the text from cutting off, we could have changed the starting frame size for the sheet which is displayed, but rather than take that approach, I modified the CertificateView code which programmatically inserts the checkbox views into the dialog.

The checkbox group now aligns to the same column as the top header labels.  Its x position is determined in the exact same way the header text fields calculate theirs.  So, the look and feel of the sheet is still balanced and appears nice and polished, despite moving around the checkboxes.  (Also looks fine with the extra details expanded.)

The only hitch is that there still might not be enough room for the text of other localizations.  We may have to check this..

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Reassigning this to Sean :)

Comment 11

[Nick Kreeger]

(In reply to comment #9)
> Created an attachment (id=238961) [edit]
> Trust CA Dialog Patch
> 
> To keep the text from cutting off, we could have changed the starting frame
> size for the sheet which is displayed, but rather than take that approach, I
> modified the CertificateView code which programmatically inserts the checkbox
> views into the dialog.
> 
> The checkbox group now aligns to the same column as the top header labels.  Its
> x position is determined in the exact same way the header text fields calculate
> theirs.  So, the look and feel of the sheet is still balanced and appears nice
> and polished, despite moving around the checkboxes.  (Also looks fine with the
> extra details expanded.)
> 
> The only hitch is that there still might not be enough room for the text of
> other localizations.  We may have to check this..
> 

Correct, this approach only fixes this problem for some localizations. We should really find a way of telling the UI to wrap the text, or do the wrap drawing our selves.

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 238961 [details] [diff] [review]
Trust CA Dialog Patch

Yeah, that sheet is pretty badly broken in other languages as-is.  (The more common one, the url mismatch sheet, is in better shape).  But let's make reworking the sheet another bug for later.  This patch is a significant improvement for everyone.

Comment 13

[froodian (Ian Leue)]

Comment on attachment 238961 [details] [diff] [review]
Trust CA Dialog Patch

I agree that this is a major improvement (even aesthetically IMO - the checkboxes look nice lined up with the header fields), so r=me on the patch.

Just to give it a little more elbow room, could you submit a revised nib (just zip it up and attach the whole thing) with a slightly wider window (and possibly slightly wider minimum width as well)?

r=me on the code changes, and a=me for the fix with a nib.

Comment 14

[Sean Murphy]

Attachment: DownloadCACertDialog.nib with increased window/sheet width

The initial width of the sheet has been increased from 550 to 600, which, when combined with the previous patch, prevents any of the labels from being cut short.  This nib resize also enables the RSA key text fields (when details are expanded) to fit cleanly without extra line breaks.  The minimum allowed width has also been changed to be identical to this new initial width, since any smaller size causes issues.

An additional problem with this nib is that the mMessageField IBOutlet (which is the descriptive text under the sheet heading saying: "You have obtained a certificate...") has its string value set to a different value programmatically.  This means that to the localizing team, the label will appear to fit all text correctly, but the value which is set in code turns out to be much longer (as happens in the French version, for instance).  If this dialog (DownloadCACertDialog) always displays the same description text, should we just set this in Interface Builder and just use a format token for any unique info (such as cert name)?   

Also, speaking of localizations, ones that require longer labels will still have the original bug (cut-off checkbox labels) occur.  I don't mind reworking the way the sheet is presented, (so this doesn't continue to cause trouble) but should this be done under a different bug report?  Or, are there other issues which deserve attention first, leaving this for later (since this fix does work in most cases)?

[See attached before/after images]

I'll need another review on this, plus a super, I believe.. Thanks everyone for all the comments so far.

Note: This was my first patch submitted to the Camino project and I look forward to becoming involved, especially fixing some more bugs.  This is awesome work and I'm glad to help out a small part of it..

Comment 15

[Sean Murphy]

Attachment: [Screenshot] Checkbox labels before/after

Comment 16

[Sean Murphy]

Attachment: [Screenshot] RSA Key text fields before/after

Comment 17

[Sean Murphy]

Attachment: [Screenshot] RSA Key text fields before/after (fixed alignment of keys)

Comment 18

[Sean Murphy]

Attachment: [Screenshot] Shows what happens to the sheet title's descriptive text under French locale.

Comment 19

[Sean Murphy]

I don't want to get too off-topic from the original bug, but concerning mMessageField's stringValue, setting the text inside Interface Builder would require us to sacrifice using the more informative format string (which inserts the certificate name into the description at %@).  The only way, unless I'm forgetting something, to avoid a setStringValue message and still use a format string would be binding displayPatternValue, which isn't a solution for us.  So, on second thought, it might be best to leave the dialog alone.

If anything is changed, maybe just set the text field value in IB to the string that is actually inserted during runtime (when [mMessageField setStringValue:] is called).  I'm thinking those who localized this nib didn't realize the description string would be replaced with a longer one by code, and were sure it just looked alright.

I hope this makes sense.  I should mention again that the original bug is fixed with the above patch and nib (with some locales excluded, which we've acknowledged).

Comment 20

[froodian (Ian Leue)]

Comment on attachment 238961 [details] [diff] [review]
Trust CA Dialog Patch

Simon, do you think you could look at this for sr?  It still leaves some to be wanted for localizations, but it's worlds better than what we have now, and we can file a followup on making it "localization-proof" ;)

Comment 21

[froodian (Ian Leue)]

Checked in on 1.8branch and trunk.