40480 - Search/Filter UI: Problems adding criteria lines past initial display area

Status: VERIFIED FIXED

(MailNews Core :: Filters, defect, P1)

Description

[laurel]

Using may24 m16 commercial build

If you add more criteria lines than fit in the visual space of the filter rules
"conditions" list there are a couple problems:

1.   The last criteria line in the visual list area will overrun the bottom
boundary of the list display area.
2.   If you use the scrollbar/slider to get to the end of the criteria line
list, you'll see the criteria widgets lose their labels and dropdown
functionality, rendering those criteria lines useless.


Steps for testing:
1.   From mail window, launch message filters (Edit|Filters) then click New
button to launch the filter rules diaog.
2.   In the filter rules dialog, click More to show criteria line then type in a
value for the text field -- subject contains yourtextstring
3.   Click More and fill in text field again and keep repeating until you add
more criteria lines than will fit in the initial display space of the list.
     Result #1:  notice the last criteria line shown overruns the bottom
boundary of the list area.
4.   Use the scrollbar/slider to go to the next/bottom criteria line which is
not shown. 
     Result #2:  notice the noun/1st and verb/2nd criteria dropdown widgets have
no labels (like "subject" and "contains").
5.   Try to use the dropdown in the blank criteria fields.
     Result #3:  the dropdowns don't work, you can't set criteria.

Note:  another more drastic example of loss of criteria line functionality is to
go to a new/empty filter rules dialog then add lots of default criteria lines
(to cause scrollbar in the list) without ever filling in the text fields.  When
you scroll up and down repeatedly it will eventually render the majority of
criteria widgets usesless.

Comment 1

[Alec Flett]

adding hyatt to CC
Dave - I need to know when an XBL widget has been bound to the document through 
the style system, so that it can initialize itself... I remember talk of an 
onbound handler or something...

Comment 2

[scottputterman]

mass moving to M18 and adding nsbeta3 keyword

Comment 3

[selmer (gone)]

Adding mail4 triage nomination keyword.

Comment 4

[lchiang]

+ per mail triage

Comment 5

[robinf]

Adding myself to cc: list.

Comment 6

[Alec Flett]

I think this is gone. however, search/filters are really screwed up because of
XBL right now so it's kinda hard to tell. 

Comment 7

[Alec Flett]

nope, not working.
the problem is that the XBL binding isn't happening on off-screen rows when
they're initialized. I need to talk to hyatt about how to fix this.

Comment 8

[selmer (gone)]

Can you continue to enter criteria if you just avoid scrolling?  In other words, 
does the more button continue to work as long as you don't scroll?

Comment 9

[Alec Flett]

