[DateTimeInput] (l10n) RTL support for <input type=time> input box

RESOLVED FIXED in Firefox 55

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
a year ago
4 months ago

People

(Reporter: jessica, Assigned: jessica)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

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

Attachments

(3 attachments, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

a year ago
Assignee: nobody → jjong
(Assignee)

Comment 1

a year ago
While playing around with Chrome and Intl.DateTimeFormat with rtl locales, I noticed that the time format is not what we think it is. We though it was just like a mirror, so with 'ar' it will look like:
  
  (x)[dayPeriod] [sec]:[min]:[hour]

where (x) is the clear button.
But it seems that the following is actually correct:

  (x)[dayPeriod] [hour]:[min]:[sec]

That is, the time part is still 'ltr'.
But with date, it is all rtl, that is:

  (x) [year]/[month]/[day]
NI to Delphine.
Date time input team is working on l10n (including RTL) now.
Any guidance to us since we all are not capable of reading rtl languages. 
How do the devs know if we're doing it correctly? Any contact person we can reach out to ?
Flags: needinfo?(lebedel.delphine)

Comment 3

a year ago
Dates (and numbers in general) are being kept as LTR on RTL languages. Given that said, the datetime control should look very similar to how it currently behave, but the horizontal scroll direction should be replaced, so the next month should be the left arrow, and the previous month should be the right arrow (as with the direction of writing).

I am a native Hebrew speaker, so I think I am capable of helping here.
Thank you Tomer! Taking ni off me, since you are in good hands now :)
Flags: needinfo?(lebedel.delphine)
(Assignee)

Comment 5

a year ago
Thanks for the input. I think I will leave the numeric part as is from the result of Intl.DateTimeFormat. I'll let you know once I have a patch to test and let's see if it's correct. :)
(Assignee)

Comment 6

11 months ago
Created attachment 8849010 [details] [diff] [review]
patch, v1.

For RTL locales, we create an extra span and set its style to `unicode-bidi: plaintext`. This property allows to display data which has already formatted following the Unicode Bidirectional Algorithm. So, we'll leverage the result obtained from Intl.DateTimeFormat, including the bidi characters.

With this patch, the display shows the same result as printing with Intl.DateTimeFormat. But I'm not sure how to handle arrow-left and arrow-right properly, cause it's not aware of bidi characters. In this patch, arrow-right moves like tabbing and arrow-left moves like reverse tabbing.

Note that Chrome browser disables moving among fields using arrow-left and arrow-right in RTL locales.
(Assignee)

Comment 7

11 months ago
Created attachment 8849013 [details]
screenshot with 'ar' and 'he' locales.
(Assignee)

Comment 8

11 months ago
Comment on attachment 8849010 [details] [diff] [review]
patch, v1.

Hi zibi and tomer, may I have your feedback on this? You may refer to attachment 8849013 [details] for the results. I'm aware that we need to format the numbers displayed, I'll handle that in bug 1344624.
Attachment #8849010 - Flags: feedback?(tomer.moz.bugs)
Attachment #8849010 - Flags: feedback?(gandalf)

Comment 9

11 months ago
Please correct me if I'm wrong, but isn't 'ar' picking up the wrong date format? 
https://en.wikipedia.org/wiki/Date_format_by_country

Comment 10

