#16740 closed defect (fixed)
ValuePickerTimePicker: onValueChanged not called when the AM/PM button is clicked
Reported by: | Sebastien Brunot | Owned by: | Eric Durocher |
---|---|---|---|
Priority: | undecided | Milestone: | 1.9 |
Component: | DojoX Mobile | Version: | 1.9.0a1 |
Keywords: | Cc: | Adrian Vasiliu | |
Blocked By: | Blocking: |
Description
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.
Attachments (1)
Change History (8)
comment:1 Changed 8 years ago by
comment:2 Changed 8 years ago by
Cc: | Adrian Vasiliu added |
---|
comment:3 Changed 8 years ago by
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.
Changed 8 years ago by
Attachment: | ticket16740.patch added |
---|
Fix for #16740: onValueChanged is now called when the AM/PM button is clicked (doh test included). IBM CCLA.
comment:4 Changed 8 years ago by
Thanks for you carefull review Adrian, I've updated the patch accordingly (calling this.onValueChanged(this.slots[0]) instead of this.onValueChanged(this)).
comment:5 Changed 8 years ago by
Milestone: | tbd → 1.9 |
---|
working on a patch (fix + doh test).