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

RESOLVED FIXED in Firefox 55

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
2 months ago
a month ago

People

(Reporter: jessica, Assigned: jessica)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox54 affected, firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

2 months ago
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`.
Blocks: 1323674
(Assignee)

Updated

a month ago
Assignee: nobody → jjong
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

a month ago
mozreview-review
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.
(Assignee)

Comment 5

a month ago
(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?
(Assignee)

Comment 6

a month ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

a month ago
mozreview-review
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 11

a month ago
mozreview-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/#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?
(Assignee)

Comment 12

a month ago
mozreview-review-reply
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.
(Assignee)

Comment 13

a month ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 16

a month ago
mozreview-review
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 :)
(Assignee)

Comment 18

a month ago
(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? :)
(Assignee)

Comment 19

a month ago
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 21

a month ago
mozreview-review
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 22

a month ago
mozreview-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+
(Assignee)

Comment 23

a month ago
Thanks :mossop.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=80f2dd3eb509bd33d31937628f5ad4dcc23cf046&group_state=expanded
Keywords: checkin-needed

Comment 24

a month ago
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

Comment 25

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a94ec94e0c0c
https://hg.mozilla.org/mozilla-central/rev/4655574830ef
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.