1389791 - xul timepicker changes pm to am when it shouldn't

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Jonathan Kamens]

1. Instantiate an XUL timepicker object.
2. Change the time in the timepicker so that it reads, e.g., 2:43 PM.
3. Highlight the hour portion of the picker and change the 2 to a 5.
4. Hit tab.
5. Observe that the "PM" changes to "AM".

There's no reason whatsoever why "PM" should change to "AM" here. It's entirely incorrect and nonintuitive behavior.

Comment 1

[Jonathan Kamens]

Attachment: patch to handle 12-hour time hour changes better

Not that I actually expect anybody to put any effort into fixing XUL timepicker bugs :-(, but here's a patch that seems to work.

Comment 2

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Comment on attachment 8896630 [details] [diff] [review]
patch to handle 12-hour time hour changes better

Sorry, not a peer. Neil is busy until September, bz also awayso trying Boris. So trying remaining peer Honza.

Comment 3

[Jonathan Kamens]

Attachment: better patch to address this issue

I missed an edge case in the last patch. I believe this patch gets it right, as well as moving around the logic a little bit to put it where it belongs.

Comment 4

[Tim Nguyen :ntim]

XUL time/datepicker is now unsupported

Comment 5

[Jonathan Kamens]

Moving to Thunderbird since timepicker has moved there.

Comment 6

[Richard Marti (:Paenglab)]

Attachment: datetimepicker.patch

Thank you Jonathan for the patch. I converted the patch into one that can be applied in c-c.

Aceman is much more better for JS reviews than I.

Comment 7

[Richard Marti (:Paenglab)]

Attachment: datepicker-reproducer.xpi

Ace, this is a extension, Jonathan wrote, to show the timepicker.

Comment 8

[:aceman]

Comment on attachment 8960033 [details] [diff] [review]
datetimepicker.patch

Review of attachment 8960033 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the nice test extension.

The patch seems to work for me, thanks.

::: common/bindings/datetimepicker.xml
@@ +470,5 @@
> +            if (aField == this.hourField) {
> +              max = 24;
> +              // User input in the hour field should be adjusted as
> +              // needed for 12-hour vs. 24-hour time.
> +              if (aNoWrap && !this.is24HourClock)

Please add {} for this if().

@@ +472,5 @@
> +              // User input in the hour field should be adjusted as
> +              // needed for 12-hour vs. 24-hour time.
> +              if (aNoWrap && !this.is24HourClock)
> +                if (aValue && aValue < 12 && this.isPM)
> +                  aValue += 12;

It is a bit strange here that you set value above 12 when this is not a 24 hour clock. But that probably comes from the use of this whole function that the resulting value is then used to set the real time value.

Comment 9

[Jonathan Kamens]

Attachment: bug1389791.patch

Added requested curly braces. The reason why we go above 12 in _constrainValue is because that function is working with the internal value for the time, which is always 24-hour format, rather than display value, which is either 12-hour or 24-hour format.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/99d37d6cd81b
timepicker changes pm to am when it shouldn't. r=aceman