195224 - [views/filter/search] Remove "More" and "Fewer" buttons and add + and - buttons per line

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, enhancement)

Description

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

spun off from bug #183994

"8. Remove "More" and "Fewer" buttons and add + and - buttons per line. This
makes it easier to insert or remove a line in a specific order. See spec.
http://www.mozilla.org/mailnews/specs/filters/#Filter%20Rules%20D"

we want this, but too risky for 1.4 alpha.  let's go for 1.5 alpha

Comment 1

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

shuehan already has a patch, so putting in 1.5 alpha for her.

Comment 2

[Stefan Borggraefe]

I resurrected Shuehan's patch and enhanced it a little bit (added tooltips to
the [+] and [-] buttons, adjusted the default size of the dialogs). It works as
expected. There is just one little problem: The scrollbar of the rule list
overlaps the [-] buttons a bit. I currently have no idea how to fix this.

Is there still interest in this UI change?

Comment 3

[Stefan Borggraefe]

Attachment: Patch

Comment 4

[Scott MacGregor]

Stefan, could I trouble you for a screen shot?

Comment 5

[Stefan Borggraefe]

Attachment: Screenshot

This is how the Search Messages dialog looks like when you just opened it.

Comment 6

[Stefan Borggraefe]

Attachment: Screenshot with more search rules

Screenshot of the Search Messages window with four search rules. This
screenshots shows the problem the current patch still has: the scrollbar
overlaps the [-] button. This only disappears when you make the dialog much
wider.

Comment 7

[Scott MacGregor]

I like this change a lot. 

Stefan, is the remaining issue just a case of the initial window widths not
being wide enough? Or maybe a search term row doesn't have any flex on it,
preventing it from taking as much horizontal space as it needs to keep the
scroll bar from clipping it...

Comment 8

[Stefan Borggraefe]

(In reply to comment #7)
> Stefan, is the remaining issue just a case of the initial window widths not
> being wide enough?

No.

> Or maybe a search term row doesn't have any flex on it,
> preventing it from taking as much horizontal space as it needs to keep the
> scroll bar from clipping it...

I think the cause of this is a more general problem. The contents of the
listcols aren't scaled properly when the size of the enclosing listbox changes.
This is already the case with the current searchTermOverlay.xul. The problem is
more or less fixed in the curent implementation by applying carefully chosen
flex values and using additional hidden listcols. But still, when you make a
window which uses this Overlay smaller, you see the contents of the rule list
aren't scaled well and overlap each other to early.

Adding a the new column with the two buttons makes this problem even more
apparent. Either I use a small value for the flex of this new listcol (I used 2
for the screenshot) and then I get the problem with the scrollbar, or I use a
higher value (like 11). Then the problem with the scrollbar only happens when
the window size is much smaller, but you get a very large white area right from
the new buttons when you make the window larger.

Maybe the implementation of the <listbox> widget needs to be improved to have a
better scaling behaviour in order solve this problem cleanly...

Comment 9

[[e-mail]]

Attachment: Screenshot, classic theme

Comment 10

[[e-mail]]

Attachment: Screenshot, retro theme

Although, strangely, my theme is unaffected by listbox shenanigans :-)

Comment 11

[Stefan Borggraefe]

Ok, Neil somhow got this right in his Retro theme.

I'm currently trying to figure out what needs to be changed in the
Classic/Modern-CSS.

Comment 12

[Stefan Borggraefe]

I still haven't figured out why the widgets in the Search criteria listbox need
so much width. Any help or hints for resolving this issue would be great!

Comment 13

[Scott MacGregor]

I've taken Stefan's patch and fixed the problem with the scrollbar cutting into
the buttons. 

Comment 14

[Scott MacGregor]

Attachment: updated patch that no longer has the clipping issue shown in the screen shots

This patch is Stefan's original patch with the following changes:

1) I added a min width (which is set by localizers) to the listbox to prevent
the widgets from running into each other when you make the dialog smaller than
the width of the list box. 

2) I added an empty list cell to the end of the first row of search terms. This
empty list cell provides us with enough space for the scrollbar to get drawn
into when it shows up instead of cutting into the less button

3) I got rid of the hidden flex columns between the search criteria elements.
These were no longer useful (as far as I could tell) now that we have a minimum
width on the list box.

Comment 15

[Stefan Borggraefe]

Comment on attachment 182113 [details] [diff] [review]
updated patch that no longer has the clipping issue shown in the screen shots

I will have a look at this patch on monday.

Comment 16

[Stefan Borggraefe]

Comment on attachment 182113 [details] [diff] [review]
updated patch that no longer has the clipping issue shown in the screen shots

