Closed Bug 1389791 Opened 5 years ago Closed 4 years ago

xul timepicker changes pm to am when it shouldn't

Categories

(Thunderbird :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 61.0

People

(Reporter: jik, Assigned: jik)

Details

(Keywords: reproducible)

Attachments

(2 files, 3 obsolete files)

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.
Not that I actually expect anybody to put any effort into fixing XUL timepicker bugs :-(, but here's a patch that seems to work.
Attachment #8896630 - Flags: review?(aryx.bugmail)
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.
Attachment #8896630 - Flags: review?(aryx.bugmail) → review?(jvarga)
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.
Attachment #8896630 - Attachment is obsolete: true
Attachment #8896630 - Flags: review?(jvarga)
Attachment #8896640 - Flags: review?(jvarga)
Attachment #8896640 - Flags: review?(jvarga)
XUL time/datepicker is now unsupported
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
Moving to Thunderbird since timepicker has moved there.
Status: RESOLVED → UNCONFIRMED
Component: XUL → General
Ever confirmed: false
OS: Unspecified → All
Product: Core → Thunderbird
Hardware: Unspecified → All
Resolution: WONTFIX → ---
Attachment #8896640 - Flags: review?(richard.marti)
Attached patch datetimepicker.patch (obsolete) — Splinter Review
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.
Attachment #8896640 - Attachment is obsolete: true
Attachment #8896640 - Flags: review?(richard.marti)
Attachment #8960033 - Flags: review?(acelists)
Ace, this is a extension, Jonathan wrote, to show the timepicker.
Assignee: nobody → jik
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
Attachment #8960033 - Flags: review?(acelists) → review+
Attached patch bug1389791.patchSplinter Review
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.
Attachment #8960033 - Attachment is obsolete: true
Attachment #8960920 - Flags: review+
Keywords: checkin-needed
Keywords: reproducible
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/99d37d6cd81b
timepicker changes pm to am when it shouldn't. r=aceman
Status: ASSIGNED → RESOLVED
Closed: 4 years ago4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 61.0
You need to log in before you can comment on or make changes to this bug.