The (custom) Help button in the Message Filter 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 FilterListDialog.xul does not need to know about the OpenHelp function.

One problem is that we're dealing with a window and not a dialog.

Which in itself is a bug, imo. ;-)

This patch changes the FilterListDialog.xul from being a <window> to being a <dialog> with standard help button.

Comment on attachment 400041 [details] [diff] [review] Change Message Filter to dialog patch v0.1 To create a Mac help button, add dlgtype="help" to the button, and @import the global dialog.css; to create a Linux help button, add icon="help" to the button.

You'll need to add class="dialog-button" as well. But the question mark looks really weird between those other buttons, so I wouldn't recommend doing it unless you move the button to the bottom right (above the statusbar - haven't tried if that is possible and how it looks, though).

That's certainly better than having the help button in the middle of the other buttons.

Changes since v0.1: * filterDialog.css now imports global dialog.css * 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.

Changes since v0.1a: * Remove import into modern as not required.

Comment on attachment 400268 [details] [diff] [review] Change Message Filter to dialog no import on modern patch v0.1b Having a help button which (incl. the separator) pockets approximatively 15% of the entire dialog is just mad. >- <button label="&helpButton.label;" accesskey="&helpButton.accesskey;" >- oncommand="openHelp('mail-filters', 'chrome://communicator/locale/help/suitehelp.rdf');"/> That's okay, though, the help doesn't belong here. > <separator/> >+ <hbox pack="start"> >+ <button dlgtype="help" class="dialog-button"/> >+ </hbox> That's a real waste of screen estate and doesn't actually make the dialog more "dialoguesk", because we still have a status bar below. How about this: - We already use a grid for layout, so put the first two lines of content in it as well, vertically aligning the "Filter Log" button with those below. - Move the "Run Now" button right behind the (now flexed) dropdown, "freeing" its old position in the "button column". - Drop the separator before the statusbar. - Put the help button in the now free lower right cell of the grid.

Changes since v0.1b: * Moved top line and help button into grid.

Comment on attachment 401960 [details] [diff] [review] Change Message Filter to dialog all in grid patch v0.1c All in all, it looks and works as it should. :) But under the hood, I found a couple of nits... >+<dialog id="filterListDialog" >+ xmlns:nc="http://home.netscape.com/NC-rdf#" >+ xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" Exchange the two namespace references. > <row> >+ <vbox> I know it's not your fault (and sorry for not mentioning this before), but the use of <vbox>es in this grid is rightout excessive. Most of them are just plain useless. <row>s are horizontal boxes by default and need one child element per grid column, and you can use attributes pack and align to steer the positioning... Please keep the DOM as flat as possible. > <menulist id="runFiltersFolder" disabled="true"/> >+ <button id="runFiltersButton" >+ label="&runFilters.label;" >+ accesskey="&runFilters.accesskey;" >+ runlabel="&runFilters.label;" >+ runaccesskey="&runFilters.accesskey;" >+ stoplabel="&stopFilters.label;" >+ stopaccesskey="&stopFilters.accesskey;" >+ oncommand="runSelectedFilters();" >+ disabled="true"/> This means that the button will possibly change its position with every selection in the serverMenu menulist, which isn't particularly nice. Either make the runFiltersFolder menulist flex or put a flexible spacer behind it. >+ <vbox align="center"> >+ <button dlgtype="help" class="dialog-button"/> > </vbox> The default Classic toolkit theme for Linux (at least, maybe Modern also?) sports a margin-top:6px rule, which you need to get rid of, because it makes the help button not center vertically correctly. >+ <progressmeter class="progressmeter-statusbar" >+ id="statusbar-icon" >+ mode="normal" >+ value="0"/> The id should be the first attribute.

Changes since v0.1c: * Removed unneeded vboxes/hboxes. * Fixed css for classic help button. * Added space before "Run Now" button. * Swapped namespace order. * Moved id attribute to start of element.

Comment on attachment 402201 [details] [diff] [review] Change Message Filter to dialog all in grid with less boxes patch v0.1d That's it!

