226956 - Bug 197315, </mailnews/*> files: Convert <window class="dialog"> to <dialog>

Status: ASSIGNED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Serge Gautherie (:sgautherie)]

See bug 197315 "ToDoList" attachment:
there are +/- 15 <.xul> files to process.

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) <sendProgress.*>

Comment 2

[Serge Gautherie (:sgautherie)]

Comment on attachment 136517 [details] [diff] [review]
(Av1) <sendProgress.*>


'r=?':
Can you review, test it ?

I wonder if |function getString(stringId)| could be simplyfied:
*removing the try+catch
*or removing the if+else
?

Comment 3

[[e-mail]]

Comment on attachment 136517 [details] [diff] [review]
(Av1) <sendProgress.*>

>-  <script type="application/x-javascript" src="chrome://messenger/content/messengercompose/sendProgress.js"/>
>+  <script type="application/x-javascript"
>+          src="chrome://messenger/content/messengercompose/sendProgress.js"/>
Please don't randomly wrap lines.

>-var itsASaveOperation = false;
>+var itIsASaveOperation = false;
"it's a save operation" -> remove apostrophes, remove spaces but upper case the
next letter -> "itsASaveOperation"

>-   if (!(stringId in dialog.strings)) {
>+  if (!(stringId in dialog.strings))
>+  {
Please don't change brace style.

>-        dump( "Invalid argument to downloadProgress.xul\n" );
>+    dump("Invalid argument to sendProgress.xul\n");
Please don't randomly change whitespace style.

Where I say "randomly", this means somewhere where you don't already have to
change the line. Even when you do have to change the line, don't change brace
style unless it's inconsistent with the rest of the file. The rest looks OK.

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) <sendProgress.*>

Patch v1, plus comment 3 suggestions.

Comment 5

[Serge Gautherie (:sgautherie)]

Attachment: (Av2b) <sendProgress.*>

Patch v2, plus full "random" line unwrapping :->

Comment 6

[Serge Gautherie (:sgautherie)]

Attachment: (Av2c) <sendProgress.*>

Patch v2b, plus a 'downloadProgress' to 'sendProgress' renaming !

Comment 7

[[e-mail]]

Comment on attachment 136690 [details] [diff] [review]
(Av2c) <sendProgress.*>

>+<!DOCTYPE dialog
>+  SYSTEM "chrome://messenger/locale/messengercompose/sendProgress.dtd">
You don't need to unwrap this for just one doctype.

>+>
Please don't do this.

Comment 8

[Serge Gautherie (:sgautherie)]

Attachment: (Av3) <sendProgress.*> [Checked in: Comment 12]

Patch v2c, plus comment 7 suggestions.

Comment 9

[[e-mail]]

Comment on attachment 136897 [details] [diff] [review]
(Av3) <sendProgress.*>
[Checked in: Comment 12]

Actually I've since discovered that other people prefer the styles that I
carped at in your previous patch.

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 136897 [details] [diff] [review]
(Av3) <sendProgress.*>
[Checked in: Comment 12]


'approval1.6=?': Trivial U.I. code cleanup.

Comment 11

[chris hofmann]

Comment on attachment 136897 [details] [diff] [review]
(Av3) <sendProgress.*>
[Checked in: Comment 12]

need to minus.	trying not to take changes that impact	localizations after
beta.  let's do this for 1.7

Comment 12

[Serge Gautherie (:sgautherie)]

Comment on attachment 136897 [details] [diff] [review]
(Av3) <sendProgress.*>
[Checked in: Comment 12]


Check in: { 12/20/2003 10:13	neil%parkwaycc.co.uk }

Comment 13

[Stefan Borggraefe]

Serge, are you currently working on this? Maybe I could help a little.

Comment 14

[Serge Gautherie (:sgautherie)]

(In reply to comment #13)
> Serge, are you currently working on this? Maybe I could help a little.

I have stopped working on this for a while:
you are free/welcome to help as much as you want ;-)

Comment 15

[Stefan Borggraefe]

Attachment: Patch

This converts all <window class="dialog"...> to <dialog>. I also included some
minor cleanup like adding missing accesskeys etc that are IMHO not worth
separate bugs and reviews. I hope this is ok.

Sidenote: I also filed bug 245736 and bug 245728 to remove two unused <window
class="dialog"...>.
Sidenote 2: There are some <dialog>s that include dialogOverlay.xul in
mailnews/. This makes no sense, does it?

Comment 16

[[e-mail]]

Just reading the patch, you weren't consistent on your access keys - sometimes
you used uppercase, and sometimes you used the actual case of the letter.

I think the original intention was for the the new folder type radiobuttons to
be horizontal.

Also, there's a rename folder picker, which appears to be currently unused, so I
wouldn't remove the overlay, just comment it out, so that someone can fix it,
unless mscott wants to ditch that picker completely of course ;-)

I notice the "View" button in the compose security dialog looks lonely, I hope
to come up with a fix for that soon :-)

Comment 17

[Stefan Borggraefe]

(In reply to comment #16)
> Just reading the patch, you weren't consistent on your access keys - sometimes
> you used uppercase, and sometimes you used the actual case of the letter.

Hmm, I don't see the problem here. I was trying to follow

  http://www.mozilla.org/projects/ui/accessibility/accesskey.html

(see especially paragraph "How is an accesskey added to a form control?" and the
first point of "How do I pick an accesskey letter?")

Have I misunderstood the rules given there?

> I think the original intention was for the the new folder type radiobuttons to
> be horizontal.

Ok, I changed this to orient="horizontal" class="indent".

> Also, there's a rename folder picker, which appears to be currently unused, so I
> wouldn't remove the overlay, just comment it out, so that someone can fix it,
> unless mscott wants to ditch that picker completely of course ;-)

Ok, I changed this so it is just commented out. IMO this picker can be removed.
The current UI for renaming folders is already to complex. Adding this picker
somewhere would make it even more complex. Instead inline renaming should be the
long-term goal in my opinion.

> I notice the "View" button in the compose security dialog looks lonely, I hope
> to come up with a fix for that soon :-)

That's cool. :-)

Comment 18

[[e-mail]]

Ignore the access keys, I wasn't exactly awake at the time ;-)

Comment 19

[Stefan Borggraefe]

Attachment: Patch V1.1

Updated patch. The only things changed are the two mentioned in comment 17.

Comment 20

[[e-mail]]

Comment on attachment 150826 [details] [diff] [review]
Patch V1.1

>+        ondialogcancel="return onCancelReplication();"
>+        onclose="return onCancelReplication();">
You don't need onclose in <dialog>s.

>+  <textbox tabindex="0" id="name" oninput="doEnabling();"/>
Please avoid tabindex.

>+        <radio id="some" selected="true" label="&download.label;"
>+               accesskey="&download.accesskey;" oncommand="setupDownloadUI(true);"/>
Nit: if you're going to wrap, do make sure that *all* of your lines are under
80 columns...

>+      var recipientsButton = document.documentElement.getButton("disclosure");
>+      recipientsButton.setAttribute("value", buttonlabels.getAttribute("recipientsLabel"));
>+      recipientsButton.removeAttribute("hidden");
Nit: to show this you would change the buttons attribute on your dialog
element.

>-function Help()
>-{
>-  return false;
>-}
Not sure that you should remove this; instead add an ondialoghelp for the
benefit of whomever should write the help.

Comment 21

[[e-mail]]

Comment on attachment 150826 [details] [diff] [review]
Patch V1.1

I forgot to mention that you left all the dialogs with the default buttonpack
of right although three of them used to have center packing.

Comment 22

[Stefan Borggraefe]

(In reply to comment #21)
> (From update of attachment 150826 [details] [diff] [review])
> I forgot to mention that you left all the dialogs with the default buttonpack
> of right although three of them used to have center packing.

I omitted the buttonpack attribute deliberately. I don't see a reason to deviate
from the dialog XBL default of buttonpacking. Why should some dialogs have their
buttons centered and some right aligned? Are there any rules for this?

IMHO the buttonpack attribute is a source for inconsistencies. E.g. compare the
New Address Book Card dialog and the Address Book Card Properties dialog in
Seamonkey. Perhaps it should just be removed from everywhere. Toolkit seems to
ignore this attibute already (in TB on Linux the both Address Book dialogs just
mentioned don't have the inconsistent button placement. Also in TB all dialog
buttons are right aligned, even the OK button of simple alerts.).

Comment 23

[Stefan Borggraefe]

Attachment: Patch V1.2 (checked in)

Addressed all review comments.

For the record the result of a discussion with Neil on IRC: Dialogs that ask
the user a question should have the buttonpack="center" attribute. The
addressbook dialogs mentioned in comment 22 don't use it correctly.

So the "Download Headers" and the "Ask Send Format" dialogs now have
buttonpack="center".

Comment 24

[Scott MacGregor]

Comment on attachment 151099 [details] [diff] [review]
Patch V1.2 (checked in)

sorry for the delay

Comment 25

[Stefan Borggraefe]

Comment on attachment 151099 [details] [diff] [review]
Patch V1.2 (checked in)

Patch checked in.

I leave this bug open for a follow-up patch that removes the inclusion of
dialogOverlay.* from files that don't use it.

For example:

http://lxr.mozilla.org/mozilla/source/mailnews/base/prefs/resources/content/Smt
pServerEdit.xul#4

Comment 26

[Stefan Borggraefe]

Attachment: More cleanup

This patch removes all references to dialogOverlay.* from files that don't use
it. 

I don't think certFetchingStatus.xul should be converted to <dialog> because it
didn't use anything from dialogOverlay.* except class="dialog" and doesn't have
the typical dialog buttons.

I also removed a <separator> in SmtpServerList.xul. I don't know how this was
still there, because I thought I removed it already in bug 223295.

Comment 27

[[e-mail]]

I think you could make it a dialog... but you would have to adapt stopFetching
to be your close button, and you might want to wait for my dialog label patch
anyway to make it easier to specify the label.

Comment 28

[[e-mail]]

Comment on attachment 152606 [details] [diff] [review]
More cleanup

Except certFetchingStatus.

Comment 29

[Stefan Borggraefe]

Comment on attachment 152606 [details] [diff] [review]
More cleanup

Ok, I won't checkin the changes to certFetchingStatus.xul and convert it later
to a <dialog> instead. Asking for sr for the rest.

Neil: What is the bug number for your dialog label patch? Or haven't you filed
one yet?

Comment 30

[[e-mail]]

It's in bug 78274. And I only just noticed alecf's holiday message :-(

Comment 31

[Stefan Borggraefe]

Attachment: More cleanup V1.1 (checked in)

Comment 32

[Stefan Borggraefe]

Comment on attachment 154129 [details] [diff] [review]
More cleanup V1.1 (checked in)

Neil, this is the same patch as before. I just included the conversion of
certFetchingStatus to <dialog> you suggested into this patch. So you only need
to review the changes in certFetchingStatus.*. Thanks! :-)

Comment 33

[Stefan Borggraefe]

There are still four references to dialogOverlay.* left in mailnews/:

1.
http://lxr.mozilla.org/mozilla/source/mailnews/addrbook/resources/content/abSelectAddressesDialog.xul#42

This dialog still needs to be converted to <dialog>. When I tried it, it always
resulted in a blank window. So I need to figure out why this happens.

2.
http://lxr.mozilla.org/mozilla/source/mailnews/base/search/resources/content/CustomHeaders.xul#40

Needs to be converted to <dialog>. Needs some special care to not break the
current behaviour according to the keyboard navigation.

3.
http://lxr.mozilla.org/mozilla/source/mailnews/import/resources/content/importDialog.xul#41

Needs to be converted to <wizard> rather than to <dialog>. This is bug 101874. I
started working on this a few months ago and have a unfinished patch lying around. 

4.
http://lxr.mozilla.org/mozilla/source/mailnews/addrbook/resources/content/abListOverlay.xul#53

This one I just overlooked. I will include the removal of this line in one of
the other patches I still need to do.