The work-arounds for the problems I encountered with Shuehan's patch are good.
:-) There is still the problem that there is an increasingly large blank area
on the right of the search criteria list when you make the dialog wider. But I
think this is acceptable.

I noticed some other things:

>Index: mail/themes/qute/mail/preferences/preferences.css
>===================================================================
>@@ -214,3 +214,4 @@
>   margin-left: 6px !important; 
>   font-weight: bold;
> }
>+

I don't think this change is needed. ;-)

>Index: mailnews/base/search/resources/content/searchTermOverlay.js
>===================================================================
>-function onMore(event)
>+function onMore(event, rowNumber)
> {
>+    if (document.getElementById("addressbookList")) {
>+      var boolRadiogroup = document.getElementById("booleanAndGroup");
>+      var boolValue = (boolRadiogroup.getAttribute("value") == "and") ? true : false;

The ternary operator is not needed here.

>+      if (boolValue) {
>+        var searchAttr = document.getElementById("searchAttr" + rowNumber);
>+        var menulist = document.getAnonymousNodes(searchAttr)[0];
>+        var popup = menulist.firstChild;
>+        var bundle = document.getElementById("bundle_searchAttrs");
>+        var nameLabel = bundle.getString("32"); // Display Name
>+        popup.childNodes[0].setAttribute("label", nameLabel);
>+        menulist.setAttribute("label", nameLabel);
>+        popup.childNodes[2].setAttribute("hidden", true); // hide "Any Number"
>+      }
>+    }
>+

I don't really understand this part. bundle_searchAttrs doesn't seem to exists:

http://lxr.mozilla.org/mozilla/search?string=bundle_searchAttrs

>     // the user just added a term, so scroll to it
>     scrollToLastSearchTerm(gTotalSearchTerms);
> }
> 
>-function onLess(event)
>+function onLess(event, rowNumber)
> {
>-    if (gTotalSearchTerms>1)
>-        removeSearchRow(--gTotalSearchTerms);
>-    if (gTotalSearchTerms==1)
>-        gSearchLessButton .setAttribute("disabled", "true");
>+    if (gTotalSearchTerms > 1) {
>+      removeSearchRow(rowNumber);    
>+      --gTotalSearchTerms;
>+    }
> 
>-    // the user removed a term, so scroll to the bottom so they are aware of it
>-    scrollToLastSearchTerm(gTotalSearchTerms);
>+    if (rowNumber == 0) { // removed first row, need to reset boolOp label
>+      var firstBoolOp = document.getElementById("boolOp0");
>+      if (!firstBoolOp.hidden) 
>+        firstBoolOp.setAttribute("value", gBooleanInitialText);
>+    }

This part of Shuehan's patch seems to be ancient code from when the search
dialog looked like this:

http://www.mozilla.org/mailnews/specs/search/#Mail

It looks like all code dealing with the boolOp-Variables in this file could be
removed.

>+
>+    if (gTotalSearchTerms==1)
>+      document.getElementById("less0").setAttribute("disabled", "true");

Nit: Spaces around ==.

>@@ -283,6 +309,19 @@
>     var enclosingBox = document.createElement('vbox');
>     var boolOp = document.createElement('label');
> 
>+    var buttonBox = document.createElement("hbox");
>+    buttonBox.setAttribute("align", "start");
>+    var moreButton = document.createElement("button");
>+    var lessButton = document.createElement("button");
>+    moreButton.setAttribute("class", "small-button");
>+    moreButton.setAttribute("oncommand", "onMore(event," + (index + 1) + ");");
>+    moreButton.setAttribute('label', '+');
>+    moreButton.setAttribute('tooltiptext', gMoreButtonTooltipText);
>+    lessButton.setAttribute("class", "small-button");
>+    lessButton.setAttribute("oncommand", "onLess(event," + index + ");");    
>+    lessButton.setAttribute('label', '-');

'\u2212' instead of '-' looks better (see bug 264477).

>@@ -304,13 +350,40 @@
>     var rowdata = new Array(enclosingBox, searchAttr,
>                             null, searchOp,
>                             null, searchVal,
>-                            null);
>+                            null, buttonBox);
>     var searchrow = constructRow(rowdata);
>     searchrow.id = "searchRow" + index;
> 
>+    // shift items in gSearchTerms
>+    if (index < gTotalSearchTerms) {    
>+      for (var i = gSearchTerms.length - 1; i >= index; --i) {      
>+        var nextSearchObj = gSearchTerms[i].obj;
>+      
>+        var newSearchObj = new searchTermContainer;
>+        gSearchTerms[i+1] = {obj:newSearchObj, scope:scope, searchTerm:searchTerm, initialized:true};
>+
>+        var nextSearchAttr = nextSearchObj.searchattribute;
>+        nextSearchAttr.id = "searchAttr" + (i+1);        
>+        
>+        var nextSearchOp = nextSearchObj.searchoperator;
>+        nextSearchOp.id = "searchOp" + (i+1);
>+              
>+        var nextSearchVal = nextSearchObj.searchvalue;
>+        nextSearchVal.id = "searchVal" + (i+1);

Nits: Add spaces around +. There are a lot of trailing spaces in this section.

>Index: mailnews/base/search/resources/locale/en-US/searchTermOverlay.dtd
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/base/search/resources/locale/en-US/searchTermOverlay.dtd,v
>retrieving revision 1.5
>diff -u -w -r1.5 searchTermOverlay.dtd
>--- mailnews/base/search/resources/locale/en-US/searchTermOverlay.dtd	12 Jun 2002 17:00:53 -0000	1.5
>+++ mailnews/base/search/resources/locale/en-US/searchTermOverlay.dtd	28 Apr 2005 22:48:25 -0000
>@@ -11,11 +11,8 @@
>   Change the values only when the localized strings in the popup menus
>   are truncated in the widgets.
>  -->
>-<!ENTITY searchTermListSpace1FlexValue "1">
>-<!ENTITY searchTermListAttributesFlexValue "4">
>-<!ENTITY searchTermListSpace2FlexValue "0">
>-<!ENTITY searchTermListOperatorsFlexValue "4">
>-<!ENTITY searchTermListSpace3FlexValue "0">
>-<!ENTITY searchTermListValueFlexValue "4">
>-<!ENTITY searchTermListSpace4FlexValue "1">
>-
>+<!ENTITY searchTermListAttributesFlexValue "5">
>+<!ENTITY searchTermListOperatorsFlexValue "5">
>+<!ENTITY searchTermListValueFlexValue "5">
>+<!ENTITY searchTermListButtonsFlexValue "2">
>+<!ENTITY searchTermListMinWidth "min-width: 57em;">

Also remove more.label/accesskey and less.label/accesskey from this file.

Comment 17

[Scott MacGregor]

Attachment: updated patch with review comments

Thanks for the comments on the patch. 

1) I removed all of the old dead code still hanging around that dealt with the
boolOp variable
2) killed the trailing white space at the end of some of the new lines
3) Added white space around several of the operators
4) Removed obsolete code from the patch that is no longer in use or no longer
makes sense
5) removed the more and less entity strings from seamonkey's searchOverlay.dtd
file

