There are a lot of dialogs in security/manager/pki/ that don't use the <dialog> element. Some are using the deprecated dialogOverlay.xul, most use custom <button>s and <key>s to mimic the behaviour of dialogs. I started because I wanted to fix bug 194940. I then realised that all other dialogs in this area also suffer from the same problems described in that bug and so I ended up with a patch that fixes all these dialogs.

This patch also removes serverCrlExpired.xul and serverCrlExpired.js. These files are currently unused and are no longer needed according to bug 92475 comment 28. Furthermore it corrects some obvious small XUL errors like flex="100%" and the like. I also removed a Help button in getpassword.xul that caused an alert with "help goes here" to appear. serverCertExpired.xul now uses "Yes" and "No" instead of "Ok" and "Cancel", because the button press is an answer to the question "Do you want to continue anyway?". cacertexists.xul could be replaced with a call to nsIPromptService.alert(), but I won't do this change, because I don't know C++. I could file a follow-up bug if wanted. The change from <window> to <dialog> in this patch is better than nothing. The rest of the patch is hopefully straightforward. It shouldn't change the functionality of these dialogs. It just helps to maintain a consistent look and feel across all mozilla dialogs.

Comment on attachment 153588 [details] [diff] [review] Patch Neil, do you have time to review this?

(In reply to comment #1) >This patch also removes serverCrlExpired.xul and serverCrlExpired.js I don't see any jar.mn changes... >cacertexists.xul could be replaced with a call to nsIPromptService.alert() Plus a stringbundle...

(In reply to comment #1) >Furthermore it corrects some obvious small XUL errors like flex="100%" <dialog> <description flex="1"/> <description flex="1"/> </dialog> Since dialogs size to content there is no spare room to flex into. >serverCertExpired.xul now uses "Yes" and "No" instead of "Ok" and "Cancel", >because the button press is an answer to the question "Do you want to >continue anyway?". Then the buttons should be "Continue" and "Cancel". (From update of attachment 153588 [details] [diff] [review]) >+ var ok = document.documentElement.getButton("accept"); >+ ok.disabled = true; Not worth splitting this, surely?

Other dialog cleanup bugs you might be interested in: bug 76248, 7708, 78815, 78835, 94378, 106730, 123851, 125577, 125812, 129097, 135403, 157712

(In reply to comment #3) > (In reply to comment #1) > >This patch also removes serverCrlExpired.xul and serverCrlExpired.js > I don't see any jar.mn changes... Yes, these aren't included in the jar.mn file anymore. It was just forgotton to cvs rm them. ><dialog> > <description flex="1"/> > <description flex="1"/> ></dialog> >Since dialogs size to content there is no spare room to flex into. Ok, I'll scan the files for these unneeded flex attributes. >>serverCertExpired.xul now uses "Yes" and "No" instead of "Ok" and "Cancel", >>because the button press is an answer to the question "Do you want to >>continue anyway?". > Then the buttons should be "Continue" and "Cancel". Ok, this is even better. ;-) >>+ var ok = document.documentElement.getButton("accept"); >>+ ok.disabled = true; > Not worth splitting this, surely? I'll make this one line.

(In reply to comment #5) > Other dialog cleanup bugs you might be interested in: > > bug 76248, 7708, 78815, 78835, 94378, 106730, 123851, 125577, 125812, 129097, > 135403, 157712 7708 seems to be a typo. Bug 78815 and bug 94378 were already fixed My patch will fix bug 106730, bug 123851, bug 125577 and bug 129097. I'm not sure what remains to be done in bug 76248 after all dialogs are converted to <dialog>.

Addresses Neil's initial comments.

Same Patch, diff -uwN instead of diff -uN this time.

(In reply to comment #6) >(In reply to comment #3) >>(In reply to comment #1) >>>This patch also removes serverCrlExpired.xul and serverCrlExpired.js >>I don't see any jar.mn changes... >Yes, these aren't included in the jar.mn file anymore. It was just forgotton >to cvs rm them. Ah yes, those entities did get removed. But what about cacertexists.js? (In reply to comment #7) >My patch will fix bug 106730, bug 123851, bug 125577 and bug 129097. Bug 125812 should be simple too, no?

Comment on attachment 153643 [details] [diff] [review] Patch V1.1 Thanks for the -w patch, I meant to ask for it before :-) >- //Set the focus so key press events work >- document.getElementById('ok-button').focus(); > window.sizeToContent(); >+ document.documentElement.getButton("accept").focus(); You forgot to remove that window.sizeToContent(); >+ document.documentElement.getButton("accept").disabled = !(pw1 == pw2); What was wrong with != ? >+ <vbox id="certlist" flex="1"/> I think you can drop this flex too. >- >- // Set the focus >- document.getElementById('ok-button').focus(); Shouldn't you replace this, rather than just removing it? >+ <hbox> >+ <textbox id="pw" type="password" flex="1"/> >+ </hbox> This is just the same as <textbox id="pw" type="password"/> > window.sizeToContent(); >- doSetOKCancel(doOK, doCancel, null, null); You missed another chance here. >+ <tabpanels> >+ <vbox id="ssl2_ciphers"/> >+ <vbox id="ssl3tls_ciphers"/> >+ <vbox id="ssl3tls_extra_ciphers"/> >+ </tabpanels> Yummy, more useless flex removal :-) >-resetPasswordButtonLabel=Reset I think you forgot to change or diff resetpassword.js

Addressed all review comments.

Comment on attachment 153721 [details] [diff] [review] Patch V1.2 diff -uN Hopefully I have all files together this time. ;-) This patch also includes a fix for bug 125812.

Comment on attachment 153721 [details] [diff] [review] Patch V1.2 diff -uN I've only tested a couple of the dialogs.

Comment on attachment 153721 [details] [diff] [review] Patch V1.2 diff -uN Jag, can you sr? I still need moa after this.

If jag says this patch makes sense, moa granted

I'll try and get to it this week, otherwise this weekend.

*** Bug 253724 has been marked as a duplicate of this bug. ***

(In reply to comment #18) > I'll try and get to it this week, otherwise this weekend. Jag: ping ;-) Any updates? If you're to busy currently to sr this patch (no problem of course!), I could try to ask someone else (thought I think you're the best super-reviewer for this patch). Please just let me know. Thanks! :-)

Comment on attachment 153721 [details] [diff] [review] Patch V1.2 diff -uN Patch bitrotted slightly. :-(

This is identical to Patch V1.2, just updated to the current trunk.

Comment on attachment 167545 [details] [diff] [review] Patch V1.3 diff -uN Transferring Neil's r+ and asking dmose for sr again.

Stefan: I probably won't get to this until early or mid next week. If you need an sr sooner, feel free to find another reviewer.

(In reply to comment #25) > Stefan: I probably won't get to this until early or mid next week. If you need > an sr sooner, feel free to find another reviewer. No, this would be great! :-)

Comment on attachment 167546 [details] [diff] [review] Patch V1.3 diff -uwN security/manager/pki/resources/content/cacertexists.xul this now seems to only have a description... maybe the caller should use nsIPromptService?

Comment on attachment 167545 [details] [diff] [review] Patch V1.3 diff -uN I didn't realize how much of this patch was XUL. Unfortunately, my XUL fu is not so strong that I can give much in the way of useful reviews for large XUL patches. Perhaps try asking jag? Also, I assume this bug affects (and thus needs to be tested in) firefox and thunderbird as well as the application suite, right?

Comment on attachment 167545 [details] [diff] [review] Patch V1.3 diff -uN jst has volunteered to do the sr. thanks johnny!

Comment on attachment 167545 [details] [diff] [review] Patch V1.3 diff -uN sr=jst

Today I tested all dialogs in Firefox. They all behave identical compared to the Suite. Nonetheless it was a good idea to do another round of tests, since I found two bugs in my patch: 1. in deleteCert.xul I called doOk() instead of doOK(). 2. in editemailcert.xul I removed two opening <vbox> tags, but only one </vbox>. This patch corrects these two things and is unchanged otherwise. This is what I want to check in shortly.

Comment on attachment 168326 [details] [diff] [review] Patch V1.4 diff -uN Transferring flags...

(In reply to comment #27) > (From update of attachment 167546 [details] [diff] [review]) > security/manager/pki/resources/content/cacertexists.xul > > this now seems to only have a description... maybe the caller should use > nsIPromptService? Yes, I know. The reason I didn't do this is my lack of C++ knowledge. ;-) I will file a follow-up bug about this small further enhancement.

Fix checked in! :-) I filed follow-up bug 273949 about cacertexists.xul.

Steffan, did this cause http://mypage.iusb.edu/~stdonner/images/server-cert.PNG?

(In reply to comment #35) > Steffan, did this cause http://mypage.iusb.edu/~stdonner/images/server-cert.PNG? Yes, this looks similar to Bug 274481. I guess I need to retest all dialogs with a very small font and then apply more wallpaper like in bug 274481. I can do this on Monday. escrowWarn.xul and serverCrlNextupdate.xul have a similar structure, so they may well also expose this bug. :-(

This is marked as resolved. I have the same problem as Stephan Donner in comment #35

Hmm, is it possible to fix the CRLmanager dialog so it has an OK button as well? On mac os X, there is no way of closing the dialog as it is now... That's another bug, of course -- just thought that while you're at it... ;)

Verified FIXED; the problem I described in comment 35 is fixed now, as well. Build 2005-01-16-05, Windows XP Seamonkey trunk.

The patch in this bug might have caused bug 297057. I didn't notice you changed the OK button call in choosetoken from doOK() to doOk().