Last Comment Bug 743974 - make layout of elements in the filter editor use the space available in the dialog more efficiently and consistently
: make layout of elements in the filter editor use the space available in the d...
Status: RESOLVED FIXED
: polish, uiwanted
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: Thunderbird 17.0
Assigned To: :aceman
:
Mentors:
Depends on: 205039
Blocks: 698434 777287
  Show dependency treegraph
 
Reported: 2012-04-10 05:59 PDT by :aceman
Modified: 2012-07-25 03:26 PDT (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
screenshot of existing problems (9.82 KB, image/png)
2012-04-10 05:59 PDT, :aceman
no flags Details
WIP patch (12.46 KB, patch)
2012-06-03 05:39 PDT, :aceman
bwinton: feedback-
Details | Diff | Review
WIP patch v2 (16.17 KB, patch)
2012-06-10 12:33 PDT, :aceman
neil: feedback-
Details | Diff | Review
patch v3 (16.09 KB, patch)
2012-06-24 08:37 PDT, :aceman
no flags Details | Diff | Review
patch v4 (12.67 KB, patch)
2012-06-25 13:23 PDT, :aceman
neil: review+
mconley: review+
bwinton: ui‑review+
Details | Diff | Review

Description :aceman 2012-04-10 05:59:05 PDT
Created attachment 613566 [details]
screenshot of existing problems

The attachment shows some identified problems in the filter editor dialog.

1. the space to the right of the add/remove buttons is different between the Rules and the Actions lists.
I'd try to make it the same and even make it smaller than it is now in the Actions list.

2. even if there is plenty of space, the fields in the Actions list do not expand (flex).  I'd try to add same flex values to them. Expanding e.g. the folder picker could help bug 698434.

3. The fields in the Rules list do have flex values. But they all expand uniformly. I'd think the header and the operator fields do not gain much from expanding. The value field should expand much more quickly as it is more important and could contain long values.

I try to play with the flex values in http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/searchTermOverlay.dtd and add some new ones for the Actions fields.

What do you think? Or is the current state intentional?
Comment 1 Blake Winton (:bwinton) (:☕️) 2012-04-10 07:24:04 PDT
I don't think the current state is particularly intentional, and the changes you suggest make sense to me.

Thanks,
Blake.
Comment 2 :aceman 2012-04-18 11:26:15 PDT
Thanks I'll look into it. There is also a spacer right of the filter name, that is just taking space that could be used for the filter name. Maybe it just looks better?
Comment 3 :aceman 2012-06-03 05:39:14 PDT
Created attachment 629596 [details] [diff] [review]
WIP patch

This is some progress. But I can't manage to make the elements flex properly.
1. The intention was to have the SearchTermAttribute flex=1, the Operator flex=1 and Value flex=3. 5 in total, then the action columns like this:
ActionType flex=1, ActionValue flex=4, 5 in total. But the SearchTermAttribute is not the same width as ActionType (yes, I considered the mix-width in the theme) when the window is wide.
2. the ActionValue element (e.g. target folder of Copy/move or Forward address) does not span the whole column as I would like it to.

Any ideas how to fix these?
Comment 4 Ian Neal 2012-06-05 14:14:32 PDT
Comment on attachment 629596 [details] [diff] [review]
WIP patch

I'm no expert with the dark art of flex, but Neil is.
Comment 5 neil@parkwaycc.co.uk 2012-06-06 16:42:56 PDT
Comment on attachment 613566 [details]
screenshot of existing problems

1. The filler adjustment has the problem that it only works in the filter editor. Better would be simply to remove the flex from the button column. If you wanted to, you could change the filter actions to have a dummy column too, and remove the filler class entirely.
2. This simply has a minimum width defined in CSS, there's no way to automatically size it although we could arrange to have a percentage of the width of the list (I don't think the full width would look right but that's bwinton's decision).
3.Not sure what the problem is here.
Comment 6 neil@parkwaycc.co.uk 2012-06-06 16:50:09 PDT
(In reply to aceman)
> 3. The fields in the Rules list do have flex values. But they all expand
> uniformly. I'd think the header and the operator fields do not gain much
> from expanding. The value field should expand much more quickly as it is
> more important and could contain long values.
The problem here is that the possible values of the search operator changes depending on the search term. This means that if we made the search operator inflexible then its width would change, which could negatively impact layout. In fact it's probably possible to get the width of the search term list to change too, but I guess that's somewhat less likely. I guess we could work around that by specifying a minimum width in the locale. By comparison the filter action column is inflexible because its list doesn't have to change.
Comment 7 :aceman 2012-06-06 23:02:12 PDT
Thanks, but could you comment on the patch, comment 3?

