Timezone converter's 12h correction logic is incorrect for times between 12pm and 1pm, and 12am and 1am
Categories
(Firefox :: Address Bar, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox91 | --- | verified |
People
(Reporter: nhnt11, Assigned: silke)
References
Details
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
The logic that adds 12 hours to e.g. "1pm" to make it "13:00" in 24h format incorrectly adds this 12h offset to "12pm" too - thus if you try to convert "12pm pst in cet" you get "9am" as the result, instead of "9pm".
I'm making a patch.
Reporter | ||
Comment 1•3 years ago
|
||
Actually - Silke, I know you've been interested in working on some JS bugs, and this would be a great one to pick up if you have time!
The incorrect logic is here.
Basically, we are setting the "meridien shift" to 12h, if the time format was "xxPM" - but we don't exclude the "12pm" case. times that are in the 12:xx range are the same in 12h and 24h format. We need to exclude this case.
After changing the logic, we should add some cases to the list of testcases here to include this scenario.
If you want to work on this, feel free to assign yourself. :)
Reporter | ||
Comment 2•3 years ago
|
||
Daisuke, looks like this bug is in beta too, but is suppressed by the fact that unit conversion is not enabled on beta yet. Is there any plan to enable this on beta any time soon, or have some kind of rollout or experiment or something? Just curious if this issue needs any extra tracking.
Assignee | ||
Comment 3•3 years ago
|
||
(In reply to Nihanth Subramanya [:nhnt11] from comment #1)
Actually - Silke, I know you've been interested in working on some JS bugs, and this would be a great one to pick up if you have time!
The incorrect logic is here.
Basically, we are setting the "meridien shift" to 12h, if the time format was "xxPM" - but we don't exclude the "12pm" case. times that are in the 12:xx range are the same in 12h and 24h format. We need to exclude this case.
After changing the logic, we should add some cases to the list of testcases here to include this scenario.
If you want to work on this, feel free to assign yourself. :)
Thanks Nihanth, I'm happy to work on this! :) Could you please assign it to me? I don't seem to have the rights to do so myself.
Reporter | ||
Comment 4•3 years ago
|
||
This bug seems to affect times between 12am and 1am as well, updating the title to reflect this.
(In reply to Silke Hofmann from comment #3)
(In reply to Nihanth Subramanya [:nhnt11] from comment #1)
Thanks Nihanth, I'm happy to work on this! :) Could you please assign it to me? I don't seem to have the rights to do so myself.
Done.
Comment 5•3 years ago
|
||
Hi Nihanth, thank you very much for finding the issue!
(In reply to Nihanth Subramanya [:nhnt11] from comment #2)
Daisuke, looks like this bug is in beta too, but is suppressed by the fact that unit conversion is not enabled on beta yet. Is there any plan to enable this on beta any time soon, or have some kind of rollout or experiment or something? Just curious if this issue needs any extra tracking.
We have one issue to enable unit conversion on the beta channel.
That is internationalization.
For unit conversion, we need to internationalize for not only output, but input.
For now, as we have not found a good way to internationalize especially for input, are supporting only "en-US". (Perhaps, need NLP module?)
When resolving this issue, we can enable it on the beta, I think.
Updated•3 years ago
|
Assignee | ||
Comment 6•3 years ago
|
||
Assignee | ||
Comment 7•3 years ago
|
||
Hey Nihanth, I'll provide some more info about the patch I've just submitted tomorrow. :)
Assignee | ||
Comment 8•3 years ago
|
||
Thank you for reviewing the patch Nihanth! I’ve just submitted an update that includes the changes you suggested. Here is everything I’ve done so far:
- For times between 12am and 1am, inputHours and meridian shift are now set to zero
- For times between 12pm and 1pm, meridian shift is now set to zero
- For pm times with hours smaller than 12, meridian shift is now set to 12
I’ve added six test cases that cover the three cases above.
Comment 10•3 years ago
|
||
bugherder |
Comment 11•3 years ago
|
||
I followed the details provided in the description on Fx 91.0a1(2021-06-29) and I can confirm this issue is fixed. Thank you!
Updated•3 years ago
|
Description
•