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

RESOLVED FIXED in Camino1.5

Status

defect
--
minor
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: alqahira, Assigned: murph)

Tracking

({fixed1.8.1, polish})

1.8 Branch
Camino1.5
PowerPC
macOS
Dependency tree / graph

Details

(Whiteboard: [good first bug], )

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.

Comment 2

14 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.
Assignee: pinkerton → sfraser_bugs
Severity: trivial → minor
Target Milestone: --- → Camino1.0
Simon, is this easy to fix or should we push this off?
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

13 years ago
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

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

Comment 11

13 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 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

13 years ago
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 16

13 years ago
Assignee

Comment 19

13 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

13 years ago
Attachment #239194 - Flags: review+
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)
Attachment #238961 - Flags: superreview?(sfraser_bugs) → superreview+
Checked in on 1.8branch and trunk.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.