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

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
General
--
minor
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Smokey Ardisson (offline for a while; not following bugs - do not email), Assigned: Sean Murphy)

Tracking

({fixed1.8.1, polish})

1.8 Branch
Camino1.5
PowerPC
Mac OS X
fixed1.8.1, polish

Details

(Whiteboard: [good first bug], URL)

Attachments

(6 attachments, 1 obsolete attachment)

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.
Created attachment 195677 [details]
illustration of the issue

Comment 2

12 years ago
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.
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.

Updated

12 years ago
Assignee: pinkerton → sfraser_bugs
Severity: trivial → minor
Target Milestone: --- → Camino1.0
Simon, is this easy to fix or should we push this off?

Comment 5

12 years ago
Should just be a nib window resize.
Status: NEW → ASSIGNED
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.
smfr, will you be able to make these changes for 1.0, if it's just resizing the nib windows?
-> 1.1
Target Milestone: Camino1.0 → Camino1.1
Assignee: sfraser_bugs → nobody
Blocks: 325880
Status: ASSIGNED → NEW
QA Contact: general
Whiteboard: [good first bug]
(Assignee)

Comment 9

11 years ago
Created attachment 238961 [details] [diff] [review]
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..
Reassigning this to Sean :)
Assignee: nobody → spmurph
(Assignee)

Updated

11 years ago
Attachment #238961 - Flags: review?(stridey)

Comment 11

11 years ago
(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 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

11 years ago
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.
Attachment #238961 - Flags: review?(stridey) → review+
(Assignee)

Comment 14

11 years ago
Created attachment 239194 [details]
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..
(Assignee)

Comment 15

11 years ago
Created attachment 239195 [details]
[Screenshot] Checkbox labels before/after
(Assignee)

Comment 16

11 years ago
Created attachment 239196 [details]
[Screenshot] RSA Key text fields before/after
(Assignee)

Comment 17

11 years ago
Created attachment 239197 [details]
[Screenshot] RSA Key text fields before/after (fixed alignment of keys)
Attachment #239196 - Attachment is obsolete: true
(Assignee)

Comment 18

11 years ago
Created attachment 239200 [details]
[Screenshot] Shows what happens to the sheet title's descriptive text under French locale.
(Assignee)

Comment 19

11 years ago
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).

Updated

11 years ago
Attachment #239194 - Flags: review+

Comment 20

11 years ago
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" ;)
Attachment #238961 - Flags: superreview?(sfraser_bugs)

Updated

11 years ago
Attachment #238961 - Flags: superreview?(sfraser_bugs) → superreview+

Comment 21

11 years ago
Checked in on 1.8branch and trunk.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.