Closed
Bug 1389791
Opened 7 years ago
Closed 7 years ago
xul timepicker changes pm to am when it shouldn't
Categories
(Thunderbird :: General, enhancement)
Thunderbird
General
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 61.0
People
(Reporter: 52qtuqm9, Assigned: 52qtuqm9)
Details
(Keywords: reproducible)
Attachments
(2 files, 3 obsolete files)
2.38 KB,
application/x-xpinstall
|
Details | |
3.10 KB,
patch
|
52qtuqm9
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
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 2•7 years ago
|
||
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)
Assignee | ||
Comment 3•7 years ago
|
||
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)
Updated•7 years ago
|
status-firefox58:
--- → wontfix
status-firefox59:
--- → fix-optional
Updated•7 years ago
|
Attachment #8896640 -
Flags: review?(jvarga)
Comment 4•7 years ago
|
||
XUL time/datepicker is now unsupported
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Assignee | ||
Comment 5•7 years ago
|
||
Moving to Thunderbird since timepicker has moved there.
Status: RESOLVED → UNCONFIRMED
status-firefox57:
affected → ---
status-firefox58:
wontfix → ---
status-firefox59:
fix-optional → ---
Component: XUL → General
Ever confirmed: false
OS: Unspecified → All
Product: Core → Thunderbird
Hardware: Unspecified → All
Resolution: WONTFIX → ---
Assignee | ||
Updated•7 years ago
|
Attachment #8896640 -
Flags: review?(richard.marti)
Comment 6•7 years ago
|
||
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)
Comment 7•7 years ago
|
||
Ace, this is a extension, Jonathan wrote, to show the timepicker.
Updated•7 years ago
|
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+
Assignee | ||
Comment 9•7 years ago
|
||
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+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•7 years ago
|
Keywords: reproducible
Comment 10•7 years ago
|
||
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: 7 years ago → 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Updated•7 years ago
|
Target Milestone: --- → Thunderbird 61.0
You need to log in
before you can comment on or make changes to this bug.
Description
•