Last Comment Bug 94789 - Search/Filters UI: need error checking on Age in Days value (numeric only).
: Search/Filters UI: need error checking on Age in Days value (numeric only).
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 17.0
Assigned To: :aceman
:
:
Mentors:
Depends on: 358909
Blocks:
  Show dependency treegraph
 
Reported: 2001-08-10 14:17 PDT by laurel
Modified: 2012-08-27 11:18 PDT (History)
10 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed fix (900 bytes, patch)
2003-08-14 23:35 PDT, leon.zhang
neil: review-
Details | Diff | Splinter Review
screenshot of proposed look (5.06 KB, image/png)
2012-07-24 08:40 PDT, :aceman
bwinton: feedback+
Details
patch (10.59 KB, patch)
2012-07-25 14:16 PDT, :aceman
rkent: review+
bwinton: ui‑review+
Details | Diff | Splinter Review
patch v2 (10.62 KB, patch)
2012-07-26 13:17 PDT, :aceman
neil: review+
Details | Diff | Splinter Review
patch v3 (10.53 KB, patch)
2012-08-25 07:10 PDT, :aceman
mconley: review+
Details | Diff | Splinter Review

Description laurel 2001-08-10 14:17:59 PDT
Using aug10 commercial trunk build

There is no checking for input type for the Age in Days value field in the
Search Messages and Filter Rules dialogs.  Age in Days should allow numeric
input only. (Do we also want to limit digits?)  Currently, user is able to enter
alphabetical characters (value gets translated to 0).
Comment 1 laurel 2001-12-14 11:11:39 PST
still exists dec14 commercial trunk
Comment 2 leon.zhang 2003-08-14 23:35:58 PDT
Created attachment 129841 [details] [diff] [review]
proposed fix
Comment 3 neil@parkwaycc.co.uk 2004-01-18 15:14:04 PST
Comment on attachment 129841 [details] [diff] [review]
proposed fix

I can't see what this will achieve but catch(e) { throw e; } is wrong anyway.
Comment 4 Wayne Mery (:wsmwk, NI for questions) 2006-07-21 11:16:50 PDT
still fails - both seamonkey and TB trunk
Comment 5 Magnus Melin 2007-06-04 09:24:53 PDT
Maybe this could use the same approach as bug 376975...
Comment 6 :aceman 2011-10-23 10:13:35 PDT
This is still a problem in TB8. But the patch could be better.
Comment 7 :aceman 2012-07-24 08:17:25 PDT
I try this. Maybe dynamically setting type="number" on the textbox depending on the field being used could work.
Comment 8 :aceman 2012-07-24 08:29:17 PDT
On the other hand applying that attribute ads the small arrows to toggle the value into the textbox and makes it a bit higher than the other fields. So it looks ugly. Bwinton, what do you think? You can add the attribute via the DOM Inspector for preview.
Comment 9 Blake Winton (:bwinton) (:☕️) 2012-07-24 08:30:26 PDT
(Hint: if you don't set some sort of f? on this, I'm never going to get around to seeing it…  Yes, I do have that much other stuff to do. :(  )
Comment 10 :aceman 2012-07-24 08:40:08 PDT
Created attachment 645326 [details]
screenshot of proposed look

This is how it would look like if we used standard <textbox type="number">.
Comment 11 :aceman 2012-07-24 14:29:01 PDT
The widget seems to be defined here:
http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWidgets.xml
Comment 12 Blake Winton (:bwinton) (:☕️) 2012-07-25 09:19:50 PDT
Comment on attachment 645326 [details]
screenshot of proposed look

I don't mind that so much.  It's a fairly standard control...  I admit it looks a little strange with the up/down beside the +/-, but I think we can live with it.

f=me.

Thanks,
Blake.
Comment 13 :aceman 2012-07-25 14:16:16 PDT
Created attachment 645883 [details] [diff] [review]
patch

It appears this numeric checking can also be used on the Size header and Junk Percent. The code dynamically sets the allowed ranges depending on the chosen header. Rkent, please comment if these other headers do not take integer values or not in the ranges I set in the patch.

Of course, this does add new element to each rule so makes the editor slower again. But I didn't want to reuse the existing textbox used for text/freeform values as in the case of change of the header the dynamic setting of type=number on it would destroy the value and setting the header back (to e.g. subject) would not restore it. Currently it is the case.

This is only the TB version of the patch. I can try to do the SM one after this is accepted for TB.
Comment 14 Blake Winton (:bwinton) (:☕️) 2012-07-26 08:44:05 PDT
Comment on attachment 645883 [details] [diff] [review]
patch

>+++ b/mail/base/content/mailWidgets.xml
>@@ -1908,16 +1911,34 @@
>+            else if (val == Components.interfaces.nsMsgSearchAttrib.AgeInDays) {
>+              let valueBox = document.getAnonymousNodes(this)[9];
>+              valueBox.setAttribute("min", "-1000000");
>+              valueBox.setAttribute("max", "1000000");
>+              this.setAttribute("selectedIndex", "9");
>+            }

±2738 years?  Really?  :)  How about ±100-ish years (40000 days), instead?

ui-r=me with this change.

Thanks,
Blake.
Comment 15 Kent James (:rkent) 2012-07-26 12:35:29 PDT
Comment on attachment 645883 [details] [diff] [review]
patch

