Closed Bug 1346085 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Layout: Form Controls, enhancement)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: jessica, Assigned: jessica)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

Bug 1346085 - Part 1: Use <span> for editable fields in date/time input box.
59 bytes, text/x-review-board-request
mossop
: review+
Details
Bug 1346085 - Part 2: Set input element's focus state when inner fields are focused/blurred.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1346085 - Part 3: RTL support for date/time input box.
59 bytes, text/x-review-board-request
mossop
: review+
zbraniecki
: review+
Details 
      No description provided.
Assignee: nobody → jjong
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)
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)
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. :)
Attached patch patch, v1. (obsolete) — — Splinter Review 
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.
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)
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
(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)));
(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.
(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 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-
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.
(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.
(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".
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. :/
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]`)
(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.
(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>!
Attachment #8851489 - Attachment is obsolete: true
Attachment #8849010 - Attachment is obsolete: true
Attachment #8849010 - Flags: feedback?(tomer.moz.bugs)
Attachment #8849010 - Flags: feedback?(gandalf)
Attachment #8849013 - Attachment is obsolete: true
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 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+
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 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 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-
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 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 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 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+
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
Depends on: 1376050
Depends on: 1415605
Depends on: 1521884
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: