Closed Bug 40480 Opened 25 years ago Closed 23 years ago

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

Categories

(MailNews Core :: Filters, defect, P1)

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.3

People

(Reporter: laurel, Assigned: sspitzer)

References

Details

(Keywords: dataloss, Whiteboard: [nsbeta1+]branch only fix landed on the branch - verified july10 branch with Mac, Win, Linux)

Attachments

(7 files)

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.
QA Contact: lchiang → laurel
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...
Status: NEW → ASSIGNED
Target Milestone: --- → M17
mass moving to M18 and adding nsbeta3 keyword
Keywords: nsbeta3
Target Milestone: M17 → M18
Adding mail4 triage nomination keyword.
Keywords: mail4
+ per mail triage
Whiteboard: [nsbeta3+]
Adding myself to cc: list.
I think this is gone. however, search/filters are really screwed up because of
XBL right now so it's kinda hard to tell. 
Whiteboard: [nsbeta3+] → [nsbeta3+] Might be working
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.
Whiteboard: [nsbeta3+] Might be working → [nsbeta3+]
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?
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 :(
P2 per mail triage
Priority: P3 → P2
PDT felt this was obscure enough to push it to P3
Priority: P2 → P3
Whiteboard: [nsbeta3+] → [nsbeta3+][PDTP3]
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.
Sounds like a very practical solution.  If memory serves, we had a hard limit of
5 filters in 4.x, for similar reasons.
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.
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...
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.
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.
This really should work.  Alec, have you made sure to do the pre-load using 
loadBindingDocument?

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..
Assignee: alecf → hyatt
Status: ASSIGNED → NEW
Clearing nsbeta3+ for triage by new owners.
Whiteboard: [nsbeta3+][PDTP3] → [PDTP3]
nsbeta3+, P3 for M18
Status: NEW → ASSIGNED
Whiteboard: [PDTP3] → [PDTP3] [nsbeta3+]
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?
Whiteboard: [PDTP3] [nsbeta3+] → [PDTP3] [nsbeta3-]
Target Milestone: M18 → Future
Hey Alec, check out the trunk and see if I fixed this bug.   XBL got a big 
overhaul today.
*** Bug 57008 has been marked as a duplicate of this bug. ***
Sorry for the extra mail. Removing mail4 keyword.
Keywords: mail4
nominating for 6.5.
Keywords: nsbeta3mail3
*** Bug 59702 has been marked as a duplicate of this bug. ***
Summary: Filter UI: Problems adding criteria lines past initial display area → Search/Filter UI: Problems adding criteria lines past initial display area
->moz0.9
Target Milestone: Future → mozilla0.9
Keywords: nsbeta1
*** Bug 65430 has been marked as a duplicate of this bug. ***
Nominated for next release. I'd really like to see this fixed -- we get quite a
few reports/complains on this.
Keywords: mailtrack
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).
*** Bug 70653 has been marked as a duplicate of this bug. ***
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.
Assignee: hyatt → waterson
Status: ASSIGNED → NEW
*** Bug 62420 has been marked as a duplicate of this bug. ***
Yep. This is a mess.

*** This bug has been marked as a duplicate of 52726 ***
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → DUPLICATE
*** Bug 72394 has been marked as a duplicate of this bug. ***
Un dupifying.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
jst: read hyatt's comments from 2001-03-02 14:09.
Target Milestone: mozilla0.9 → ---
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...
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.
Severity: normal → major
Whiteboard: [PDTP3] [nsbeta3-]
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.
Severity: major → critical
Keywords: dataloss, nsdogfood
--> me
Assignee: waterson → hyatt
Status: REOPENED → NEW
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.2
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.
Assignee: hyatt → putterman
Status: ASSIGNED → NEW
reassigning to sspitzer.
Assignee: putterman → sspitzer
Priority: P3 → P1
*** Bug 86376 has been marked as a duplicate of this bug. ***
Whiteboard: [nsbeta1+]
moving to 0.9.3 per PDT
Target Milestone: mozilla0.9.2 → mozilla0.9.3
*** Bug 87012 has been marked as a duplicate of this bug. ***
8 duplicates.  Mostfreq?
Mostfreq. If not now, then tomorrow... I hope PDT realizes exactly how unusable
this simple problem renders MailNews as a viable mail client...
Keywords: mostfreq
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!
Keywords: nsenterprise
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.
Keywords: nsBranch
accepting.  investigating...

I remember a similar bug for the compose widget.  I'll see what I can find out.
Status: NEW → ASSIGNED
just got a brain dump from alecf, investigating a fix.
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?
adding alecf back.
I think I can eliminate the call to getRowCount() [and just use 
gTotalSearchTerms].

new patch coming soon.
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)
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.

> 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 :-/
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.
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.
Whiteboard: [nsbeta1+] → [nsbeta1+] fix for nsBranch in hand, will come up with something else for trunk
*** Bug 88488 has been marked as a duplicate of this bug. ***
Seth, you didn't drop your work on this one did you?  It's a critical bug.
updating 0.9.2-->0.9.3

Also, is it necessary to have nsCatFood and nsdogfood?
> 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.
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...
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. =)
*** Bug 89125 has been marked as a duplicate of this bug. ***
*** Bug 89361 has been marked as a duplicate of this bug. ***
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.
Keywords: rtmnsrtm
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.
> 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.
Regarding #2:

Do you mean that their default value is not set?

That's a bug on my list.
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!
*** Bug 89322 has been marked as a duplicate of this bug. ***
adding PDT+
Whiteboard: [nsbeta1+] fix for nsBranch in hand, will come up with something else for trunk → [nsbeta1+][PDT+] fix for nsBranch in hand, will come up with something else for trunk
Seth, once reviews are done, please check in on the branch.
Whiteboard: [nsbeta1+][PDT+] fix for nsBranch in hand, will come up with something else for trunk → [nsbeta1+][PDT+] fixed, needs reviews
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.
Whiteboard: [nsbeta1+][PDT+] fixed, needs reviews → [nsbeta1+][PDT+] branch only fix landed on the branch
> 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.
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?
Whiteboard: [nsbeta1+][PDT+] branch only fix landed on the branch → [nsbeta1+]branch only fix landed on the branch
Yes, I've been following the comments. Will test in tomorrow's branch.
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?
Attached file dr's rules.dat file
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...
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.
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.
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
Oops, you're right, I don't think I picked up your fix. I'll check it out today.
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.
Whiteboard: [nsbeta1+]branch only fix landed on the branch → [nsbeta1+]branch only fix landed on the branch - verified july10 branch with Mac, Win, Linux
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"


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

bienvenu, can you review the fix?
r/sr=bienvenu
fix checked into the trunk.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago23 years ago
Resolution: --- → FIXED
No longer blocks: 90699
*** Bug 91878 has been marked as a duplicate of this bug. ***
All looks OK to me with aug28 commercial trunk builds: mac OS 9.0, win98, linux
rh6.2
Status: RESOLVED → VERIFIED
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: