46.38 KB, image/jpeg
1.55 KB, patch
|Details | Diff | Splinter Review|
7.25 KB, application/zip
51.28 KB, image/gif
57.61 KB, image/gif
46.11 KB, image/png
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.
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?
Target Milestone: Camino1.0 → Camino1.1
13 years ago
Assignee: sfraser_bugs → nobody
Status: ASSIGNED → NEW
QA Contact: general
Whiteboard: [good first bug]
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
(In reply to comment #9) > Created an attachment (id=238961)  > 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+
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..
Created attachment 239197 [details] [Screenshot] RSA Key text fields before/after (fixed alignment of keys)
Attachment #239196 - Attachment is obsolete: true
Created attachment 239200 [details] [Screenshot] Shows what happens to the sheet title's descriptive text under French locale.
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 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
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.