ValuePickerTimePicker: onValueChanged not called when the AM/PM button is clicked

[Sebastien Brunot]

This is a side effect from the fix for ticket #16502.

It can be reproduced using dojox/mobile/test_ValuePicker-dialog.html: display the test page, click "Use 24-hour format", then click "Set time" to display the time picker dialog. On the time picker, click the AM (or PM, depending on the time of the day you're doing this) button: the time is not updated in the header of the dialog.

[Sebastien Brunot]

working on a patch (fix + doh test).

[Adrian Vasiliu]

Good catch. However, I think the proposed change in ValuePickerTimePicker:

if(this.onValueChanged){ 
  this.onValueChanged(this); 
} 

should better be replaced by:

if(this.onValueChanged){ 
  this.onValueChanged(this.slots[0]); 
} 

The reason being that onValueChanged is defined on ValuePicker (ValuePickerTimePicker's base class) as follows:

onValueChanged: function(/*dojox/mobile/ValuePickerSlot*/slot)

that is it requires a slot argument. To avoid a regression for listeners that may rely on getting a slot argument, let's pass the hour slot instead of the ValuePickerTimePicker instance. (For test_ValuePicker-dialog.html this doesn't matter because the test doesn't use the argument.)

That said, I think the regression in this test after #16502 reveals a deficiency in the design of ValuePickerTimePicker. What happens is the following:

• ValuePickerTimePicker has two slots (hour and minutes) and, if picker's is24h prop. is false, a button to switch AM vs PM. The widget has a values property which is an array of length 2 (hour and minutes values), and also a values12 (sic) property which additionally contains the "AM" or "PM" string.
• ValuePickerTimePicker's click handler for the AM/PM button calls the setter of values12 and this setter calls the setter of values.
• Then, _PickerBase's custom setter of values applies the value to each of the two slots.
• This ends with applying again the same (unchanged) value to the hour and minute slots.
• Now, before #16502, the slots used to notify for value changes even if the value was unchanged... This is why the onValueChanged listener of the test used to be notified - and it was even notified twice for each click on the AM/PM button (once for each slot).
• Since #16502, which aims precisely to avoid "unnecessary" notifications, this doesn't hold anymore, hence the missing update of the dialog title in the test.
• In the longer run (Dojo 2.0...), ValuePickerTimePicker would probably need a deeper redesign. For the time being, the currently suggested fix seems a good compromise in the frame of the existing design.

[Sebastien Brunot]

Fix for #16740: onValueChanged is now called when the AM/PM button is clicked (doh test included). IBM CCLA.

[Sebastien Brunot]

Thanks for you carefull review Adrian, I've updated the patch accordingly (calling this.onValueChanged(this.slots[0]) instead of this.onValueChanged(this)).

[cjolif]

In [30683]:

fixes #16740. onValueChanged is now called when the AM/PM button is clicked (doh test included). Thanks Sebastien Brunot & Adrian Vasiliu (IBM CCLA).

[cjolif]

In [30684]:

fixes #16740. onValueChanged is now called with the right slot. Thanks Sebastien Brunot & Adrian Vasiliu (IBM CCLA).