Closed Bug 1301312 Opened 8 years ago Closed 8 years ago

[DateTimeInput] localization for <input type=time> input box

Categories

(Core :: Layout: Form Controls, defect)

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

Tracking

()

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

People

(Reporter: jessica, Assigned: jessica)

References

(Blocks 1 open bug)

Details

(Whiteboard: [milestone5])

Attachments

(5 files, 3 obsolete files)

Bug 1301312 - Part 1: Localize AM/PM strings for <input type=time>.
59 bytes, text/x-review-board-request
mconley
: review+
zbraniecki
: review+
Details
Bug 1301312 - Part 2: Localize placeholders for <input type=time>.
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1301312 - Part 3: Order fields in <input type=time> based on locale datetime format.
59 bytes, text/x-review-board-request
mconley
: review+
zbraniecki
: review+
Details
Bug 1301312 - Part 4: Order fields in <input type=date> based on locale datetime format.
59 bytes, text/x-review-board-request
zbraniecki
: review+
mconley
: review+
Details
Bug 1301312 - Part 5: Handle input element's attribute change explicitly.
59 bytes, text/x-review-board-request
smaug
: review+
Details
No description provided.
Whiteboard: [milestone5]
Assignee: nobody → jjong
Attached patch Part 2: placeholders, v1. (obsolete) — — Splinter Review
Attachment #8835912 - Attachment is obsolete: true
Attachment #8835913 - Attachment is obsolete: true
Attachment #8835914 - Attachment is obsolete: true
Comment on attachment 8839384 [details] Bug 1301312 - Part 3: Order fields in <input type=time> based on locale datetime format. If I pass this.mHourField, this.mMinuteField, etc, directly to organizeFieldsOrder(), assigning objects to the variables do not really take effect. So, I needed to put it inside a object and then let organizeFieldsOrder() modify the internals of the object. I'm not sure if there's a better way to do this. I'm open to suggestions.
Comment on attachment 8839382 [details] Bug 1301312 - Part 1: Localize AM/PM strings for <input type=time>. https://reviewboard.mozilla.org/r/114068/#review115950 Thanks! I'm requesting an additional review from gandalf here though in case there's a simpler way that I don't know about. ::: toolkit/content/widgets/datetimebox.xml:487 (Diff revision 1) > + let dayPeriodLength = > + (this.mAMIndicator.length > this.mPMIndicator.length) ? > + this.mAMIndicator.length : this.mPMIndicator.length; > + > + this.mDayPeriodField.size = dayPeriodLength; I think this can probably be simplified to: ```JavaScript let dayPeriodLength = Math.max(this.mAMIndicator.length, this.mPMIndicator.length); ``` ::: toolkit/content/widgets/datetimebox.xml:578 (Diff revision 1) > + hour: 'numeric', > + minute: 'numeric', > + second: 'numeric', Nit: Noticing a mix of single quotes and double-quotes here. Might as well use double, since that seems more common in this file. ::: toolkit/content/widgets/datetimebox.xml:588 (Diff revision 1) > + for (let i = 0; i < amParts.length; i++) { > + let obj = amParts[i]; > + if (obj.type == "literal" && obj.value.trim()) { > + separator = obj.value; > + } else if (obj.type == "dayPeriod") { > + amString = obj.value; > + } > + > + if (separator && amString) { > + break; > + } > + } I guess this is okay, but I'm going to needinfo gandalf in case there's a better way to do this (I'm afraid I'm not too familiar with the Intl API).
Attachment #8839382 - Flags: review?(mconley) → review+
Comment on attachment 8839383 [details] Bug 1301312 - Part 2: Localize placeholders for <input type=time>. https://reviewboard.mozilla.org/r/114070/#review115974 ::: toolkit/content/widgets/datetimebox.xml:463 (Diff revision 1) > + this.mHourPlaceHolder = ]]>"&time.hour.placeholder;"<![CDATA[; > + this.mMinutePlaceHolder = ]]>"&time.minute.placeholder;"<![CDATA[; > + this.mSecondPlaceHolder = ]]>"&time.second.placeholder;"<![CDATA[; > + this.mMillisecPlaceHolder = ]]>"&time.millisecond.placeholder;"<![CDATA[; > + this.mDayPeriodPlaceHolder = ]]>"&time.dayperiod.placeholder;"<![CDATA[; Points for ingenuity! But I think it's more idiomatic (and probably more readable) to use a .properties file to introduce strings to script. See below. ::: toolkit/locales/en-US/chrome/global/datetimebox.dtd:7 (Diff revision 1) > +<!ENTITY time.hour.placeholder "--"> > +<!ENTITY time.minute.placeholder "--"> > +<!ENTITY time.second.placeholder "--"> > +<!ENTITY time.millisecond.placeholder "--"> > +<!ENTITY time.dayperiod.placeholder "--"> I _think_ the more common way to introduce strings to JavaScript (even in XBL bindings) is via a .properties file and a string bundle. Example: http://searchfox.org/mozilla-central/rev/39e4b25a076c59d2e7820297d62319f167871449/toolkit/content/widgets/autocomplete.xml#1538-1547 Not suggesting a getter for each string that you need - just to use a .properties file and to use GetStringFromName on a string bundle instance to get what you need.
Attachment #8839383 - Flags: review?(mconley) → review-
Attachment #8839382 - Flags: review?(gandalf)
Comment on attachment 8839384 [details] Bug 1301312 - Part 3: Order fields in <input type=time> based on locale datetime format. https://reviewboard.mozilla.org/r/114072/#review115982 ::: toolkit/content/widgets/datetimebox.xml:494 (Diff revision 1) > - this.mDayPeriodField = > - document.getAnonymousElementByAttribute(this, "anonid", "input-three"); > - this.mDayPeriodField.classList.remove("numeric"); > + if (this.mIsDayPeriodInFront) { > + this.organizeFieldsOrder(this.mDayPeriodField, > + this.mSpaceSeparator, > + this.mHourField, > + this.mMinuteSeparator, > + this.mMinuteField); > + } else { > + this.organizeFieldsOrder(this.mHourField, > + this.mMinuteSeparator, > + this.mMinuteField, > + this.mSpaceSeparator, > + this.mDayPeriodField); > + } > + > + this.mHourField = this.mHourField.anonElement; > + this.mMinuteSeparator = this.mMinuteSeparator.anonElement; > + this.mMinuteField = this.mMinuteField.anonElement; > + this.mSpaceSeparator = this.mSpaceSeparator.anonElement; > + this.mDayPeriodField = this.mDayPeriodField.anonElement; If I understand this correctly, you're using `mIsDayPeriodInFront` to determine which elements to attach meaning / events to by assigning different values to your `this.m*` properties. That feels brittle. What might be simpler is to apply an attribute or CSS class to ourselves based on `mIsDayPeriodInFront`, and then in the stylesheet, use the `order` rule to change the layout of the input fields. scottwu did something similar in bug 1337319. Would something like that work here? I feel like that'd probably be simpler.
Attachment #8839384 - Flags: review?(mconley) → review-
Comment on attachment 8839382 [details] Bug 1301312 - Part 1: Localize AM/PM strings for <input type=time>. https://reviewboard.mozilla.org/r/114068/#review116046 ::: toolkit/content/widgets/datetimebox.xml:489 (Diff revision 1) > document.getAnonymousElementByAttribute(this, "anonid", "input-three"); > this.mDayPeriodField.classList.remove("numeric"); > > + let dayPeriodLength = > + (this.mAMIndicator.length > this.mPMIndicator.length) ? > + this.mAMIndicator.length : this.mPMIndicator.length; I think it would be more readable via `Math.max`. ::: toolkit/content/widgets/datetimebox.xml:578 (Diff revision 1) > + hour: 'numeric', > + minute: 'numeric', > + second: 'numeric', Why do you add `second` here? It seems like you only want the hour:minute separator. ::: toolkit/content/widgets/datetimebox.xml:583 (Diff revision 1) > + hour: 'numeric', > + minute: 'numeric', > + second: 'numeric', > + hour12: true, > + timeZone: "UTC" > + }); You have a better API for retrieving dayperiod bits: let data = mozIntl.getDisplayNames(aLocales, { keys: [ 'dates/gregorian/dayperiods/am', 'dates/gregorian/dayperiods/pm', ] }); will return an object with the right values for the locales. :) Then you only need one formatToParts which should be constructed the same way as your result widget (so, hour12: true for hourCycle h12, and false for h24) and you should then basically take the literals and interplot them with widgets. I imagine it as something like this: let formatter = Intl.DateTimeFormat(aLocales, { hour: 'numeric', minute: 'numeric', hour12: aHourCycle == 'h12' }); formatter.formatToParts(Date.now()).map(part => { switch (part.type) { case 'hour': root.appendChild(hourWidget); break; case 'minute': root.appendChild(minuteWidget); break; case 'dayPeriod': root.appendChild(dayPeriodWidget); break; default: let span = document.createElement('span'); span.textContent = part.value; root.appendChild(span); break; }; });
Attachment #8839382 - Flags: review?(gandalf) → review-
Comment on attachment 8839383 [details] Bug 1301312 - Part 2: Localize placeholders for <input type=time>. https://reviewboard.mozilla.org/r/114070/#review116064 ::: toolkit/locales/en-US/chrome/global/datetimebox.dtd:7 (Diff revision 1) > +<!ENTITY time.hour.placeholder "--"> > +<!ENTITY time.minute.placeholder "--"> > +<!ENTITY time.second.placeholder "--"> > +<!ENTITY time.millisecond.placeholder "--"> > +<!ENTITY time.dayperiod.placeholder "--"> Yes pls. Use .properties over .dtd in XBL whenever you can.
Comment on attachment 8839384 [details] Bug 1301312 - Part 3: Order fields in <input type=time> based on locale datetime format. https://reviewboard.mozilla.org/r/114072/#review116070 ::: toolkit/content/widgets/datetimebox.xml:494 (Diff revision 1) > - this.mDayPeriodField = > - document.getAnonymousElementByAttribute(this, "anonid", "input-three"); > - this.mDayPeriodField.classList.remove("numeric"); > + if (this.mIsDayPeriodInFront) { > + this.organizeFieldsOrder(this.mDayPeriodField, > + this.mSpaceSeparator, > + this.mHourField, > + this.mMinuteSeparator, > + this.mMinuteField); > + } else { > + this.organizeFieldsOrder(this.mHourField, > + this.mMinuteSeparator, > + this.mMinuteField, > + this.mSpaceSeparator, > + this.mDayPeriodField); > + } > + > + this.mHourField = this.mHourField.anonElement; > + this.mMinuteSeparator = this.mMinuteSeparator.anonElement; > + this.mMinuteField = this.mMinuteField.anonElement; > + this.mSpaceSeparator = this.mSpaceSeparator.anonElement; > + this.mDayPeriodField = this.mDayPeriodField.anonElement; I suggested a slightly different solution, which should also handle the ordering in a more resiliant way. I think either is fine or you can even combine those two and use mine to set the CSS order if you like. What matter to me is that: a) You use DateTimeFormat on today, not on 1970 date (date formats change over time) b) You use it as close to the place where you display as possible c) You display everything you don't recognize as your widget, rather than trying to write APIs that will handle all combinations of hour/minute/dayPeriod orders. ::: toolkit/content/widgets/datetimebox.xml:1051 (Diff revision 1) > } > } > // prepend zero > if (value < 10) { > value = "0" + value; > } Sorry for drive-by review here, but I'm afraid that if this code is intended to format numbers, it will not handle non-arabic numbers properly. Every number here should be formatter using `Intl.NumberFormat` and if you want padding, use `minimumIntegerDigits: 2`.
Attachment #8839384 - Flags: review-
Comment on attachment 8839382 [details] Bug 1301312 - Part 1: Localize AM/PM strings for <input type=time>. https://reviewboard.mozilla.org/r/114068/#review115950 > I think this can probably be simplified to: > > ```JavaScript > let dayPeriodLength = Math.max(this.mAMIndicator.length, this.mPMIndicator.length); > ``` Will do. > Nit: Noticing a mix of single quotes and double-quotes here. Might as well use double, since that seems more common in this file. Will do.
Comment on attachment 8839383 [details] Bug 1301312 - Part 2: Localize placeholders for <input type=time>. https://reviewboard.mozilla.org/r/114070/#review116228 ::: toolkit/locales/en-US/chrome/global/datetimebox.dtd:7 (Diff revision 1) > +<!ENTITY time.hour.placeholder "--"> > +<!ENTITY time.minute.placeholder "--"> > +<!ENTITY time.second.placeholder "--"> > +<!ENTITY time.millisecond.placeholder "--"> > +<!ENTITY time.dayperiod.placeholder "--"> Will change to use .properties file. Thanks.
Comment on attachment 8839384 [details] Bug 1301312 - Part 3: Order fields in <input type=time> based on locale datetime format. https://reviewboard.mozilla.org/r/114072/#review116230 ::: toolkit/content/widgets/datetimebox.xml:494 (Diff revision 1) > - this.mDayPeriodField = > - document.getAnonymousElementByAttribute(this, "anonid", "input-three"); > - this.mDayPeriodField.classList.remove("numeric"); > + if (this.mIsDayPeriodInFront) { > + this.organizeFieldsOrder(this.mDayPeriodField, > + this.mSpaceSeparator, > + this.mHourField, > + this.mMinuteSeparator, > + this.mMinuteField); > + } else { > + this.organizeFieldsOrder(this.mHourField, > + this.mMinuteSeparator, > + this.mMinuteField, > + this.mSpaceSeparator, > + this.mDayPeriodField); > + } > + > + this.mHourField = this.mHourField.anonElement; > + this.mMinuteSeparator = this.mMinuteSeparator.anonElement; > + this.mMinuteField = this.mMinuteField.anonElement; > + this.mSpaceSeparator = this.mSpaceSeparator.anonElement; > + this.mDayPeriodField = this.mDayPeriodField.anonElement; Oh, I wasn't aware of the order property. If it's ok, I will try with the solution :zibi proposed in Part 1. In reply to :zibi's comment: a) I though using Date(0) was safer, can you elaborate more? b) Do you mean calling DateTimeFormart just before display? c) Do you mean if I find a type in formatToParts that I don't recognize, I should still append it to the root? ::: toolkit/content/widgets/datetimebox.xml:1051 (Diff revision 1) > } > } > // prepend zero > if (value < 10) { > value = "0" + value; > } Actually, we only allow arabic numbers 0-9 in the numeric input fields. If I understand correctly the spec, we're not localizing 'numbers', but only placeholders and fields like dayPeriod. I tested Chrome with zh-TW, and the numeric fields are still displayed with arabic numbers. `minimumIntegerDigits` in Intl.NumberFormat is a good helper though.
Comment on attachment 8839382 [details] Bug 1301312 - Part 1: Localize AM/PM strings for <input type=time>. https://reviewboard.mozilla.org/r/114068/#review116046 > I think it would be more readable via `Math.max`. Will do. > Why do you add `second` here? It seems like you only want the hour:minute separator. Yes, I think hour and minute would be enough. > You have a better API for retrieving dayperiod bits: > > let data = mozIntl.getDisplayNames(aLocales, { > keys: [ > 'dates/gregorian/dayperiods/am', > 'dates/gregorian/dayperiods/pm', > ] > }); > > will return an object with the right values for the locales. :) > > Then you only need one formatToParts which should be constructed the same way as your result widget (so, hour12: true for hourCycle h12, and false for h24) and you should then basically take the literals and interplot them with widgets. > > I imagine it as something like this: > > let formatter = Intl.DateTimeFormat(aLocales, { > hour: 'numeric', > minute: 'numeric', > hour12: aHourCycle == 'h12' > }); > > formatter.formatToParts(Date.now()).map(part => { > switch (part.type) { > case 'hour': > root.appendChild(hourWidget); > break; > case 'minute': > root.appendChild(minuteWidget); > break; > case 'dayPeriod': > root.appendChild(dayPeriodWidget); > break; > default: > let span = document.createElement('span'); > span.textContent = part.value; > root.appendChild(span); > break; > }; > }); I'm aware of 'mozIntl.getDisplayNames', it's just we need to expose it via webAPI to use it here, but the structure of mozIntl.getDisplayNames is a little bit complex to turn it into webAPI, so I though it'd be simpler to make use of Intl.DateTimeFormat instead. Do you suggest we still use 'mozIntl.getDisplayNames' here? If so, I'll file a separate bug to expose 'mozIntl.getDisplayNames'. The approach of .appendChild based on the result of formatToParts is really a good idea! That means we'll start with a empty root, right? Haven't thought of that before, but that means we will need to refactor input type=date as well. We have a tight timeline, but I'll try to refine this. Thanks a lot.
Thanks to Mike and Zibi for the review comments and sorry for my lack of experience with JS and CSS. I've learned a lot from your suggestions and responded some of them, could you check if there is anything you like to add? Thanks again!
> In reply to :zibi's comment: > a) I though using Date(0) was safer, can you elaborate more? I don't understand when Date(0) would be safer than current date? > b) Do you mean calling DateTimeFormart just before display? Yes, basically, you want to get to the point in your code where you have all the "replacements" (so, HTML widgets you're going to inject instead of {type: 'hour'} part etc.) and then execute the formatToParts().map() > c) Do you mean if I find a type in formatToParts that I don't recognize, I should still append it to the root? If it's a literal, yes. If it's a non-literal, that's more tricky, but since we're in that case definitely in damage control scenario, my vote is for yes as well. After all, all those pieces are pieces of a date/time string. All you want to do is replace some of them with widgets to select the right value. > I tested Chrome with zh-TW, and the numeric fields are still displayed with arabic numbers. zh-TW is not a good example to test again. Try 'ar'. You should definitely format the output (the digits you will display) because that's what the date time formatter will do as well. See: (new Date()).toLocaleString('ar'); If this is more widespread among date/time selectors, you may want to file a separate bug and run it via UX team. > Do you suggest we still use 'mozIntl.getDisplayNames' here? If so, I'll file a separate bug to expose 'mozIntl.getDisplayNames'. Yeah, I would suggest taking that if possible. If it'll become a blocker, we may have to resort to what you did here, but I'd love to avoid dirty hacks in new code as much as possible :) > The approach of .appendChild based on the result of formatToParts is really a good idea! That means we'll start with a empty root, right? Haven't thought of that before, but that means we will need to refactor input type=date as well. We have a tight timeline, but I'll try to refine this. Thanks a lot. I'm totally fine with doing that incrementally if you'll end up deciding that you can't make it in time. I recommend this approach for two reasons: 1) It's clean to write and maintain 2) It also handles nicely any reaction to language changes that you may encounter (just clear the root and rebuild the widgets from mapping over a single formatToParts)
Depends on: 1341994
Comment on attachment 8839383 [details] Bug 1301312 - Part 2: Localize placeholders for <input type=time>. https://reviewboard.mozilla.org/r/114070/#review116228 > Will change to use .properties file. Thanks. It seems that I can't use stringbundle, need chrome privileges: > Failed to get stringbundle: > TypeError: Components.classes is undefined
Comment on attachment 8839383 [details] Bug 1301312 - Part 2: Localize placeholders for <input type=time>. https://reviewboard.mozilla.org/r/114070/#review116228 > Yes, that is actually what I did. But the implementation of stringbundle uses Component.Classes to get nsIStringBundleService: > http://searchfox.org/mozilla-central/rev/c9ccb8ec1f5acda78e6344ffb87aa3a409031e3d/toolkit/content/widgets/stringbundle.xml#50 So, I did not update this patch cause it seems that we can't use stringbundle here.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #19) > > I tested Chrome with zh-TW, and the numeric fields are still displayed with arabic numbers. > > zh-TW is not a good example to test again. Try 'ar'. > > You should definitely format the output (the digits you will display) > because that's what the date time formatter will do as well. > > See: > > (new Date()).toLocaleString('ar'); > > If this is more widespread among date/time selectors, you may want to file a > separate bug and run it via UX team. Filed bug 1344624 for this. Thanks.
Comment on attachment 8839382 [details] Bug 1301312 - Part 1: Localize AM/PM strings for <input type=time>. https://reviewboard.mozilla.org/r/114068/#review119088 ::: toolkit/content/widgets/datetimebox.xml:582 (Diff revision 2) > + > + let amString, pmString; > + let keys = [ "dates/gregorian/dayperiods/am", > + "dates/gregorian/dayperiods/pm" ]; > + > + try { I'm not sure if wrapping the calls in try/catch is a good idea. Generally, the APIs are recently designed, on path for ECMA402 standardization and are intended to not throw without a good reason. Like, they won't throw on missing data. So, the only scenario when they can throw is when something serious happens, like, either callsite will provide wrong arguments, or some general failure happens. In both cases I feel like the code should not hide it, and it'll be much harder to debug. But I'll defer to :mconely to decide on the general approach.
Attachment #8839382 - Flags: review?(gandalf) → review+
Comment on attachment 8839382 [details] Bug 1301312 - Part 1: Localize AM/PM strings for <input type=time>. https://reviewboard.mozilla.org/r/114068/#review119100 ::: toolkit/content/widgets/datetimebox.xml:582 (Diff revision 2) > + > + let amString, pmString; > + let keys = [ "dates/gregorian/dayperiods/am", > + "dates/gregorian/dayperiods/pm" ]; > + > + try { Thanks zibi. The webapi does throw in other cases, like when jsapi fails or if mozIntl is not available (see https://bugzilla.mozilla.org/attachment.cgi?id=8843541). My intention was to fail silently, since this is an internal error, and use the default values as a fallback (see above).
> Thanks zibi. The webapi does throw in other cases, like when jsapi fails or if mozIntl is not available Unexpected jsapi failure indicates a serious system error. If you want to operate in a scenario where mozIntl is not available (Android build are the only scenario), then I believe you should do an explicit if-block instead of implicit try/catch. Wrapping up every API call in try in an attempt to recover seems to me like a bad strategy, but as I said I'll defer to :mconely for the decision.
Comment on attachment 8839384 [details] Bug 1301312 - Part 3: Order fields in <input type=time> based on locale datetime format. https://reviewboard.mozilla.org/r/114072/#review119316 ::: toolkit/content/widgets/datetimebox.xml:521 (Diff revision 2) > + } > > - let separator = document.createElementNS(HTML_NS, "span"); > - separator.textContent = aSeparatorText; > - separator.setAttribute("class", "datetime-separator"); > - container.insertBefore(separator, this.mSpaceSeparator); > + let hour, minute, second, millisecond; > + [hour, minute, second] = value.split(":"); > + if (second) { > + let index = second.indexOf("."); is this value normalized to be an en-US format (or ISO)? Because the separators are locale specific otherwise. ::: toolkit/content/widgets/datetimebox.xml:613 (Diff revision 2) > + break; > + default: > + // Remove bidirectional formatting characters, we'll handle > + // directions on our own. > + let value = part.value.replace( > + /[\u200e\u200f\u202a\u202b\u202c\u202d\u202e]/g, ""); can you explain this part? Why do you need to clear the BiDi marks?
Attachment #8839384 - Flags: review?(gandalf) → review-
Comment on attachment 8839384 [details] Bug 1301312 - Part 3: Order fields in <input type=time> based on locale datetime format. https://reviewboard.mozilla.org/r/114072/#review119322
Attachment #8839384 - Flags: review- → review?(gandalf)
Comment on attachment 8839384 [details] Bug 1301312 - Part 3: Order fields in <input type=time> based on locale datetime format. https://reviewboard.mozilla.org/r/114072/#review119316 > is this value normalized to be an en-US format (or ISO)? Because the separators are locale specific otherwise. This is based on the whatwg spec (which follows ISO 8601): https://html.spec.whatwg.org/multipage/infrastructure.html#valid-time-string > can you explain this part? Why do you need to clear the BiDi marks? When parsing date in 'ar', I found that there is a LRM character in front of the separator '/', which is output as: dd mm // yy I am removing the character cause we'll handle RTL in a separate bug using css flexbox. Note that, for time, there is no LRM character in front of ':', but I just added this for consistency.
Comment on attachment 8839383 [details] Bug 1301312 - Part 2: Localize placeholders for <input type=time>. https://reviewboard.mozilla.org/r/114070/#review119808 ::: toolkit/content/widgets/datetimebox.xml:461 (Diff revision 2) > + this.mHourPlaceHolder = ]]>"&time.hour.placeholder;"<![CDATA[; > + this.mMinutePlaceHolder = ]]>"&time.minute.placeholder;"<![CDATA[; > + this.mSecondPlaceHolder = ]]>"&time.second.placeholder;"<![CDATA[; > + this.mMillisecPlaceHolder = ]]>"&time.millisecond.placeholder;"<![CDATA[; > + this.mDayPeriodPlaceHolder = ]]>"&time.dayperiod.placeholder;"<![CDATA[; Out of curiosity, what happens if there's a " character inside the dtd string? Does it automatically escape? I've never done string insertion like this, so I can't say how it'll act with much confidence. :/ If there really isn't a better way to do this right now, we should at the very least file a new bug to find a right way, and reference that bug from this chunk of code.
Attachment #8839383 - Flags: review?(mconley) → review+
Comment on attachment 8839383 [details] Bug 1301312 - Part 2: Localize placeholders for <input type=time>. https://reviewboard.mozilla.org/r/114070/#review119808 > Out of curiosity, what happens if there's a " character inside the dtd string? Does it automatically escape? I've never done string insertion like this, so I can't say how it'll act with much confidence. :/ > > If there really isn't a better way to do this right now, we should at the very least file a new bug to find a right way, and reference that bug from this chunk of code. Having a " character inside the dtd string produces a XML Parsing Error, but single quote inside the dtd string would work in this case. Another thing we can do is to use 'property': ```xml <property name="mHourPlaceholder"> <getter> return "&time.hour.placeholder;"; </getter> </property> ``` But this doesn't solve the " character issue, it just makes the code prettier, I think?
Comment on attachment 8839383 [details] Bug 1301312 - Part 2: Localize placeholders for <input type=time>. https://reviewboard.mozilla.org/r/114070/#review119808 > Having a " character inside the dtd string produces a XML Parsing Error, but single quote inside the dtd string would work in this case. > > Another thing we can do is to use 'property': > > ```xml > <property name="mHourPlaceholder"> > <getter> > return "&time.hour.placeholder;"; > </getter> > </property> > ``` > > But this doesn't solve the " character issue, it just makes the code prettier, I think? Oh, but using '\&quot;' in dtd string works with both methods (the original and using property).
Comment on attachment 8839383 [details] Bug 1301312 - Part 2: Localize placeholders for <input type=time>. https://reviewboard.mozilla.org/r/114070/#review119808 > Oh, but using '\&quot;' in dtd string works with both methods (the original and using property). Alright - I guess that's typical for these .dtd things. Thanks for checking! Would it be possible to file a follow-up bug to try to find a better way of accessing these strings from the binding in the content scope?
Comment on attachment 8839384 [details] Bug 1301312 - Part 3: Order fields in <input type=time> based on locale datetime format. https://reviewboard.mozilla.org/r/114072/#review120522 This seems okay to me, so long as gandalf also gives his thumbs up, as he's likely to spot i18n stuff that I might miss. :) ::: toolkit/content/widgets/datetimebox.xml:521 (Diff revision 2) > - container.insertBefore(separator, this.mSpaceSeparator); > + let index = second.indexOf("."); > + if (index != -1) { > + millisecond = second.substring(index + 1); > + second = second.substring(0, index); > + } Alternatively: ```JS let [second, millisecond] = second.split("."); ``` ::: toolkit/content/widgets/datetimebox.xml:590 (Diff revision 2) > + > + let formatter = Intl.DateTimeFormat(this.mLocales, options); > + formatter.formatToParts(Date.now()).map(part => { > + switch (part.type) { > + case "hour": > + root.appendChild(this.mHourField); A suggestion here, so that we don't kick off layout stuff each time we add one of these things is to add each node to a document fragment, and then once you're done looping over the formatted parts, append the fragment to the edit-wrapper.
Attachment #8839384 - Flags: review?(mconley) → review+
Comment on attachment 8843845 [details] Bug 1301312 - Part 4: Order fields in <input type=date> based on locale datetime format. https://reviewboard.mozilla.org/r/117450/#review120528 ::: toolkit/content/widgets/datetimebox.xml:51 (Diff revision 1) > - this.mDayField.setAttribute("pginterval", this.mDayPageUpDownInterval); > - this.mYearField.setAttribute("min", this.mMinYear); > - this.mYearField.setAttribute("max", this.mMaxYear); > - this.mYearField.setAttribute("pginterval", > - this.mYearPageUpDownInterval); > + this.mYearPageUpDownInterval); > + this.mYearField.maxLength = this.mMaxYear.toString().length; You're already setting the `maxlength` inside `createEditField`. Why do you need to do it again here?
Attachment #8843845 - Flags: review?(mconley) → review+
Comment on attachment 8839384 [details] Bug 1301312 - Part 3: Order fields in <input type=time> based on locale datetime format. https://reviewboard.mozilla.org/r/114072/#review120580 ::: toolkit/content/widgets/datetimebox.xml:977 (Diff revision 2) > - value - this.mMaxHourInHour12 : value; > if (aValue == "00") { > - value = this.mMaxHourInHour12; > + value = this.mMaxHour; > + } else { > + // Change to 12hr format if user input is greater than 12. > + value = (value > this.mMaxHour) ? value % this.mMaxHour : value; one edge case here is that if the user typed "24" he most probably meant "12", not "0" which this will evaluate to.
Attachment #8839384 - Flags: review?(gandalf) → review+
Comment on attachment 8843845 [details] Bug 1301312 - Part 4: Order fields in <input type=date> based on locale datetime format. https://reviewboard.mozilla.org/r/117450/#review120582 ::: toolkit/content/widgets/datetimebox.xml:83 (Diff revision 1) > + day: "numeric" > + }); > + formatter.formatToParts(Date.now()).map(part => { > + switch (part.type) { > + case "year": > + root.appendChild(this.mYearField); same suggestion as :mconley had in the previous patch. Let's use a DocumentFragment here and append once we're done. ::: toolkit/content/widgets/datetimebox.xml:95 (Diff revision 1) > + break; > + default: > + // Remove bidirectional formatting characters, we'll handle > + // directions on our own. > + let value = part.value.replace( > + /[\u200e\u200f\u202a\u202b\u202c\u202d\u202e]/g, ""); can you factor this out (and in the other patch) into a function like `stripDirFormattingChars` and document it with links to UAX9 - http://www.unicode.org/reports/tr9/#Directional_Formatting_Characters
Attachment #8843845 - Flags: review?(gandalf) → review+
The overall code looks good! Two things that I'd like to make explicit are: 1) I'm concerned about the catch-all try/catch since a) mozIntl API is designed not to throw unless something critical happens b) the way I was taught to think about code is that if you don't know how your code may throw, you should not assume you can handle it in `catch` so, it's not very different from placing Math.max in try/catch. Sure, you may avoid breaking your code in this call but if this thing throw, you maybe should. The most likely reason it'll throw is that it got wrong arguments passed, and this try/catch will silence it and make it impossible to ever discover the bug. 2) I'd like to suggest we do additional testing on RTL locales (L10n Drivers team can help you connect with the RTL localizers who can help) once we have this, because bidi is very fragile and we take over control over it here. :) None of those are blocking, but just wanted to state them explicitly
Comment on attachment 8839383 [details] Bug 1301312 - Part 2: Localize placeholders for <input type=time>. https://reviewboard.mozilla.org/r/114070/#review119808 > Alright - I guess that's typical for these .dtd things. Thanks for checking! > > Would it be possible to file a follow-up bug to try to find a better way of accessing these strings from the binding in the content scope? Sure, filed Bug 1346114 for this.
Comment on attachment 8839384 [details] Bug 1301312 - Part 3: Order fields in <input type=time> based on locale datetime format. https://reviewboard.mozilla.org/r/114072/#review120580 > one edge case here is that if the user typed "24" he most probably meant "12", not "0" which this will evaluate to. In 12hr format, if user enters "2", the cursor will jump automatically to the next field, cause it's impossible to have a hour of 2X in a 12hr clock. So, this is only for "13" - "19".
Comment on attachment 8843845 [details] Bug 1301312 - Part 4: Order fields in <input type=date> based on locale datetime format. https://reviewboard.mozilla.org/r/117450/#review120582 > can you factor this out (and in the other patch) into a function like `stripDirFormattingChars` and document it with links to UAX9 - http://www.unicode.org/reports/tr9/#Directional_Formatting_Characters Sure, will do.
Comment on attachment 8843845 [details] Bug 1301312 - Part 4: Order fields in <input type=date> based on locale datetime format. https://reviewboard.mozilla.org/r/117450/#review120528 > You're already setting the `maxlength` inside `createEditField`. Why do you need to do it again here? Because createEditField takes the same parameter for maxlength and size, and we are giving it 4 (this.mYearLength), however we want size to be 4 but maxlength to be the length of the maximum year, which is 275760.
Comment on attachment 8839384 [details] Bug 1301312 - Part 3: Order fields in <input type=time> based on locale datetime format. https://reviewboard.mozilla.org/r/114072/#review120522 > Alternatively: > > ```JS > let [second, millisecond] = second.split("."); > ``` Right, will do. > A suggestion here, so that we don't kick off layout stuff each time we add one of these things is to add each node to a document fragment, and then once you're done looping over the formatted parts, append the fragment to the edit-wrapper. That's a good idea, will do.
Thanks Zibi and Mike for all your suggestions and review. (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #43) > The overall code looks good! Two things that I'd like to make explicit are: > > 1) I'm concerned about the catch-all try/catch since > a) mozIntl API is designed not to throw unless something critical happens > b) the way I was taught to think about code is that if you don't know how > your code may throw, you should not assume you can handle it in `catch` > > so, it's not very different from placing Math.max in try/catch. Sure, you > may avoid breaking your code in this call but if this thing throw, you maybe > should. > > The most likely reason it'll throw is that it got wrong arguments passed, > and this try/catch will silence it and make it impossible to ever discover > the bug. You convinced me :) We shoudn't fail silently if wrong arguments were passed or other serious failures. I got rid of the try/catch statement in the latest patch. > > 2) I'd like to suggest we do additional testing on RTL locales (L10n Drivers > team can help you connect with the RTL localizers who can help) once we have > this, because bidi is very fragile and we take over control over it here. :) I agree, we know so little about RTL locales and how their date/time should look like. I'll ask our PM's (Wesley Huang)'s help for this once bug 1346085 and bug 1344624 are landed. > > None of those are blocking, but just wanted to state them explicitly Thanks again!
So I ran into some try failures [1] due to: - "xbl:inherits" does not work with dynamically added input text boxes. We'll need to find a way to propagate attribute changes (tabindex, readonly, etc). - In `rebuildEditFields` we are removing all children from the "edit-wrapper" root, and some removed elements are reused later. This will cause this assertion: "Must have binding parent when in native anonymous subtree which is in document". We'll recreate the elements and not reuse them. - Other eslint errors. Will upload new updated patches to fix these. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=56c7dda59b308b616973c1b503a3acd3112f6bd2
Comment on attachment 8839384 [details] Bug 1301312 - Part 3: Order fields in <input type=time> based on locale datetime format. https://reviewboard.mozilla.org/r/114072/#review121450 ::: toolkit/content/widgets/datetimebox.xml:562 (Diff revision 4) > + this.mHourField = this.createEditField(this.mHourPlaceHolder, > + this.mMaxLength, true, this.mMinHour, this.mMaxHour, > + this.mHourPageUpDownInterval); > + this.mMinuteField = this.createEditField(this.mMinutePlaceHolder, > + this.mMaxLength, true, this.mMinMinute, this.mMaxMinute, > + this.mMinSecPageUpDownInterval); I have moved the creation of `mHourField` and `mMinuteField` here, cause in `rebuildEditFields()`, we remove all child elements, and then append `mHourField` and `mMinuteField` again, which hits the following assertion: ASSERTION: Must have binding parent when in native anonymous subtree which is in document. To fix this, we create new elements each time.
Hi Mike, since I made some changes to fix try failures, can I have your last check? See comment 60 for details. Thanks, and sorry, should have made sure try passes first. :(
Flags: needinfo?(mconley)
Comment on attachment 8846512 [details] Bug 1301312 - Part 5: Handle input element's attribute change explicitly. https://reviewboard.mozilla.org/r/119574/#review121476 ::: layout/forms/nsDateTimeControlFrame.cpp:376 (Diff revision 1) > nsDateTimeControlFrame::AttributeChanged(int32_t aNameSpaceID, > nsIAtom* aAttribute, > int32_t aModType) > { > NS_ASSERTION(mInputAreaContent, "The input area content must exist!"); > + nsCOMPtr<nsIDateTimeInputArea> inputAreaContent = QI is a bit slow, so could you do this only in case it is known that SetEditAttribute or RemoveEditAttribute will be called. ::: toolkit/content/widgets/datetimebox.xml:1299 (Diff revision 1) > + } > + > + let editRoot = > + document.getAnonymousElementByAttribute(this, "anonid", "edit-wrapper"); > + > + for (let i = 0; i < editRoot.children.length; i++) { Nit, could you rather access access editRoot's firstChild and from that then nextSibling or so. That would avoid creating children object. for (let child = editRoot.firstChild; child; child = child.nextSibling) { .... } ::: toolkit/content/widgets/datetimebox.xml:1323 (Diff revision 1) > + } > + > + let editRoot = > + document.getAnonymousElementByAttribute(this, "anonid", "edit-wrapper"); > + > + for (let i = 0; i < editRoot.children.length; i++) { same here
Attachment #8846512 - Flags: review?(bugs) → review+
Comment on attachment 8846512 [details] Bug 1301312 - Part 5: Handle input element's attribute change explicitly. https://reviewboard.mozilla.org/r/119574/#review121480
Still seems okay. Thanks for checking!
Flags: needinfo?(mconley)
Comment on attachment 8846512 [details] Bug 1301312 - Part 5: Handle input element's attribute change explicitly. https://reviewboard.mozilla.org/r/119574/#review121476 > QI is a bit slow, so could you do this only in case it is known that SetEditAttribute or RemoveEditAttribute will be called. Sure, will do. > Nit, could you rather access access > editRoot's firstChild and from that then > nextSibling or so. That would avoid creating children object. > > > > for (let child = editRoot.firstChild; child; child = child.nextSibling) { > .... > } Will do. > same here Will do.
Heads up that bug 1346819 will land tonight and will require a minor update in part 1 of this patch (you'll want to go for BCP47)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #66) > Heads up that bug 1346819 will land tonight and will require a minor update > in part 1 of this patch (you'll want to go for BCP47) Thanks for the heads up. If it's going to land soon, I can wait for it and rebase on top of it. :)
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a5a2ead687cb Part 1: Localize AM/PM strings for <input type=time>. r=gandalf,mconley https://hg.mozilla.org/integration/autoland/rev/d5be3275c987 Part 2: Localize placeholders for <input type=time>. r=mconley https://hg.mozilla.org/integration/autoland/rev/66b7075919c9 Part 3: Order fields in <input type=time> based on locale datetime format. r=gandalf,mconley https://hg.mozilla.org/integration/autoland/rev/8fda74ef8da6 Part 4: Order fields in <input type=date> based on locale datetime format. r=gandalf,mconley https://hg.mozilla.org/integration/autoland/rev/6d303d1651cc Part 5: Handle input element's attribute change explicitly. r=smaug
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: