226958 - Bug 197315, "other" files: Convert <window class="dialog"> to <dialog>

Status: ASSIGNED

(SeaMonkey :: UI Design, defect)

Description

[Serge Gautherie (:sgautherie)]

See bug 197315 "ToDoList" attachment:
there are +/- 6 <.xul> files to process:
{
  ** /extensions, 3 files
  ** /embedding, 1 files
  ** /profile, 1 files
  ** /xpinstall, 1 files
}

</xpfe> and </mailnews> files have a dedicated bug(s).

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) <xpistatus.*>

This dialog is called
at
<http://lxr.mozilla.org/seamonkey/source/xpinstall/src/nsXPInstallManager.cpp#394>

{
393	    rv = wwatch->OpenWindow(0, 
394				   
"chrome://communicator/content/xpinstall/xpistatus.xul",
395				    "_blank", 
396				    "chrome,centerscreen,titlebar,resizable",
397				    params, 
398				    getter_AddRefs(newWindow));
}

I don't know how to test this patch.!.

Comment 2

[Serge Gautherie (:sgautherie)]

Comment on attachment 136427 [details] [diff] [review]
(Av1) <xpistatus.*>


'r=?': (see comment 1)
Can you review, test, check it in ?
Or fix it, or explain to me what else needs to be done, since I wonder a little
on this one !

Comment 3

[[e-mail]]

Comment on attachment 136427 [details] [diff] [review]
(Av1) <xpistatus.*>

>-            document.getElementById("ok").disabled = false;
>+            document.getElementById("accept").disabled = false;
>             document.getElementById("cancel").disabled = true;
It would be neat if you could focus the accept button.

>+    document.getElementById("accept").disabled = true;
>     document.getElementById("cancel").focus();
The syntax for dialog buttons is slightly different:
document.documentElement.getButton("accept") [or "cancel"]

Comment 4

[[e-mail]]

Comment on attachment 136427 [details] [diff] [review]
(Av1) <xpistatus.*>