About the filler, in the patch I have made buttons in both lists (rules and actions) to have the filler, so they have the same padding. Yes, it is there only in filter editor and search dialog does not have the padding (in both lists). But it is uniform inside each dialog (which is the point of this bug). If the same padding is wanted in search dialog, I can add it. Flex on buttons' column is removed in the patch.
Comment 8 Blake Winton (:bwinton) (:☕️) 2012-06-07 07:44:57 PDT
Comment on attachment 629596 [details] [diff] [review]
WIP patch

I'm not sure how to fix the issues you're reporting, but I'll add another one.  ;)  When I expand the box, and then collapse it, the fields don't shrink, leading to crazy cut-off fields.  So, given that, and the problems you're hitting, I guess I'm going to say f-, but I do think that, at least visual-wise, you're heading in the right direction…

Thanks,
Blake.
Comment 9 :aceman 2012-06-07 12:33:35 PDT
If you click on a dropdown in the list the fields will shrink as needed.
But that problem is there regardless of my patch.
Is there a way to force reflow of the whole dialog on resize?
Comment 10 neil@parkwaycc.co.uk 2012-06-10 10:13:42 PDT
(In reply to :aceman from comment #7)
> About the filler, in the patch I have made buttons in both lists (rules and
> actions) to have the filler, so they have the same padding. Yes, it is there
> only in filter editor and search dialog does not have the padding (in both
> lists). But it is uniform inside each dialog (which is the point of this
> bug). If the same padding is wanted in search dialog, I can add it. Flex on
> buttons' column is removed in the patch.
Some padding is needed in the search dialog; because you removed the dummy column, the search dialog has no padding at all so if you have many search criteria then the scrollbar overlaps the buttons.
Comment 11 neil@parkwaycc.co.uk 2012-06-10 10:15:51 PDT
Comment on attachment 629596 [details] [diff] [review]
WIP patch

It turns out that I can't comment on the patch because you didn't update the suite locales so I can't actually open the filter editor.
Comment 12 :aceman 2012-06-10 11:05:37 PDT
Sorry about that, I'll fix the last 2 comments.
Yes I removed the dummy column. So I'll replace it with a properly spaced filler in both dialogs.
Comment 13 :aceman 2012-06-10 12:33:34 PDT
Created attachment 631760 [details] [diff] [review]
WIP patch v2

OK, try this in SM.
Comment 14 neil@parkwaycc.co.uk 2012-06-10 13:46:12 PDT
Comment on attachment 631760 [details] [diff] [review]
WIP patch v2

This doesn't work very well because the action popup changes in size depending on which action you select. This is because flex applies to the spare space, which varies depending on whether the action has a target. You can work around this by adding a width to the action listcol so that its allocation of space doesn't depend on its content.

Normally if a listcell is too narrow then its contents simply overflow, so I added orient="vertical" align="start" pack="center" which enforces a maximum width of the contents to be that of the cell. Unfortunately for you this also disables your attempts to make the contents flex to the width of the cell.

Also flex="1" on a listcell makes no sense.
Comment 15 :aceman 2012-06-24 08:37:32 PDT
Created attachment 636167 [details] [diff] [review]
patch v3

Changing orient to horizontal seems to expand the action value as I want.
Short of aligning the rules columns and the action columns (both filter attribute and filter action columns have flex=1 and they still do not align when when the other columns have a total flex of 4), the patch does what I wanted.
Comment 16 neil@parkwaycc.co.uk 2012-06-24 16:25:31 PDT
Comment on attachment 636167 [details] [diff] [review]
patch v3

When I tried to apply this patch I got more rejects than successes :-(

>-                    orient="vertical" align="start" pack="center"/>
>+                    orient="horizontal" align="start" pack="center"/>
Switching the orient also switches the meaning of the align and pack. You had two options here:
1. Because orient="horizontal" pack="start" align="center" is the default for a listcell, you could just remove the orient="vertical" align="start" pack="center" completely
2. You could just change to align="stretch", which would avoid having to add flex="1" everywhere to the children.

>+        <xul:hbox align="start" flex="1">
Why?

>+        <xul:menupopup flex="1">
Is this to fix bwinton's issue? I can't see what other effect it's supposed to have.
Comment 17 :aceman 2012-06-24 23:25:22 PDT
I added flex all the way from listcol (thus listcell) to the lowers element so that the contents stretch. It may be that some of them are superfluous. Maybe that is what you say in point 2. Into which element should I add align="stretch"?
Comment 18 :aceman 2012-06-25 13:22:06 PDT
(In reply to neil@parkwaycc.co.uk from comment #16)
> >+        <xul:hbox align="start" flex="1">
> Why?
Copied from the rules list so that the buttons have the same enclosing box and so same style applied. But I'll remove the flex.

> >+        <xul:menupopup flex="1">
> Is this to fix bwinton's issue? I can't see what other effect it's supposed
> to have.
No, I can't fix bwinton's issue. Any idea why that happens? I'll remove the flex.
Comment 19 :aceman 2012-06-25 13:23:19 PDT
Created attachment 636469 [details] [diff] [review]
patch v4

Thanks, I used the stretch and could remove many of the flex="1".
Comment 20 neil@parkwaycc.co.uk 2012-07-01 09:31:48 PDT
(In reply to neil@parkwaycc.co.uk from comment #16)
> When I tried to apply this patch I got more rejects than successes :-(
Oops, it does help to unapply the previous patch...
Comment 21 neil@parkwaycc.co.uk 2012-07-01 09:52:24 PDT
(In reply to aceman from comment #18)
> (In reply to neil@parkwaycc.co.uk from comment #16)
> > >+        <xul:hbox align="start" flex="1">
> > Why?
> Copied from the rules list so that the buttons have the same enclosing box
> and so same style applied. But I'll remove the flex.
I don't see a style that depends on the enclosing box. (The only reason the rules list uses an enclosing box is that the code that constructs the list can only handle a single direct child of each listcell.)

> > >+        <xul:menupopup flex="1">
> > Is this to fix bwinton's issue? I can't see what other effect it's supposed
> > to have.
> No, I can't fix bwinton's issue. Any idea why that happens?
No, I thought it was to do with menupopup sizing but that doesn't appear to be the case.
Comment 22 :aceman 2012-07-01 10:49:56 PDT
(In reply to neil@parkwaycc.co.uk from comment #21)
> (In reply to aceman from comment #18)
> > (In reply to neil@parkwaycc.co.uk from comment #16)
> > > >+        <xul:hbox align="start" flex="1">
> > > Why?
> > Copied from the rules list so that the buttons have the same enclosing box
> > and so same style applied. But I'll remove the flex.
> I don't see a style that depends on the enclosing box. (The only reason the
> rules list uses an enclosing box is that the code that constructs the list
> can only handle a single direct child of each listcell.)
Maybe no style today (except for differing flex), but one day something may be applied to them. The point of this bug is to cleanup sizes and spacing and make them consistent in those 2 lists. So is it a problem adding that hbox? There is no visible difference for now. Maybe performance?
Comment 23 neil@parkwaycc.co.uk 2012-07-01 12:58:59 PDT
(In reply to aceman from comment #22)
> The point of this bug is to cleanup sizes and spacing
> and make them consistent in those 2 lists.
Ah yes, I see you have the word "consistently" in the description.
[You would lose out slightly on blame though.]

(In reply to neil@parkwaycc.co.uk from comment #14)
> This doesn't work very well because the action popup changes in size
> depending on which action you select.
Any advance on this?
Comment 24 :aceman 2012-07-01 13:07:35 PDT
(In reply to neil@parkwaycc.co.uk from comment #23)
> (In reply to neil@parkwaycc.co.uk from comment #14)
> > This doesn't work very well because the action popup changes in size
> > depending on which action you select.
> Any advance on this?
No, it still happens and I don't know why. It is caused by the patch (doesn't happen before). But is it a problem? Too ugly?
Comment 25 neil@parkwaycc.co.uk 2012-07-02 13:52:18 PDT
(In reply to aceman from comment #24)
> (In reply to  #23)
> > (In reply to comment #14)
> > > This doesn't work very well because the action popup changes in size
> > > depending on which action you select.
> > Any advance on this?
> No, it still happens and I don't know why. It is caused by the patch
> (doesn't happen before). But is it a problem? Too ugly?
Well, I think it is. I'd ask bwinton but he's away :-(
Comment 26 Blake Winton (:bwinton) (:☕️) 2012-07-12 07:27:04 PDT
Comment on attachment 636469 [details] [diff] [review]
patch v4

No, it's not too ugly.  ui-r=me.  :)

Thanks,
Blake.
Comment 27 neil@parkwaycc.co.uk 2012-07-12 12:16:46 PDT
Comment on attachment 636469 [details] [diff] [review]
patch v4

Well, I guess we'll see if anyone else complains ;-)
Comment 28 Mike Conley (:mconley) - (needinfo me!) 2012-07-18 11:08:59 PDT
Comment on attachment 636469 [details] [diff] [review]
patch v4

Review of attachment 636469 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!  Thanks aceman!
Comment 29 Ryan VanderMeulen [:RyanVM] 2012-07-18 17:26:22 PDT
https://hg.mozilla.org/comm-central/rev/e4522bcdf0b3

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