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)
MailNews Core
Filters
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)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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•25 years ago
|
||
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...
Updated•25 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → M17
Comment 2•25 years ago
|
||
mass moving to M18 and adding nsbeta3 keyword
Keywords: nsbeta3
Target Milestone: M17 → M18
Comment 6•24 years ago
|
||
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
Comment 7•24 years ago
|
||
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+]
Comment 8•24 years ago
|
||
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•24 years ago
|
||
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 11•24 years ago
|
||
PDT felt this was obscure enough to push it to P3
Priority: P2 → P3
Whiteboard: [nsbeta3+] → [nsbeta3+][PDTP3]
Comment 12•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
This really should work. Alec, have you made sure to do the pre-load using
loadBindingDocument?
Comment 19•24 years ago
|
||
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
Comment 20•24 years ago
|
||
Clearing nsbeta3+ for triage by new owners.
Whiteboard: [nsbeta3+][PDTP3] → [PDTP3]
Comment 21•24 years ago
|
||
nsbeta3+, P3 for M18
Status: NEW → ASSIGNED
Whiteboard: [PDTP3] → [PDTP3] [nsbeta3+]
Comment 22•24 years ago
|
||
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
Comment 23•24 years ago
|
||
Hey Alec, check out the trunk and see if I fixed this bug. XBL got a big
overhaul today.
Reporter | ||
Comment 24•24 years ago
|
||
*** Bug 57008 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 27•24 years ago
|
||
*** 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
Reporter | ||
Comment 29•24 years ago
|
||
*** Bug 65430 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 30•24 years ago
|
||
Nominated for next release. I'd really like to see this fixed -- we get quite a
few reports/complains on this.
Comment 31•24 years ago
|
||
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•24 years ago
|
||
*** Bug 70653 has been marked as a duplicate of this bug. ***
Comment 33•24 years ago
|
||
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
Comment 34•24 years ago
|
||
*** Bug 62420 has been marked as a duplicate of this bug. ***
Comment 35•24 years ago
|
||
Yep. This is a mess.
*** This bug has been marked as a duplicate of 52726 ***
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → DUPLICATE
Reporter | ||
Comment 36•24 years ago
|
||
*** Bug 72394 has been marked as a duplicate of this bug. ***
Comment 38•24 years ago
|
||
jst: read hyatt's comments from 2001-03-02 14:09.
Target Milestone: mozilla0.9 → ---
Comment 39•24 years ago
|
||
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•24 years ago
|
||
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•23 years ago
|
||
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.
Updated•23 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.2
Comment 43•23 years ago
|
||
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
Reporter | ||
Comment 45•23 years ago
|
||
*** Bug 86376 has been marked as a duplicate of this bug. ***
Updated•23 years ago
|
Whiteboard: [nsbeta1+]
Reporter | ||
Comment 47•23 years ago
|
||
*** Bug 87012 has been marked as a duplicate of this bug. ***
Comment 48•23 years ago
|
||
8 duplicates. Mostfreq?
Comment 49•23 years ago
|
||
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
Reporter | ||
Comment 50•23 years ago
|
||
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
Comment 51•23 years ago
|
||
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
Assignee | ||
Comment 52•23 years ago
|
||
accepting. investigating...
I remember a similar bug for the compose widget. I'll see what I can find out.
Status: NEW → ASSIGNED
Assignee | ||
Comment 53•23 years ago
|
||
just got a brain dump from alecf, investigating a fix.
Assignee | ||
Comment 54•23 years ago
|
||
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?
Assignee | ||
Comment 55•23 years ago
|
||
Assignee | ||
Comment 56•23 years ago
|
||
adding alecf back.
Assignee | ||
Comment 57•23 years ago
|
||
I think I can eliminate the call to getRowCount() [and just use
gTotalSearchTerms].
new patch coming soon.
Assignee | ||
Comment 58•23 years ago
|
||
Comment 59•23 years ago
|
||
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)
Assignee | ||
Comment 60•23 years ago
|
||
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•23 years ago
|
||
> 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•23 years ago
|
||
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•23 years ago
|
||
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.
Assignee | ||
Updated•23 years ago
|
Whiteboard: [nsbeta1+] → [nsbeta1+] fix for nsBranch in hand, will come up with something else for trunk
Comment 64•23 years ago
|
||
*** Bug 88488 has been marked as a duplicate of this bug. ***
Comment 65•23 years ago
|
||
Seth, you didn't drop your work on this one did you? It's a critical bug.
Comment 66•23 years ago
|
||
updating 0.9.2-->0.9.3
Also, is it necessary to have nsCatFood and nsdogfood?
Keywords: mozilla0.9.2 → mozilla0.9.3
Assignee | ||
Comment 67•23 years ago
|
||
> 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•23 years ago
|
||
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•23 years ago
|
||
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. =)
Reporter | ||
Comment 70•23 years ago
|
||
*** Bug 89125 has been marked as a duplicate of this bug. ***
Comment 71•23 years ago
|
||
*** Bug 89361 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 72•23 years ago
|
||
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.
Assignee | ||
Comment 73•23 years ago
|
||
Assignee | ||
Comment 74•23 years ago
|
||
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.
Assignee | ||
Comment 75•23 years ago
|
||
> 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•23 years ago
|
||
Regarding #2:
Do you mean that their default value is not set?
That's a bug on my list.
Comment 77•23 years ago
|
||
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!
Reporter | ||
Comment 78•23 years ago
|
||
*** Bug 89322 has been marked as a duplicate of this bug. ***
Comment 79•23 years ago
|
||
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
Comment 80•23 years ago
|
||
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
Assignee | ||
Comment 81•23 years ago
|
||
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
Assignee | ||
Comment 82•23 years ago
|
||
> 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.
Assignee | ||
Comment 83•23 years ago
|
||
Comment 84•23 years ago
|
||
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
Reporter | ||
Comment 85•23 years ago
|
||
Yes, I've been following the comments. Will test in tomorrow's branch.
Comment 86•23 years ago
|
||
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•23 years ago
|
||
Comment 88•23 years ago
|
||
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...
Assignee | ||
Comment 89•23 years ago
|
||
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.
Assignee | ||
Comment 90•23 years ago
|
||
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.
Reporter | ||
Comment 91•23 years ago
|
||
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•23 years ago
|
||
Oops, you're right, I don't think I picked up your fix. I'll check it out today.
Reporter | ||
Comment 93•23 years ago
|
||
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
Assignee | ||
Comment 94•23 years ago
|
||
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"
Assignee | ||
Comment 95•23 years ago
|
||
Assignee | ||
Comment 96•23 years ago
|
||
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•23 years ago
|
||
r/sr=bienvenu
Assignee | ||
Comment 98•23 years ago
|
||
Assignee | ||
Comment 99•23 years ago
|
||
fix checked into the trunk.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago → 23 years ago
Resolution: --- → FIXED
*** Bug 91878 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 101•23 years ago
|
||
All looks OK to me with aug28 commercial trunk builds: mac OS 9.0, win98, linux
rh6.2
Status: RESOLVED → VERIFIED
Updated•20 years ago
|
Product: MailNews → Core
Updated•16 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•