Closed Bug 1344624 Opened 4 years ago Closed 4 years ago

[DateTimeInput] (l10n) Numbers in numeric field should be formatted based on locale.

Categories

(Core :: Layout: Form Controls, enhancement)

Product:
Core
Component:
Layout: Form Controls
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox54 --- wontfix
firefox55 --- verified

People

(Reporter: jessica, Assigned: jessica)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1344624 - [DateTimeInput] (l10n) Part 1: Display formatted numbers in <input type=time>.
59 bytes, text/x-review-board-request
mossop
: review+
Details
Bug 1344624 - [DateTimeInput] (l10n) Part 2: Display formatted numbers in <input type=date>.
59 bytes, text/x-review-board-request
mossop
: review+
Details 
Currently, we only show arabic numbers in numeric fields, but as :zibi mentioned in bug 1301312 comment 13, numbers should be formatted using `Intl.NumberFormat`.
Assignee: nobody → jjong
Comment on attachment 8849916 [details]
Bug 1344624 - [DateTimeInput] (l10n) Part 1: Display formatted numbers in <input type=time>.

https://reviewboard.mozilla.org/r/122646/#review124954

::: toolkit/content/widgets/datetimebox.xml:774
(Diff revision 1)
> -            time += "." + this.mMillisecField.value;
> +            // Convert milliseconds to fraction of second.
> +            if (millisecond < 10) {
> +              millisecond = "00" + millisecond;
> +            } else if (millisecond < 100) {
> +              millisecond = "0" + millisecond;
> +            }

You can juse use millisecond.padStart(3, 0);

::: toolkit/content/widgets/datetimebox.xml:1021
(Diff revision 1)
> -            if (value < 10) {
> -              value = "0" + value;
> +          aField.setAttribute("rawValue", value);
> +
> +          let minDigits = aField.size;
> +          let formatted = new Intl.NumberFormat(this.mLocales,
> +            { minimumIntegerDigits: minDigits,
> +              useGrouping: false }).format(value);

Is there a way to cache the formatter to reuse it for all operations?

If you're not caching it, I think it's clearer to do:

`
value.toLocaleString(this.mLocales, {
  minimumIntegerDigits: minDigits,
  useGrouping: false
});
`

also, you should be able to drop the `this.mLocales` since JS context should now be localized to the browser locale when you pass `undefined`.
Attachment #8849916 - Flags: review?(mconley) → review?(dtownsend)
Attachment #8849917 - Flags: review?(mconley) → review?(dtownsend)
I hope it's alright that I'm redirecting! I think I said this before, but (along with lightening my review queue and making sure you get a speedy review) I'm hoping we can up the bus-factor on this datetime picker stuff.
(In reply to Mike Conley (:mconley) (Catching up on reviews and needinfos) from comment #4)
> I hope it's alright that I'm redirecting! I think I said this before, but
> (along with lightening my review queue and making sure you get a speedy
> review) I'm hoping we can up the bus-factor on this datetime picker stuff.

No problem at all. :)
Do you want me to redirect all the future patches to :mossop (or someone else) from now on? Or you will do it depending of your review load?
Comment on attachment 8849916 [details]
Bug 1344624 - [DateTimeInput] (l10n) Part 1: Display formatted numbers in <input type=time>.

https://reviewboard.mozilla.org/r/122646/#review124954

> You can juse use millisecond.padStart(3, 0);

Will do, thanks for the suggestion.

> Is there a way to cache the formatter to reuse it for all operations?
> 
> If you're not caching it, I think it's clearer to do:
> 
> `
> value.toLocaleString(this.mLocales, {
>   minimumIntegerDigits: minDigits,
>   useGrouping: false
> });
> `
> 
> also, you should be able to drop the `this.mLocales` since JS context should now be localized to the browser locale when you pass `undefined`.

We can't cache it, since minimumIntegerDigits may be different, for example for year field. I can change it to `value.toLocaleString(this.mLocales, { minimumIntegerDigits: minDigits, useGrouping: false });`.

I'd rather set locale explicitly, since I'm not sure what default locale is JS context using (browser UI? or user preferred language?) see Bug 999003.
Oh, funny. I think I accidentally fixed bug 999003... :)
Comment on attachment 8849916 [details]
Bug 1344624 - [DateTimeInput] (l10n) Part 1: Display formatted numbers in <input type=time>.

https://reviewboard.mozilla.org/r/122646/#review125544

::: toolkit/content/widgets/datetimebox.xml:1431
(Diff revision 2)
> +          this.updateResetButtonVisibility();
> +        ]]>
> +        </body>
> +      </method>
> +
> +      <method name="setFieldValue">

Why add this at all if it is just going to throw?
Comment on attachment 8849917 [details]
Bug 1344624 - [DateTimeInput] (l10n) Part 2: Display formatted numbers in <input type=date>.

https://reviewboard.mozilla.org/r/122648/#review125552

::: toolkit/content/widgets/datetimebox.xml:1135
(Diff revision 2)
>          this.mStep = this.mInputElement.step;
>          this.mIsPickerOpen = false;
>  
>          this.mResetButton =
>            document.getAnonymousElementByAttribute(this, "anonid", "reset-button");
> +        this.mResetButton.style.visibility = "hidden";

Can you explain why this is here?
Comment on attachment 8849916 [details]
Bug 1344624 - [DateTimeInput] (l10n) Part 1: Display formatted numbers in <input type=time>.

https://reviewboard.mozilla.org/r/122646/#review125544

> Why add this at all if it is just going to throw?

It's just a way to remind derived classed to override this method.
Comment on attachment 8849917 [details]
Bug 1344624 - [DateTimeInput] (l10n) Part 2: Display formatted numbers in <input type=date>.

https://reviewboard.mozilla.org/r/122648/#review125552

> Can you explain why this is here?

Sure. The idea is to unify the places where we call `updateResetButtonVisibility()`. I'd like it to be called only when we set/clear a field.
So, here I change the default visibility of the button to "hidden", and remove the call to `updateResetButtonVisibility()` in constructors. If input element value is not empty, `setFieldValue()` will be called and the reset button visibility will be updated. In the contrary, if input element value is empty, the reset button will stay hidden.
Comment on attachment 8849916 [details]
Bug 1344624 - [DateTimeInput] (l10n) Part 1: Display formatted numbers in <input type=time>.

https://reviewboard.mozilla.org/r/122646/#review125768

::: toolkit/content/widgets/datetimebox.xml:1412
(Diff revisions 2 - 3)
> -            return "";
> +            return undefined;
>            }
>  
>            let value = aField.getAttribute("rawValue");
>            // Avoid returning 0 when field is empty.
> -          return (this.isEmpty(value) ? "" : Number(value));
> +          return (this.isEmpty(value) ? undefined : Number(value));

Sorry, uploaded revision 3 with a tiny change: return `undefined` instead of empty string in `getFieldValue()`. Picker expects either a number or undefined.
I have a general question. Apologies if I misunderstand the code, since I'm just reading bits of it and I don't think I grasp the whole scope, but it seems to me that what the XBL is doing is:

 - iterating over formatted date/time
 - stripping it of BIDI control chars
 - injecting placeholders ('--') when value is not set
 - injecting manually constructed and padded value formattings where the value is set

Wouldn't it make more sense to overall just stick to the DateTimeFormat representation by doing (sorry for pseudocode):

```
let displayedValue = dtf.formatToParts((part) => {
  switch (part.type) {
    case 'hour':
      return valyes.hour === null ? placeholders.hour : part.value;
      break;
    case 'minute':
      return values.minute === null ? placeholders.minute : part.value;
      break;
    case 'dayperiod':
      return values.dayperiod === null ? placeholders.dayperiod : part.value;
      break;
    default:
      return part.value;
  }
}).join('');
```

and control the text caret position the by just adjusting it as the user types/clicks?

It seems like it would do two things:
 - avoid duplication of numeric formatting, both transliteration and paddings
 - avoid having to manually control the BIDI chars
 - instead of trying to control every token in the formatted string, it would make certain parts editable, but if Intl.DateTimeFormat would return a token we don't expect, it would just be displayed and not-editable.

Once again, sorry if I misunderstand, but prefer to ask in case the idea may make some sense :)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #17)

Your understanding is basically correct, we iterate over formatted date/time and crate an input text for editable fields and a span for non-editable fields. We make use of .placeholder attribute to set the localized placeholder. Then use .value to display the formatted/padded value.

If I understand correctly, are you suggesting we should treat the date/time value as only one string and put it in a span? But how do we make only certain parts content-editable?
If using a whole string, we'd need to know where the caret position is and what field it belongs to. That's hard, specially with localized string values.
For example, if a user wants to increment the day value by clicking the up-arrow key, we need to know where the caret is, what field it belongs to, maybe use Date API to increment day, then set the new date/time string back. And for highlighting a field, we need to know where the selection starts and where it should end.

It's also possible, but I think it's a little painful and error-prone to handle all of this by ourselves. Having separate elements (input text in our case) is the first thing that we thought of when discussing about implementation. Having separate elements make things easier when the user interacts with individual fields, like tabbing, keyboard navigation, focus/blur, styling for individual fields, etc.

Does this answer your question? Or did I understand your question correctly? :)
BTW, we are preserving the BIDI control chars after bug 1346085, see attachment 8849010 [details] [diff] [review]. We'll need it and rely on it after all. :)
Ok, so it's basically complex output or complex input and you decided to go for an easy input. :)

Thanks for the explanation!
Comment on attachment 8849916 [details]
Bug 1344624 - [DateTimeInput] (l10n) Part 1: Display formatted numbers in <input type=time>.

https://reviewboard.mozilla.org/r/122646/#review125946
Attachment #8849916 - Flags: review?(dtownsend) → review+
Comment on attachment 8849917 [details]
Bug 1344624 - [DateTimeInput] (l10n) Part 2: Display formatted numbers in <input type=date>.

https://reviewboard.mozilla.org/r/122648/#review125948
Attachment #8849917 - Flags: review?(dtownsend) → review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a94ec94e0c0c
[DateTimeInput] (l10n) Part 1: Display formatted numbers in <input type=time>. r=mossop
https://hg.mozilla.org/integration/autoland/rev/4655574830ef
[DateTimeInput] (l10n) Part 2: Display formatted numbers in <input type=date>. r=mossop
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a94ec94e0c0c
https://hg.mozilla.org/mozilla-central/rev/4655574830ef
Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Verified as fixed using the latest Nightly AR build 55.0a1 (2017-05-07) on Ubuntu 16.04 x64, Windows 10 x64 and Mac OS X 10.11.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
You need to log in before you can comment on or make changes to this bug.