183419 - Message Filter dialog and Search dialogs (Message, Address) need close facilities (was buttons)

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[Karsten Düsterloh]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3a) Gecko/20021126
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3a) Gecko/20021126

For consistency reasons, the Message Filters dialog (Tools->Message Filters)
should have a "close" (or "exit" or "OK" etc.) button. Currently, you need to
click the upper right hand X or press Esc.

Reproducible: Always

Steps to Reproduce:

Comment 1

[R.K.Aa.]

See bug 168935.

*** This bug has been marked as a duplicate of 176157 ***

Comment 2

[laurel]

marking verified

Comment 3

[Mike Cowperthwaite]

Reopening, and overriding dupe; changing to enhancement.

This bug asks for a Close button, not an OK/Cancel.  OK/Cancel (and Apply) do 
not make sense for modeless dialogs whose effects cannot be withdrawn (e.g. 
deleting a Filter in Message Filters); so having bug 176157 resolved as Invalid 
is reasonable.  (Preferences and Junk Mail Control windows have Cancel, and 
closing the window with that prevents the changes made from taking effect.  
Message Filters does not have that flexibility.)

Wanting a Close button so that the dialog can be dismissed with regular keyboard 
navigation (tabbing, plus space or enter, rather than Ctrl-F4) is an appropriate 
and reasonable request.  This is present already in the Form Manager, Cookie 
Manager, Image Manager, and Password Manager.

Besides Message Filters, a Close button is requested for Search Messages.  It 
would also be appropriate for Download Manager, altho that window's current 
design doesn't have a spot for a button.

Comment 4

[Mike Cowperthwaite]

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

Comment 5

[(not reading, please use [e-mail] instead)]

mass reassign

Comment 6

[Leston Buell]

This is on Linux (and presumably elsewhere), as well. Please change platform to
"All".

Comment 7

[Karsten Düsterloh]

OS -> all as of comment #6

Comment 8

[Ian Batterbee]

Just a comment.. I think it is acceptable to expect users to use the close
window command (ie click on X) for the macintosh platform, as it is common for
macintosh applications to behave this way.

That is not the case however with windows and most unix GUIs, and the missing
close button is inconsistant with most of the other mozilla dialogs.

Comment 9

[Paul]

Using TWM, for example, I find that I cannot add message filters.  Because there
is no way to close the filter dialog, any changes I make are lost.

I can make changes in, say, GNOME, because I can click the 'X' once I've finished.


Thanks

Comment 10

[(not reading, please use [e-mail] instead)]

over to mscott for 1.7 alpha.

Comment 11

[Mike Cowperthwaite]

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

Comment 12

[Mike Cowperthwaite]

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

Comment 13

[Jo Hermans]

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

Comment 14

[Mark Banner (:standard8)]

Attachment: Patch to add close buttons.

This patch adds close buttons onto the Filter List and Search dialogs.

Comment 15

[[e-mail]]

Comment on attachment 164770 [details] [diff] [review]
Patch to add close buttons.

Well, I'm not convinced about the search dialog, the patch leaves a big gap
between Search Subfolders and the Match radiobuttons. And the position of the
Close button in the Filter List window is definitely wrong.

Comment 16

[Mark Banner (:standard8)]

Attachment: Revised Patch

Another attempt.

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 166467 [details] [diff] [review]
Revised Patch

Neil, I've revised this patch for the close buttons.

The Filter List Dialog location of close & help buttons now follows that of the
Password and Image Manager dialogs.

The Search Dialog doesn't quite as the close/help buttons wouldn't feel right
in that corner to me, so I've rearranged them at the top of the dialog.

See what you think, if you don't think its right I'll catch you on IRC
sometime.

Comment 18

[[e-mail]]

I'm still not convinced about the filter list, perhaps because it's got an
existing neat column of buttons... the search dialog looks quite nice though.

Comment 19

[[e-mail]]

Comment on attachment 166467 [details] [diff] [review]
Revised Patch

Looks like the o shortcut is already in use in the search window...

Comment 20

[Mark Banner (:standard8)]

Attachment: Patch v3

New patch - revised some of the access keys so that we can actually use one for
close. The help access key is now also consistent with other dialogs.

Filter List Editor also revised - Close & Help contained within same column as
the rest of the buttons.

Comment 21

[Mark Banner (:standard8)]

Assigning to self and cc'ing scott. If this is not alright Scott please feel
free to take it back.

Comment 22

[Mark Banner (:standard8)]

Opps, sorry it didn't re-assign it to me.

Comment 23

[Mark Banner (:standard8)]

Ok, now accepting as assigned. Sorry for the spam.

Comment 24

[Mark Banner (:standard8)]

Comment on attachment 166755 [details] [diff] [review]
Patch v3