Comment 18

[Stefan Borggraefe]

Comment on attachment 182600 [details] [diff] [review]
updated patch with review comments

>Index: mailnews/base/search/resources/content/searchTermOverlay.js
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/base/search/resources/content/searchTermOverlay.js,v
>retrieving revision 1.38
>diff -u -w -r1.38 searchTermOverlay.js
>--- mailnews/base/search/resources/content/searchTermOverlay.js	1 Dec 2004 22:26:51 -0000	1.38
>+++ mailnews/base/search/resources/content/searchTermOverlay.js	4 May 2005 17:51:09 -0000
>@@ -42,15 +42,15 @@
> var gSearchTerms = new Array;
> var gSearchRemovedTerms = new Array;
> var gSearchScope;
>-var gSearchLessButton;
> var gSearchBooleanRadiogroup;
> 
> // cache these so we don't have to hit the string bundle for them
>-var gBooleanOrText;
>-var gBooleanAndText;
> var gBooleanInitialText;

gBooleanInitialText can be removed, too.

>+    if (document.getElementById("addressbookList")) {
>+      var boolRadiogroup = document.getElementById("booleanAndGroup");
>+      var boolValue = boolRadiogroup.getAttribute("value") == "and";
>+      if (boolValue) {
>+        var searchAttr = document.getElementById("searchAttr" + rowNumber);
>+        var menulist = document.getAnonymousNodes(searchAttr)[0];
>+        var popup = menulist.firstChild;
>+        popup.childNodes[2].setAttribute("hidden", true); // hide "Any Number"
>+      }
>+    }

As far as I can see this code-block is never executed. Am I wrong?

>Index: mailnews/base/search/resources/locale/en-US/search.properties
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/base/search/resources/locale/en-US/search.properties,v
>retrieving revision 1.12
>diff -u -w -r1.12 search.properties
>--- mailnews/base/search/resources/locale/en-US/search.properties	8 May 2003 14:51:05 -0000	1.12
>+++ mailnews/base/search/resources/locale/en-US/search.properties	4 May 2005 17:51:09 -0000
>@@ -29,3 +29,5 @@
> andSearchText=and the
> initialSearchText=the

