Opened 11 years ago
Closed 10 years ago
#10988 closed defect (fixed)
onChange event does not fire when reverting value after using priorityChange false
Reported by: | mattkime | Owned by: | Douglas Hays |
---|---|---|---|
Priority: | high | Milestone: | 1.6 |
Component: | Dijit | Version: | 1.4.2 |
Keywords: | Cc: | ||
Blocked By: | Blocking: |
Description (last modified by )
onChange does not get called in this specific circumstance -
1) set a value on a form widget - .attr("value", valueA);
2) set the value without priorityChange - .attr("value", valueB, false);
3) revert the value with priorityChange - .attr("value", valueA);
The onChange event will fire for #1
(as it should), fail to fire for #2
(as it should), and fail to fire for #3
(as it SHOULDN'T)
The reason is because the _lastValueReported property of the widget object still has valueA after valueB has been set without priority change. If it had been set to valueC in step #3
, it would have fired onChange as expected.
Attachments (3)
Change History (21)
Changed 11 years ago by
Attachment: | priorityChange.html added |
---|
comment:2 Changed 11 years ago by
Description: | modified (diff) |
---|---|
Owner: | set to Douglas Hays |
Priority: | highest → normal |
severity: | critical → normal |
This is expected behavior as explained in #10466, basically so that onChange() isn't called twice w/the same value. (And after all, a variable called _lastValueReported should contain the last value that was _reported_.) You can get the behavior you want by resetting _lastValueReported after a call to attr("value", foo, false)
.
Passing to Doug, I think we want to close this as wontfix but maybe he can add more info. I think there might have been some internal reason we don't do this too, but not sure.
comment:3 Changed 11 years ago by
I am working from an understanding of priorityChange referenced here - http://docs.dojocampus.org/releasenotes/1.4
attr() (as a setter) has been enhanced to take optional arguments. The most common case for this is attr("value", val, false) which sets the value of a form widget without calling onChange(). (The third argument is called priorityChange.)
To summarize - If the value changes, onChange fires, unless priorityChange is false.
---
I reference _lastReportedValue only to understand the inner workings of priorityChange and am not concerned with it at all otherwise. Perhaps its more clear if we focus only on value changes and events -
"Curious behavior 1" dijit field value is 0 set value 1, priorityChange false (no onChange, expected) set value 0, priorityChange true (value has been changed, but no event fired)
"Curious behavior 2" dijit field value is 0 set value 1, priorityChange false (no onChange, expected) set value 1, priorityChange true (onChange fired although the value wasn't changed)
one thing to note is that whether the onChange event is fired is dependent upon whether it matches the last priorityChange value that the field had. I can understand that this is expected behavior after examining the code (after all, thats what it does) but its extremely unexpected, particularly when you reread the doc I quoted at the top.
If this is expected behavior, can you describe a beneficial use case?
While the problem does look similar to #10466, judging by the fix its completely different. Looking back through the svn history on the _FormWidget.js file, this behavior has remained unchanged since _handleOnChange was first implemented, back in the 1.0ish days.
comment:4 follow-up: 5 Changed 11 years ago by
The _handleOnChange function checks the last reported value so that the onChange handler (presumably connecting to a server) causes minimal network traffic. So if an intermediate value that wasn't reported to the server is esentially thrown away, then the server doesn't need to do any expensive operations like database lookups. There's an even more "curious" scenario,
value=A:priority=true, value=B:priority=false, value=A:priority:false, value=A:priority=true.[[BR]]
I'm not sure if you think this scenario should fire onChange or not on the last set value.
Also it seems like the attached patch only sets _lastValueReported if intermediateChanges=true which is the more rare scenario.
comment:5 Changed 11 years ago by
I'm not familiar with the scenario where onChange causes network traffic / server hits. Can you elaborate?
From my current understanding of the onChange event and priorityChange, no onChange event should fire because the value has not been changed with priorityChange true.
My patch preserved the relationship between intermediateChanges and setting _lastValueReported because I don't understand the purpose of the intermediateChanges variable.
Can I get confirmation that the behavior does not match the documentation? We could agree on how it should behave and then discuss how to make that happen while satisfying existing needs.
Many thanks.
comment:6 Changed 11 years ago by
It looks like the filteringSelect requires this behavior. Changing the value of a filteringSelect sets the value twice, once with priorityChange false and once with priorityChange true. I don't understand why this happens, but it does mean that the second time the value is set it won't fire an event because the value hasn't changed.
comment:7 Changed 11 years ago by
The documentation definitely needs to be updated, it's inexact, as you said.
IIRC priorityChange and intermediateChanges were mainly meant for actions like setting a slider value (by dragging the handle), whether Slider fires onChange() events during the drag (mousemove), or just at the end of the drag (mouse up). If each onChange() was reported to the server then of course that difference will affect network traffic.
comment:8 Changed 11 years ago by
Stepping back, it's not that the priorityChange is broken, it's that people want a different flag, maybe called "reportChange". The priorityChange change flag is basically an implementation trick to make the intermediateChanges flag work. It's not useful to developers using dojo.
Simply, people want (and expect):
- widget.set("value", val, false) - sets value but doesn't call onChange()
- widget.set("value", val, true) - sets value, calls onChange()
- widget.set("value", val) - wish this didn't call onChange() but it should, for back-compat
- user changes widget's value w/mouse or keyboard - calls onChange()
Changing the API to work like that is complicated because of implementation details, like the FilteringSelect problem. Otherwise, I think it's possible. That nominal API change wouldn't upset anyone because no one really understands what priorityChange means anyway. We could convert the API to:
_setValueAttr: function(value, reportChange, priorityChange)
...moving priorityChange as a third parameter that would never be specified, except internally.
As an aside, I wish that intermediateChanges=false flag simply meant that the value of the widget (ex: Slider) didn't change on mouse-move, but only on mouse-up. Instead intermediateChanges=false means that mouse-move changes the widget's value, but the change isn't reported until mouse-up. That is probably complicating things.
comment:9 Changed 11 years ago by
Milestone: | tbd → 1.6 |
---|
Deferring to 1.6 to give everyone time to think of the best solution. I'm currently leaning towards allowing consecutive onChange events to fire with the same value if there were 1 or more unreported intermediate value changes.
comment:10 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:11 Changed 10 years ago by
I'd like this to be changed a little bit, so that from in the onChange doesn't fire in the scenario from Doug's comment 4:
- initial value=A
- set value=A, priority=true
- set value=B, priority=false
- set value=A, priority=false
- set value=A, priority=true
IOW, fire onChange (only) when the value changes and priorityChange is true.
comment:12 Changed 10 years ago by
comment:13 Changed 10 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
comment:15 Changed 10 years ago by
I'm also getting an error in Spinner_mouse.html, in the spinner2_max test (reporting onchange of 1549 instead of 1550).
There's another behavior with spinner that I didn't anticipate with this change. After spinning the value (via up/down buttons) and then blurring the widget, there's no onChange event. Not sure how to handle that it seems like people will be expecting an onChange event in that case.
comment:16 Changed 10 years ago by
Tests are failing due to this scenario:
- initial value=A
- set value=B, priority=false
- set value=B, priority=true
When priorityChange==true, the value did not change since the previous value and thus no onChange was fired, which used to be covered by the _pendingOnChange boolean. This scenario seems to contradict the requested behavior in comment:11.
comment:17 Changed 10 years ago by
OK, sorry, I didn't realize that would cause so many issues, let's go back to the previous version, [22600].
comment:18 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
ui to demonstrate problem