onChange event does not fire when reverting value after using priorityChange false

[mattkime]

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.

[mattkime]

ui to demonstrate problem

[mattkime]

patch file which fixes the problem

[mattkime]

doesn't affect just 1.4.2, but all 1.x versions

[dante]

patch reattached as .patch

[bill]

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.

[mattkime]

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.

[Douglas Hays]

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.

[mattkime]

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.

[mattkime]

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.

[bill]

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.

[bill]

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.

[Douglas Hays]

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.

[Douglas Hays]

(In [22600]) Fixes #10988. Changed the onChange processing algorithm: fire onChange on priorityChange=true (or intermediateChanges=true) if there was an actual change in the value at anytime since the last onChange. Added additional automated tests.

[bill]

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:

0. initial value=A
1. set value=A, priority=true
2. set value=B, priority=false
3. set value=A, priority=false
4. set value=A, priority=true

IOW, fire onChange (only) when the value changes and priorityChange is true.

[Douglas Hays]

(In [22606]) References #10988. Change the onChange behavior for the 1 scenario in which the intermediate value (priorityChange=false) changes but then changes back to the _lastValueReported, squelching the pending onChange

[bill]

[22606] is causing failures in !ComboBox_mouse.html on IE8,

	expected
		3
	but got
		2

 with hint: 
		# custom labelFunc calls

failures in the "select value" and "blur value" tests. This is in addition to the issues I listed in #10946 about [22557].

[bill]

Also in Slider_a11y.html (tested on FF3.6), about the HOME key.

[bill]

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.

[Douglas Hays]

Tests are failing due to this scenario:

1. initial value=A
2. set value=B, priority=false
3. 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.

[bill]

OK, sorry, I didn't realize that would cause so many issues, let's go back to the previous version, [22600].

[Douglas Hays]

(In [22639]) Fixes #10988. Rollback to [22600] and add an additional onChange scenario test. Fix helpers::isVisible to return true if any part of the node is showing since sometimes a ComboBox? menuitem was only partially visible due to random screen sizing.