orSearchText, andSearchText and initialSearchText can also now be removed from
search.properties (in mail/ and mailnews/).

>Index: mailnews/base/search/resources/locale/en-US/searchTermOverlay.dtd
>===================================================================
>+<!ENTITY searchTermListMinWidth "min-width: 57em;">
>+a
>\ No newline at end of file

Ooops. ;-)

Comment 19

[Scott MacGregor]

Attachment: updated patch

includes fixes for the latest round of comments.

That one particular code block is still called by address book search and
removing it causes address book search to stop working (at least for me :)).

Comment 20

[[e-mail]]

(In reply to comment #14)
>1) I added a min width (which is set by localizers) to the listbox to prevent
>the widgets from running into each other when you make the dialog smaller than
>the width of the list box. 
Beats me how I missed it before, but there a menulist { min-width: 15px; } style
in searchDialog.css and filterDialog.css - remove them and things are somewhat
nicer; it's more obvious in the filter editor where there's less other cruft and
I was able to shrink it to 375 pixels wide without loss of function.

I can see what the menulist > menupopup > menuitem { padding-right: 2px; } is
trying to do but I can't see why it's not appearing to take effect.

Comment 21

[Stefan Borggraefe]

> That one particular code block is still called by address book search and
> removing it causes address book search to stop working (at least for me :)).

When I replace this code block with

if (document.getElementById("addressbookList"))
{
  alert("called");
}

What do I need to do to see the alert?

Sorry, I'm probably blind. :-(

Comment 22

[Scott MacGregor]

Attachment: updated patch

1) Removes the min-widths per Neil's comment. Although I couldn't see a
difference. Neil were you also suggesting a change to the width of these
dialogs as part of this change?

2) Removed a dead code segment for the addressbooklist if block which as Stefan
pointed out is not called.

Comment 23

[Stefan Borggraefe]

(In reply to comment #22)
> 1) Removes the min-widths per Neil's comment. Although I couldn't see a
> difference.

I also can't see a difference with the latest patch in this bug. But with the
min-widths removed the width of the drop-down boxes changes depending on what
you are selecting. This doesn't look too well IMHO.

Comment 24

[Stefan Borggraefe]

Comment on attachment 182798 [details] [diff] [review]
updated patch

>-menulist {
>-  min-width: 15em;
>-}

I would prefer these not to be removed, but r=me either way. :-)

Comment 25

[[e-mail]]

(In reply to comment #23)
>(In reply to comment #22)
>>1) Removes the min-widths per Neil's comment. Although I couldn't see a
>>difference.
>I also can't see a difference with the latest patch in this bug.
The effect of the min-widths is more noticeable in the filter editor; the search
dialog's large row of buttons that obscures the effect noted in bug 261411.
>But with the min-widths removed the width of the drop-down boxes changes
>depending on what you are selecting. This doesn't look too well IMHO.
Good point. Changing the min-widths to widths fixes that.
I still don't know why the Retro theme doesn't need to set widths though...

Comment 26

[[e-mail]]

Comment on attachment 182798 [details] [diff] [review]
updated patch

>-menulist {
>-  min-width: 15em;
width: 15em; now seems to produce the best result.

>+.small-button {
>+  min-width: 3em;
>+  padding: 0px;
>+}
>+
>+button {
>+  margin: 0px 1px;
>+}
Did you mean to change the margins on all the buttons, not just the +/- ones?

>-        style="width: 52em; height: 36em;"
>+        style="width: 57em; height: 35em;"
I think this new size is too wide. But I also think that the old size is too
tall! I'd actually prefer 52em by 34em if possible.

>+      <listbox flex="1" id="searchTermList" style="&searchTermListMinWidth;">
With the change to menulist { width: 15em; } you shouldn't need this minimum
width any more.

>         <listcols>
>           <listcol flex="&searchTermListAttributesFlexValue;"/>
>           <listcol flex="&searchTermListOperatorsFlexValue;"/>
>           <listcol flex="&searchTermListValueFlexValue;"/>
>+          <listcol flex="&searchTermListButtonsFlexValue;"/>
>+          <listcol/>
I don't see why the buttons need to flex. And you seem to have an extra column
here. Perhaps the old code put each button in its own column?

>         <!-- this is what the listitems will look like:
It would be nice if you could update this comment.

There are still some bugs with this. For example:
1. Create a new Filter
2. Click on '+' twice.
3. Click on '-' in the first row.
4. Change the first Subject to Date.

Comment 27

[Scott MacGregor]

Attachment: updated patch

New patch based on the latest set of feedback:

1) Menulists now have a width of 12em; 
2) dialogs now have a width of 52em and a height of 34em, that with a menu list
width of 12em allowed the dialog to fit without horizontal truncation with
normal sized fonts and large fonts
3) got rid of the button CSS style rule which effected all buttons in the
dialog, moved them into the .small-button class
4) got rid of the listbox fixed width: searchTermListMinWidth