>-/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
>- *
>- * The contents of this file are subject to the Netscape Public
>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
>+
>+/* The contents of this file are subject to the Netscape Public
Also don't change this unless you can point to a definitive document that
specifies that the mode line should be separate.

Comment 5

[HJ]

"It would be neat if you could focus the accept button."

And what about the default attribute?
http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/bindings/dialog.xml#199

Comment 6

[HJ]

I just realized that you're not changing the default selected/focused button. I
was doing something myself for MultiZilla, so that's what got me confused.

Comment 7

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) <xpistatus.*>

Patch v1, plus comment 3 and comment 4 suggestions.

Comment 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 136478 [details] [diff] [review]
(Av2) <xpistatus.*>


'r=?': (see comment 7)
Can you review, test, check it in ?
Or fix it, or explain to me what else needs to be done, since I wonder a little
on this one !

Comment 9

[[e-mail]]

Comment on attachment 136478 [details] [diff] [review]
(Av2) <xpistatus.*>

Well this looks OK at first glance, I'll try to test it soon.

Comment 10

[[e-mail]]

Comment on attachment 136478 [details] [diff] [review]
(Av2) <xpistatus.*>

> <window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
Oops ;-)

>-</window>
>+</dialog>
The rest of the patch is ok, though.

Comment 11

[Serge Gautherie (:sgautherie)]

Attachment: (Av3) <xpistatus.*> [Check in: see Av3b]

Patch v2, plus comment 10 suggestions.
(my mistake)

Comment 12

[Serge Gautherie (:sgautherie)]

Comment on attachment 136628 [details] [diff] [review]
(Av3) <xpistatus.*>
[Check in: see Av3b]


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

Comment 13

[HJ]

Hmm,  guess Neil missed them, but can you please line up like this:

+<?xml-stylesheet type="text/css"
-  href="chrome://communicator/skin/xpinstall/xpinstall.css"?>
+                 href="chrome://communicator/skin/xpinstall/xpinstall.css"?>

+    <stringbundle id="xpinstallBundle"
-      src="chrome://communicator/locale/xpinstall/xpinstall.properties"/>
+                 
src="chrome://communicator/locale/xpinstall/xpinstall.properties"/>

Comment 14

[HJ]

Darn, that line is to long, but I guess you know what I mean. 

-> type/href and id/src should line up.

Comment 15

[Serge Gautherie (:sgautherie)]

Re comment 13 and comment 14:

For the purpose of this bug,
since it was pure reformatting,
I may rather undo theses line wraps !!?

If you do want me to wrap the lines,
my understanding of your comments is that it will look like:
{
<?xml-stylesheet
  type="..." href="..."?>
<stringbundle
  id="..." src="..."/>
}
no matter if the lines still go after column 79 !??

Comment 16

[HJ]

Ok, the comments are a bit confusing, but this is what I had in mind:

<?xml-stylesheet type="text/css"...
                 href="..."?>

<stringbundle id="xpinstallBundle"...
              src="..."/>

Comment 17

[Serge Gautherie (:sgautherie)]

Re comment 16:

HJ:
I see (and it was my first though when I did the patch).

neil:
What should I do now:
1) Leave the patch as it is ? :-(
2) Apply HJ's comment, when the result is <= 79 columns
   and revert to previous state when > 79 anyway. :-|
3) Revert all to previous state (> 79). :-)

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 136628 [details] [diff] [review]
(Av3) <xpistatus.*>
[Check in: see Av3b]

This is not important for 1.6, and I don't have time to spend sr'ing it. 
Suggest you take HJ's style point to heart and prepare a 1.7a patch.

/be

Comment 19

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 136628 [details] [diff] [review]
(Av3) <xpistatus.*>
[Check in: see Av3b]

+<?xml-stylesheet type="text/css"
+  href="chrome://communicator/skin/xpinstall/xpinstall.css"?>

No newlines in stylesheet includes please, or in the stringbundle use. 

Do that and sr=[e-mail]

Comment 20

[Asa Dotzler [:asa]]

Comment on attachment 136628 [details] [diff] [review]
(Av3) <xpistatus.*>
[Check in: see Av3b]

a=asa (on behalf of drivers) for checkin to 1.6beta

Comment 21

[Serge Gautherie (:sgautherie)]

Attachment: (Av3b) <xpistatus.*> [Checked in: Comment 23]

Patch v3, plus 2 line unwrapping (per comments).

Comment 22

[Serge Gautherie (:sgautherie)]

Comment on attachment 136750 [details] [diff] [review]
(Av3b) <xpistatus.*>
[Checked in: Comment 23]

neil:
Can you
"tranfer" r=neil, sr=ben (comment 19), a=asa from patch v3,
and check it in ?

Comment 23

[[e-mail]]

Comment on attachment 136628 [details] [diff] [review]
(Av3) <xpistatus.*>
[Check in: see Av3b]

I checked in the patch with ben's changes.

Comment 24

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) <profileMigrationProgress.*>

Comment 25

[Serge Gautherie (:sgautherie)]

Comment on attachment 136781 [details] [diff] [review]
(Bv1) <profileMigrationProgress.*> 


I tried to test with
window.open( "",
"chrome://communicator/content/profile/profileMigrationProgress.xul", "_blank",
"centerscreen,modal,titlebar" );
but got a blank window only :-(

The actual call is at
/profile/pref-migrator/src/nsPrefMigration.cpp, line 338+.

Comment 26

[[e-mail]]

Comment on attachment 136781 [details] [diff] [review]
(Bv1) <profileMigrationProgress.*> 

Yuck, what an ugly dialog.
I don't think it's worth converting this.

Comment 27

[Serge Gautherie (:sgautherie)]

Attachment: (Bv2) <profileMigrationProgress.*> trivial cleanup only

Patch v1, plus comment 26 suggestions.

Comment 28

[Serge Gautherie (:sgautherie)]

Comment on attachment 136980 [details] [diff] [review]
(Bv2) <profileMigrationProgress.*> trivial cleanup only


'r=?': (see comment 27)
Can you review, check it in ?

Comment 29

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) <WalletViewer.*>

Comment 30

[Serge Gautherie (:sgautherie)]

Comment on attachment 136988 [details] [diff] [review]
(Cv1) <WalletViewer.*>

I tested Cancel/Help buttons only.

I wonder about the purpose of |window.doneLoading = false;|.

Can you (super-)review, test, check it in ?

Comment 31

[[e-mail]]

window.doneLoading was copied from nsPrefWindow.js where it doesn't appear to be
used either ;-)

Could you post the patch that you claim gives you that JS exception?

Comment 32

[[e-mail]]

Comment on attachment 136980 [details] [diff] [review]
(Bv2) <profileMigrationProgress.*> trivial cleanup only

Here's a better comment:
<!-- TODO: Make profile migration cancelable and convert this to a dialog -->

Comment 33

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1b) <WalletViewer.* ++>

Patch Cv1, plus all(=2) |window.doneLoading = false;| removals.

Can you (super-)review, test, check it in ?

Comment 34

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1b_js_error) <WalletViewer.* ++> {for debug only !} [See coment 36]

Comment 31 requested patch:
It's Cv1b, with |Startup()| moved from xul to js file.

neil:
Since you're interested, I did some more tests: see the comments in the patch.
I guess that you'll want me to update Cv1b: tell me in what way :->

Comment 35

[Serge Gautherie (:sgautherie)]

Comment on attachment 136980 [details] [diff] [review]
(Bv2) <profileMigrationProgress.*> trivial cleanup only


'<profileMigrationProgress.*>' subject eventually moved to bug 228106.

Comment 36

[[e-mail]]

Comment on attachment 137212 [details] [diff] [review]
(Cv1b_js_error) <WalletViewer.* ++> {for debug only !} [See coment 36]

Ah, I see what's going on here. The wallet code was copied from the prefs code,
which allows each page to have a startup function, although none of the wallet
pages actually uses it (WalletUrlspecific.xul uses generate() instead).
Unfortunately what the pages do is to include WalletViewer.js for no reason at
all, thus causing confusion when you try to move Startup there.

Comment 37

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1c) <WalletViewer.* ++>

Patch Cv1b, plus comment 36 suggestions.

Comment 38

[Serge Gautherie (:sgautherie)]

Comment on attachment 137239 [details] [diff] [review]
(Cv1c) <WalletViewer.* ++>


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

Comment 39

[[e-mail]]

Comment on attachment 137239 [details] [diff] [review]
(Cv1c) <WalletViewer.* ++>

>+        ondialogaccept="return hWalletViewer.onAccept();"

>-      onOK:
>+      onAccept:

>-          if ('Startup' in window.frames[ this.contentFrame ]) {
>-            window.frames[ this.contentFrame ].Startup();
>-          }
>-
I don't like these changes, this dialog should resemble the preferences dialog,
also WalletUrlspecific should eventually get fixed to use Startup  anyway
(unless you'd care to fix it too, of course...)

Comment 40

[Serge Gautherie (:sgautherie)]

Attachment: (Cv2) <Wallet*.*> (+ <*pref*>)

Patch Cv1c, plus comment 39 suggestions.

Comment 41

[Serge Gautherie (:sgautherie)]

Comment on attachment 137491 [details] [diff] [review]
(Cv2) <Wallet*.*> (+ <*pref*>)

I forgot to write:
I did make some basic tests which were successful :-)

Comment 42

[[e-mail]]

Comment on attachment 137491 [details] [diff] [review]
(Cv2) <Wallet*.*> (+ <*pref*>)

Would you mind putting the pref panel changes in their own bug?

Comment 43

[Serge Gautherie (:sgautherie)]

Attachment: (Cv2b) <Wallet*.*>

This really is patch Cv2, minus the <*pref*> part (moved to bug 228904).

Comment 44

[[e-mail]]

Comment on attachment 137678 [details] [diff] [review]
(Cv2b) <Wallet*.*>

>-      onOK:
>+    onAccept:
Nit: indentation wrong

>-        onload="Startup();"
>+        onload="onLoad();"
I thought the whole point was that we didn't have to change this.

>-        width="720" height="470"> 
>+        width="720" height="470"
>+        buttons="accept,cancel,help"
>+        ondialogaccept="return hWalletViewer.onAccept();"
>+        ondialogcancel="return hWalletViewer.onCancel();"
>+        ondialoghelp="doHelpButton();">
You could leave the width and height at the end (shorter patch ;-)

Comment 45

[Serge Gautherie (:sgautherie)]

Attachment: (Cv2c) <Wallet*.*> [Checked in: Comment 47]

This is patch Cv2b, without the useless paramater to
|onload="parent.initPanel();"|.

Comment 46

[Alec Flett]

Comment on attachment 137732 [details] [diff] [review]
(Cv2c) <Wallet*.*>
[Checked in: Comment 47]

damn, nice cleanup sr=alecf assuming it all works well.

Comment 47

[Serge Gautherie (:sgautherie)]

Comment on attachment 137732 [details] [diff] [review]
(Cv2c) <Wallet*.*>
[Checked in: Comment 47]


Checked in by { neil%parkwaycc.co.uk	Dec 20 10:15 }