Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#17955 closed defect (fixed)

NumberTextBox places incorrect value in field on blur in a specific scenario

Reported by: Terence Kent Owned by: Bill Keese <bill@…>
Priority: undecided Milestone: 1.10
Component: Dijit - Form Version: 1.9.3
Keywords: Cc:
Blocked By: Blocking:

Description

I've run into an extremely specific issue with NumberTextBox? fields where the value placed into the field on blur is not valid when a fixed width constraint pattern is used.

The issue is very easy to demonstrate and I've created an example to reproduce the issue here:

http://jsfiddle.net/terencekent/jhPe6/

(Side note: I don't seem to have the ability to associate a priority with this issue, but if I could, I would mark it as low priority because I assume I one of only a handful of people who will ever experience it)

Change History (17)

comment:1 Changed 5 years ago by Terence Kent

If it's likely to be accepted, I'm happy to submit a patch for this via pull request. I can't imagine anybody else would want to spend time on something this specific.

comment:2 Changed 5 years ago by bill

Sure, I'd be happy to take a patch for that. Would definitely be nice to fix. I wonder if the problem is isolated to NumberTextBox? (and descendants) or if it can occur with plain ValidationTextBox? (or plain RangeBoundTextBox?).

comment:3 Changed 5 years ago by Terence Kent

Technically, all text boxes could be effected by this type of issue. The root of the issue is in dijit/form/_TextBoxMixin.js where it grabs a filtered value and applies it, without making sure the filtered value parses into the passed value.

I'll place my patch there unless I can find a reason not too.

comment:4 Changed 5 years ago by bill

Thanks. FYI, when Doug was working on this, he was really adamant about supporting "round trip" (as he called it), where myWidget.set(get("value")) would never change the value nor the displayedValue, even if a NumberTextBox? had some nonsense displayedValue like "foo". So I'm surprised we have this bug. Anyway, something to keep in mind.

comment:5 Changed 5 years ago by Terence Kent

@Bill

It is fairly straightforward to change the code in dijit/form/_TextBoxMixin to support the "round trip" functionality you mentioned. It involves making sure the filtered versions of the 'value' and the parsed 'formattedValue' are synonyms (via the compare method).

However, there are some complications I'd like to bring up and explain my approach to before submitting a pull request.

(If this is the wrong place to have this type of discussion, please point me to the right one. It didn't seem like the developer mailing list was quite right based on Dylans note to me)

1. Following the "round trip" logic exactly would break rounding in dijit/form/NumberTextBoxes

For example, if a constraint pattern has a decimal precision of 3 (e.g. #.###), and a numeric value of 1.1999 is entered, it will be round to 1.2. Strictly speaking (and based on the current code as well), that would break the "round trip". I believe that allowing rounding to break the "round trip" requirement this case is the right thing to do. My patch currently includes support for this by updating the NumberTextBoxMixin?'s filter() method.

2. The filter() method looks like it will be changed in 2.0

The heart of making this patch reliable is using the parse() and filter() methods against the formatted value. However, the filter method has a comment that it will be changed in 2.0, and broken out into two separate methods. I assume it is ok to rely on this method until it actually is retired, since it's a fundamental widget component.

comment:6 Changed 5 years ago by bill

If this is the wrong place to have this type of discussion, please point me to the right one. It didn't seem like the developer mailing list was quite right based on Dylans note to me

Well we used to try to keep tickets short and put discussion in the mailing list, or lately into github, but I don't have religious feelings about it.

  1. Following the "round trip" logic exactly would break rounding in dijit/form/NumberTextBoxes

I see your argument. You should check your change carefully though, including running all the regression tests. My fear is that .set("value", get("value")) gets called at weird times that you don't expect. I don't want a TextBox?'s displayed value to suddenly change as the user is typing it in, or to change because the user pressed the "submit" button on a dijit/form/Form, etc.

What's the desirable behavior when a user types in 1.1999 and then tabs away? Should the value be changed to 1.2, or would users find that incredibly annoying? Note the user might have accidentally put the decimal point in the wrong place, so we should think twice before erasing data the user entered.

Basically I'm just saying that we need to be careful.

  1. The filter() method looks like it will be changed in 2.0

I would love to see that broken into two separate methods but yes, it's OK to rely on that method until it's actually retired. BTW, if you ever get the urge to clean up the TextBox? code too (doing that split work yourself), that would be great too.

comment:7 Changed 5 years ago by Terence Kent

Well we used to try to keep tickets short and put discussion in the mailing list, or lately into github, but I don't have religious feelings about it.

Ok, for future reference I'll use the mailing list or github. I'll keep this conversation in the issue though, so it's all in one place.

Should the value be changed to 1.2, or would users find that incredibly annoying? Note the user might have accidentally put the decimal point in the wrong place, so we should think twice before erasing data the user entered.

It's definitely a tough one and I can see good reasons for going either way with it. There is an existing implied decision on this point, either intentionally or not, in the unit test "rouding:simple" in digit/tests/form/robot/Spinner_a11y.html. This test makes sure that entering a numeric value with additional precision is rounded on blur.

At this point, I assume it's best if I submit a pull request with rounding allowed, and we can remove that part if necessary.


BTW, if you ever get the urge to clean up the TextBox? code too (doing that split work yourself), that would be great too.

If I have time, absolutely :-). Our projects are picking up here, so I doubt I'll be too active for next few months.

comment:8 Changed 5 years ago by bill

There is an existing implied decision on this point, either intentionally or not, in the unit test "rouding:simple" in digit/tests/form/robot/Spinner_a11y.html.

Good point, in that case let's stick with the precedent. So I"m OK with your proposed behavior. Just as long as it doesn't convert "a1000" into "" or NaN etc.

comment:9 Changed 5 years ago by Bill Keese <bill@…>

Owner: set to Bill Keese <bill@…>
Resolution: fixed
Status: newclosed

In f018c601b407592956dd56cd41a5edfe332d55d2/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:10 Changed 5 years ago by Terence Kent

@wkeese

Can you please re-open this issue? @edchat pointed out a rookie mistake on my part which was causing him some trouble. I've patched the issue and created a pull request for the fix.

Sorry about the hassle!

comment:11 Changed 5 years ago by bill

Milestone: tbd1.10

No worries, I will check in your patch now (so no need to reopen the issue).

comment:12 Changed 5 years ago by Bill Keese <bill@…>

In 2d7c8919fddfe55f0e39a6c150c5758fb867e5f2/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:13 Changed 5 years ago by Bill Keese <bill@…>

In ba88f8e643398cd6b7c086847c346909ff942223/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:14 Changed 5 years ago by Bill Keese <bill@…>

In d38b0bd2e94af706b27aff49779e706b1fb903bf/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:15 Changed 5 years ago by Bill Keese <bill@…>

In 00568aa6e4f319e1440918e6bd79e36bbbc7046e/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:15 Changed 5 years ago by Bill Keese <bill@…>

In eb4d66a779f5a044c011c7f24d6f4641ba00ef74/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:16 Changed 5 years ago by Lukas Kubasek

Commit f018c601b407592956dd56cd41a5edfe332d55d2 and changes in the filter() method fixing ​this issue have introduced a bug for the range based 'places' constraint definition (NumberTextBox? using e.g. constraints:{places:'0,4'} ). I have raised a new ticket #18367 addressing that issue.

Note: See TracTickets for help on using tickets.