Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#16502 closed defect (fixed)

Change event trouble with SpinWheelDatePicker

Reported by: Paul Christopher Owned by: Eric Durocher
Priority: undecided Milestone: 1.9
Component: DojoX Mobile Version: 1.8.2
Keywords: Cc:
Blocked By: Blocking:

Description

Description

See attached test case: I have encountered several problems when working with the SpinWheelDatePicker widget.

  • You can do SpinWheelDatePicker.set('value', '2012-12-12') and SpinWheelDatePicker.get('value'). However SpinWheelDatePicker.watch('value', function(){...}) does not work.
  • Listening to SpinWheelDatePicker.on('change', function(){...}) does not work either.

You need to listen to onYearSet, onMonthSet, onDaySet. But this is prone to problems, too:

  • Choose the next day. Console output is e.g.
    onDaySet: 23
    

That's correct.

  • Now choose the next month. Console output is e.g.
    onDaySet: 23
    onMonthSet: Jan
    

onDaySet seems to be fired incorrectly always.

  • Now choose the next year. Console output is e.g.
    onDaySet: 23
    onYearSet: 2013
    

Again, changing the year does also give you a change event for the day although the day slot has not been changed.

  • Now click the button "2020-06-06" and clear the console. Click on the button "2001-12-29". Console output is
    onDaySet: 30
    onYearSet: 2001
    onDaySet: 30
    onMonthSet: Dec
    onDaySet: 29
    

The scrolling animation seems to trigger onDaySet events, too. However doing .set('value', '2001-12-29') should result in a single watch('value')-event and/or a single onDaySet event (and not multiple).

Writing your own watch('value') function is not possible since a simple .set('value') event results - due to the scrolling animation? - in multiple watch events with different values, e.g.

Custom watch: 2001-01-30
Custom watch: 2001-01-30
Custom watch: 2001-12-30
Custom watch: 2001-12-30
Custom watch: 2001-12-29

Note: day AND month change their value gradually.

All in all, this makes it really hard for me to work with SpinWheelDatePicker. Doing .set('2001-12-29') should IMHO immediately fire one single watch event right after set has happened and before the scrolling animation has finished, i.e. the scrolling animation caused by .set should not trigger new change events with intermediate values.

Discussion

This might not only be an issue of SpinWheelDatePicker, but also of SpinWheelTimePicker and ValuePicker?

Attachments (4)

testSpinWheel.html (2.8 KB) - added by Paul Christopher 7 years ago.
PatchWatchSpinWheelDatePicker.patch (4.7 KB) - added by Paul Christopher 7 years ago.
testPatchWatchSpinWheelDatePicker.html (3.1 KB) - added by Paul Christopher 7 years ago.
patch16502.patch (37.4 KB) - added by Adrian Vasiliu 7 years ago.
Fixes to avoid unnecessary calls of handlers and watches when the date changes; fixes for user-defined handlers (before this fix, invalid days weren't grayed/removed when a user-defined handler was used); fix unwanted global var in ValuePickerSlot.js; fix spelling of name of temporary private var in SpinWheelSlot.js; cleanup of test_SpinWheelDatePicker.html; added testing code in test_ValuePickerDatePicker.html; added 2 new DOH tests (doh/SpinWheelDatePicker and doh/ValuePickerDatePicker) - Adrian Vasiliu (IBM, CCLA)

Download all attachments as: .zip

Change History (18)

Changed 7 years ago by Paul Christopher

Attachment: testSpinWheel.html added

comment:1 Changed 7 years ago by Paul Christopher

Added a patch which implements a basic watch functionality and prevents firing onDaySet events when the year or month is changed only (CLA on file).

I think the watch functionality is not a nice-to-have-feature but a must for some corner cases. E.g. if you connect to onDaySet and within the custom onDaySet callback you try to get the widgets value by using SpinWheelDatePicker.get('value'), you get an error if you e.g. try to select 31 Feb 2012: "dateObject is null" (line 120, dojo/date/stamp.js).

Simply connecting to watch('value') works around this issue and implements the behaviour most Dojo programmers are used to and are comfortable with: You can "set" or "get" a value and you can "watch" for value changes.

Changed 7 years ago by Paul Christopher

Changed 7 years ago by Paul Christopher

comment:2 Changed 7 years ago by Paul Christopher

Patch seems not to work with 1.8.3 anymore: You need to aspect "slideTo" in "startup" and not "buildRendering" so as to fix the issue.

comment:3 Changed 7 years ago by Paul Christopher

I now had a look at SpinWheelTimePicker. It suffers of the same problems. Maybe even a normal SpinWheel widget with several slots is prone to the same problems, too. Even maybe a ValuePicker widget. So watching value changes needs to be fixed "at the root", i.e. in _PickerBase?

comment:4 Changed 7 years ago by Adrian Vasiliu

Thanks for raising it, there are indeed troubles.

My attached patch16502.patch goes differently than yours while ensuring onDay/Month/YearSet and watching the values of the slots work better. No extra call of onDaySet, as it can be tested using the DOH test that I included in the patch, and also by checking the console messages of dojox/mobile/tests/test_SpinWheelDatePicker.html.

I've also ran your initial test case over the patched lib, and the extra onDaySet is gone.

There might be further improvements that could be done, but I hope these fixes make it usable for you (and others).

comment:5 Changed 7 years ago by Paul Christopher

Adrian, thanks for looking into this. I did not know, that I can watch the value of a specific slot (as introduced by #15825) using watch('value', function()[..}). That is very helpful since the event is fired before the spinwheel start spinning after a set('value', newValue) has happened. Thus it is quite easy to collapse several watch events (of the various slots) into one single watch event by using a simple setTimeout of 10ms.

However with #15825 and your attached patch, I still do have some problems:

1) If the widget's value is 31-Jan-2013 and if I change the month to Feb, the invalid days are grayed out but the day slot is not repositioned back to 28. The used to be the case in the past always.

2) If the widget' value is 29-Feb-2012 (2012 was a leap year) and if I change the year back to 2013, 29 is grayed out but the slot is not repositioned either.

