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

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[laurel]

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]

still exists dec14 commercial trunk

Comment 2

[leon.zhang]

Attachment: proposed fix

Comment 3

[[e-mail]]

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)]

still fails - both seamonkey and TB trunk

Comment 5

[Magnus Melin [:mkmelin]]

Maybe this could use the same approach as bug 376975...

Comment 6

[:aceman]

This is still a problem in TB8. But the patch could be better.

Comment 7

[:aceman]

I try this. Maybe dynamically setting type="number" on the textbox depending on the field being used could work.

Comment 8

[:aceman]

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) (:☕️)]

(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]

Attachment: screenshot of proposed look

This is how it would look like if we used standard <textbox type="number">.

Comment 11

[:aceman]

The widget seems to be defined here:
http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWidgets.xml

Comment 12

[Blake Winton (:bwinton) (:☕️)]

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]

Attachment: 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) (:☕️)]

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)]

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]

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)]

(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]

Attachment: patch v2

Comment 19

[[e-mail]]

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]

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

[[e-mail]]

(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

[[e-mail]]

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]

Attachment: 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) (:⚙️)]

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]]

https://hg.mozilla.org/comm-central/rev/b275fd8eabca