66163 - Implement generic masking for textfields

Status: NEW

(Core :: XUL, defect)

Description

[Blake Ross]

Clients should be able to specify generic masks for textfields, like only 
allowing letters, numbers, or alphanumeric characters.  Just assigning this to 
me for the work...

Comment 1

[Blake Ross]

er...really.

Comment 2

[Blake Ross]

Attachment: [patch] first pass...this is ugly even for me

Comment 3

[Blake Ross]

cc'ing timeless/alec for r/sr

I plan on adding more masking flags later.

Comment 4

[timeless]

<property name="allows"
onset="this.setAttribute('allows', val); return val;"
onget="return this.getAttribute('allows');"/>
don't you only need onset/onget if you're doing something unusual?

var strBundleService = 
Components.classes["@mozilla.org/intl/stringbundle;1"].getService()
.QueryInterface(Components.interfaces.nsIStringBundleService);
shouldn't we cache these services?

/*** Attribute "allowflags" ... *****/ outside the CDATA, is that proper? 
shouldn't you use <!-- --> ?

return (alphabet.indexOf(aChar.toLowerCase()) != -1 || (aChar == " " && 
allowSpace));
please use return statement; instead of return (statement);
[asside:
javascript:alert(0==0&&0) javascript:alert(0==(0&&0)) so i guess == is 
evaluated before &&]
probably:
return alphabet.indexOf(aChar.toLowerCase()) != -1 ||
       (aChar == " " && allowSpace);

you use:
var allowSpace = (flags.indexOf("allow-space") != -1);
I think the outer () is unnecessary, but if brendan tells me that you should 
keep it...

please follow the following indentation style [unless brendan overrules, this 
disclaimer applies to everything]
switch (variable) {
case caseStatement:
  statements;
case caseStatement:
  statements;
default:
  statements;
}

as always aim for 80 columns

I think beyond the above, it's ok asside from your concerns about it being 
'really ugly'

Comment 5

[Blake Ross]

> don't you only need onset/onget if you're doing something unusual?

I don't think so...

> var strBundleService = 
> Components.classes["@mozilla.org/intl/stringbundle;1"].getService()
> .QueryInterface(Components.interfaces.nsIStringBundleService);
> shouldn't we cache these services?

They are.  That's the default value for the pref, it's not re-retrieved every 
time.

> /*** Attribute "allowflags" ... *****/ outside the CDATA, is that proper? 
> shouldn't you use <!-- --> ?

Yes, sorry; those should be xml comments.

Comment 6

[Alec Flett]

minor nit: if you use ....getService(Components.interfaces.nsIFoo) instead of
....getService().QueryInterface(Components.interfaces.nsIFoo) you'll avoid an
extra AddRef/Release.

major nit: It seems like we should be caching an alphabet, essentially breaking
up the task of determining the mask from the task of testing against it. This
way you could (in the future) actually prevent certain keys from ever being
entered, such that I could never even type a "b" when the mask is set to "numbers"

Comment 7

[Blake Ross]

> This way you could (in the future) actually prevent certain keys from ever 
> being entered, such that I could never even type a "b" when the mask is set to 
> "numbers"

I don't understand this; how does this differ from how it is now?

Comment 8

[Alec Flett]

oh man!
I didn't read the code closely enough
this is a huge performance suck.
you're doing a lot of work every time the user hits a key... 
My comments above still apply, except that you ARE doing the work every time the
user hits a key. Find me on IRC, I'll explain

Comment 9

[Blake Ross]

Yeah, it's silly to keep retrieving the flags/string each time the user types 
(although keep in mind we only do all this work when there's a mask).  I'll 
attach a new patch when I get home, I'm in class.

Comment 10

[Brendan Eich [:brendan]]

blake: use RegExps, that's what they're for (for example, a regexp of the form
[a-z0-9] compiles into a bitmap for constant-time compiled-C-code character in
set membership tests).

Also,

+        catch(ex) {
+          null;
+        }

will get you a strict "useless expression" warning now.  Why null;?

/be

Comment 11

[David Hyatt]

I do not see a need for this feature to be built right into all textfields. 
Could you explain your motivations here?

Comment 12

[David Hyatt]

I especially don't like the additional raw property "stringBundle".  Every
textfield gets more bloated with the addition of raw properties.  Getters and
setters can be shared among all textfields, but raw properties cannot.  Why
wouldn't that property also be a getter/setter?

I would prefer that this capability not be built into the baseline text field. 
You could (if people are demanding this functionality) always put it into a
derived binding (e.g., textfield mask="true") with binding applied using
textfield[mask="true"]).

Comment 13

[Blake Ross]

We currently have many textfields that require only numbers or letters (e.g. in 
prefs alone: dpi, global history expiration, number of session history entries, 
maximum number of recent composer pages, number of characters to wrap plain 
text messages at,  number of addressbook cards, cache, and so forth), but each 
has their own way of handling this, and their own inconsistent 
implementations.  Someone suggested this as an easy and convenient way of doing 
this generically in another bug.

The derived binding sounds fine.

Comment 14

[David Hyatt]

One idea jag had (he was leaning over my shoulder here) was that the mask attr
actually specify a mask... e.g.,

<textfield mask="[a-zA-Z]"/>

Regular expressions would be the way to go with the mask, as brendan said.  For
example the above mask would allow you to input only alphabetic chars.

Comment 15

[Matthew T (active 1999-2002)]

[Even though Hyatt is the toolkit god, I don't think he really believes this is 
an ActiveX wrapper bug. Back to -->Toolkit/Widgets.]

Comment 16

[Ben Goodger (use ben at mozilla dot org for email)]

(bugzilla glitched up as he submitted his post and thought he wanted to change 
product, he had to reset back to browser which I think selected ActiveX)

Comment 17

[Dan Rosen]

blake: you still interested in working on this? i'd kind of like to take a crack
at it if you're not :)

Comment 18

[benc]

I've been thinking about writing a javascript library that would do validation
for network data structures (IPv4, IPv6 addresses, DNS, port numbers, netmasks, etc.

It would be an extension/re-write of the PAC util functions.