Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#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)

ticket16740.patch (4.2 KB) - added by Sebastien Brunot 7 years ago.
Fix for #16740: onValueChanged is now called when the AM/PM button is clicked (doh test included). IBM CCLA.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 7 years ago by Sebastien Brunot

working on a patch (fix + doh test).

comment:2 Changed 7 years ago by Eric Durocher

Cc: Adrian Vasiliu added

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

Changed 7 years ago by Sebastien Brunot

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 7 years ago by 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)).

comment:5 Changed 7 years ago by cjolif

Milestone: tbd1.9

comment:6 Changed 7 years ago by cjolif

Resolution: fixed
Status: newclosed

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

comment:7 Changed 7 years ago by cjolif

In [30684]:

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

Note: See TracTickets for help on using tickets.