Comment on attachment 402201 [details] [diff] [review] Change Message Filter to dialog all in grid with less boxes patch v0.1d >- <label control="filterTree" id="filterHeader">&filterHeader.label;</label> >+ <label id="filterHeader" >+ value="&filterHeader.label;" >+ control="filterTree"/> Why this change? > </row> > >+ <row flex="1"> >+ <tree id="filterTree" >+ flex="1" Don't need flex="1" on a grid member. > </row> >+ <row align="center"> Add a blank line perhaps? >+ <vbox align="center"> >+ <button dlgtype="help" class="dialog-button"/> I guess this is needed to make it work right on the Mac? >+.dialog-button { >+ margin-top: 1px !important; Does this work OK on the Mac? (On trunk, once the Mac styles are moved to button.css we won't need to import dialog.css, so the problem goes away.)

(In reply to comment #17) > (From update of attachment 402201 [details] [diff] [review]) > >- <label control="filterTree" id="filterHeader">&filterHeader.label;</label> > >+ <label id="filterHeader" > >+ value="&filterHeader.label;" > >+ control="filterTree"/> > Why this change? Just so it matches the other label elements within the grid. > > > </row> > >+ <row align="center"> > Add a blank line perhaps? There is already one, it is half between a set of - and + > > >+ <vbox align="center"> > >+ <button dlgtype="help" class="dialog-button"/> > I guess this is needed to make it work right on the Mac? stefanh suggested it was needed. > > >+.dialog-button { > >+ margin-top: 1px !important; > Does this work OK on the Mac? stefanh would have to confirm. > (On trunk, once the Mac styles are moved to > button.css we won't need to import dialog.css, so the problem goes away.) I'll create a spin off bug to remove the import (and if need be the rule above too).

Comment on attachment 402201 [details] [diff] [review] Change Message Filter to dialog all in grid with less boxes patch v0.1d >- <label control="filterTree" id="filterHeader">&filterHeader.label;</label> >+ <label id="filterHeader" >+ value="&filterHeader.label;" >+ control="filterTree"/> This change is wrong, the label needs to be able to wrap in other locales. sr=me with this fixed. > </row> > >+ <row align="center"> Oh yeah, I overlooked it.

Changes since v0.1d: * Label id="filterHeader" back to original style. * Added comments for trunk css.

Comment on attachment 403069 [details] [diff] [review] Change Message Filter to dialog all in grid with revised label patch v0.1e [Checked in: Comment 28] Asking stefanh for review from a Mac angle.

Comment on attachment 403069 [details] [diff] [review] Change Message Filter to dialog all in grid with revised label patch v0.1e [Checked in: Comment 28] +/* This should not be needed on trunk once Mac styles are moved to button.css */ +.dialog-button { + margin-top: 1px !important; +} + Btw, Is this comment really right? Maybe it's too late in the night for me, but what style rules are you going to move to move in order to not need this on non-mac?

And also, what style rules are you planning to move to mac's button.css in order to skip the import rule?

(In reply to comment #22) > (From update of attachment 403069 [details] [diff] [review]) > +/* This should not be needed on trunk once Mac styles are moved to button.css > */ > Btw, Is this comment really right? Maybe it's too late in the night for me, but > what style rules are you going to move to move in order to not need this on > non-mac? (In reply to comment #23) > And also, what style rules are you planning to move to mac's button.css in > order to skip the import rule? See comment #17

In particular, the /* ::::: dialog buttons ::::: */ section.

In the long-time perspective I guess the css file needs to be forked since the filter dialog needs more mac-love in order to look good.

Comment on attachment 403069 [details] [diff] [review] Change Message Filter to dialog all in grid with revised label patch v0.1e [Checked in: Comment 28] Requesting a= for low risk, polish patch

Comment on attachment 403069 [details] [diff] [review] Change Message Filter to dialog all in grid with revised label patch v0.1e [Checked in: Comment 28] http://hg.mozilla.org/comm-central/rev/676a5cd80eab

(In reply to comment #23) > And also, what style rules are you planning to move to mac's button.css in > order to skip the import rule? See bug 520371.