11 months ago
(In reply to Tomer Cohen :tomer from comment #9)
> Please correct me if I'm wrong, but isn't 'ar' picking up the wrong date
> format? 
> https://en.wikipedia.org/wiki/Date_format_by_country

For example, according to the list above, ar-JO supposed be dd/mm/yyyy while ar-AF supposed to be yyyy/mm/dd or dd/mm/yyyy.

Intl.DateTimeFormat('ar-JO').format(new Date(Date.UTC(2012, 11, 20, 3, 0, 0)));
(Assignee)

Comment 11

11 months ago
(In reply to Tomer Cohen :tomer from comment #10)
> (In reply to Tomer Cohen :tomer from comment #9)
> > Please correct me if I'm wrong, but isn't 'ar' picking up the wrong date
> > format? 
> > https://en.wikipedia.org/wiki/Date_format_by_country
> 
> For example, according to the list above, ar-JO supposed be dd/mm/yyyy while
> ar-AF supposed to be yyyy/mm/dd or dd/mm/yyyy.
> 
> Intl.DateTimeFormat('ar-JO').format(new Date(Date.UTC(2012, 11, 20, 3, 0,
> 0)));

Hmm, but printing `Intl.DateTimeFormat('ar-JO').format(new Date(Date.UTC(2012, 11, 20, 3, 0, 0)));` gives me:
  ٢٠‏/١٢‏/٢٠١٢
which I think is 2012/12/20, right?
Well, it's 20/12/2012, just RTL.

Can we first get the numerals in the right script before we evaluate this code? I think it'll be easier to see.
(Assignee)

Comment 13

11 months ago
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #12)
> Well, it's 20/12/2012, just RTL.
> 
> Can we first get the numerals in the right script before we evaluate this
> code? I think it'll be easier to see.

Sure, then I'll handle bug 1344624 first.
Comment hidden (mozreview-request)
(Assignee)

Comment 15

11 months ago
Comment on attachment 8851489 [details]
Bug 1346085 - [DateTimeInput] (l10n) RTL support for input type date and time input box.

Sorry, let me check persian locale first.
Attachment #8851489 - Flags: review?(gandalf)
Attachment #8851489 - Flags: review?(dtownsend)
Attachment #8851489 - Flags: review-
(Assignee)

Updated

11 months ago
Attachment #8851489 - Flags: review-
:jessica, so, I know you're reworking it, but I  already started looking at the code. My major concern is that you're trying to hardcode a behavior that is locale dependent.

I believe you should not be doing the manual field ordering like you do in the patch at all.

`formatToParts` gives you the fields in order, and you should display them in that order. Any "jumps" between fields work visually - so if I'm in "ar" locale and I see "[dayperiod] [hour]:[minute]" and I'm in [hour] field, when I press "<-" I should go to [dayperiod] and if I press "->" I should go to [minute].

That's the same for all locales so I don't see why you added the special <span> for hour:minute in RTL case.
(Assignee)

Comment 17

11 months ago
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #16)
> :jessica, so, I know you're reworking it, but I  already started looking at
> the code. My major concern is that you're trying to hardcode a behavior that
> is locale dependent.

Thanks for your time on this.

> 
> I believe you should not be doing the manual field ordering like you do in
> the patch at all.

Do you mean creating an extra span for RTL locales?

> 
> `formatToParts` gives you the fields in order, and you should display them
> in that order. Any "jumps" between fields work visually - so if I'm in "ar"
> locale and I see "[dayperiod] [hour]:[minute]" and I'm in [hour] field, when
> I press "<-" I should go to [dayperiod] and if I press "->" I should go to
> [minute].

Take `ar` and a time as an example, `formatToParts` gives me:
 type: hour value: ٣
 type: literal value: :
 type: minute value: ٤٠
 type: literal value: :
 type: second value: ٥٠
 type: literal value: 
 type: dayPeriod value: م

If I display in this order, it'd be: [hour]:[minute]:[second] [dayPeriod] instead of [dayPeriod] [hour]:[minute]:[second].
I can either set the direction to `rtl` or `ltr` or `auto`, which sets the direction based on the first strong character it finds (this is the same as setting `unicode-bidi: plaintext`). Bidi text is not handled automatically, so we need to handle on our own by separating the numeric part.

With `ar` and a date, `formatToParts` gives me:
 type: day value: ٢٠
 type: literal value: &lrm;‏/ 
 type: month value: ٣ 
 type: literal value: ‏&lrm;/
 type: year value: ٢٠١٧

Since there is a strong character `&lrm;`, the whole span direction becomes `rtl`, so it is displayed as: [year]/[month]/[day]
However, the document order is different from the rendered order. The document order is still [day]/[month]/[year], and it's the only information we've got, so the tabbing order jumps starting from day, then to month, then to year. (Chrome behavior is the same, it just disables the keyboard navigation so it's not that weird).

> 
> That's the same for all locales so I don't see why you added the special
> <span> for hour:minute in RTL case.

Another thing is that, I noticed that with "fa" locale, Intl.DatetimeFormat returns the date in Persian calendar format, with an extra literal "era". But we only plan to support Gregorian calendar, so I think we should skip the type/value that we don't know, to avoid surprises.
> If I display in this order, it'd be: [hour]:[minute]:[second] [dayPeriod] instead of [dayPeriod] [hour]:[minute]:[second].

I don't think that this is true. Thanks to bidi control chars, it will be properly displayed, see: http://jsbin.com/xufogaduze/edit?html,js,output

> However, the document order is different from the rendered order.

According to our arabic localizer, this is the expected behavior. :)

> Another thing is that, I noticed that with "fa" locale, Intl.DatetimeFormat returns the date in Persian calendar format, with an extra literal "era". But we only plan to support Gregorian calendar, so I think we should skip the type/value that we don't know, to avoid surprises

a) I think it's a bug in fa locale that has been reported to CLDR and will be fixed in the next CLDR.
b) I think I'd like to suggest doing the opposite.

Treat whatever comes as a string, and just alter the bits you know with editable blocks.

If, for whatever reason, I send you [hour][literal][unknown][minute][dayperiod], then I think you should just replace [hour] and [minute] with editable <spans> and leave the rest as is.

It's not surprising to the reader who expects to see the date this way. And all you do is turn some of the elements to be editable.
Also,  I believe you want to revert the location of the [x] clear button, see https://developer.mozilla.org/en-US/docs/Archive/B2G_OS/Firefox_OS_apps/Firefox_OS_in_Arabic#Input_areas (FxOS days RTL guidelines) or Android RTL guidelines.
(Assignee)

Comment 21

11 months ago
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #18)
> > If I display in this order, it'd be: [hour]:[minute]:[second] [dayPeriod] instead of [dayPeriod] [hour]:[minute]:[second].
> 
> I don't think that this is true. Thanks to bidi control chars, it will be
> properly displayed, see: http://jsbin.com/xufogaduze/edit?html,js,output

It doesn't displayed correctly anymore if you change any of the parts to an <input> instead of a <span>. :(

> 
> > However, the document order is different from the rendered order.
> 
> According to our arabic localizer, this is the expected behavior. :)