This works fine for me, and the code seems fine. The only thing that I would have done differently is that I don't see the reason to insert the new node at position 9, instead of just tacking onto the end at position 10. But I don't think that is worth a respin of the patch.

As I am not an official peer, you need to get approval from, say, bwinton that this review is sufficient before check-in (or get another review).
Comment 16 :aceman 2012-07-26 12:55:41 PDT
Thanks. I wanted to leave "Custom" on the last position so it had to be shifted to 10. Hopefully I managed to convert all the users of it.
The patch needs a respin nevertheless because of bwinton's comment, so you can still have it your way.
Comment 17 Kent James (:rkent) 2012-07-26 13:02:38 PDT
(In reply to :aceman from comment #16)
> Thanks. I wanted to leave "Custom" on the last position so it had to be
> shifted to 10. Hopefully I managed to convert all the users of it.
> The patch needs a respin nevertheless because of bwinton's comment, so you
> can still have it your way.

No, I would not bother. It's no big deal.
Comment 18 :aceman 2012-07-26 13:17:35 PDT
Created attachment 646287 [details] [diff] [review]
patch v2
Comment 19 neil@parkwaycc.co.uk 2012-07-30 14:09:19 PDT
Comment on attachment 646287 [details] [diff] [review]
patch v2

I don't know if this is a bug with numberbox, but I did notice that if you switched between numeric valued fields then the up/down buttons didn't always enable themselves appropriately. For example, if you initially choose "Age In Days" then you can't choose negative days until you click up and then down again. On the other hand, with the value back at 0, if you flip it over to size, then the down arrow is still enabled, although it disables if you try to click it.

The patch applies (with a -570 line offset) directly to suite's mailWidgets.xml, so that won't be a problem.

>+              valueBox.setAttribute("min", "-37000"); // ~-100 years
>+              valueBox.setAttribute("max", "37000"); // ~100 years
I would prefer a nice round number, such as 10000, although that's only 27 years so it might not be enough for some people...
Comment 20 :aceman 2012-07-30 14:26:51 PDT
That seems to be a problem with the widget. You can see it too in the Account manager, set "check for new messages every" to 1 then uncheck the option and check it again. Both arrows are enabled but when you click down (from 1) the value does not change but the arrow gets disabled.

OK, what about 40000 days as bwinton suggests?
Comment 21 neil@parkwaycc.co.uk 2012-08-24 05:20:17 PDT
(In reply to :aceman from comment #20)
> That seems to be a problem with the widget. You can see it too in the
> Account manager, set "check for new messages every" to 1 then uncheck the
> option and check it again. Both arrows are enabled but when you click down
> (from 1) the value does not change but the arrow gets disabled.
That's actually partly an account manager bug and partly a widget bug. The account manager bug is that it sets the disabled attribute rather than the property. The numberbox widget bug is that the disabled properly just sets the attribute without considering the arrows.

> OK, what about 40000 days as bwinton suggests?
Ah yes, thanks for pointing that out!
Comment 22 neil@parkwaycc.co.uk 2012-08-24 05:23:28 PDT
Comment on attachment 646287 [details] [diff] [review]
patch v2

>+            else if (val == Components.interfaces.nsMsgSearchAttrib.AgeInDays) {
>+              let valueBox = document.getAnonymousNodes(this)[9];
>+              valueBox.setAttribute("min", "-37000"); // ~-100 years
>+              valueBox.setAttribute("max", "37000"); // ~100 years
>+              this.setAttribute("selectedIndex", "9");
>+            }
>+            else if (val == Components.interfaces.nsMsgSearchAttrib.Size) {
>+              let valueBox = document.getAnonymousNodes(this)[9];
>+              valueBox.setAttribute("min", "0");
>+              valueBox.setAttribute("max", "1000000000");
>+              this.setAttribute("selectedIndex", "9");
>+            }
>+            else if (val == Components.interfaces.nsMsgSearchAttrib.JunkPercent) {
>+              let valueBox = document.getAnonymousNodes(this)[9];
>+              valueBox.setAttribute("min", "0");
>+              valueBox.setAttribute("max", "100");
>+              this.setAttribute("selectedIndex", "9");
>+            }
r=me if you change these to use min/max properties instead of attributes. For example, valueBox.max = 40000; // a little over 100 years
Comment 23 :aceman 2012-08-25 07:10:50 PDT
Created attachment 655307 [details] [diff] [review]
patch v3

So when do I need to use attributes and when properties? What is the difference?

(Also, using properties does not seem to solve the issue Neil had).
Comment 24 Mike Conley (:mconley) - (needinfo me!) 2012-08-27 10:09:45 PDT
Comment on attachment 655307 [details] [diff] [review]
patch v3

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

I've never looked at this binding before, and I'm not thrilled at how bound it is to the indices of its child nodes - screams brittle to me.

But that's out of scope for this patch, which handles the problem described in the bug correctly. Assuming the max/min values you've set are acceptable to bwinton, this is r=me.

Thanks aceman!
Comment 25 Ryan VanderMeulen [:RyanVM] 2012-08-27 11:18:23 PDT
https://hg.mozilla.org/comm-central/rev/b275fd8eabca

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