743974 - make layout of elements in the filter editor use the space available in the dialog more efficiently and consistently

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[:aceman]

Attachment: 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) (:☕️)]

I don't think the current state is particularly intentional, and the changes you suggest make sense to me.

Thanks,
Blake.

Comment 2

[:aceman]

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]

Attachment: 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]

Comment on attachment 629596 [details] [diff] [review]
WIP patch

I'm no expert with the dark art of flex, but Neil is.

Comment 5

[[e-mail]]

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

[[e-mail]]

(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]

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) (:☕️)]

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]

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

[[e-mail]]

(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

[[e-mail]]

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]

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]

Attachment: WIP patch v2

OK, try this in SM.

Comment 14

[[e-mail]]

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]

Attachment: 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

[[e-mail]]

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]

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]

(In reply to [e-mail] 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]

Attachment: patch v4

Thanks, I used the stretch and could remove many of the flex="1".

Comment 20

[[e-mail]]

(In reply to [e-mail] 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

[[e-mail]]

(In reply to aceman from comment #18)
> (In reply to [e-mail] 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]

(In reply to [e-mail] from comment #21)
> (In reply to aceman from comment #18)
> > (In reply to [e-mail] 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

[[e-mail]]

(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 [e-mail] 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]

(In reply to [e-mail] from comment #23)
> (In reply to [e-mail] 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

[[e-mail]]

(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) (:☕️)]

Comment on attachment 636469 [details] [diff] [review]
patch v4

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

Thanks,
Blake.

Comment 27

[[e-mail]]

Comment on attachment 636469 [details] [diff] [review]
patch v4

Well, I guess we'll see if anyone else complains ;-)

Comment 28

[Mike Conley (:mconley) (:⚙️)]

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]]

https://hg.mozilla.org/comm-central/rev/e4522bcdf0b3