Last Comment Bug 515228 - Help button in Search Addresses dialog lacks Help icon
: Help button in Search Addresses dialog lacks Help icon
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: MailNews: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1a1
Assigned To: Ian Neal
:
:
Mentors:
Depends on: 543160
Blocks: 672155
  Show dependency treegraph
 
Reported: 2009-09-08 13:12 PDT by Karsten Düsterloh
Modified: 2015-06-02 17:20 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Change Search Messages/Addresses to dialog patch v0.1 (13.23 KB, patch)
2009-09-11 08:25 PDT, Ian Neal
neil: superreview-
Details | Diff | Splinter Review
Screenshot-Advanced Address Book Search (26.58 KB, image/png)
2009-09-11 08:29 PDT, Ian Neal
no flags Details
Screenshot-Search Messages (31.13 KB, image/png)
2009-09-11 08:30 PDT, Ian Neal
no flags Details
Change Search Messages/Addresses to dialog without overlay patch v0.1a (13.08 KB, patch)
2009-09-12 05:48 PDT, Ian Neal
mnyromyr: review-
Details | Diff | Splinter Review
Screenshot-Advanced Address Book Search (moved Help button) (26.24 KB, image/png)
2009-09-12 05:56 PDT, Ian Neal
no flags Details
Screenshot-Search Messages (moved Help button) (30.60 KB, image/png)
2009-09-12 05:57 PDT, Ian Neal
no flags Details
Change Search Messages/Addresses to dialog help on right patch v0.1b (19.30 KB, patch)
2009-09-21 16:23 PDT, Ian Neal
mnyromyr: review-
Details | Diff | Splinter Review
Screenshot-Advanced Address Book Search right help (26.74 KB, image/png)
2009-09-21 16:24 PDT, Ian Neal
no flags Details
Screenshot-Search Messages right help (30.91 KB, image/png)
2009-09-21 16:24 PDT, Ian Neal
no flags Details
Change Search Messages/Addresses to dialog with command last patch v0.1c (21.93 KB, patch)
2009-09-26 10:10 PDT, Ian Neal
mnyromyr: review+
Details | Diff | Splinter Review
Remove Close Window key patch v0.1d [Checkin: comment 27] (24.52 KB, patch)
2009-10-08 17:16 PDT, Ian Neal
iann_bugzilla: review+
neil: superreview+
kairo: approval‑seamonkey2.0-
Details | Diff | Splinter Review

Description Karsten Düsterloh 2009-09-08 13:12:32 PDT
The (custom) Help button in the Search Messages dialog lacks the Help icon used by standard dialog Help buttons. Furthermore, the Help button definition should come from helpMessengerOverlay.xul as well, so that ABSearchDialog.xul does not need to know about the OpenHelp function.
Comment 1 Karsten Düsterloh 2009-09-08 13:13:21 PDT
Search Addresses dialog, of course.
Comment 2 Ian Neal 2009-09-11 08:25:05 PDT
Created attachment 400043 [details] [diff] [review]
Change Search Messages/Addresses to dialog patch v0.1

This patch changes the SearchDialog.xul from being a <window> to being a <dialog> with standard help button.
Comment 3 Ian Neal 2009-09-11 08:29:56 PDT
Created attachment 400045 [details]
Screenshot-Advanced Address Book Search
Comment 4 Ian Neal 2009-09-11 08:30:26 PDT
Created attachment 400046 [details]
Screenshot-Search Messages
Comment 5 neil@parkwaycc.co.uk 2009-09-11 09:10:57 PDT
Comment on attachment 400043 [details] [diff] [review]
Change Search Messages/Addresses to dialog patch v0.1

As per bug 515227. Oh, and if you really want the help button to be in the overlay, then give the container an id and overlay the container to add it.
Comment 6 Stefan [:stefanh] 2009-09-11 10:12:52 PDT
You should put the help button in the lower right corner of the dialog instead.
Comment 7 Ian Neal 2009-09-12 05:48:21 PDT
Created attachment 400267 [details] [diff] [review]
Change Search Messages/Addresses to dialog without overlay patch v0.1a

