#13339 closed defect (fixed)
FilteringSelect: logic error in _openResultList breaks paging results
Reported by: | ben hockey | Owned by: | |
---|---|---|---|
Priority: | high | Milestone: | 1.7 |
Component: | DojoX MVC | Version: | 1.7.0b1 |
Keywords: | Cc: | [email protected]…, Douglas Hays | |
Blocked By: | Blocking: |
Description (last modified by )
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
Attachments (2)
Change History (22)
comment:1 Changed 10 years ago by
Description: | modified (diff) |
---|---|
Milestone: | tbd → 1.7 |
Owner: | changed from Douglas Hays to bill |
Summary: | dijit.form.FilteringSelect - logic error in _openResultList breaks paging results → FilteringSelect: logic error in _openResultList breaks paging results |
comment:2 Changed 10 years ago by
Cc: | [email protected]… added |
---|---|
Component: | Dijit - Form → DojoX MVC |
Owner: | changed from bill to ben hockey |
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.
comment:3 Changed 10 years ago by
Owner: | changed from ben hockey to rahul |
---|
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.
comment:4 Changed 10 years ago by
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.
comment:5 Changed 10 years ago by
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'
.
Changed 10 years ago by
Attachment: | 13339.html added |
---|
comment:6 Changed 10 years ago by
Component: | DojoX MVC → Dijit - Form |
---|---|
Owner: | changed from rahul to bill |
comment:7 follow-up: 9 Changed 10 years ago by
Cc: | Douglas Hays added |
---|
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?
comment:8 Changed 10 years ago by
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().
comment:9 Changed 10 years ago by
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?
comment:10 Changed 10 years ago by
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.
Changed 10 years ago by
Attachment: | 13339onChange.html added |
---|
no onchange event while user is typing (put in top level directory next to dojo/ and dijit/)
comment:11 Changed 10 years ago by
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.
comment:12 Changed 10 years ago by
Agree on maintaining parity between onChange and watch callbacks at all times.
comment:13 follow-up: 14 Changed 10 years ago by
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.
comment:14 Changed 10 years ago by
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 252 252 binding.watch("value", function (name, old, current){ 253 253 if(old === current){return;} 254 if(pThis.get('value') === current){return;} 254 255 pThis.set("value", current); 255 256 }), 256 257 // 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?
comment:15 Changed 10 years ago by
Component: | Dijit - Form → DojoX MVC |
---|---|
Owner: | bill deleted |
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.
comment:16 Changed 10 years ago by
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.
comment:18 Changed 10 years ago by
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.
comment:19 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:20 Changed 10 years ago by
thanks ed - let me know if there are any problems. i'll fix them next week.
Oh, sounds like I broke this w/the refactor to be able to use dojo.store. But I don't understand your statement that:
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.