Convert dialogs of the PSM to <dialog>

VERIFIED FIXED

Status

SeaMonkey
UI Design
VERIFIED FIXED
14 years ago
10 years ago

People

(Reporter: Stefan Borggraefe, Assigned: Stefan Borggraefe)

Tracking

(Blocks: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

14 years ago
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.
(Assignee)

Comment 1

14 years ago
Created attachment 153588 [details] [diff] [review]
Patch

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.
(Assignee)

Comment 2

14 years ago
Comment on attachment 153588 [details] [diff] [review]
Patch

Neil, do you have time to review this?
Attachment #153588 - Flags: review?(neil.parkwaycc.co.uk)

Comment 3

14 years ago
(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...

Comment 4

14 years ago
(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?

Comment 5

14 years ago
Other dialog cleanup bugs you might be interested in:

bug 76248, 7708, 78815, 78835, 94378, 106730, 123851, 125577, 125812, 129097,
135403, 157712 

(Assignee)

Comment 6

14 years ago
(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.
(Assignee)

Comment 7

14 years ago
(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>.
Status: NEW → ASSIGNED
(Assignee)

Comment 8

14 years ago
Created attachment 153643 [details] [diff] [review]
Patch V1.1

Addresses Neil's initial comments.
Attachment #153588 - Attachment is obsolete: true
(Assignee)

Comment 9

14 years ago
Created attachment 153644 [details] [diff] [review]
Patch V1.1 diff -uwN

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

Updated

14 years ago
Attachment #153588 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Updated

14 years ago
Attachment #153643 - Flags: review?(neil.parkwaycc.co.uk)

Comment 10

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

14 years ago
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
Attachment #153643 - Flags: review?(neil.parkwaycc.co.uk) → review-
(Assignee)

Comment 12

14 years ago
Created attachment 153721 [details] [diff] [review]
Patch V1.2 diff -uN

Addressed all review comments.
Attachment #153643 - Attachment is obsolete: true
Attachment #153644 - Attachment is obsolete: true
(Assignee)

Comment 13

14 years ago
Created attachment 153722 [details] [diff] [review]
Patch V1.2 diff -uwN
(Assignee)

Comment 14

14 years ago
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.
Attachment #153721 - Flags: review?(neil.parkwaycc.co.uk)

Comment 15

14 years ago
Comment on attachment 153721 [details] [diff] [review]
Patch V1.2 diff -uN

I've only tested a couple of the dialogs.
Attachment #153721 - Flags: review?(neil.parkwaycc.co.uk) → review+
(Assignee)

Comment 16

14 years ago
Comment on attachment 153721 [details] [diff] [review]
Patch V1.2 diff -uN

Jag, can you sr?

I still need moa after this.
Attachment #153721 - Flags: superreview?(jag)

Comment 17

14 years ago
If jag says this patch makes sense, moa granted

Comment 18

14 years ago
I'll try and get to it this week, otherwise this weekend.
(Assignee)

Comment 19

14 years ago
*** Bug 253724 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 20

14 years ago
(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! :-)
(Assignee)

Updated

14 years ago
Attachment #153721 - Flags: superreview?(jag) → superreview?(dmose)
Product: Core → Mozilla Application Suite
(Assignee)

Comment 21

13 years ago
Comment on attachment 153721 [details] [diff] [review]
Patch V1.2 diff -uN

Patch bitrotted slightly. :-(
Attachment #153721 - Attachment is obsolete: true
Attachment #153721 - Flags: superreview?(dmose)
(Assignee)

Updated

13 years ago
Attachment #153722 - Attachment is obsolete: true
(Assignee)

Comment 22

13 years ago
Created attachment 167545 [details] [diff] [review]
Patch V1.3 diff -uN

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

Comment 23

13 years ago
Created attachment 167546 [details] [diff] [review]
Patch V1.3 diff -uwN
(Assignee)

Comment 24

13 years ago
Comment on attachment 167545 [details] [diff] [review]
Patch V1.3 diff -uN

Transferring Neil's r+ and asking dmose for sr again.
Attachment #167545 - Flags: superreview?(dmose)
Attachment #167545 - Flags: review+
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.
(Assignee)

Comment 26

13 years ago
(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?
Attachment #167545 - Flags: superreview?(dmose) → superreview?

Updated

13 years ago
Attachment #167545 - Flags: superreview?
Comment on attachment 167545 [details] [diff] [review]
Patch V1.3 diff -uN

jst has volunteered to do the sr.  thanks johnny!
Attachment #167545 - Flags: superreview?(jst)
Comment on attachment 167545 [details] [diff] [review]
Patch V1.3 diff -uN

sr=jst
Attachment #167545 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 31

13 years ago
Created attachment 168326 [details] [diff] [review]
Patch V1.4 diff -uN

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.
(Assignee)

Updated

13 years ago
Attachment #167545 - Attachment is obsolete: true
Attachment #167546 - Attachment is obsolete: true
(Assignee)

Comment 32

13 years ago
Comment on attachment 168326 [details] [diff] [review]
Patch V1.4 diff -uN

Transferring flags...
Attachment #168326 - Flags: superreview+
Attachment #168326 - Flags: review+
(Assignee)

Comment 33

13 years ago
(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.
(Assignee)

Comment 34

13 years ago
Fix checked in! :-) I filed follow-up bug 273949 about cacertexists.xul.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Updated

13 years ago
Blocks: 274481
(Assignee)

Comment 36

13 years ago
(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. :-(

Comment 37

13 years ago
This is marked as resolved. I have the same problem as Stephan Donner in comment #35
(Assignee)

Updated

13 years ago
Blocks: 275527

Comment 38

13 years ago
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.
Status: RESOLVED → VERIFIED

Comment 40

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

Updated

10 years ago
Blocks: 444752

Updated

10 years ago
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.