FilteringSelect: logic error in _openResultList breaks paging results

[ben hockey]

in _openResultList in dijit.form.FilteringSelect, there is a check

if(query[this.searchAttr] !== this._lastQuery){
        return;
}

in _setValueAttr this._lastQuery is set to value where value should be a String.

this._lastQuery = value;

in _startSearch of _AutoCompleterMixin this._lastQuery and query[this.searchAttr] are both set to a RegExp?

this._lastQuery = query[this.searchAttr] = q;

if the _setValueAttr is called while waiting for resPromise in _AutoCompleterMixin to resolve then the call to _openResultList in the callback for resPromise will fail to update the result list

_this._openResultList(res, query, options);

when using dojox/mvc, this race condition can be seen. i'm attaching a test case to demonstrate. in the test case, when the page loads, highlight the text currently in the FilteringSelect? and start typing 't'. you'll be presented with a dropdown with an option for more choices. select more choices - you'll see that the next page does not appear. NOTE: if you try this a 2nd time it will work.

dojox/mvc exposes this problem but i believe its correct to say that the problem is with dijit

[bill]

Oh, sounds like I broke this w/the refactor to be able to use dojo.store. But I don't understand your statement that:

if the _setValueAttr is called while waiting for resPromise in _AutoCompleterMixin to resolve then the call to _openResultList in the callback for resPromise will fail to update the result list

Isn't that correct behavior? If somebody calls myComboBox.set("value", ...) it should effectively cancel any previous requests.

TBH the whole _lastQuery thing seems like a hack, I think that any new requests should just cancel any previous requests, but still I don't understand what bug you are describing.

[Douglas Hays]

Calling widget.set('value', blah) causes FilteringSelect? to do a store lookup to get the item value. This cancels the search that was in progress whose results will arrive asynchronously at some time in the future, thus the code is working more or less correctly. The correct behavior would be to close the popdown since the search was canceled. But this would not be what MVC wants to happen. So I'll defer fixing that until MVC figures out how it wants to handle this so that the popup is not closed. Note that the item object could have been specified on the widget.set call as an optional parameter and then not interfere with the search operation.

[ben hockey]

i see... i agree that setting the value while waiting for a page of search results should be considered canceling the search and result in closing the dropDown. it would make sense to me to make the FilteringSelect? behave like this.

however, mvc is only setting the value in response to watching the value of the FilteringSelect?. is there a way to prevent the FilteringSelect? from changing its own value every time a different option is suggested from the dropDown and only change the value (triggering a call to watchers) once an option is selected? i think that would also prevent this from happening.

btw, if this is an mvc issue i think those are meant to be assigned to rahul.

[bill]

Are you saying that FilteringSelect calls the watch() call as the user is typing the value, and auto-completion triggers, like in the second example in test_FilteringSelect.html? I setup watch("value", ...) to fire only when onChange() fires, and (at least in that example) onChange() only fires when the user actually selects a value.

[ben hockey]

yes - the watch is called as the displayed value changes.

i'll update the example to have a watch that logs to the console. in the example, the original displayed value is 'one'. if you select all the text and then start typing 't', the watch is called and 'two' is the suggested option. if you then type 'w', the list will be reduced to 'two' and 'twelve' but no watch callbacks are called this time because the suggested option is still 'two'. if you type 'e' then you will see watch callbacks fired again because the suggested option changes to 'twelve'.

[bill]

Hmm, well _handleOnChange() is getting called with priorityChange==false upon auto completion, and that's causing the watch() notification. Or to put it another way, when intermediateChanges==true, onChange() is called for auto completion.

I guess that's correct behavior. I could make the watch() notifications only occur on major changes (priorityChange==true), or make the notification obey the intermediateChanges flag. Not sure what's best?

[bill]

PS: I think originally I had thought that individual watchers could say what kind of updates they wanted, or at least ignore updates they didn't want, ex:

// get updates as the user drags
mySlider.watch("value", callback) 

// only get updates on mouse up
myCombo.watch("value", callback, true)

or at least have the info available in the callback:

function callback(value, priorityChange){
  if(priorityChange) console.log("mouse up, value: " + value);
  else console.log("mouse move, value: " + value);

Never got around to implementing it though. It would require overriding Stateful.watch().

[ben hockey]

Replying to bill:

Hmm, well _handleOnChange() is getting called with priorityChange==false upon auto completion, and that's causing the watch() notification. Or to put it another way, when intermediateChanges==true, onChange() is called for auto completion.

maybe i'm misunderstanding the comment but currently even with intermediateChanges as false (the default) i'm observing watch and onChange notifications for each change in suggested auto completion. is that expected?

in fact, in the example attached to this ticket i don't see any difference in behavior for watch or onChange notifications by changing the value for intermediateChanges and repeating the steps i've outlined in comment:5. can you confirm that you're observing this same behavior?

[bill]

I don't see onChange() notifications while the user is typing; presumably I would if I set intermediateChanges=true. Might be easier to test this w/a TextBox first, and then move on to ComboBox.

[bill]

no onchange event while user is typing (put in top level directory next to dojo/ and dijit/)

[ben hockey]

as per our conversation on irc...

if you add back the dojox/mvc dependency you'll see onChange firing. however, it seems onChange is actually being fired in response to mvc updating the widget's value (which happens in response to the widget's watch callback firing). so there seems to be no problem about how onChange is firing. this just leaves the question about which conditions should cause the watch callbacks to be fired.

my preference is to have the firing of watch callbacks observe the intermediateChanges flag in a similar way to onChange.

i wouldn't like to change the watch API to deviate from Stateful's watch. i don't think it would be good to have something like dojox/mvc need to differentiate between a Widget and a plain Stateful object when using watch.

[rahul]

Agree on maintaining parity between onChange and watch callbacks at all times.

[Ed Chatelain]

Doug was asking me why dojox/mvc was calling set("value",...) on the widget when the update originated from the widget, the short answer is because the value set on the widget does not match the value sent in the watch. MVC is setting up a watch for the "value" on the widget, and the "value" on the bound dojo.stateful. When the widget value is updated the watch is fired and the bound dojo.stateful is updated, and when the bound dojo.stateful is updated the watch is fired and the watch will update the widget's value, if the widget's value does not match the new value. So in this case starting out the widget and the bound dojo.stateful have a value of 4, when "t" is typed a watch is fired with a new value of 2, so we update the bound dojo.stateful value to 2, which fires the watch for the bound dojo.stateful, which sees that the widget value is 4, and the new value is 2, so it updates the widget value to 2.

[ben hockey]

Replying to edchat:

Doug was asking me why dojox/mvc was calling set("value",...) on the widget when the update originated from the widget, the short answer is because the value set on the widget does not match the value sent in the watch.

there is no check for a matching value. this diff adds one and fixes the problem.

_DataBindingMixin.js

@@ -251 +251 @@
                                         // 1. value - no default
                                         binding.watch("value", function (name, old, current){
                                                 if(old === current){return;}
+                                                if(pThis.get('value') === current){return;}
                                                 pThis.set("value", current);
                                         }),
                                         // 2. valid - default "true"

MVC is setting up a watch for the "value" on the widget, and the "value" on the bound dojo.stateful. When the widget value is updated the watch is fired and the bound dojo.stateful is updated, and when the bound dojo.stateful is updated the watch is fired and the watch will update the widget's value, if the widget's value does not match the new value.

not quite right... there is no check to see if the value matches the widget's value. see above

So in this case starting out the widget and the bound dojo.stateful have a value of 4, when "t" is typed a watch is fired with a new value of 2, so we update the bound dojo.stateful value to 2, which fires the watch for the bound dojo.stateful, which sees that the widget value is 4, and the new value is 2, so it updates the widget value to 2.

again... not quite right, there is no check for the widget's value. see above again.

if i'm wrong and there is in fact a check for the widget's value then i'll apologize in advance but please show me what i've missed.

going forward if we can maintain the following contract and mvc adds a check for the current value of the widget before setting the value (the diff above) then i think we can avoid setting the widget's value when its not needed.

the contract: at the time of initiating the 'value' watch callbacks the value of the widget reported by widget.get('value') MUST match the value passed to the watch callback as the current value of the widget.

of course, there's no way to prevent one of the watch callbacks from doing something that would change the value reported by widget.get('value') but it should at least start out that way before the first callback is called. i don't know if any widgets currently exist where this is not already the case but can we agree that this contract is a reasonable assumption for mvc to use and consider it a defect where it is not the case in dijit?

[bill]

I guess that we can assume dijit sets this.value before calling the watch() callback. That would need to be tested of course, maybe there was a reason that it did things in the opposite order.

Generally speaking it's silly to even call get("value") when watch() just told you what the value is, but I suppose it makes things simpler for the MVC code.

[ben hockey]

the get('value') is not in the watch callback for the widget's change. it's in the callback of the watch for the bound model. it's needed because there are 2 cases that need to be supported in the watch callback of the bound model:

• something other than the widget changes the value in the bound model. this leads to needing to update the value of the widget.
• a change in the value of the widget fires the watch for the widget and this sets the value in the bound model. this triggers the watch callback for the bound model which should not update the widget in this case.

the watch callback on the bound model needs to get the value of the widget and compare it with the value that was just set on the model and only update the widget if they are not equal. this is how it determines the source of the change in value on the model. as long as the value reported by widget.get('value') matches the value passed to the watch callback then it will continue to work. hence the contract outlined above.

[Ed Chatelain]

Ben, you are right. I think you should commit your fix.

[Ed Chatelain]

By the way, I ran the full set of MVC tests with your fix, and all passed, and I will add a new robot test to cover this case next week.

[ben hockey]

(In [25748]) add check to make sure widget's value is not updated when it is the source of the value change on the binding. fixes #13339 !strict

[ben hockey]

thanks ed - let me know if there are any problems. i'll fix them next week.