Neil, revised as requested. I'm happier with the Filter List Dialog layout as
well now, though not 100% sure about order of buttons. Don't really want to
move the "run now" one as that lines up with the run on which folders option.

Anyway, see what you think.

Comment 25

[Mark Banner (:standard8)]

Comment on attachment 166755 [details] [diff] [review]
Patch v3

Cancelling review... I'm still not happy with the location of the buttons on
the filter dialog.

Also I think we should be updated the search addresses dialog as well.

Comment 26

[Mark Banner (:standard8)]

Attachment: Patch v4

Ok, I'm a lot happier with this patch - there is now a space in the list of
buttons which provides a visual break between close & new which I think is a
lot better.

I have also included the search addressbook dialog so that will match the
search message one.

Comment 27

[Mark Banner (:standard8)]

Updating target and Altering title from:
Message Filters and Message Search dialogs need close buttons.

Comment 28

[Mark Banner (:standard8)]

Neil, any chance you could have another look at the patch on this one sometime?
Thanks.

Comment 29

[[e-mail]]

Well, the Search windows look OK but I think you could improve on the filter
list   window - I don't understand why you put Help or Close on their own line.

Comment 30

[Mark Banner (:standard8)]

Comment on attachment 168243 [details] [diff] [review]
Patch v4

Cancelling review request, new patch coming soon.

Comment 31

[Mark Banner (:standard8)]

Attachment: Patch v5

Revised patch to incorporate better layout of filter list dialog. Search
dialogs unchanged from previous.

Comment 32

[[e-mail]]

Comment on attachment 174154 [details] [diff] [review]
Patch v5

>       <row>
>-        <description>&filterHeader.label;</description>
>+        <vbox pack="end">
>+          <description>&filterHeader.label;</description>
>+        </vbox>
<row align="end"> should work here. r=me with this fixed.

>@@ -172,25 +178,24 @@
I'm not particularly convinced by the help button move, but whatever...

Comment 33

[Mark Banner (:standard8)]

Attachment: Patch to fix nit

Patch that fixes Neil's nit, reusing r from previous patch, requesting sr.

Comment 34

[Scott MacGregor]

I disagree that these windows should have explicit close buttons. Especiallly
for windows that don't need a cancel button like AB and Mail Search. We've been
moving away from that design model for quite some time now. Look at the Themes,
Extensions, and RSS manager dialogs. This patch moves us backwards not forwards. 

Comment 35

[Mike Cowperthwaite]