You mean tabbing in the reverse order? But wouldn't it be weird for the left/right arrow keys jump in the reverse direction?

> 
> > Another thing is that, I noticed that with "fa" locale, Intl.DatetimeFormat returns the date in Persian calendar format, with an extra literal "era". But we only plan to support Gregorian calendar, so I think we should skip the type/value that we don't know, to avoid surprises
> 
> a) I think it's a bug in fa locale that has been reported to CLDR and will
> be fixed in the next CLDR.
> b) I think I'd like to suggest doing the opposite.
> 
> Treat whatever comes as a string, and just alter the bits you know with
> editable blocks.
> 
> If, for whatever reason, I send you
> [hour][literal][unknown][minute][dayperiod], then I think you should just
> replace [hour] and [minute] with editable <spans> and leave the rest as is.
> 
> It's not surprising to the reader who expects to see the date this way. And
> all you do is turn some of the elements to be editable.

The problem with "fa" is that, with "era", because it contains strong characters, the whole direction becomes `rtl`.

> Also,  I believe you want to revert the location of the [x] clear button,
> see
> https://developer.mozilla.org/en-US/docs/Archive/B2G_OS/Firefox_OS_apps/
> Firefox_OS_in_Arabic#Input_areas (FxOS days RTL guidelines) or Android RTL
> guidelines.

