Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Time Picker / Input Time Picker / Input type="time"] Handle 24 hour format #4756

Closed
macandcheese opened this issue Jun 16, 2022 · 25 comments
Closed
Assignees
Labels
4 - verified Issues that have been released and confirmed resolved. design Issues that need design consultation prior to development. enhancement Issues tied to a new feature or request. estimate - 5 A few days of work, definitely requires updates to tests. i18n-l10n issues dealing with internationalization/localization p - medium Issue is non core or affecting less that 60% of people using the library ready for dev Issues ready for development implementation.

Comments

@macandcheese
Copy link
Contributor

Description

Currently, the time-picker, input-time-picker, and input type="time" do not allow for the use of 24 hour time format from an end-user use case.

Acceptance Criteria

Allow the ability to control the format - 12/24hr, in these components / component configurations.

Relevant Info

A native (non-calcite) input of type="time" automatically changes from 12 -> 24 hour format when I changed my Time Format in OS preferences to 24-hour time (Mac / Chrome). I could not replicate this behavior with calcite-input of type time.

Which Component

[Time Picker / Input Time Picker / Input type="time"]

Example Use Case

No response

@macandcheese macandcheese added enhancement Issues tied to a new feature or request. 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Jun 16, 2022
@annierm18 annierm18 added the i18n-l10n issues dealing with internationalization/localization label Jun 17, 2022
@driskull driskull added the design Issues that need design consultation prior to development. label Jun 29, 2022
@jcfranco
Copy link
Member

jcfranco commented Jul 6, 2022

@annierm18 Can you confirm if this needs to be configurable? So far, the format has been automatically determined by the locale.

@eriklharper
Copy link
Contributor

Sounds like the issue is just that the component doesn't respond to browser or OS locale changes properly. Is that correct?

@eriklharper
Copy link
Contributor

From the looks of it, this doesn't seem possible to do with the existing browser APIs. I'm going to do some more research, but I don't think there's a way to access this system setting via javascript in the browser.

@benelan
Copy link
Member

benelan commented Jul 14, 2022

IIRC navigator.language should be the system's preferred language when provided.

Last time I looked into this I found it really depends on the browser and OS. For example, FireFox:

All major operating systems have a Settings UI for selecting those preferences, and since Firefox does not provide its own, Gecko looks into the OS for them. A special API mozilla::intl::OSPreferences handles communication with the host operating system, retrieving regional preferences and altering internationalization formatting with user preferences.

https://firefox-source-docs.mozilla.org/intl/locale.html#regional-preferences

@macandcheese said he is using Mac/Chrome:
https://developer.chrome.com/docs/extensions/reference/i18n/#mac-os-x

Where as it looks like Chrome won't use Window's system settings and you have to make shortcuts:
https://developer.chrome.com/docs/extensions/reference/i18n/#windows

I don't think input uses navigator.language atm

@eriklharper
Copy link
Contributor

Yeah, and even if the specific browser provides a hook into the OS locale, which would imply a default hour cycle, it would still need to hook into the OS hour cycle checkbox to determine if a user has overridden the default.

@annierm18
Copy link
Contributor

Thank you for the info @eriklharper and @benelan! I will discuss this with Babak and will follow up soon.

@annierm18
Copy link
Contributor

Update on this - the i18n team would like for this to be implemented if it's possible.

@eriklharper
Copy link
Contributor

Update on this - the i18n team would like for this to be implemented if it's possible.

What we can support is an hour-cycle property, but we won't be able to set the hour cycle based on the user's OS setting. I can look into doing this in browser-specific time format settings, but after the research I did I discovered the browsers don't have a hook into the OS-level system preferences like I assumed they would. Perhaps in the future this is something they will implement.

Would a property that overrides the existing locale's hour cycle suffice?

@benelan
Copy link
Member

benelan commented Jul 18, 2022

What we can support is an hour-cycle property, but we won't be able to set the hour cycle based on the user's OS setting.

I'm pretty sure it's possible using navigator.language like I mentioned above. What about something like this?

/*
 * Detects navigator locale 24h time preference
 * It works by checking whether hour output contains AM ('1 AM' or '01 h')
 * based on the user's preferred language
 */
const isBrowserLocale24h = () =>
  !new Intl.DateTimeFormat(navigator.language, { hour: "numeric" })
    .format(0)
    .match(/AM/);

@benelan
Copy link
Member

benelan commented Jul 18, 2022

Looking at the guys code you would probably have to check for "PM" too:

const isBrowserLocale24h = () =>
  !new Intl.DateTimeFormat(navigator.language, { hour: "numeric" })
    .format(0)
    .match(/[AM|PM]/);

@benelan
Copy link
Member

benelan commented Jul 18, 2022

Alright I tested the guys code and it doesn't work at all lol. The AM/PM string is dependant on the locale.
https://codepen.io/benesri/pen/RwMpGeZ?editors=0011

This seems to work based on my limited testing

const isBrowserLocale24h = () =>
  !new Intl.DateTimeFormat(navigator.language, { hour: "numeric" })
    .format(new Date(1, 1, 1, 14))
    .match(new RegExp(Number(2).toLocaleString(navigator.language)));