(In reply to comment #34)
> I disagree that these windows should have explicit close buttons.

Has the problem where some Linux window managers do not (or did not) provide a 
default Close Window widget (comment 8 & 9) been addressed by some other means? 
Otherwise, some solution is necessary.


> windows that don't need a cancel button like AB and Mail Search. 

The point of this bug is that a window that doesn't need a Cancel button should 
have *some* button or other navigable, visible access to allow closure, that 
doesn't rely on the window manager.  Under Windows, at least, there is a 
keyboard technique: Alt+F4 closes the modeless windows, and in TB but not the 
suite, so does <ESC>.  Do these keystrokes, or equivalents, work under Linux?  
Mac?

Address Book has a menu with File|Exit (as does FF's Bookmark Manager) and so is 
not a good model for these other windows.


> We've been moving away from that design model for quite some time now. 
> Look at the Themes, Extensions, and RSS manager dialogs.

Those modeless dialogs are in fact all candidates for Close buttons; also the 
Manage CRLs dialog, in both the Suite (modeless) and Firefox (modal).  I haven't 
seen the new RSS Manager yet, but the 1.0 version has an OK without a Cancel, 
which is just a mislabelled Close button.

On the other hand, there are these windows which all feature a Close button or 
an OK without a Cancel (modeless except as noted):
 Check Spelling (modal -- TB only?  I don't have spellcheck installed for Moz.)
 Identity Manager (modal)
 Password Manager (modal under TB)
 Certificate Manager ("OK"; modal under TB)
 Security Device Manager ("OK"; modal under TB)
 Print Preview ("Close" in toolbar; does not close via <esc>)

Windows 2000, at least, doesn't have too many system dialogs left that don't 
have either a Cancel button or a menu with an Exit; but those that I can find at 
the moment all provide a Close button:  Organize Favorites, and a couple of 
Internet Options windows: Per-Site Privacy Options and Certificates.  These all 
have analogous structure to the Message Filters and the various item-manager 
dialogs -- which model includes the (modeless) Themes and Extensions dialogs, 
and I would say that they, also, should not only have a Close button, but should 
be 


See also Find In This Message (modeless), which *has* a Cancel button that is 
unnecessary, because searching text on an already-loaded buffer is almost 
instantaneous for any practical application.  This dialog should lose the 
Cancel, either replaced by Close or, if Scott's viewpoint prevails, removed 
entirely.  

(Putting Cancel on the text search dialog is also seen in Microsoft Word and 
Acrobat Reader, but that may be more justifiable because it can take an 
appreciable amount of time to search a large Word document.  The Cancel remains 
active during searching, providing a means to stop the search, but it also 
dismisses the dialog, so it's still an imperfect UI.)

Comment 36

[Harald Dunkel]

Maybe this is too basic to be mentioned here, but every GUI developer on XWindow
should verify his program at least once without a running window manager.

Regards

Harri

Comment 37

[Mark Banner (:standard8)]

Comment on attachment 174211 [details] [diff] [review]
Patch to fix nit

Looks like scott doesn't want this for TB, however Neil is considering for the
Suite. Therefore removing review request for the time being until we resolve
this.

Comment 38

[Mike Cowperthwaite]

(In reply to comment #35)
> the (modeless) Themes and Extensions dialogs [...] should not only have
> a Close button, but should be 

... modal (for consistency with other similar TB dialogs).

Comment 39

[Matthew T (active 1999-2002)]

I was asked to comment here, but I've said all I want to about this subject 
<http://mpt.phrasewise.com/2002/09/04#a332>. In brief: Please don't.

As for the keyboard, if a window has a "Cancel" or "Stop" button, it should 
close with Escape, and if it has neither, it should close with accel+W.

Comment 40

[Mark Banner (:standard8)]

(In reply to comment #39)
> I was asked to comment here, but I've said all I want to about this subject 
> <http://mpt.phrasewise.com/2002/09/04#a332>. In brief: Please don't.
> 
> As for the keyboard, if a window has a "Cancel" or "Stop" button, it should 
> close with Escape, and if it has neither, it should close with accel+W.

Neil and I have discussed this on IRC, and we have decided to leave out the
close (or equivalent) button as per the current design, but to add in the
accel+W keyboard option to close the window.

Comment 41

[Ian Batterbee]

Well, it's been two years that have passed since I first commented on this bug.
I initially thought that the dialog needed a close to bring it in line with
other mozilla dialogs, however having just checked it seems that a large number
of them now act the same way. I'm not sure if they have changed since this bug
was opened or not, but the same original request still seems to be valid - we
need consistency between dialog boxes in mozilla.

I've read Matthew's lengthy comments on why there shouldn't be a close button,
and his reference to MS's latest add/remove programs dialog, which does not
include a close button, and I think I understand where he's coming from.

However, there are still dialogs in Mozilla that seem to act the same way as the
mail filters (in that they have instant apply), but DO provide a close button.

Can we at least have them changed to be consistent within the application ?

Examples:
Tools / Password Manager / Manage Stored Passwords (has a close button)
View / Message Security Info (has an okay button which closes)

Comment 42

[Mark Banner (:standard8)]

(In reply to comment #41)
> However, there are still dialogs in Mozilla that seem to act the same way as the
> mail filters (in that they have instant apply), but DO provide a close button.
> 
> Can we at least have them changed to be consistent within the application ?

Yes, but please raise them in a new/different bug(s).

Comment 43

[Mark Banner (:standard8)]

Attachment: Allow accel-w to close the filter & search dialogs (SeaMonkey version)

This patch allows accel-w to close the filter and {AB,}search dialogs - SeaMonkey version. Thunderbird coming up in a while.

Comment 44

[Mark Banner (:standard8)]

Comment on attachment 202828 [details] [diff] [review]
Allow accel-w to close the filter & search dialogs (SeaMonkey version)

Opps, cancelling reviews - looking at Thunderbird, this has already been implemented, and reminds me that onSearchStop should be called before closing the window.

Comment 45

[Mark Banner (:standard8)]

Attachment: Allow accel-w to close the filter & search dialogs (revised)

Ok, Thunderbird has actually already got the accel-W functionality - implemented by bug 101153.

So this bug just now needs to do the same for SeaMonkey. This revised patch also makes sure we stop the search functions/save the filters when we close the window.

Note to reviewers: we already have closeCmd.key defined in FilterListDialog.dtd.

Comment 46

[[e-mail]]

Comment on attachment 202830 [details] [diff] [review]
Allow accel-w to close the filter & search dialogs (revised)

>+         oncommand="onSearchStop(); window.close();"/>
This is ugly. Unloading the window should cause the search to stop if necessary - there is already an unload handler, so you could chain it in there.

>+         oncommand="onFilterClose(); window.close();"/>
This is actually incorrect. In case a run filter operation is in progress, onFilterClose returns a boolean confirming the close. (Worse still, it actually calls window.close itself in this case. It also does some other unload-type stuff which should be broken out into an unload handler.)

Additionally, it would be helpful if you then put window.tryToClose = onFilterClose; somewhere in startup so that goQuitApplication (in globalOverlay.js) can find it.

Comment 47

[Mark Banner (:standard8)]

Attachment: Allow accel-w to close the filter & search dialogs (revised v2).

Revised addressing Neil's comments.

Comment 48

[[e-mail]]

Comment on attachment 203154 [details] [diff] [review]
Allow accel-w to close the filter & search dialogs (revised v2).

Nice :-)

Comment 49

[Karsten Düsterloh]

Comment on attachment 203154 [details] [diff] [review]
Allow accel-w to close the filter & search dialogs (revised v2).

The key for closing a window (key_close) is defined in platformCommunicatorOverlay.xul, which is included by utilityOverlay.xul, which in turn is used by both ABSearchDialog.xul and SearchDialog.xul (though not by FilterListDialog.xul yet). So redefining it is not a good idea in terms of ease of consistency.
This key_close specifies command="cmd_close", and that's the place where your custom close code should go! The dialogs don't need to know what the close key actually is...

>Index: mailnews/base/search/resources/content/ABSearchDialog.xul
>===================================================================
>+  <keyset id="AbSearchDialogKeys">
>+    <key id="closeAbSearch" key="&closeCmd.key;" modifiers="accel"
>+         oncommand="window.close();"/>
>+  </keyset>

BTW: It is good practice in XUL to start IDs with lower case and especially prefix key-IDs with key_, i.e. please use key_closeAbSearch. But this should be 

  <commandset id="abSearchDialogCommands">
    <command id="cmd_close" oncommand="window.close();"/>
  </commandset>

  <keyset id="abSearchDialogKeys">
    <key id="key_close"/>
  </keyset>

anyway (see intro).

>Index: mailnews/base/search/resources/content/FilterListDialog.xul
>===================================================================
>+  <keyset id="dialogKeys">
>+    <key id="closeFilterList" key="&closeCmd.key;" modifiers="accel"
>+         oncommand="if (onFilterClose()) window.close();"/>
>+  </keyset>

Since we don't include dialogOverlay.xul anyway, we can use something more fitting for id="dialogKeys", e.g. id="filterListKeys". After including utilityOverlay.xul, the above command/key structure should be used here, too.

BTW: closeCmd.key should be removed from FilterListDialog.dtd, it doesn't belong there.

>Index: mailnews/base/search/resources/content/SearchDialog.xul
>===================================================================
>+    <key id="closeSearch" key="&closeCmd.key;" modifiers="accel"
>+         oncommand="window.close();"/>

See above.

>Index: mailnews/base/search/resources/locale/en-US/SearchDialog.dtd
>===================================================================
>+<!-- for both SearchDialog.xul and ABSearchDialog.xul -->
>+<!ENTITY closeCmd.key                "W">
>+

Unneeded.

Comment 50

[Mark Banner (:standard8)]

Attachment: Allow accel-w to close the filter & search dialogs (revised v3).

Revised per mnyromyr's comments.

Comment 51

[Karsten Düsterloh]

Comment on attachment 204941 [details] [diff] [review]
Allow accel-w to close the filter & search dialogs (revised v3).

Sorry for the delay.

Comment 52

[Mark Banner (:standard8)]

Modified summary to reflect what we are actually doing.

Comment 53

[Mark Banner (:standard8)]

Patch checked in:
/cvsroot/mozilla/mailnews/base/search/resources/content/ABSearchDialog.xul,v  <--  ABSearchDialog.xul
new revision: 1.17; previous revision: 1.16
done
Checking in mailnews/base/search/resources/content/FilterListDialog.js;
/cvsroot/mozilla/mailnews/base/search/resources/content/FilterListDialog.js,v  <--  FilterListDialog.js
new revision: 1.62; previous revision: 1.61
done
Checking in mailnews/base/search/resources/content/FilterListDialog.xul;
/cvsroot/mozilla/mailnews/base/search/resources/content/FilterListDialog.xul,v  <--  FilterListDialog.xul
new revision: 1.69; previous revision: 1.68
done
Checking in mailnews/base/search/resources/content/SearchDialog.xul;
/cvsroot/mozilla/mailnews/base/search/resources/content/SearchDialog.xul,v  <--  SearchDialog.xul
new revision: 1.86; previous revision: 1.85
done
Checking in mailnews/base/search/resources/locale/en-US/FilterListDialog.dtd;
/cvsroot/mozilla/mailnews/base/search/resources/locale/en-US/FilterListDialog.dtd,v  <--  FilterListDialog.dtd
new revision: 1.19; previous revision: 1.18
done

Comment 54

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 204941 [details] [diff] [review]
Allow accel-w to close the filter & search dialogs (revised v3).

a=me for sm1.1

Comment 55

[Mark Banner (:standard8)]

Fix checked into branch.