345510 - Add <textbox type="number">

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Neil Deakin]

Thought there might be a bug on this already, but couldn't find one. Now that spinbuttons is available, add a <textbox type="number"> which allows numeric values only.

Documentation is at http://wiki.mozilla.org/XUL:Specs:NumberBox
Testcases are at http://xulplanet.com/ndeakin/tests/xts/numberbox/

Comment 1

[Neil Deakin]

Attachment: Add numberbox

Comment 2

[Alex Vincent [:WeirdAl]]

Neil, I plan on implementing Web Forms 2 <html:input type="number"/> (see bug 344616) as a XBL binding.  With that, <xul:textbox type="number"/> would naturally work without this extra binding.  Maybe you should implement this that way and just take over the bug.  (I haven't figured out how to make extended html:input bindings actually render yet - the other Neil suggests a display attribute on the binding, which I haven't tested yet, and there's also bug 344615 comment 1 to deal with.) 

Also, please note bug 312869 (context menus for textboxes), in particular comment 27 (which I haven't figured out how to do just yet - long day).  I want to nip the botch in the bud in new code.

Comment 3

[Alex Vincent [:WeirdAl]]

surkov was also talking about something like this for XForms, and I had filed bug 344655 for sharing common code between WF2 and XF.

Comment 4

[Neil Deakin]

For webforms, xforms, etc, you'd probably want the inner content to be hidden much like file upload widgets do. I'd suggest just implementing something akin to nsFileControlFrame.cpp that wraps this XUL numberbox binding.

Comment 5

[[e-mail]]

Comment on attachment 230170 [details] [diff] [review]
Add numberbox

>Index: xpfe/global/resources/content/bindings/numberbox.xml
Just reviewing this file for tonight.

>+      <stylesheet src="chrome://global/skin/textbox.css"/>
Don't you pick up this file by inheritance?

>+          return (this.min != Infinity && this.max != Infinity);
Rather than putting this test here I feel it belongs in _validateValue

>+          if (val)
>+            this.setAttribute('wraparound', 'true');
>+          else
>+            this.removeAttribute('wraparound');
>+          return val;
You should update the buttons too.

>+      <property name="min">
minvalue feels better to me.

>+          if (typeof val == "number")
>+            this.setAttribute("min", val);
>+          if (this.value < val)
>+            this._validateValue(this.inputField, val, false);
Why bother validating the value if you don't set the attribute?

>+          if (typeof val != "number")
>+            return;
return val;

>+        <parameter name="field"/>
This is always the same as this.inputField isn't it?

>+        <parameter name="val"/>
This isn't a setter; feel free to call it "value".

>+            val = Number(val);
>+            if (isNaN(val)) val = 0;
val = Number(val) || 0; is an alternative construct.

>+            if (places != Infinity)
>+              val = val.toFixed(places);
The problem here is that val could now be a string or still be a number.
Looking at the code below I also think it might be easier to round later.

>+            if ((min != Infinity) && (val < min)) {
I think you should support -Infinity as the limiting minimum value.
That also lets you simplify the test to value < minValue.

>+            else if ((max != Infinity) && (val > max)) {
Again val > max suggests max != Infinity thus saving you checking twice.

>+        var dsymbol = (Number(5.4)).toLocaleString().match(/\D/);
>+        if (dsymbol != null)
>+          this.decimalSymbol = dsymbol[0];
I think there's an easier way of doing this with RegExp:
if (/\D/.test(5.4))
  this.decimalSymbol = RegExp.lastMatch;

>+        var val = this.inputField && this.inputField.value;
>+        if (!val || isNaN(Number(val)))
>+          val = 0;
Not sure why you need to test for this.inputField here;
Could perhaps use var val = this.inputField.value || 0;

>+            if (event.charCode == 45 && (this.min < 0 || this.min == Infinity))
Again, allowing this.min to be -Infinity simplifies this check.

Comment 6

[Michael Ventnor]

+          if (event.charCode) {
+            if (event.charCode == this.decimalSymbol.charCodeAt(0) &&
+                this.decimalPlaces &&
+                String(this.inputField.value).indexOf(this.decimalSymbol) == -1)
+              return;
+
+            if (event.charCode == 45 && (this.min < 0 || this.min == Infinity))
+              return;
+
+            if (event.charCode < 48 || event.charCode > 57)
+              event.preventDefault();
+          }

I'm worried that this will block the backspace (charcode 8) or delete (charcode 127) keys. Are these taken into consideration anywhere else? Or can deletion not be blocked and I'm just being paranoid?

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> surkov was also talking about something like this for XForms, and I had filed
> bug 344655 for sharing common code between WF2 and XF.
> 

XForms allow to typify instance data, therefore it's fine if xforms input controls will support typified input. Currently xforms can be hosted in XHTML and XUL documents and we should have widgets like numberbox both for XHMTL and for XUL.

Neil, if you can do your widget for XHTML (for example input type="number") and widget for XUL then it will grace.

Comment 8

[[e-mail]]

Comment on attachment 230170 [details] [diff] [review]
Add numberbox

>Index: themes/classic/global/mac/numberbox.css
I don't have a Mac; maybe you could get Mano or someone to review this bit?

>Index: themes/classic/global/unix/numberbox.css
>+  margin: 2px 4px;
Isn't this margin cascaded anyway?

>+  background: none;
I'm not sure why you need to override the background colour here as your xbl child nodes should cover the entire area of the textbox.

>+.numberbox-input {
>+  -moz-appearance: none !important;
>+  padding: 0;
>+  padding-bottom: 1px;
>+  margin: 0;
>+  border: none;
>+}
>+  
>+.numberbox-input html|*.textbox-input {
>+  padding-right: 6px !important;
>+  text-align: right;
>+}
I'm not convinced that any of these styles will apply; what I think you are looking for is html|*.numberbox-input also I'm not convinced that this is the right place to add padding.

>+.numberbox-input-box {
I feel that this could do with a -moz-box-align: center; which would also allow you to reduce the padding to the default textbox padding. The same goes for the windows version; meanwhile the Modern version doesn't seem to have any padding at all.

>+      <xul:spinbuttons anonid="buttons" xbl:inherits="disabled"
>+                       onup="this.parentNode._modifyUp();"
>+                       ondown="this.parentNode._modifyDown();"/>
The spinbuttons seem to take focus which confuses the textbox binding preventing you from tabbing forward.

>+      </property>
>+      <property name="wrapAround">
You may have a reason for your apparently inconsistent spacing, but I couldn't work it out ;-)

>+      <handler event="keypress">
You're trapping all the Ctrl- Alt- and Meta- keys here; I don't know if you can use modifiers="shift any" here or whether you have to check using JS.

>+      <handler event="keypress" keycode="VK_UP">
>+        this._modifyUp();
>+      </handler>
>+      <handler event="keypress" keycode="VK_DOWN">
>+        this._modifyDown();
>+      </handler>
You need to prevent the default action of these keys; <handler preventDefault="true"> might suffice.

Comment 9

[Neil Deakin]

Attachment: Address review comments

>+      <property name="min">
> minvalue feels better to me.

I used min and max to be consistent with <scale>. Do you think I should change scale to minvalue/minValue?

Comment 10

[[e-mail]]

(In reply to comment #9)
>Created an attachment (id=230578)
>Address review comments
Well, it addresses most of them, but I don't see any arguments for or changes in reply to the following:

>Index: themes/classic/global/mac/numberbox.css
I don't have a Mac; maybe you could get Mano or someone to review this bit?

>+  background: none;
I'm not sure why you need to override the background, let alone the background colour here as your xbl child nodes should cover the entire area of the textbox.

>+.numberbox-input {
I'm not convinced that any of these styles will apply; what I think you are looking for is html|*.numberbox-input

>+.numberbox-input-box {
The Modern version doesn't seem to have any padding at all.

>+      <xul:spinbuttons anonid="buttons" xbl:inherits="disabled"
>+                       onup="this.parentNode._modifyUp();"
>+                       ondown="this.parentNode._modifyDown();"/>
The spinbuttons seem to take focus which confuses the textbox binding preventing you from tabbing forward.

>+      <handler event="keypress">
You're trapping all the Ctrl- Alt- and Meta- keys here; I don't know if you can use modifiers="shift any" here or whether you have to check using JS.

Comment 11

[Neil Deakin]

(In reply to comment #10)

> >Index: themes/classic/global/mac/numberbox.css
> I don't have a Mac; maybe you could get Mano or someone to review this bit?
> 
I intended to.

> >+  background: none;
> I'm not sure why you need to override the background, let alone the background
> colour here as your xbl child nodes should cover the entire area of the
> textbox.

Oops, missed this in the Mac theme.

> 
> >+.numberbox-input {
> I'm not convinced that any of these styles will apply; what I think you are
> looking for is html|*.numberbox-input

I misinterpreted this comment and will look into it again.

> 
> >+.numberbox-input-box {
> The Modern version doesn't seem to have any padding at all.

I'll investigate.

> The spinbuttons seem to take focus which confuses the textbox binding
> preventing you from tabbing forward.

I don't see any issue here. Which platforms?

> You're trapping all the Ctrl- Alt- and Meta- keys here; I don't know if you can
> use modifiers="shift any" here or whether you have to check using JS.
> 

I'm checking event.charCode for printable characters, which is the check that I want to do. 

Comment 12

[Neil Deakin]

> 
> > >+  background: none;
> > I'm not sure why you need to override the background, let alone the background
> > colour here as your xbl child nodes should cover the entire area of the
> > textbox.
> 
> Oops, missed this in the Mac theme.
> 
Actually, no, the background: none is right here as the spinbuttons are drawn outside the textbox, and no background should appear in the gap between them.

Comment 13

[[e-mail]]

(In reply to comment #11)
>>The spinbuttons seem to take focus which confuses the textbox binding
>>preventing you from tabbing forward.
>I don't see any issue here. Which platforms?
Windows/Linux, or Mac with Full Keyboard Accessibility. I looked more closely and the problem is that each individual button in the spinbutton is still in the tab order, which seems unintentional although I overlooked that before.

>>You're trapping all the Ctrl- Alt- and Meta- keys here; I don't know if you can
>>use modifiers="shift any" here or whether you have to check using JS.
>I'm checking event.charCode for printable characters, which is the check that I
>want to do.
Things like Ctrl+C still have printable charCodes.

(In reply to comment #12)
>Actually, no, the background: none is right here as the spinbuttons are drawn
>outside the textbox, and no background should appear in the gap between them.
OK, but would background-color: transparent; suffice in that case?

Comment 14

[Neil Deakin]

Attachment: address review comments

Hopefully, I've addressed all the review comments

Comment 15

[Neil Deakin]

Comment on attachment 230935 [details] [diff] [review]
address review comments

requesting additional review on Mac changes from mano as suggested by Neil

Comment 16

[[e-mail]]

Comment on attachment 230935 [details] [diff] [review]
address review comments

>Index: themes/classic/global/mac/numberbox.css
>+  background: transparent;
Nit: background-color: transparent; ?

>Index: themes/classic/global/unix/numberbox.css
>+  padding: 2px 2px 3px 4px;
This variable padding looks a bit odd to me, 3px all round looks nicer.

>Index: themes/modern/global/numberbox.css
>+  padding: 1px 0px 1px 2px;
Here I think 1px all round is better (I also considered 2px).

I also discovered that the caret seems to disappear, but I've no idea why, maybe you need to ask mrbkap?

Comment 17

[[e-mail]]

Comment on attachment 230935 [details] [diff] [review]
address review comments

>Index: themes/classic/global/win/numberbox.css
>+.numberbox-input-box {
>+  -moz-box-align: center;
>+}
The weirdness that is <html:input size=1> confused me, but I think that 2px padding works best in Windows classic. r+sr=me with these tweaks.

Comment 18

[Asaf Romano (gone)]

Comment on attachment 230935 [details] [diff] [review]
address review comments

r=mano on the pinstripe and mac-classic theme changes.

Comment 19

[Neil Deakin]

Checked in

Comment 20

[Neil Deakin]

Tests are bug 378018