Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#12116 closed defect (fixed)

CurrencyTextBox/NumberTextBox: clash with <input type="number">

Reported by: James Thomas Owned by: Douglas Hays
Priority: high Milestone: 1.7
Component: Dijit - Form Version: 1.5
Keywords: Cc: Adam Peller
Blocked By: Blocking:

Description (last modified by bill)

The following declarative widget instantiation:

<input dojoType="dijit.form.CurrencyTextBox" type="number" required name="sample"/>

renders correctly on Chrome 8.0.5, although we get number spinners inside the input field.

The input field works correctly from values 0 - 999 but it goes above 999 the field blanks itself.

Tracing the issue through I found out why.

Values from 0 to 999 are formatted without a comma but 1000 is the first value that requires one. When the new user value is being set, eventually we hit this code in "TextBox.js#_setValueAttr":

if(formattedValue != null && formattedValue != undefined && ((typeof formattedValue) != "number" || !isNaN(formattedValue)) && this.textbox.value != formattedValue){
                this.textbox.value = formattedValue;
                this._set("displayedValue", this.get("displayedValue"));

The input field is blanked when this.textbox.value is set to "1,000.00". Looking at the rendered HTML, the native input field has a type of "number".

Looking at the rendered HTML for this widget, I see the following.

<input class="dijitReset dijitInputInner" dojoattachpoint="textbox,focusNode" autocomplete="off"''' "type="number"''' id="dijit_form_SampleCurrencyTextBox_0" tabindex="0" aria-valuemin="-9000000000000" aria-valuemax="9000000000000" aria-required="true" aria-invalid="true" value="" aria-valuenow="NaN">

According to the HTML5 spec (

The value sanitization algorithm is as follows: If the value of the element is not a valid floating point number, then set it to the empty string instead.

1,000.00 doesn't conform to a valid FP number and get sanitises to a blank string, which is what we see.

This can be fixed in the user application by not specifying the type as "number" but should the dijit code be manually setting a different type of the native attribute to ensure this doesn't occur?

I tested this on FF 3.6 which doesn't support the type="number" attribute and it didn't occur.

Attachments (1)

12116.patch (6.2 KB) - added by Douglas Hays 8 years ago.
possible fix

Download all attachments as: .zip

Change History (13)

comment:1 Changed 8 years ago by bill

Component: GeneralDijit
Description: modified (diff)
Owner: changed from anonymous to Douglas Hays
Summary: dijit.form.CurrencyTextBox clash with <input type="number">CurrencyTextBox/NumberTextBox: clash with <input type="number">

Sounds like the same problem will happen with NumberTextBox. Perhaps it's as simple as removing type from attributeMap (well technically, it's difficult to remove it and still do inheritance, but can override the value setting it to null or undefined).

comment:2 Changed 8 years ago by Douglas Hays

Bill, I just wanted to double-check. It's been dijit policy to not add code to workaround issues like this. The type attribute was left as user-defined to allow for type=hidden, and less importantly type=password. I think not allowing these cases may cause more negative reactions than if we leave it as-is, and if we want to sanity check certain values then we need to check for other values like type=button that also cause problems for those widgets.

comment:3 Changed 8 years ago by James Thomas

Doug & Bill,

I think this issue is going to be more problematic in the future with the increased number of input type values specified in the HTML5 spec.

If we leave it as-is, could we document this issue somewhere obvious for users? I can see this being a fairly easy issue to fall into for users and it's not at all obvious what's going wrong without debugging through Dojo itself.

comment:4 Changed 8 years ago by Adam Peller

Cc: Adam Peller added

comment:5 Changed 8 years ago by bill

Doug - you are right that it's the dijit policy not to check argument types etc. And I didn't think of type=hidden, I suppose someone might be doing that (although it seems useless) so we shouldn't break that.

So, I can see the arguments for both sides, but I think we should support type=number though for a few reasons:

1) Because NumberTextBox and CurrencyTextBox do in fact hold numbers

2) Because some developers (for better or worse) try to write degradable HTML. Ex:

<input type=number dojoType=dijit.form.NumberTextBox/>

Of course with the new parser they should be writing:

<input type=number data-dojo-type=dijit.form.NumberTextBox/>

in which case the parser will skip the type=... declaration. But that won't be guaranteed until 2.0.

3) The error that occurs is subtle; developers might specify type=number and not notice that things are functioning incorrectly.

For the same reasons, DateTextBox with type=date should also work, as should TimeTextBox with type=time.

comment:6 Changed 8 years ago by Douglas Hays

Milestone: tbd1.6
Status: newassigned

OK, will do, but I still disagree. This kind of code (overriding a user-override) thwarts widget subclassing.

comment:7 Changed 8 years ago by bill

Thanks, I hope we don't end up regretting it. I didn't consider subclassing before although I can't think of a case where a subclass would want type=number etc. applied to the <input> node.

comment:8 Changed 8 years ago by Douglas Hays

Milestone: 1.61.7

comment:9 Changed 8 years ago by bill

Component: DijitDijit - Form

Changed 8 years ago by Douglas Hays

Attachment: 12116.patch added

possible fix

comment:10 Changed 8 years ago by Douglas Hays

Attached a patch for review. The type=text needs to be overridden before _WidgetBase::buildRendering runs since that does a string substitution for the template's type value. postMixInProperties was a natural choice since _setTypeAttr couldn't be used.

comment:11 Changed 8 years ago by Douglas Hays

Resolution: fixed
Status: assignedclosed

(In [25144]) Fixes #12116. Force type=text for Date/Time/NumberTextBox?, and associated automated test.

comment:12 Changed 8 years ago by haysmark

(In [25603]) Fix case so test runs from non-Windows servers. Refs #12116.

Note: See TracTickets for help on using tickets.