[patch] [cla] HorizontalSlider fires onChange for every arrow key increment/decrement

[Mark Wubben]

If I focus the slider handle in a HorizontalSlider and use the arrow keys to increment or decrement the slider, an onChange event is fired every time I press or hold the arrow key. IMO this causes the onChange to be fired way too often.

I've attached a patch that ensures onChange fires only if there has been no new arrow key event in the past 200ms. intermediateChanges can still override this behaviour.

[bill]

Talked to Doug about this.

When dragging the slider handle with intermediateChanges=false, an onChange event only occurs at the end of the drag operation, on mouse up, not on mouse move.

So, we can see the desire to have an equivalent way to get that effect when controlling the slider with the keyboard, or when pressing the up/down arrow icons.

However, rather than using a timer, it seems like onChange should only fire on key-up (when controlling via keyboard) or mouse-up (when pressing the arrow icons). That seems analogous to the way dragging the handle works (where onChange() fires on mouse up). It probably amounts to more-or-less the same behavior from the user's perspective, since users are unlikely (or perhaps incapable) of pressing a key twice in 200ms anyway.

On a semi-related note, we also talked about collapsing multiple onChange() notifications that occur very close together to prevent browser overload. This would be done just by deferring onChange notifications w/a setTimeout() of 0ms, and then collapsing multiple onChange notifications if they occur before the browser fires that setTimouet(). That seems like a reasonable change to me too, but unrelated to this discussion as it would probably only take effect when intermediateChanges=true.

[Douglas Hays]

(In [18972]) Fixes #9566. References #9531. Call onChange within a setTimeout/0 to allow processing to quiesce. Collapse rapid onChange calls to prevent browser overload.

[Douglas Hays]

proposed fix - needs testing verification

[Mark Wubben]

Works, as far as I can tell.

There is, however, a potential compatibility issue. I have subclassed HorizontalSlider?, and was calling a custom init() method after setting its value. The onChange() handler would check if the slider had initialized, because it needed to ignore the first value set. With this patch, onChange is fired in a new callstack, so the slider will have been initialized, so it didn't ignore the first value set. Any other widget relying on a synchronous execution of onChange will fail in a similar way.

[Douglas Hays]

We probably need to iron out if the asynchronous onChange is overall a good or bad thing before committing this patch. Normally, initial values are established during widget creation and these are automatically filtered from calling onChange. It sounds like you have some private initialization that runs after create() that changes the value.

[Mark Wubben]

Hang on, is the patch different from what you committed?

The slider is part of another widget, and right now it's being created from the widget template, rather than programatically. So yes, I set its value after it's started, after which I call another init method. My code is probably just bad, but other code might be bad as well.

[Douglas Hays]

The 9531.patch has not been committed yet. The asynchonous onChange was committed as part of another ticket. Hopefully you can initialize the widget value in the markup and avoid the extra onChange.

[Douglas Hays]

patch using new optional boolean to _setValueAttr to stop onChange after attr('value',val,false)

[Douglas Hays]

[19830] Fixes #9531 !strict. Adds ability to passthru extra parameters to this.attr setter construct allowing _setValueAttr to get the priorityChange=false parameter which effectively removes onChange events except when intermediateChanges=true.

[Douglas Hays]

Fixed by [19839], not revision 19830.