93732 - Composer dialog cleanup

Status: VERIFIED FIXED

(SeaMonkey :: Composer, defect, P4)

Description

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

Image Properties, Page Colors, Insert Table, Table Cell/Properties dialogs are 
all showing weird layout symptoms:

1) Grid columns aren't showing up
2) textfields draw over buttons
3) buttons etc run off screen
4) dialog size oddly when sizeToContent() called.

Patch in a sec...

Comment 1

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

Attachment: patch to fix.

Comment 2

[jag (Peter Annema)]

r=jag

Comment 3

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

Attachment: better image props patch

Comment 4

[Joe Hewitt (gone)]

sr=hewitt

Comment 5

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

Fixed!

Comment 6

[jag (Peter Annema)]

Doh, collission on my r=jag. So for the record ... :-)

Comment 7

[Tracy Walker [:tracy]]

verified won windows 2001-08-06-06-trunk

Comment 8

[Charles Manske]

What happened to module owner permission before checking in fixes?
We (Composer UI engineers) are not happy at all about these changes that were
made without CCing us or asking for our review.
Who said you can redesign dialogs? You moved the "Advanced Edit" button to 
different locations so it is not consistent across all dialogs, as it was 
before. Moving it to the lower-left corner, along with "Ok", "Cancel" etc, may
be a good idea, but then it should be done for all dialogs.
You made significant changes to the Preview window in the Image dialog, which 
I also think needs more review and discussion.
I want these changes backed out until we can discuss and approve them.

Comment 9

[Charles Manske]

A specific note about the patch:
I think specifying widths for the dialog like this:
style="width: 43em;" in the Image dialog
style="width: 25em;" in the Insert Table dialog
is a bad idea. It may appear to fix errors in dialog layout, but it will only
obscure finding those errors in other languages, especially German.

Comment 10

[Kathleen :Brade]

As far as I can tell from the original bug that touched the image dialog was 
trying to fix the chopping of buttons in the dialog.

Currently in my Mac build from today, Modern theme, the choose image button 
doesn't appear the first time and then is still chopped on subsequent openings of 
that dialog.

Comment 11

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

Changes removed. Good day. 

Comment 12

[Charles Manske]

Ben, thanks, but lets be constructive about this. You had lots of good 
suggestions in the patch, which we want to investigate. I like the idea of 
locating the Advanced Edit button in the lower left corner, for example.
Reopening to hold onto the attachments.
Adding dependency on bug 87652 to monitor the issue of what overlay ID to use 
for center vs. right-aligned "Ok", "Cancel", "Help" buttons.

Comment 13

[Charles Manske]

Taking the bug.

Comment 14

[rubydoo123]

moving out 9.4

Comment 15

[Charles Manske]

After various discussions, we decided that moving the "Advanced Edit" button to
the lower-left corner of all property dialogs is not a good idea. For some 
dialogs, this would be too crowded. The primary reason is that in dialogs that
have a Help button, that button should be in the lower left corner in Mac.

Comment 16

[R.K.Aa.]

table properties/advanced edit button doesn't trigger anything on Linux. Is that
covered by this bug?

Comment 17

[Charles Manske]

No! Please file a new bug on that issue.

Comment 18

[Syd Logan]

What is the status of this bug? 0.9.5 realistic? Is a patch forthcoming?

Comment 19

[Syd Logan]

spam composer change

Comment 20

[Charles Manske]

There's a few dialogs that need minor tweaking -- mainly to put "Ok" "Cancel"
buttons in lower-right corner rather than centered. Easy to do.

Comment 21

[Brian King [:kinger]]

It's not just the ok and cancel buttons, but there is still alout of
'autostretch' and 'valign' attributes lying around (e.g EdTableProps.xul) that
could be replaced with box-pack and box-align, etc.

http://mozilla.org/projects/xul/layout.html

Comment 22

[Charles Manske]

Right, I'm aware of the "autostretch" that needs to be purged. Working on it.

Comment 23

[Charles Manske]