Changes since v0.1:
* searchDialog.css now imports global dialog.css for default theme.
* help button is now moved to where help button is usually found on a dialog
(dlgtype="help" seems to give icon="help" automagically).
* added class="dialog-button" to help button.
Comment 8 Ian Neal 2009-09-12 05:56:34 PDT
Created attachment 400270 [details]
Screenshot-Advanced Address Book Search (moved Help button)
Comment 9 Ian Neal 2009-09-12 05:57:12 PDT
Created attachment 400271 [details]
Screenshot-Search Messages (moved Help button)
Comment 10 Stefan [:stefanh] 2009-09-12 06:04:44 PDT
Is it possible to put the Help button in the right corner?
Comment 11 Ian Neal 2009-09-12 06:19:56 PDT
(In reply to comment #10)
> Is it possible to put the Help button in the right corner?

Yes, but would have to put some #ifdef's in for *nix vs the rest.
Comment 12 neil@parkwaycc.co.uk 2009-09-12 09:14:05 PDT
(In reply to comment #0)
> the Help button definition should come from helpMessengerOverlay.xul as well,
> so that ABSearchDialog.xul does not need to know about the OpenHelp function.
helpMessengerOverlay.xul was only created to add buttons to shared dialogs...

(In reply to comment #10)
> Is it possible to put the Help button in the right corner?
It would certainly make more sense. It looks terrible in the left corner.
Comment 13 neil@parkwaycc.co.uk 2009-09-12 09:15:01 PDT
Comment on attachment 400267 [details] [diff] [review]
Change Search Messages/Addresses to dialog without overlay patch v0.1a

>-<window id="searchAddressBookWindow"
>+<dialog id="searchAddressBookWindow"
I still don't see the point of the window/dialog conversion.
Comment 14 Karsten Düsterloh 2009-09-12 19:26:36 PDT
Comment on attachment 400267 [details] [diff] [review]
Change Search Messages/Addresses to dialog without overlay patch v0.1a

What's the reason for The Great Accesskey Renaming?

Basically, the lone Help button down left is rather pointless. Just move one line up and to the outer right there.

Furthermore, the top address book dropdown should flex horizontally.
Comment 15 Ian Neal 2009-09-21 16:23:25 PDT
Created attachment 401966 [details] [diff] [review]
Change Search Messages/Addresses to dialog help on right patch v0.1b

Changes since v0.1b:
* Moved to right and inline with rest of the buttons in both dialogs.

Help now takes H accesskey (as is the default on most other dialogs/windows) hence the accesskey shuffle.
Comment 16 Ian Neal 2009-09-21 16:24:02 PDT
Created attachment 401967 [details]
Screenshot-Advanced Address Book Search right help
Comment 17 Ian Neal 2009-09-21 16:24:27 PDT
Created attachment 401968 [details]
Screenshot-Search Messages right help
Comment 18 Karsten Düsterloh 2009-09-25 15:22:11 PDT
Comment on attachment 401966 [details] [diff] [review]
Change Search Messages/Addresses to dialog help on right patch v0.1b

Basically okay, just some nits:

>+++ b/suite/locales/en-US/chrome/mailnews/SearchDialog.dtd
> <!ENTITY openButton.label            "Open">
> <!ENTITY openButton.accesskey        "n">

I think this Open should get the O...

>+<dialog id="searchAddressBookWindow"
>         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>         xmlns:nc="http://home.netscape.com/NC-rdf#"
>         onload="searchOnLoad();"
>         onunload="onSearchStop(); searchOnUnload();"
>+        buttons="help"
>+        ondialoghelp="return openHelp('mail_advanced_ab_search');"

Command handler attributes should go last.

>           <label value="&abSearchHeading.label;" accesskey="&abSearchHeading.accesskey;" control="abPopup"/>
>           <menulist id="abPopup" oncommand="SelectDirectory(this.value);">
>             <menupopup id="abPopup-menupopup" class="addrbooksPopup"/>
>           </menulist>
>           <spacer flex="10"/>
>           <button id="search-button" oncommand="onSearchButton(event);" default="true"/>

Linux Classic at least has a problem with styling menulists. Although there should be enough space here, the menuitem "Personal Address Book" gets truncated to "Personal Addre..." in the menulist and to "Personal Address..." in the popup. How'd you feel about making the top menulist in all three dialogs (Message Search, AB Search, Filters) flex?

>+        <button dlgtype="help" class="dialog-button"/>

As with the filters dialog, the Help button draws an unwanted "margin-top: 6px" rule here. :-/

>+<dialog id="searchMailWindow"
>         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>         xmlns:nc="http://home.netscape.com/NC-rdf#"
>         onload="searchOnLoad();"
>         onunload="onSearchStop(); searchOnUnload();"
>+        buttons="help"
>+        ondialoghelp="return openHelp('search_messages');"

Command handler attributes should go last.
Comment 19 Ian Neal 2009-09-26 10:10:30 PDT
Created attachment 403035 [details] [diff] [review]
Change Search Messages/Addresses to dialog with command last patch v0.1c

Changes since v0.1b:
* Removed 6px margin from the top of help button in classic.
* Changed order of attributes on dialog box.

As discussed changes to size of menuitem will happen in bug 515967.
Comment 20 neil@parkwaycc.co.uk 2009-10-03 08:49:14 PDT
Testing this patch made me notice that there's a bug in searchDialog.css (both themes) whereby for some reason the padding on menuitems is overridden. This unfortunately isn't the whole story for address book search, because it suffers from the additional indignity that the width of menulists is overridden, when in fact only the message folders menulist should have its width set.

When I added the "Search local system" checkbox to SearchDialog.xul it was convenient that I could add it to what was a blank cell in the "grid". I'm not sure that I would have put it there had the Help button been at the bottom.

Now that these are dialogs, should we remove the Ctrl+W key to close them?
Comment 21 Ian Neal 2009-10-08 15:23:23 PDT
(In reply to comment #20)
> Testing this patch made me notice that there's a bug in searchDialog.css (both
> themes) whereby for some reason the padding on menuitems is overridden. This
> unfortunately isn't the whole story for address book search, because it suffers
> from the additional indignity that the width of menulists is overridden, when
> in fact only the message folders menulist should have its width set.
See my comment#19
> 
> When I added the "Search local system" checkbox to SearchDialog.xul it was
> convenient that I could add it to what was a blank cell in the "grid". I'm not
> sure that I would have put it there had the Help button been at the bottom.
So should it go in parallel to the other checkbox?
> 
> Now that these are dialogs, should we remove the Ctrl+W key to close them?
Yes, probably.
Comment 22 neil@parkwaycc.co.uk 2009-10-08 15:56:33 PDT
(In reply to comment #21)
> See my comment#19
Sorry, I didn't realise the significance.

> So should it go in parallel to the other checkbox?
Yes, probably ;-)
Comment 23 Ian Neal 2009-10-08 17:16:07 PDT
Created attachment 405366 [details] [diff] [review]
Remove Close Window key patch v0.1d [Checkin: comment 27]

Changes since v0.1c:
* Move checkboxes in Search Messages to be in parallel.
* Remove Ctrl-W from new dialog boxes.
Comment 24 Stefan [:stefanh] 2009-10-09 00:24:21 PDT
Regarding the removal of Accel+W: Is this just because it's a dialog? If that's the case, is there anyone that will think it's a dialog? I mean, I have never seen a dialog with a status bar...
Comment 25 Ian Neal 2009-10-09 15:42:54 PDT
Comment on attachment 405366 [details] [diff] [review]
Remove Close Window key patch v0.1d [Checkin: comment 27]

I'll ask for a= as even there are locale changes they are just accesskeys and the one string that is removed, even if localizers don't remove it it should make no difference.
Comment 26 Robert Kaiser 2009-10-10 07:03:24 PDT
Comment on attachment 405366 [details] [diff] [review]
Remove Close Window key patch v0.1d [Checkin: comment 27]

I don't like the locale changes for 2.0 final, esp. not after RC1 has been cut. The accesskeys would possibly be OK, but the removed strings mean all dashboard entries turning orange, which is not something I'd like to have in final release preparations.
We can probably look into taking that for 2.0.1 with the necessary thunder in the L10n group/list, but please not before 2.0 proper.
Comment 27 Ian Neal 2009-11-03 04:40:41 PST
Comment on attachment 405366 [details] [diff] [review]
Remove Close Window key patch v0.1d [Checkin: comment 27]

http://hg.mozilla.org/comm-central/rev/cefd439cd3c2
Comment 28 Jens Hatlak (:InvisibleSmiley) 2010-04-02 07:54:37 PDT
The switch to a dialog made here caused bug 543160.
Comment 29 Jens Hatlak (:InvisibleSmiley) 2011-07-18 14:09:50 PDT
(In reply to comment #24)
> Regarding the removal of Accel+W: Is this just because it's a dialog? If
> that's the case, is there anyone that will think it's a dialog? I mean, I
> have never seen a dialog with a status bar...

I don't really get the logic behind that either; the whole reasoning that was put into words here comes down to "because it's a dialog [<dialog>], Ctrl+W should not close it", but I don't understand where that's coming from. Please enlighten us and leave your comments over at bug 672155.
Comment 30 Rainer Bielefeld 2015-04-25 23:31:54 PDT
Are we talking about the lifebelt symbol in Help button left from "Help" text?
I still do not see that  with EN-US Seamonkey 2.33.1 (German Language pack)  Gecko/20100101 Build 20150321194901 (Default Theme) on German WIN7 64bit.
Comment 31 Rainer Bielefeld 2015-04-25 23:41:04 PDT
Hm, I never saw that lifebelt in any help button.
Comment 32 Ian Neal 2015-06-02 15:45:13 PDT
I remember it in the past, but I think it depends on the OS (and OS theme).
Comment 33 rsx11m 2015-06-02 17:20:05 PDT
Yeah, it's an OS icon I think, and as such depends on the desktop theme.
See attachment 8602370 [details] for a prefpane example on Linux with KDE4.

Note You need to log in before you can comment on or make changes to this bug.