Yes, I've done that by setting the .dir for "input-box-wrapper".
(Assignee)

Comment 22

11 months ago
I'm considering changing our implementation to use <span> instead of <input> for inner input fields. That way, bidi control chars will work properly with inline elements. But we need to handle focus/blur, disabled, readonly, maxlength, etc on our own. I hope it worths the big change. :/

Comment 23

11 months ago
Please note that there are no RTL control characters on Hebrew, for example, and I am not fully convinced why there are control characters placed there for Arabic in the first place. Secondly, please note that according to Wikipedia (I am unable to locate this exact data on CLDR…) there are some formatting differences between Arabic-speaking countries (comment 9).
> But we need to handle focus/blur, disabled, readonly, maxlength, etc on our own. I hope it worths the big change. :/

I'm sorry it's that complex :( 
I have little understanding of the complexity of the input controls and I believe the decision is ultimately yours here. 
My naive understanding is that it actually is hard - looking at http://searchfox.org/mozilla-central/search?q=platformHTMLBindings.xml&case=true&path=

On the other hand, when I play with Chrome bindings here, it seems that they also don't use <input> (many text bindings don't work in date/time like pasting etc.), so maybe it's ok to just support arrow keys, tabs, and character input?

After all, that's what all the client-side form validators (for credit cards, phone numbers etc.) seem to use.

The bottom line is - I think we evaluated the complexity of the Intl aspect of this binding. Now it's a matter of evaluating the complexity of the input binding. If you come up with the conclusion that <input> is easier, I'll help you make it work. :)

:tomer:
> and I am not fully convinced why there are control characters placed there for Arabic in the first place.

To control the RTL/LTR direction of timecomponent of time (`[RTL][LTR]<hour>:<minute>[/LTR] <dayPeriod>[/RTL]`)

Comment 25

11 months ago
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #24)
> To control the RTL/LTR direction of timecomponent of time
> (`[RTL][LTR]<hour>:<minute>[/LTR] <dayPeriod>[/RTL]`)

Hebrew does display time in the same manner you should expect from other languages, such as 13:33:37 (hms). I'm not sure if Arabic speakers would expect it to be displayed on screen as reversed, such as 73:33:31 or 37:33:13, even though their writing direction (for non numeric context) is from the right to the left. Same goes with date formats.
> I'm not sure if Arabic speakers would expect it to be displayed on screen as reversed, such as 73:33:31 or 37:33:13, even though their writing direction (for non numeric context) is from the right to the left. Same goes with date formats.

They would not, the order (from our perspective) is: "[dayperiod] [hour]:[minute]". The RTL applies to dayperiod vs hour+minute combo, but hour+minute combo stays in LTR - that's what I expressed in the example above.

I verified it with :mermi.
(Assignee)

Comment 27

11 months ago
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #24)
> > But we need to handle focus/blur, disabled, readonly, maxlength, etc on our own. I hope it worths the big change. :/
> 
> I'm sorry it's that complex :( 
> I have little understanding of the complexity of the input controls and I
> believe the decision is ultimately yours here. 
> My naive understanding is that it actually is hard - looking at
> http://searchfox.org/mozilla-central/search?q=platformHTMLBindings.
> xml&case=true&path=

Actually, we just need to replace the <input> with <span> in our binding content, then set and handle readonly, disabled attributes explicitly. I have a working patch, might ask for review tomorrow.

> 
> On the other hand, when I play with Chrome bindings here, it seems that they
> also don't use <input> (many text bindings don't work in date/time like
> pasting etc.), so maybe it's ok to just support arrow keys, tabs, and
> character input?

Yes, Chrome uses <span>s as well for their inner text fields, maybe for the same reason.

> 
> After all, that's what all the client-side form validators (for credit
> cards, phone numbers etc.) seem to use.
> 
> The bottom line is - I think we evaluated the complexity of the Intl aspect
> of this binding. Now it's a matter of evaluating the complexity of the input
> binding. If you come up with the conclusion that <input> is easier, I'll
> help you make it work. :)