3) If the widget's value is 28-Feb-2013 and if you try to select "31", it gives me a series of watch events (instead of none or just a single one?):

Watch -- Value is: 31
Watch -- Value is: 28
Watch -- Value is: 28

Concerning the first two issues, I think onDaySet's

if(daysInMonth < v[2]){ 
  this.slots[2].set("value", daysInMonth); 
} 

should be moved to _getDaysInMonth. And if that part of the code is moved into _getDaysInMonth, I think there is no need for a separate _disableEndDaysOfMonth?

Concerning the third issue, I do not really know why 28 is fired twice. Maybe since the first time, "28" is a number, the second time "28" is a string (and it should always be a string instead of a number)? Should spinWheelSlots.js stopAnimation really always call _set('value'...), if the value is - strictly speaking - not choosable and thus considered as invalid?

comment:6 Changed 7 years ago by Adrian Vasiliu

Thanks a lot for the testing and feedback, you are right about these issues. I'll come back with a fixed fix...

comment:7 Changed 7 years ago by Adrian Vasiliu

I have reworked and extended the patch to also fix similar issues with ValuePickerDatePicker. In my testing so far, the 3 issues you raised are now gone. The patch includes new DOH tests which check the number of events, whether invalid days of spin wheels are grayed out etc. I hope it will go well in your testing too :-)

comment:8 Changed 7 years ago by Paul Christopher

Adrian, I have just tested again and it works perfectly now! Thanks for fixing this!

comment:9 Changed 7 years ago by Adrian Vasiliu

Thanks again for your very careful testing (and for finding the issues)!

comment:10 Changed 7 years ago by Adrian Vasiliu

(Uploaded the patch again to include a fix in the DOH tests such that they are locale and browser independent; now the tests run also on IE9 and with locales others than EN.)

Changed 7 years ago by Adrian Vasiliu

Attachment: patch16502.patch added

Fixes to avoid unnecessary calls of handlers and watches when the date changes; fixes for user-defined handlers (before this fix, invalid days weren't grayed/removed when a user-defined handler was used); fix unwanted global var in ValuePickerSlot.js; fix spelling of name of temporary private var in SpinWheelSlot.js; cleanup of test_SpinWheelDatePicker.html; added testing code in test_ValuePickerDatePicker.html; added 2 new DOH tests (doh/SpinWheelDatePicker and doh/ValuePickerDatePicker) - Adrian Vasiliu (IBM, CCLA)

comment:11 Changed 7 years ago by cjolif

Milestone: tbd1.9

comment:12 Changed 7 years ago by cjolif

Resolution: fixed
Status: newclosed

fixed in [30431]

comment:13 Changed 7 years ago by Paul Christopher

Thanks for fixing this! It really caused me trouble with SpinWheel.

Not sure - in case you have overlooked it: Could #16518 also be fixed? I have already added a proposal for a fix as well: A new background image and a new CSS gradient. I think it is a bug that is much more perceivable by the user (but quite easy to fix).

comment:14 Changed 7 years ago by Adrian Vasiliu

Be confident :-) I can't promise it will happen very soon, but yes, we will also treat #16518.

Note: See TracTickets for help on using tickets.