nope :(
any rows that are created outside of the currently visible area do not get 
initialized. The reason is that the XBL isn't resolved until the row is scrolled 
into view the first time.. but I've already tried to initialize it.

I'm wondering if the solution might be to go back to the old 4.x behavior of 
having the window grow, instead of having the region scroll

hyatt, do you have any suggestions for this? The problem is that I need to 
initialize the XBL widgets on creation, not at scroll-into-view time :(

Comment 10

[lchiang]

P2 per mail triage

Comment 11

[Jim Roskind]

PDT felt this was obscure enough to push it to P3

Comment 12

[selmer (gone)]

Can we take the cheap way out for now and just make the area bigger and not
scrollable?  Seems like the workaround is to create an extra filter for the
overflow of conditions.  Do we have a limit on the number of filters?  If not, I
think this proposal is good enough for now.

Comment 13

[Peter Trudelle]

Sounds like a very practical solution.  If memory serves, we had a hard limit of
5 filters in 4.x, for similar reasons.

Comment 14

[lchiang]

It's funny.  Laurel tells me one of the topmost requests from 4.x was not to 
have a limit on number of filters.  Looks like we now have to.

Comment 15

[Peter Trudelle]

Yeah, but we only heard from a tiny percentage of users; anyone who was really
serious about filters was using another product. If we do have a limitation in
6.0, we could lift it in 6.0.1...

Comment 16

[Alec Flett]

yes, I think that setting a maximium is a decent solution for now.

I'll do that, but leave this bug open because I can fix it.

for my own reference here's what I need to do to fix it:
Change the initialization of new rows to simply initialize the data in the DOM 
attributes rather than through XBL. Then in the onbinding event, suck in all 
the data from the DOM attributes... that way even if these things are created 
off-screen, the widgets will be initialized AFTER the frames have been created.

Comment 17

[David Hyatt]

Wait, this sounds like a bad XBL problem.  I would want to fix this if so.

Is the problem that you're trying to refer to offscreen anonymous content? Let 
me know if that's what you're doing.  If so, please bounce this to me, because 
it's trivial to fix.

Comment 18

[David Hyatt]

This really should work.  Alec, have you made sure to do the pre-load using 
loadBindingDocument?

Comment 19

[Alec Flett]

yep.. but the problem seems to be in the tree when you create rows that are not 
scrolled into view. I'm reassinging to you so you can poke at it - just 
Edit->Message Filters then edit/create a filter with more than 4-5 rows..

Comment 20

[Peter Trudelle]

Clearing nsbeta3+ for triage by new owners.

Comment 21

[Peter Trudelle]

nsbeta3+, P3 for M18

Comment 22

[Peter Trudelle]

nsbeta3-/future.  This no longer fits the profile for bugs we can commit to for 
nsbeta3.  perhaps this can be worked around in the app by limiting the number 
of rules to the available space?

Comment 23

[David Hyatt]

Hey Alec, check out the trunk and see if I fixed this bug.   XBL got a big 
overhaul today.

Comment 24

[laurel]

*** Bug 57008 has been marked as a duplicate of this bug. ***

Comment 25

[scottputterman]

Sorry for the extra mail. Removing mail4 keyword.

Comment 26

[laurel]

nominating for 6.5.

Comment 27

[laurel]

*** Bug 59702 has been marked as a duplicate of this bug. ***

Comment 28

[Peter Trudelle]

->moz0.9

Comment 29

[laurel]

*** Bug 65430 has been marked as a duplicate of this bug. ***

Comment 30

[laurel]

Nominated for next release. I'd really like to see this fixed -- we get quite a
few reports/complains on this.

Comment 31

[Dan Rosen]

This has changed a bit, at least for me. To reproduce the bug, I have to do the
following:

1. Open filters dialog, and open an existing or new filter.
2. Add criteria until one or more criterion is *completely* off-screen (not even
partially in the scrolled area). For me this is the fifth criterion...
3. Close the dialog (click OK or hit Enter).

(Note: the last criteria line no longer goes past the frame boundary, it is now
properly clipped).

4. Reopen the filter, and scroll down to the criteria that were completely
off-screen.

RESULT: those criteria are blank (dropdowns have no options, textfields have no
text).

Comment 32

[Dan Rosen]

*** Bug 70653 has been marked as a duplicate of this bug. ***

Comment 33

[David Hyatt]

The exact problem here is that elements that are created using createElement do
not have their mDocument member pointers set until they are actually inserted
into the document.  Note that for elements created using cloneNode we do set the
mDocument member pointer.  We are being inconsistent here.

The fix IMO for this bug is to ensure that mDocument is set immediately upon the
creation of the element.  This will fix the bug.

Reassigning to waterson, who I believe has plans to tackle this now that the XUL
content model has moved into the content DLL.

Comment 34

[Peter Trudelle]

*** Bug 62420 has been marked as a duplicate of this bug. ***

Comment 35

[Chris Waterson]

Yep. This is a mess.

*** This bug has been marked as a duplicate of 52726 ***

Comment 36

[laurel]

*** Bug 72394 has been marked as a duplicate of this bug. ***

Comment 37

[Chris Waterson]

Un dupifying.

Comment 38

[Chris Waterson]

jst: read hyatt's comments from 2001-03-02 14:09.

Comment 39

[Johnny Stenback (:jst)]

So what exactly are the problems that are caused by mDocument being null in
document.createElement() created elements? Is this the same XBL bindings don't
get attached to elements that are not in the document yet? If so, I think we
need to sit down and discuss this. IMO XBL bindings should only be attached to
elements that are in the document, so created or cloned elements should not have
XBL bindings attached to them unless we rewrite the rules...

Comment 40

[Dan Rosen]

Nominating for all sorts of stuff. Sorry for the noise, but if you want to use
mozilla as a serious mail program, you *need* filters... Clearing status
whiteboard from NS6.0 RTM.

Comment 41

[Dan Rosen]

More fun keywords. I was just looking through my "rules.dat" and found out that
this causes dataloss. If you try to edit a mail filter, the blank values are
written back out to the data file and you lose your filter rules.

Comment 42

[David Hyatt]

--> me

Comment 43

[David Hyatt]

I don't think the cage match between me and jst ;) is something that should be 
resolved before RTM.  This particular bug in mail/news has always been 
something that could be worked around in the JS/XUL/XBL code pretty easily.  
I'm going to reassign this to putterman.  If alecf is willing to volunteer some 
time, he and I together could probably explain what's necessary to work around 
this.

Comment 44

[scottputterman]

reassigning to sspitzer.

Comment 45

[laurel]

*** Bug 86376 has been marked as a duplicate of this bug. ***

Comment 46

[scottputterman]

moving to 0.9.3 per PDT

Comment 47

[laurel]

*** Bug 87012 has been marked as a duplicate of this bug. ***

Comment 48

[Jonathan S Abrams]

8 duplicates.  Mostfreq?

Comment 49

[Dan Rosen]

Mostfreq. If not now, then tomorrow... I hope PDT realizes exactly how unusable
this simple problem renders MailNews as a viable mail client...

Comment 50

[laurel]

There are a lot more than 8 duplicates, because for a long while this bug was
marked a duplicate of 52726 and a lot of dupl's were tracked to that report!

Comment 51

[scottputterman]

I wish there were more keywords left for me to add.  Seriously, we know about
it, we want to fix it and it's on the list of bugs to look into for a 0.9.3 and
a limbo build for ns.  Oh wait, I guess that means there is a keyword left for
me to add. Adding nsbranch.

Comment 52

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

accepting.  investigating...

I remember a similar bug for the compose widget.  I'll see what I can find out.

Comment 53

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

just got a brain dump from alecf, investigating a fix.

Comment 54

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

summary of alecf's brain dump:

"the problem is basically this the tree only creates frames for rows that are 
visible frames are created with CSS.  CSS is how XBL gets bound to UI.  which 
basically means that the XBL widgets that I made don't get created until they 
are scrolled into view"

alecf suggested I add bindingattached/bindingdetach handlers to the XBL 
widgets, but I was unable to get anywhere with that.  (probably pilot error on 
my part, but I'll check with hyatt.)

my work around involves scrolling before appending new rows.  

When editing an existing filter, we scroll to the bottom, add the first row, 
scroll to the bottom, add the next row, scroll, etc.  (when I'm all done, I 
scroll back to the top.)

I also made it so when the user hits "More", we scroll down to the new row.  
(Since they'll probably want to edit it.)

alecf, can you review?

Comment 55

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

Attachment: work around fix.

Comment 56

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

adding alecf back.

Comment 57

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

I think I can eliminate the call to getRowCount() [and just use 
gTotalSearchTerms].

new patch coming soon.

Comment 58

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

Attachment: patch, v2. remove call to rowCount(), fix 2 js warnings.

Comment 59

[Alec Flett]

sr=alecf, for the branch - I think we need to figure out why bindingattached
isn't firing before we check this into the trunk...(to see if we can fix it the
"real" way)

Comment 60

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

todo, before this can land on the trunk (from alecf)

"find out why the bindingattached events aren't firing - if it turns out to be 
something really bad, then we check in the workaround.  if it turns out to be 
something trivial, then we fix it the right way."

alecf also writes:  "xbl widgets won't ever get bound when they're off screen.  
that's just how it works".

I'll look into that before I land on the trunk.

Comment 61

[Dan Rosen]

> alecf also writes:  "xbl widgets won't ever get bound when they're off screen.
> that's just how it works".

If I understood hyatt correctly, that's not *exactly* the case. Items in an XBL
<tree> widget which are offscreen are { display: none }, or something like that,
because otherwise <tree> would be *really* slow all the time. It's a slightly
more circuitous reason (even if I don't have it exactly right) but I guess it
amounts to the same thing. Hope this helps :-/

Comment 62

[Alec Flett]

something like that, yes - it's not that they have display:none, but rather that
the tree does not send the non-visible content rows through CSS, which means
that frames are never created, and also means that xbl is not bound.

Comment 63

[David Hyatt]

Remember that bindingattached/detached were changed to <constructor> and
<destructor> a few months ago, and they are placed in the <implementation> now.
 Make sure you're using the current syntax.

Comment 64

[Koike Kazuhiko]

*** Bug 88488 has been marked as a duplicate of this bug. ***

Comment 65

[Håkan Waara]

Seth, you didn't drop your work on this one did you?  It's a critical bug.

Comment 66

[Jonathan S Abrams]

updating 0.9.2-->0.9.3

Also, is it necessary to have nsCatFood and nsdogfood?

Comment 67

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

> Seth, you didn't drop your work on this one did you?  It's a critical bug.

No, I haven't forgotten about this.  I'm fixing my nsBranch bugs right now 
(which will become mozilla 0.9.2.1).

when I get some cycles, I'll investigate a fix this for the trunk.

Comment 68

[Dan Rosen]

Seth: has the hack landed on the branch yet? This *is* one of your nsBranch bugs
:) r=dr on the 6/23 patch if you need it...

Comment 69

[Scott MacGregor]

This fix can't land on the branch yet because PDT has not started taking limbo
bugs yet. Hence the nsbranch keyword. This is a limbo bug. =)

Comment 70

[laurel]

*** Bug 89125 has been marked as a duplicate of this bug. ***

Comment 71

[Håkan Waara]

*** Bug 89361 has been marked as a duplicate of this bug. ***

Comment 72

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

http://bugzilla.mozilla.org/showattachment.cgi?attach_id=39815 is still waiting 
for approval for nsBranch.

I'm starting on a fix for the trunk.  so far, so good.  I've added constructors 
and destructors to the search op, term and values (like hyatt and alecf 
suggested), now on to the save and restore part.

Comment 73

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

Attachment: draft of a fix suitable for the trunk

Comment 74

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

ok, that was a first draft of a fix for the trunk.

the way it works is I save all the scopes and terms off in arrays.

when the constructor of the searchattribute widget is called, I initialize the 
scope and term of the searchTermObj from the arrays.

it works fine for search, but for filters there are some bugs I need to work 
out, like:

1)  edit a filter with > 3 rows.  on load, hit fewer without scrolling down.  
then hit ok.

** Error saving element 3: TypeError: this.searchvalue.saveTo is not a function

I think there might be some code already to handle this.

2)  priority pickers show up with the right value, but they look blank.  (I've 
seen this before...)