I think using the code above by default would be good, but there is still the case of the OS/browser combos that don't use the locale system settings. So I could see Erik's prop recommendation as well. Thoughts @jcfranco ?

@eriklharper
Copy link
Contributor

Correct, I wouldn't expect this to work anyway because Intl.DateTimeFormat is just a formatting api. It knows nothing about user preferences.

@eriklharper
Copy link
Contributor

That code snippet won't work consistently enough to rely on it. Didn't work for me when I changed my OS time preference to 24 hour with an en-US locale.

@benelan
Copy link
Member

benelan commented Jul 18, 2022

Yeah I'm starting to think Chrome's documentation lied to me. Here it says:

On Mac or Linux? Chrome will automatically display in the default system language for your computer.

I have Windows, but I asked Franco to change his system language to Spanish and Chrome did not automatically display in Spanish. My understanding is that navigator.language is the browsers preferred locale. So if changing the locale in the system actually affected Chrome then that code should work, but maybe the doc is outdated.

@eriklharper
Copy link
Contributor

The kicker there is "default system language". The trick is getting it to recognize the non-default hour setting, which it doesn't appear to support.

@annierm18
Copy link
Contributor

Implementing support an for hour-cycle property that @eriklharper mentioned sounds like a good option

@driskull
Copy link
Member

driskull commented Nov 9, 2022

@eriklharper
Copy link
Contributor

Can we use this? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/Locale/hourCycle

Yes, we would use this to set the default value of the hour-cycle prop based on the locale. Being a prop, hour-cycle would also be overridable from the locale's default hour cycle.

@geospatialem geospatialem added p - low Issue is non core or affecting less that 10% of people using the library and removed airtable labels May 25, 2023
@geospatialem geospatialem added this to the Design Backlog milestone Jun 1, 2023
@eriklharper
Copy link
Contributor

Update: Almost done writing tests for all the supported locales, and looking at a handful of some locale-specific issues with typing in localized meridiem characters and one with the arab numbering-system, and then refactoring from hour-cycle to hour-format before this is ready. Getting close :)

@geospatialem geospatialem added 2 - in development Issues that are actively being worked on. and removed 1 - assigned Issues that are assigned to a sprint and a team member. labels Aug 26, 2024
eriklharper added a commit that referenced this issue Jan 1, 2025
**Related Issue:** #4756

## Summary

This PR exposes a new `hour-format` property to `input-time-picker` and
`time-picker` that overrides the locale's default `hourCycle` setting,
allowing a 12-hour locale to be formatted in 24-hour time and vice
versa. Confirmed with the i18n team that the bulgarian `ч.` character
(abbreviation for "hours") should not display for short and medium time
formats.

---------

Co-authored-by: Kitty Hurley <[email protected]>
@eriklharper eriklharper added 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed 2 - in development Issues that are actively being worked on. labels Jan 1, 2025
@github-actions github-actions bot assigned geospatialem and DitwanP and unassigned eriklharper Jan 1, 2025
Copy link
Contributor

github-actions bot commented Jan 1, 2025

Installed and assigned for verification.

@geospatialem
Copy link
Member

@eriklharper I'm encountering what may be unexpected behavior with the component when hour-format="24" and entering a number over 12 for the hour. Once the minute is added the value appears accurate, but prior to selection it doesn't match the number, nor are numbers 13+ added to the component initially. Is this expected?

Sample with 3.0.0-next.87 and a screen grab of the behavior
verify-4756

@eriklharper
Copy link
Contributor

eriklharper commented Jan 2, 2025

@eriklharper I'm encountering what may be unexpected behavior with the component when hour-format="24" and entering a number over 12 for the hour. Once the minute is added the value appears accurate, but prior to selection it doesn't match the number, nor are numbers 13+ added to the component initially. Is this expected?

Nice catch there. Not expected for sure. Seems to be an initial state kind of behavior where the formatting is displaying as the default 12-hour even though the correct 24-hour hour value is being correctly stored. Looks like that behavior ceases in subsequent updates, but agreed that this isn't expected. I would call this a bug for sure, probably low-ish on the severity, but definitely something we should fix.

@geospatialem geospatialem added 4 - verified Issues that have been released and confirmed resolved. and removed 3 - installed Issues that have been merged to master branch and are ready for final confirmation. labels Jan 3, 2025
@geospatialem
Copy link
Member

@eriklharper I'm encountering what may be unexpected behavior with the component when hour-format="24" and entering a number over 12 for the hour. Once the minute is added the value appears accurate, but prior to selection it doesn't match the number, nor are numbers 13+ added to the component initially. Is this expected?

Sample with 3.0.0-next.87 and a screen grab of the behavior verify-4756 verify-4756

Created #11198 to address the unrelated behavior for 25.R1. Closing out the remaining effort above as complete and verified. 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - verified Issues that have been released and confirmed resolved. design Issues that need design consultation prior to development. enhancement Issues tied to a new feature or request. estimate - 5 A few days of work, definitely requires updates to tests. i18n-l10n issues dealing with internationalization/localization p - medium Issue is non core or affecting less that 60% of people using the library ready for dev Issues ready for development implementation.
Projects
None yet
Development

No branches or pull requests

10 participants