Tasks relating to general dialog cleanup:
1. Be sure all dialogs right-align the Ok / Cancel [/Help] buttons along the
bottom.
2. Replace 'autostretch' with appropriate XUL 1.0 syntax.
3. Simplify "grid" usage to use boxes instead if possible.

Comment 24

[Charles Manske]

4. Add "onunload" handlers on all dialogs (see bug 96891) so clicking on "X" 
goes through appropriate JS code to cleanup dialog.

Comment 25

[Charles Manske]

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

Comment 26

[Charles Manske]

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

Comment 27

[Charles Manske]

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

Comment 28

[Charles Manske]

5. Change global variable "dialog" to "gDialog" in all dialog JS files.
(formerly bug 92247)

Comment 29

[Charles Manske]

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

Comment 30

[Charles Manske]

Task from bug 102674: 
6. okCancelHelpButtonsRight should not be inside tabbox

Comment 31

[Charles Manske]

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

Comment 32

[Charles Manske]

From bug 94749:
7. Simplify image map code in Image properties dialog

Comment 33

[Charles Manske]

Changing milestone

Comment 34

[Charles Manske]

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

Comment 35

[Charles Manske]

Note that task 7 (simplify Image Map code) is no longer part of this bug.

This task touches every dialog XUL file as well, so including here
7. Remove class="dialog" from all buttons

Comment 36

[Charles Manske]

Attachment: A very big patch.

Comment 37

[Charles Manske]

As daunting as the patch on 10/09/01 seems, it the changes fit simple patterns
that are repeated in just about all of the XUL or JS files:
1. Be sure all dialogs right-align the Ok / Cancel [/Help] buttons along the
bottom.
Look for this sort of change:
-  <hbox id="okCancelButtons"/> 
+  <hbox id="okCancelButtonsRight"/> 

2. Replace 'autostretch' with appropriate XUL 1.0 syntax.
Look for changes like this:
-    <hbox autostretch="never">
+    <hbox align="center">
or 
-    <hbox autostretch="never" valign="middle">
+    <hbox align="center">
Also, align="start" is another common replacement, when the child elements
should be aligned starting at the top or left.

3. Simplify "grid" usage to use boxes instead if possible.
Actually, this didn't need to be done. Some grids were replaced with boxes in
other bugs, and there were no other instances that needed to be fixed. 
A few columns had some flex added to them (e.g., EdInsertTable.xul) to
improve layout.

4. Add "onunload" handlers on all dialogs (see bug 96891) so clicking on "X" 
goes through appropriate JS code to cleanup dialog.
You'll see a lot of examples of this addition to the <window> element:
+    onunload="onCancel()"

5. Change global variable "dialog" to "gDialog" in all dialog JS files.
By far the largest change. Every dialog includes EdDialogCommon.js, and since
it was already refering to the global "dialog" object, I move the initialization
of "gDialog" there:
+var gDialog = new Object;
Thus the declaration and initialization of "dialog" was removed from all 
dialogs. Every instance of "dialog." is replaced with "gDialog."

6. okCancelHelpButtonsRight should not be inside tabbox
This change is only in EdTableProps.xul, see changes after 
"@@ -249,17 +250,16 @@"

7. Remove class="dialog" from all buttons
Again, lots of simple replacements to remove the "dialog" class from buttons
only. it is left on all dialog <window> elements.

Comment 38

[Charles Manske]

Testing note: Basically, every dialog should work and look about the same (in 
some cases, better, I hope!) as it does before these changes. The Ok/Cancel
(and Help, if included) buttons should be aligned to the right side.
To test task 4: 
1. Bring up each dialog
2. Move it to a different location
3. Click on the "X" in the upper right corner. 
4. Launch the dialog again. It should appear in the same location as where you 
moved it because the "onunload" is (usually) calling "onCancel()", which should
save the current location. I currently see one dailog that isn't doing that:
The List Properties dialog is always comming up just under the caption of the
main window, and doesn't remember its location, no matter how you close the
dialog. I'm investigating that as a separate issue.

Comment 39

[Charles Manske]

Review was by Syd (r=syd)