I'm also worried about some of the edge cases, I still need to test more.

Comment 75

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

> 2)  priority pickers show up with the right value, but they look blank.  (I've 
seen this before...)

I think this is a known bug with the priority picker.  it happens in builds
without my patch.

Comment 76

[Håkan Waara]

Regarding #2:

Do you mean that their default value is not set?

That's a bug on my list.

Comment 77

[Håkan Waara]

I applied the patch and played around with the FilterEditor and Search window
for a while. Everything worked fine, and I couldn't even reproduce the problem
with Fewer button you saw, Seth. Maybe it's some other hack on your side which
causes it.

Nice work!

Comment 78

[laurel]

*** Bug 89322 has been marked as a duplicate of this bug. ***

Comment 79

[scottputterman]

adding PDT+

Comment 80

[selmer (gone)]

Seth, once reviews are done, please check in on the branch.

Comment 81

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

the branch only fix
(http://bugzilla.mozilla.org/showattachment.cgi?attach_id=39815) has landed on
the branch.

I'll continue to work on the trunk version of the fix, as this needs to be fixed
on the trunk ASAP.

Comment 82

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

> 1)  edit a filter with > 3 rows.  on load, hit fewer without scrolling down.  
> then hit ok.
> ** Error saving element 3: TypeError: this.searchvalue.saveTo is not a 
function

