#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 )
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)
Change History (13)
Changed 12 years ago by
Attachment: | HorizontalSlider.js.patch added |
---|
comment:1 Changed 12 years ago by
Milestone: | tbd → 1.4 |
---|---|
Owner: | set to Douglas Hays |
comment:2 Changed 12 years ago by
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 |
comment:3 Changed 12 years ago by
comment:4 Changed 12 years ago by
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 12 years ago by
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 12 years ago by
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 12 years ago by
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 12 years ago by
Attachment: | 9531.2.patch added |
---|
patch using new optional boolean to _setValueAttr to stop onChange after attr('value',val,false)
comment:8 Changed 12 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:10 Changed 10 years ago by
Component: | Dijit → Dijit - Form |
---|
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.