After extensive testing, one change in EdDialogCommon.js to avoid the 
warning caused by trying to close the window twice:
@@ -1143,7 +1147,8 @@
 function onCancel()
 {
   SaveWindowLocation();
-  window.close();
+  // Close dialog by returning true
+  return true;
 }

Comment 40

[Kathleen :Brade]

in pref-editing.xul, I see:
  <row  align="center">
You should remove the extra space before "align"

Although not introduced with this patch, you should remove the extra spaces on 
these lines in EdAEHTMLAttributes.js:
  var name  = TrimString(gDialog.AddHTMLAttributeNameInput.value);
  gDialog.AddHTMLAttributeValueInput.value =  GetTreeItemValueStr( ...
  gDialog.AddHTMLAttributeNameInput.value =  selectedName;
  if ( value != gDialog.AddHTMLAttributeValueInput.value )

In EdColorProps.js, why do we need these lines?  Can they all be removed?
   // Setting a color automatically changes into UseCustomColors mode
-  //dialog.CustomColorsRadio.checked = true;
+  //gDialog.CustomColorsRadio.checked = true;

In EdDialogTemplate.js, why do we need these line?  Can they be removed?
   // GET EACH CONTROL -- E.G.:
-  //dialog.editBox = document.getElementById("editBox");
+  //gDialog.editBox = document.getElementById("editBox");

and

   // SET FOCUS TO FIRST CONTROL
-  //SetTextboxFocus(dialog.editBox);
+  //SetTextboxFocus(gDialog.editBox);

and

 // Set attribute example:
-//  imageElement.setAttribute("src",dialog.srcInput.value);
+//  imageElement.setAttribute("src",gDialog.srcInput.value);

If we can't remove them, can we add a space to the last one (before "gDialog")?

In EdImageProps.js, add a space after "if":
  if(gDialog.PreviewImage)
and remove extra spaces from these lines:
  switch ( gDialog.alignTypeSelect.value )
  globalElement.setAttribute( "align", gDialog.alignTypeSelect.value );

In EdInsertTable.js, are these lines missing ';' at the end?
  rows = ValidateNumber(gDialog.rowsInput, null, 1, maxRows, null, null, true)
  columns = ValidateNumber(gDialog.columnsInput, null, 1, maxColumns, null, ...
Also in EdInsertTable.js, ValidateData looks odd to me; this isn't anything 
introduced with this patch so maybe we should file a new bug on this function?  
In particular, I don't like (1) "gValidationError", (2) I don't see where rows/
cols values are used, (3) the result for ValidNumber isn't always assigned, and 
(4) are "rows" and "columns" globals? if so, why aren't border and width?

In EdLinkProps.js, add a space on these lines (between parameters):
  gDialog.linkTextCaption.setAttribute("label",GetString("LinkImage"));
  gDialog.linkTextMessage.setAttribute("value",imageElement.src);
  gDialog.linkTextCaption.setAttribute("label",GetString("LinkText"));
  gDialog.linkTextMessage.setAttribute("value",TruncateStringAtWordEnd...
  gDialog.linkTextMessage.setAttribute("value",GetString("MixedSelection"));
  gDialog.linkTextInput.setAttribute("hidden","true");

I'm sure I missed some other whitespace inconsistencies but these are the ones 
that I noticed at a glance.

Comment 41

[[e-mail]]

Speaking of whitespace, I don't suppose you can remove trailing whitespace on
lines while you're about it?

Also, my personal preferences is for gDialog = {}; instead of new Object();

Comment 42

[Simon Fraser [no longer active]]

Comment on attachment 52791 [details] [diff] [review]
A very big patch.

rs=sfraser

Comment 43

[Charles Manske]

All of the whitespace and other changes suggested by brade and neil were made.

Comment 44

[Charles Manske]

checked in.
There are other new developments in XUL 1.0, such as replacing 
<window class="dialog"
with
<dialog
but I'll do this in another bug.

Comment 45

[Charles Manske]

2nd phase of XUL 1.0 dialog cleanup is bug 104113

Comment 46

[sujay]

verified.