Thanks, you've been super helpful. We just didn't know much about localization, not to mention bidi text, when implementing this. Never knew it was that complicated! Using <input> is easier for handling text inputs, but using an inline element like <span>, we can rely on the UA default behavior for the rendering of bidi text, which is more complicated. So I say, let's give a try with <span>!
(Assignee)

Updated

11 months ago
Attachment #8851489 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8849010 - Attachment is obsolete: true
Attachment #8849010 - Flags: feedback?(tomer.moz.bugs)
Attachment #8849010 - Flags: feedback?(gandalf)
(Assignee)

Updated

11 months ago
Attachment #8849013 - Attachment is obsolete: true

Comment 31

11 months ago
mozreview-review
Comment on attachment 8852332 [details]
Bug 1346085 - Part 1: Use <span> for editable fields in date/time input box.

https://reviewboard.mozilla.org/r/124556/#review127272

::: toolkit/content/widgets/datetimebox.css:18
(Diff revision 1)
>    color: inherit;
> +  font-family: monospace;
>  }
>  
> -.datetime-input {
> +.datetime-edit-field {
>    -moz-appearance: none;

This shouldn't be necessary any longer

::: toolkit/content/widgets/datetimebox.css:38
(Diff revision 1)
> +  color: HighlightText;
> +  outline: none;
>  }
>  
> -.datetime-input[readonly],
> -.datetime-input[disabled] {
> +.datetime-edit-field[disabled="true"],
> +                    [readonly="true"]  {

Don't you need the .datetime-edit-field prefix on the readonly part too?
Attachment #8852332 - Flags: review?(dtownsend)

Comment 32

11 months ago
mozreview-review
Comment on attachment 8852334 [details]
Bug 1346085 - Part 3: RTL support for date/time input box.

https://reviewboard.mozilla.org/r/124560/#review127284
Attachment #8852334 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 33

11 months ago
mozreview-review-reply
Comment on attachment 8852332 [details]
Bug 1346085 - Part 1: Use <span> for editable fields in date/time input box.

https://reviewboard.mozilla.org/r/124556/#review127272

> This shouldn't be necessary any longer

Will remove it.

> Don't you need the .datetime-edit-field prefix on the readonly part too?

Right, I missed it, thanks.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 37

11 months ago
mozreview-review
Comment on attachment 8852332 [details]
Bug 1346085 - Part 1: Use <span> for editable fields in date/time input box.

https://reviewboard.mozilla.org/r/124556/#review127758
Attachment #8852332 - Flags: review?(dtownsend) → review+

Comment 38

11 months ago
mozreview-review
Comment on attachment 8852333 [details]
Bug 1346085 - Part 2: Set input element's focus state when inner fields are focused/blurred.

https://reviewboard.mozilla.org/r/124558/#review128134

please test the case when web page is moving focus in onfocus or onblur handler.

::: commit-message-e2254:3
(Diff revision 2)
> +Bug 1346085 - Part 2: Set input element's focus state when inner fields are focused/blurred. r?smaug
> +
> +Since the inner fields of date/time input are not <input> text anymore, we

what are they then?

::: dom/html/HTMLInputElement.cpp:3938
(Diff revision 2)
>    }
>  
>    // Stop the event if the related target's first non-native ancestor is the
>    // same as the original target's first non-native ancestor (we are moving
>    // inside of the same element).
>    if ((mType == NS_FORM_INPUT_TIME || mType == NS_FORM_INPUT_DATE) &&

Hmm, aren't we missing IsTrusted() check somewhere here?

::: dom/html/HTMLInputElement.cpp:3945
(Diff revision 2)
>        (aVisitor.mEvent->mMessage == eFocus ||
>         aVisitor.mEvent->mMessage == eFocusIn ||
>         aVisitor.mEvent->mMessage == eFocusOut ||
>         aVisitor.mEvent->mMessage == eBlur)) {
>      nsCOMPtr<nsIContent> originalTarget =
> -      do_QueryInterface(aVisitor.mEvent->AsFocusEvent()->mRelatedTarget);
> +      do_QueryInterface(aVisitor.mEvent->AsFocusEvent()->mOriginalTarget);

oops

::: toolkit/content/widgets/datetimebox.xml:1564
(Diff revision 2)
>                target.classList.contains("datetime-edit-field")) {
>              if (target.disabled) {
>                return false;
>              }
>              this.mLastFocusedField = target;
> +            this.mInputElement.setFocusState(true);

Hmm, so if the web page has moved focus to another element in a focus event listener, <input> element might not have focus anymore.

Maybe you need to check that document.activeElement is the <input> element?

::: toolkit/content/widgets/datetimebox.xml:1579
(Diff revision 2)
>            this.log("onBlur originalTarget: " + aEvent.originalTarget);
>  
>            let target = aEvent.originalTarget;
>            target.setAttribute("typeBuffer", "");
>            this.setInputValueFromFields();
> +          this.mInputElement.setFocusState(false);

Similar here, but check the input element isn't anymore the active element?
Attachment #8852333 - Flags: review?(bugs) → review-
(Assignee)

Comment 39

11 months ago
mozreview-review-reply
Comment on attachment 8852333 [details]
Bug 1346085 - Part 2: Set input element's focus state when inner fields are focused/blurred.

https://reviewboard.mozilla.org/r/124558/#review128134

Sure. I'll add some tests for this.

> what are they then?

They are `<span>` now, see Part 1. :)
Will make the commit message clearer.

> Hmm, aren't we missing IsTrusted() check somewhere here?

Will add the check.

> oops

:/

> Hmm, so if the web page has moved focus to another element in a focus event listener, <input> element might not have focus anymore.
> 
> Maybe you need to check that document.activeElement is the <input> element?

Right, will add the check.

> Similar here, but check the input element isn't anymore the active element?

Will do.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 43

11 months ago
Comment on attachment 8852333 [details]
Bug 1346085 - Part 2: Set input element's focus state when inner fields are focused/blurred.

Hi Olli, sorry for ni'ing you while your review queue is closed. I've addressed your review comment in comment 38, would you review it again or should I ask someone else?
Flags: needinfo?(bugs)

Comment 44

11 months ago
mozreview-review
Comment on attachment 8852333 [details]
Bug 1346085 - Part 2: Set input element's focus state when inner fields are focused/blurred.

https://reviewboard.mozilla.org/r/124558/#review129652

Sorry about delay
Attachment #8852333 - Flags: review+

Comment 45

11 months ago
mozreview-review
Comment on attachment 8852334 [details]
Bug 1346085 - Part 3: RTL support for date/time input box.

https://reviewboard.mozilla.org/r/124560/#review129708
Attachment #8852334 - Flags: review?(gandalf) → review+
(Assignee)

Comment 46

11 months ago
Thanks mossop, zibi and smaug!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ef8c6b3d39f67897c5afad2d42a2f5174a96c7b&group_state=expanded
Flags: needinfo?(bugs)
Keywords: checkin-needed

Comment 47

11 months ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/be43edc6c00f
Part 1: Use <span> for editable fields in date/time input box. r=mossop
https://hg.mozilla.org/integration/autoland/rev/ccf5ed509e88
Part 2: Set input element's focus state when inner fields are focused/blurred. r=smaug
https://hg.mozilla.org/integration/autoland/rev/feaeb4c4a114
Part 3: RTL support for date/time input box. r=gandalf,mossop
Keywords: checkin-needed

Comment 48

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/be43edc6c00f
https://hg.mozilla.org/mozilla-central/rev/ccf5ed509e88
https://hg.mozilla.org/mozilla-central/rev/feaeb4c4a114
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

8 months ago
Depends on: 1376050
Depends on: 1415605
You need to log in before you can comment on or make changes to this bug.