Opened 10 years ago

Closed 10 years ago

Last modified 8 years ago

#9531 closed defect (fixed)

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

Reported by: Mark Wubben Owned by: Douglas Hays
Priority: high Milestone: 1.4
Component: Dijit - Form Version: 1.3.1
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by bill)

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.

Attachments (3)

HorizontalSlider.js.patch (2.0 KB) - added by Mark Wubben 10 years ago.
9531.patch (3.9 KB) - added by Douglas Hays 10 years ago.
proposed fix - needs testing verification
9531.2.patch (11.5 KB) - added by Douglas Hays 10 years ago.
patch using new optional boolean to _setValueAttr to stop onChange after attr('value',val,false)

Download all attachments as: .zip

Change History (13)

Changed 10 years ago by Mark Wubben

Attachment: HorizontalSlider.js.patch added

comment:1 Changed 10 years ago by Adam Peller

Milestone: tbd1.4
Owner: set to Douglas Hays

comment:2 Changed 10 years ago by bill

Description: modified (diff)
Summary: [cla][patch] HorizontalSlider fires onChange for every arrow key increment/decrement[patch] [cla] HorizontalSlider fires onChange for every arrow key increment/decrement

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.

comment:3 Changed 10 years ago by 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.

Changed 10 years ago by Douglas Hays

Attachment: 9531.patch added

proposed fix - needs testing verification

comment:4 Changed 10 years ago by 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.

comment:5 Changed 10 years ago by 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.

comment:6 Changed 10 years ago by 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.

comment:7 Changed 10 years ago by 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.

Changed 10 years ago by Douglas Hays

Attachment: 9531.2.patch added

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

comment:8 Changed 10 years ago by Douglas Hays

Resolution: fixed
Status: newclosed

[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.

comment:9 Changed 10 years ago by Douglas Hays

Fixed by [19839], not revision 19830.

comment:10 Changed 8 years ago by bill

Component: DijitDijit - Form
Note: See TracTickets for help on using tickets.