Search/Filters UI: need error checking on Age in Days value (numeric only).

RESOLVED FIXED in Thunderbird 17.0

Status

MailNews Core
Filters
--
minor
RESOLVED FIXED
16 years ago
5 years ago

People

(Reporter: laurel, Assigned: aceman)

Tracking

Trunk
Thunderbird 17.0
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

5.06 KB, image/png
bwinton
: feedback+
Details
10.53 KB, patch
mconley
: review+
Details | Diff | Splinter Review
(Reporter)

Description

16 years ago
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).
(Reporter)

Updated

16 years ago
QA Contact: esther → laurel
(Reporter)

Comment 1

16 years ago
still exists dec14 commercial trunk

Comment 2

14 years ago
Created attachment 129841 [details] [diff] [review]
proposed fix

Updated

14 years ago
Attachment #129841 - Flags: superreview?
Attachment #129841 - Flags: review?
Attachment #129841 - Flags: review? → review?(neil.parkwaycc.co.uk)

Comment 3

14 years ago
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.
Attachment #129841 - Flags: superreview?
Attachment #129841 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #129841 - Flags: review-
Product: Browser → Seamonkey

Updated

12 years ago
Assignee: sspitzer → mail
still fails - both seamonkey and TB trunk
Assignee: mail → nobody
Component: MailNews: Main Mail Window → MailNews: Filters
Product: Mozilla Application Suite → Core
QA Contact: laurel → filters

Comment 5

10 years ago
Maybe this could use the same approach as bug 376975...
Depends on: 358909
Product: Core → MailNews Core
(Assignee)

Comment 6

6 years ago
This is still a problem in TB8. But the patch could be better.
(Assignee)

Comment 7

5 years ago
I try this. Maybe dynamically setting type="number" on the textbox depending on the field being used could work.
Assignee: nobody → acelists
(Assignee)

Comment 8

5 years ago
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.
(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. :(  )
(Assignee)

Comment 10

5 years ago
Created attachment 645326 [details]
screenshot of proposed look

This is how it would look like if we used standard <textbox type="number">.
Attachment #645326 - Flags: feedback?(bwinton)
Attachment #645326 - Attachment is patch: false
Attachment #645326 - Attachment mime type: text/plain → image/png
(Assignee)

Comment 11

5 years ago
The widget seems to be defined here:
http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWidgets.xml
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.
Attachment #645326 - Flags: feedback?(bwinton) → feedback+
(Assignee)

Comment 13

5 years ago
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.
Attachment #129841 - Attachment is obsolete: true
Attachment #645883 - Flags: ui-review?(bwinton)
Attachment #645883 - Flags: review?(kent)
Attachment #645883 - Flags: feedback?(neil)
(Assignee)

Updated

5 years ago
Attachment #645883 - Flags: feedback?(mkmelin+mozilla)
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.
Attachment #645883 - Flags: ui-review?(bwinton) → ui-review+

Comment 15

5 years ago
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).
Attachment #645883 - Flags: review?(kent) → review+
(Assignee)

Comment 16

5 years ago
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.
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED

Comment 17

5 years ago
(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.
(Assignee)

Comment 18

5 years ago
Created attachment 646287 [details] [diff] [review]
patch v2
Attachment #645883 - Attachment is obsolete: true
Attachment #645883 - Flags: feedback?(neil)
Attachment #645883 - Flags: feedback?(mkmelin+mozilla)
Attachment #646287 - Flags: review?(neil)
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...
(Assignee)

Comment 20

5 years ago
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?
(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 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
Attachment #646287 - Flags: review?(neil) → review+
(Assignee)

Comment 23

5 years ago
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).
Attachment #646287 - Attachment is obsolete: true
Attachment #655307 - Flags: review?(mconley)
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!
Attachment #655307 - Flags: review?(mconley) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/b275fd8eabca
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 17.0
You need to log in before you can comment on or make changes to this bug.