I can reproduce this with a filter with 6 terms, and I hit "less" twice.  
(changes are not saved and I get the js error.

> 2)  priority pickers show up with the right value, but they look blank.  

this is bug #78429, I've got a simple work around to address the problem.

Comment 83

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

Attachment: new version, includes fix for #78429 and scrolls to botton on "More"

Comment 84

[scottputterman]

removing pdt+ to get off radar.  I'm not sure what keyword to use, but Laurel,
can you test this on the branch build when tomorrow's build comes out?

Comment 85

[laurel]

Yes, I've been following the comments. Will test in tomorrow's branch.

Comment 86

[Dan Rosen]

The fix on the branch doesn't seem to work for me, in a fresh CVS pull from this
morning just before the tree opened... Are you sure your workaround worked, or
am I on crack?

Comment 87

[Dan Rosen]

Attachment: dr's rules.dat file

Comment 88

[Dan Rosen]

Attached my rules.dat file, which I've been having to edit by hand... Steps to
reproduce, with my filters: open up the "Spam" filter and see the blank criteria
lines...

Comment 89

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

dr when did you pull and build?  I checked this in on the branch at 2pm.

I'll go try your rules.dat on my branch build.

Comment 90

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

dr's rules.dat loads fine for me on my nsBranch build.

dr:  try this morning's build.  I think you tried before I checked in.

