Help button in Message Filter dialog lacks Help icon

RESOLVED FIXED in seamonkey2.0

Status

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: mnyromyr, Assigned: iann_bugzilla)

Tracking

({fixed-seamonkey2.0})

Trunk
seamonkey2.0
fixed-seamonkey2.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

9 years ago
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.
(Reporter)

Updated

9 years ago
OS: Linux → All
Hardware: x86 → All
Summary: Help button in Message Filter dialog lacks help icon → Help button in Message Filter dialog lacks Help icon

Comment 1

9 years ago
One problem is that we're dealing with a window and not a dialog.
(Reporter)

Comment 2

9 years ago
Which in itself is a bug, imo. ;-)
(Assignee)

Comment 3

9 years ago
Created attachment 400041 [details] [diff] [review]
Change Message Filter to dialog patch v0.1

This patch changes the FilterListDialog.xul from being a <window> to being a <dialog> with standard help button.
Assignee: nobody → iann_bugzilla
Status: NEW → ASSIGNED
Attachment #400041 - Flags: ui-review?(stefanh)
Attachment #400041 - Flags: superreview?(neil)
Attachment #400041 - Flags: review?(mnyromyr)
(Assignee)

Comment 4

9 years ago
Created attachment 400047 [details]
Screenshot-Message Filters

Comment 5

9 years ago
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.
Attachment #400041 - Flags: superreview?(neil) → superreview-

Comment 6

9 years ago
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).
(Assignee)

Comment 7

9 years ago
Created attachment 400153 [details]
Screenshot-Message Filters (help button moved)
Attachment #400047 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #400041 - Flags: ui-review?(stefanh)
Attachment #400041 - Flags: review?(mnyromyr)

Comment 8

9 years ago
That's certainly better than having the help button in the middle of the other buttons.
(Assignee)

Comment 9

9 years ago
Created attachment 400171 [details] [diff] [review]
Change Message Filter to dialog without overlay patch v0.1a

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.
Attachment #400041 - Attachment is obsolete: true
Attachment #400171 - Flags: ui-review?(stefanh)
Attachment #400171 - Flags: superreview?(neil)
Attachment #400171 - Flags: review?(mnyromyr)
(Assignee)

Updated

9 years ago
Attachment #400171 - Flags: ui-review?(stefanh)
Attachment #400171 - Flags: superreview?(neil)
Attachment #400171 - Flags: review?(mnyromyr)
(Assignee)

Comment 10

9 years ago
Created attachment 400268 [details] [diff] [review]
Change Message Filter to dialog no import on modern patch v0.1b

Changes since v0.1a:
* Remove import into modern as not required.
Attachment #400171 - Attachment is obsolete: true
Attachment #400268 - Flags: ui-review?(stefanh)
Attachment #400268 - Flags: superreview?(neil)
Attachment #400268 - Flags: review?(mnyromyr)
(Reporter)

Comment 11

9 years ago
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.
Attachment #400268 - Flags: review?(mnyromyr) → review-
(Assignee)

Updated

9 years ago
Attachment #400268 - Flags: ui-review?(stefanh)
Attachment #400268 - Flags: superreview?(neil)
(Assignee)

Comment 12

9 years ago
Created attachment 401960 [details] [diff] [review]
Change Message Filter to dialog all in grid patch v0.1c

Changes since v0.1b:
* Moved top line and help button into grid.
Attachment #400268 - Attachment is obsolete: true
Attachment #401960 - Flags: review?(mnyromyr)
(Assignee)

Comment 13

9 years ago
Created attachment 401961 [details]
Screenshot-Message Filters in grid
Attachment #400153 - Attachment is obsolete: true
(Reporter)

Comment 14

9 years ago
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.
Attachment #401960 - Flags: review?(mnyromyr) → review-
(Assignee)

Comment 15

9 years ago
Created attachment 402201 [details] [diff] [review]
Change Message Filter to dialog all in grid with less boxes patch v0.1d

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.
Attachment #401960 - Attachment is obsolete: true
Attachment #402201 - Flags: review?(mnyromyr)
(Reporter)

Comment 16

9 years ago
Comment on attachment 402201 [details] [diff] [review]
Change Message Filter to dialog all in grid with less boxes patch v0.1d

That's it!
Attachment #402201 - Flags: review?(mnyromyr) → review+
(Assignee)

Updated

9 years ago
Attachment #402201 - Flags: superreview?(neil)
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.)
(Assignee)

Comment 18

9 years ago
(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.
Attachment #402201 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 20

9 years ago
Created attachment 403069 [details] [diff] [review]
Change Message Filter to dialog all in grid with revised label patch v0.1e [Checked in: Comment 28]

Changes since v0.1d:
* Label id="filterHeader" back to original style.
* Added comments for trunk css.
Attachment #402201 - Attachment is obsolete: true
Attachment #403069 - Flags: superreview+
Attachment #403069 - Flags: review+
(Assignee)

Comment 21

9 years ago
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.
Attachment #403069 - Flags: review?(stefanh)

Updated

9 years ago
Attachment #403069 - Flags: review?(stefanh) → review+

Comment 22

9 years ago
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?

Comment 23

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

Comment 24

9 years ago
(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.

Comment 26

9 years ago
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.
(Assignee)

Comment 27

9 years ago
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
Attachment #403069 - Flags: approval-seamonkey2.0?

Updated

9 years ago
Attachment #403069 - Flags: approval-seamonkey2.0? → approval-seamonkey2.0+
(Assignee)

Comment 28

9 years ago
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
Attachment #403069 - Attachment description: Change Message Filter to dialog all in grid with revised label patch v0.1e → Change Message Filter to dialog all in grid with revised label patch v0.1e [Checked in: Comment 28]
(Assignee)

Updated

9 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: fixed-seamonkey2.0
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0
(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.
You need to log in before you can comment on or make changes to this bug.