I will spin a up a new bug to track the bug Neil pointed out which I was able
to reproduce.

Comment 28

[[e-mail]]

Comment on attachment 183120 [details] [diff] [review]
updated patch

>+          <listcol flex="&searchTermListButtonsFlexValue;"/>
>+          <listcol/>
>         </listcols>
> 
>         <!-- this is what the listitems will look like:
I still don't understand a) why the fourth column is flexible b) why there's a
fifth column c) whether the explanatory comment will be updated.

>+.small-button {
>+  min-width: 3em;
>+  padding: 0px;
>+  margin: 0px 1px;
>+}
>+
>+listbox {
>+  padding-top: 2px;
>+}
>+
> menulist {
>-  min-width: 15em;
>+  width: 12em;
> }
For consistency it would be nice to add these new styles in the same place in
each file; themes/classic/messenger/filterDialog.css seems to be the odd one
out here.

Comment 29

[Scott MacGregor]

Neil, the extra list col was to try to get aroudn the problem where the
scrolllbars for the listbox are drawn on top of the less button. By adding an
extra list col we get just enough padding for the scrollbar to get drawn over
that empty listcol instead of the buttons. See
https://bugzilla.mozilla.org/show_bug.cgi?id=195224#c8

Looking at CVS history, I believe the other listcols have flexes on them so that
localizers can give an item more horizontal width. 

Comment 30

[[e-mail]]

(In reply to comment #29)
>Neil, the extra list col was to try to get aroudn the problem where the
>scrolllbars for the listbox are drawn on top of the less button.
That looks like a recent regression; in 1.6 the scrollbar simply pushes the
listcells over to the left. If that bug subsequently gets fixed would you mind
if I removed the extra column?

>See https://bugzilla.mozilla.org/show_bug.cgi?id=195224#c8
I clicked on this only to find that this was a circumlocutious way to refer to
comment 8. Bugzilla does not require your assistance to link to comments.

>Looking at CVS history, I believe the other listcols have flexes on them so
>that localizers can give an item more horizontal width. 
Sure, but the +/- buttons aren't localized.

Comment 31

[[e-mail]]

(In reply to comment #29)
>Neil, the extra list col was to try to get aroudn the problem where the
>scrolllbars for the listbox are drawn on top of the less button.
Ah, I see what's going on there: without the patch the rightmost column is the
textbox which flexes so doesn't mind whether there's a scrollbar or not, but
with the patch the rightmost column doesn't flex sufficiently. As an alternative
to the extra column you could use listboxbody { overflow-y: scroll; } which has
the advantage of working whatever size the scrollbar happens to be.

Comment 32

[Scott MacGregor]

(In reply to comment #31)
> Ah, I see what's going on there: without the patch the rightmost column is the
> textbox which flexes so doesn't mind whether there's a scrollbar or not, but
> with the patch the rightmost column doesn't flex sufficiently. As an alternative
> to the extra column you could use listboxbody { overflow-y: scroll; } which has
> the advantage of working whatever size the scrollbar happens to be.

I just tried this and now I get a disabled set of scrollbars when scrollbars are
not necessary. 

Comment 33

[[e-mail]]

I wasn't expecting you to prefer it but thought you should at least be aware.

Comment 34

[Asa Dotzler [:asa]]

Comment on attachment 183120 [details] [diff] [review]
updated patch

a=asa

Comment 35

[Scott MacGregor]

I filed Bug #2940979 to track the bug Neil pointed out here:  
https://bugzilla.mozilla.org/show_bug.cgi?id=195224#c26

hopefully we can remove the listcol as well once the scroll bar listbox bug is
correctly fixed.

Comment 36

[[e-mail]]

I assume you meant to say you filed bug 294079 to address comment 26.

Comment 37

[Scott MacGregor]

isn't that what I said?

Comment 38

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #37)
> isn't that what I said?

Not really, since Bug #2940979 does not exist.

Comment 39

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED in Seamonkey trunk build 2005-05-26-05 on Windows XP.  Looks great!