Comment 91

[laurel]

Update on branch tests: So far, so good on july10 branch build.
OK with july10 win98 commercial branch build:
Tried filters and search UI, able to add and use the criteria lines after
initial (3 lines) display. 
After lines are added, criteria section scrolls to the bottom of the list (a
little jumpiness in the display, but OK).
Can edit lines, scroll to various lines, dropdowns usable.
Search: able to clear long criteria list properly.
For filters, all added lines properly written to rules.dat file -- I tried with
10 lines.  Able to edit existing filters with 10 or so criteria lines, properly
saved to rules.dat.

More testing, onto Mac and linux

Comment 92

[Dan Rosen]

Oops, you're right, I don't think I picked up your fix. I'll check it out today.

Comment 93

[laurel]

OK with jul710 commercial branch builds on Mac OS 9.0 and linux rh6.2.
Noticed some minor issues which I'm discussing with Seth and will log separately
if appropriate. 
Considering the branch fix verified.

Comment 94

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

new patch coming very soon that address these two problems:

1)  edit a filter with 6 terms.  on load, hit fewer once, then hit ok.  the 
removal needs to work.

2)  open a new filter with several (say 6) terms.  hit ok.  you should not get 
any js errors to the console about "this.searchvalue.save is not a function"

Comment 95

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

Attachment: new patch.

Comment 96

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

I'll now work on fixing the code to use one array (instead of several parallel 
arrays).

bienvenu, can you review the fix?

Comment 97

[David :Bienvenu]

r/sr=bienvenu

Comment 98

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

Attachment: new fix, single array.

Comment 99

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

fix checked into the trunk.

Comment 100

[Christopher Aillon (sabbatical, not receiving bugmail)]

*** Bug 91878 has been marked as a duplicate of this bug. ***

Comment 101

[laurel]

All looks OK to me with aug28 commercial trunk builds: mac OS 9.0, win98, linux
rh6.2