Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2108813002:
Added strings of i18n and made the pin-keyboard work for rtl lang. (Closed)
|
Created:
4 years, 5 months ago by sammiequon Modified:
4 years, 5 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, dzhioev+watch_chromium.org, achuith+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded strings of i18n and made the pin-keyboard work for rtl lang.
BUG=622785, 623622
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/23157c0328600c84af5880c229c25d92a34816df
Cr-Commit-Position: refs/heads/master@{#403945}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Rebased. #Patch Set 3 : Fixed patch set 1 errors. #
Total comments: 5
Patch Set 4 : Fixed patch set 3 errors. #
Total comments: 27
Patch Set 5 : Fixed patch set 4 error. #
Total comments: 16
Patch Set 6 : Fixed patch set 5 errors. #
Total comments: 4
Patch Set 7 : Fixed patch set 6 errors. #
Total comments: 4
Patch Set 8 : Fixed jsdoc. #Patch Set 9 : Added comment. #
Total comments: 1
Patch Set 10 : Rebased. #Patch Set 11 : Rebased. #
Messages
Total messages: 47 (17 generated)
Description was changed from ========== Added strings of i18n and made the pin-keyboard work for rtl lang. BUG=622785 ========== to ========== Added strings of i18n and made the pin-keyboard work for rtl lang. BUG=622785 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Added strings of i18n and made the pin-keyboard work for rtl lang. BUG=622785 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Added strings of i18n and made the pin-keyboard work for rtl lang. BUG=622785 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
sammiequon@chromium.org changed reviewers: + jdufault@chromium.org
jdufault@ - Please take a look.
https://codereview.chromium.org/2108813002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2108813002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:13187: + <message name="IDS_LOGIN_POD_EMPTY_PIN_TEXT" desc="Text to display in the pin field for user pod when no pin has been entered."> The PIN keyboard is not inherently related to login; what about making these non-login specific so they are IDS_PIN_KEYBOARD_HINT_TEXT_PIN_PASSWORD IDS_PIN_KEYBOARD_HINT_TEXT_PIN IDS_PIN_KEYBOARD_CLEAR Then all of the PIN keyboard strings are located next to each-other and shared between settings and login. https://codereview.chromium.org/2108813002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2108813002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:109: placeholder="$i18n{pinPlaceholder}" Move this up or down a line so that the value/on-keydown bindings stay together. https://codereview.chromium.org/2108813002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2108813002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:61: changeRTLIfNeeded_: function(password) { What about updateRtlState? changeRTL implies a one-way mapping. I'd also change RTL to Rtl, since it makes the function name easier to read. https://codereview.chromium.org/2108813002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:63: var shouldRtl = !regex.test(password); What about Number.isInteger(+password)? +password converts password to a number; it gives NaN if it fails. The Number.isInteger check verifies there are no periods/etc. https://codereview.chromium.org/2108813002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/2108813002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:375: builder->Add("pinPlaceholder", IDS_LOGIN_POD_EMPTY_PIN_TEXT); Please change pinKeyboard* to pin* or pin* to pinKeyboard* so they all have the same prefix. https://codereview.chromium.org/2108813002/diff/1/ui/login/account_picker/use... File ui/login/account_picker/user_pod_template.html (right): https://codereview.chromium.org/2108813002/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_template.html:14: <pin-keyboard type="password"></pin-keyboard> What about an enable-password boolean property?
https://codereview.chromium.org/2108813002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2108813002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:13187: + <message name="IDS_LOGIN_POD_EMPTY_PIN_TEXT" desc="Text to display in the pin field for user pod when no pin has been entered."> On 2016/06/28 18:38:47, jdufault wrote: > The PIN keyboard is not inherently related to login; what about making these > non-login specific so they are > > IDS_PIN_KEYBOARD_HINT_TEXT_PIN_PASSWORD > IDS_PIN_KEYBOARD_HINT_TEXT_PIN > IDS_PIN_KEYBOARD_CLEAR > > Then all of the PIN keyboard strings are located next to each-other and shared > between settings and login. Done. https://codereview.chromium.org/2108813002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2108813002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:109: placeholder="$i18n{pinPlaceholder}" On 2016/06/28 18:38:47, jdufault wrote: > Move this up or down a line so that the value/on-keydown bindings stay together. Done. https://codereview.chromium.org/2108813002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2108813002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:61: changeRTLIfNeeded_: function(password) { On 2016/06/28 18:38:47, jdufault wrote: > What about updateRtlState? changeRTL implies a one-way mapping. > > I'd also change RTL to Rtl, since it makes the function name easier to read. Done. https://codereview.chromium.org/2108813002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:63: var shouldRtl = !regex.test(password); On 2016/06/28 18:38:47, jdufault wrote: > What about Number.isInteger(+password)? > > +password converts password to a number; it gives NaN if it fails. The > Number.isInteger check verifies there are no periods/etc. Done. https://codereview.chromium.org/2108813002/diff/1/ui/login/account_picker/use... File ui/login/account_picker/user_pod_template.html (right): https://codereview.chromium.org/2108813002/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_template.html:14: <pin-keyboard type="password"></pin-keyboard> On 2016/06/28 18:38:47, jdufault wrote: > What about an enable-password boolean property? Done. Does not seem to let me use - or capitalize p.
https://codereview.chromium.org/2108813002/diff/1/ui/login/account_picker/use... File ui/login/account_picker/user_pod_template.html (right): https://codereview.chromium.org/2108813002/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_template.html:14: <pin-keyboard type="password"></pin-keyboard> On 2016/06/28 20:59:31, sammiequon wrote: > On 2016/06/28 18:38:47, jdufault wrote: > > What about an enable-password boolean property? > > Done. Does not seem to let me use - or capitalize p. See the "Property name to attribute name mapping" section in [1]. Basically, the polymer property name should be enablePassword, which is used here as enable-password. 1: https://www.polymer-project.org/1.0/docs/devguide/properties https://codereview.chromium.org/2108813002/diff/40001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2108813002/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:1193: Enter PIN Merge these strings with the ones in generated_resources.grd. https://codereview.chromium.org/2108813002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2108813002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:62: var shouldRtl = !Number.isInteger(+password); Add a comment describing this behavior. shouldRtl => isNumeric https://codereview.chromium.org/2108813002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:111: if (value != previous && value != null) { Why do you need this check? Do we need the value of previous?
https://codereview.chromium.org/2108813002/diff/1/ui/login/account_picker/use... File ui/login/account_picker/user_pod_template.html (right): https://codereview.chromium.org/2108813002/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_template.html:14: <pin-keyboard type="password"></pin-keyboard> On 2016/06/28 21:13:22, jdufault wrote: > On 2016/06/28 20:59:31, sammiequon wrote: > > On 2016/06/28 18:38:47, jdufault wrote: > > > What about an enable-password boolean property? > > > > Done. Does not seem to let me use - or capitalize p. > > See the "Property name to attribute name mapping" section in [1]. > > Basically, the polymer property name should be enablePassword, which is used > here as enable-password. > > 1: https://www.polymer-project.org/1.0/docs/devguide/properties Done. https://codereview.chromium.org/2108813002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2108813002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:62: var shouldRtl = !Number.isInteger(+password); On 2016/06/28 21:13:22, jdufault wrote: > Add a comment describing this behavior. > > shouldRtl => isNumeric Done. https://codereview.chromium.org/2108813002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:111: if (value != previous && value != null) { On 2016/06/28 21:13:22, jdufault wrote: > Why do you need this check? Do we need the value of previous? Done.
https://codereview.chromium.org/2108813002/diff/60001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2108813002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:13187: + <message name="IDS_PIN_KEYBOARD_HINT_TEXT_PIN" desc="Text to display in the pin field for user pod when no pin has been entered."> Please move these strings so they do not split up the IDS_LOGIN_POD_* section. https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:29: enablePassword: { Document this https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:32: observer: 'onInputTypeChange_' nit: rename to onEnablePasswordChange_ https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:35: placeholder: { This should be private; the value for this should be driven by enablePassword. Make sure to add /** @private */ once those changes are made. https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:37: value: '', Since enablePassword defaults to false this should probably default to '$i18n{pinKeyboardPlaceholderPin}' https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:38: observer: 'onPlaceHolderChange_' Instead of using an observer to update the element just use a binding. 1. Add this to the HTML: placeholder="[[placeholder_]]" 2. Remove onPlaceHolderChange_ https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:69: // Converts a password to number, then verifies there are no periods/etc. Instead of hooking into the pin-change event, we can bind the class so the class is reevaluated whenever the `input` property changes. <input class="[[computeRtlClass_(value)]]"></input> Then computeRtlClass_ will return 'ltr' if the text should be ltr or just '' if it is rtl. (assuming pin-keyboard-input-non-pin is moved into pin_keyboard.html and renamed to ltr) https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:119: this.$$('#pin-input').type = 'password'; Let's bind the input type to a property. The property will look like inputType_: { type: String, value: 'password' } Then this can be simplified to this.inputType_ = value ? 'password' : 'number'; The HTML will be updated to look like <input id="pin-input" type="[[inputType_]]"></input> https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/people_page.html (right): https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/people_page/people_page.html:165: <pin-keyboard placeholder="$i18n{pinKeyboardPlaceholderPin}"> Drop this change; placeholder should be driven by the pin-keyboard element and not externally modifiable. https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:376: IDS_PIN_KEYBOARD_HINT_TEXT_PIN_PASSWORD); Also export pinKeyboardPlaceholderPin https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc (right): https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:750: {"pinKeyboardPlaceholderPin", IDS_PIN_KEYBOARD_HINT_TEXT_PIN}, Also export pinKeyboardPlaceholderPinKeyboard https://codereview.chromium.org/2108813002/diff/60001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2108813002/diff/60001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:69: html[dir=rtl] .pin-keyboard-input-non-pin { This should be in the pin css. https://codereview.chromium.org/2108813002/diff/60001/ui/login/account_picker... File ui/login/account_picker/user_pod_template.html (right): https://codereview.chromium.org/2108813002/diff/60001/ui/login/account_picker... ui/login/account_picker/user_pod_template.html:14: <pin-keyboard placeholder="$i18n{pinKeyboardPlaceholderPinPassword}" Remove placeholder attribute, have the pin-keyboard control it. https://codereview.chromium.org/2108813002/diff/60001/ui/login/account_picker... ui/login/account_picker/user_pod_template.html:15: enable-password="true"> nit: just enable-password
https://codereview.chromium.org/2108813002/diff/60001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2108813002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:13187: + <message name="IDS_PIN_KEYBOARD_HINT_TEXT_PIN" desc="Text to display in the pin field for user pod when no pin has been entered."> On 2016/06/28 23:06:09, jdufault wrote: > Please move these strings so they do not split up the IDS_LOGIN_POD_* section. Done. https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:29: enablePassword: { On 2016/06/28 23:06:09, jdufault wrote: > Document this Done. https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:32: observer: 'onInputTypeChange_' On 2016/06/28 23:06:09, jdufault wrote: > nit: rename to onEnablePasswordChange_ Done. https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:35: placeholder: { On 2016/06/28 23:06:09, jdufault wrote: > This should be private; the value for this should be driven by enablePassword. > Make sure to add /** @private */ once those changes are made. Done. https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:37: value: '', On 2016/06/28 23:06:09, jdufault wrote: > Since enablePassword defaults to false this should probably default to > '$i18n{pinKeyboardPlaceholderPin}' I do not think this works, my current default is Enter PIN, which should always be overrided by onEnablePasswordChange_. https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:38: observer: 'onPlaceHolderChange_' On 2016/06/28 23:06:09, jdufault wrote: > Instead of using an observer to update the element just use a binding. > > 1. Add this to the HTML: > > placeholder="[[placeholder_]]" > > 2. Remove onPlaceHolderChange_ Done. https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:69: // Converts a password to number, then verifies there are no periods/etc. On 2016/06/28 23:06:09, jdufault wrote: > Instead of hooking into the pin-change event, we can bind the class so the class > is reevaluated whenever the `input` property changes. > > <input class="[[computeRtlClass_(value)]]"></input> > > Then computeRtlClass_ will return 'ltr' if the text should be ltr or just '' if > it is rtl. > > (assuming pin-keyboard-input-non-pin is moved into pin_keyboard.html and renamed > to ltr) Done. https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:119: this.$$('#pin-input').type = 'password'; On 2016/06/28 23:06:09, jdufault wrote: > Let's bind the input type to a property. The property will look like > > inputType_: { > type: String, > value: 'password' > } > > Then this can be simplified to > > this.inputType_ = value ? 'password' : 'number'; > > The HTML will be updated to look like > > <input id="pin-input" type="[[inputType_]]"></input> Done. https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/people_page.html (right): https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/people_page/people_page.html:165: <pin-keyboard placeholder="$i18n{pinKeyboardPlaceholderPin}"> On 2016/06/28 23:06:10, jdufault wrote: > Drop this change; placeholder should be driven by the pin-keyboard element and > not externally modifiable. Done. https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:376: IDS_PIN_KEYBOARD_HINT_TEXT_PIN_PASSWORD); On 2016/06/28 23:06:10, jdufault wrote: > Also export pinKeyboardPlaceholderPin Done. https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc (right): https://codereview.chromium.org/2108813002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:750: {"pinKeyboardPlaceholderPin", IDS_PIN_KEYBOARD_HINT_TEXT_PIN}, On 2016/06/28 23:06:10, jdufault wrote: > Also export pinKeyboardPlaceholderPinKeyboard Done. https://codereview.chromium.org/2108813002/diff/60001/ui/login/account_picker... File ui/login/account_picker/user_pod_template.html (right): https://codereview.chromium.org/2108813002/diff/60001/ui/login/account_picker... ui/login/account_picker/user_pod_template.html:14: <pin-keyboard placeholder="$i18n{pinKeyboardPlaceholderPinPassword}" On 2016/06/28 23:06:10, jdufault wrote: > Remove placeholder attribute, have the pin-keyboard control it. Done. https://codereview.chromium.org/2108813002/diff/60001/ui/login/account_picker... ui/login/account_picker/user_pod_template.html:15: enable-password="true"> On 2016/06/28 23:06:10, jdufault wrote: > nit: just enable-password Done.
https://codereview.chromium.org/2108813002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2108813002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:19: .pin-keyboard-input-non-pin { Drop the 'pin-keyboard-' prefix. https://codereview.chromium.org/2108813002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:113: class="[[computeRtlClass_(value)]]" Maybe computeInputClass_? https://codereview.chromium.org/2108813002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2108813002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:35: * or passowrd. nit: password https://codereview.chromium.org/2108813002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:87: * password. */ Maybe move this function definition to the bottom of the file so that the pin value logic stays together? https://codereview.chromium.org/2108813002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:89: // Converts a password to number, then verifies there are no periods/etc. Can you make the comment be a bit more specific? Maybe something like this: // +password will convert a string to a number or to NaN if that's not // possible. Number.isInteger will verify the value is not a NaN and that it // does not contain decimals. // // This heuristic will fail for inputs like '1.0'. I wonder if the regex is a better solution, since this heuristic does have a few failure cases. Up to you which approach you want to go with. Sorry about the churn if you end up reverting to a regex. https://codereview.chromium.org/2108813002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:91: var enableRtl = !isNumeric && (document.dir == 'rtl'); I'd put the document.dir == 'rtl' condition first since it is more general. https://codereview.chromium.org/2108813002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:93: return newClass; Eliminate the newClass var and directly return the expression. https://codereview.chromium.org/2108813002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:138: this.placeholder_ = value ? this.i18n('pinKeyboardPlaceholderPinPassword') : The placeholder_ and inputType_ can be removed if they become computed values instead. So follow the same pattern as the class; ie, <input type="[[computeInputType_(enablePassword)]]" placeholder="[[computeInputPlaceholder_(enablePassword)]]">
https://codereview.chromium.org/2108813002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2108813002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:19: .pin-keyboard-input-non-pin { On 2016/06/29 21:06:29, jdufault wrote: > Drop the 'pin-keyboard-' prefix. Done. https://codereview.chromium.org/2108813002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:113: class="[[computeRtlClass_(value)]]" On 2016/06/29 21:06:29, jdufault wrote: > Maybe computeInputClass_? Done. https://codereview.chromium.org/2108813002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2108813002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:35: * or passowrd. On 2016/06/29 21:06:29, jdufault wrote: > nit: password Done. https://codereview.chromium.org/2108813002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:87: * password. */ On 2016/06/29 21:06:29, jdufault wrote: > Maybe move this function definition to the bottom of the file so that the pin > value logic stays together? Done. https://codereview.chromium.org/2108813002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:89: // Converts a password to number, then verifies there are no periods/etc. On 2016/06/29 21:06:29, jdufault wrote: > Can you make the comment be a bit more specific? Maybe something like this: > > // +password will convert a string to a number or to NaN if that's not > // possible. Number.isInteger will verify the value is not a NaN and that it > // does not contain decimals. > // > // This heuristic will fail for inputs like '1.0'. > > I wonder if the regex is a better solution, since this heuristic does have a few > failure cases. Up to you which approach you want to go with. Sorry about the > churn if you end up reverting to a regex. Done. https://codereview.chromium.org/2108813002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:91: var enableRtl = !isNumeric && (document.dir == 'rtl'); On 2016/06/29 21:06:29, jdufault wrote: > I'd put the document.dir == 'rtl' condition first since it is more general. Done. https://codereview.chromium.org/2108813002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:93: return newClass; On 2016/06/29 21:06:29, jdufault wrote: > Eliminate the newClass var and directly return the expression. Done. https://codereview.chromium.org/2108813002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:138: this.placeholder_ = value ? this.i18n('pinKeyboardPlaceholderPinPassword') : On 2016/06/29 21:06:29, jdufault wrote: > The placeholder_ and inputType_ can be removed if they become computed values > instead. > > So follow the same pattern as the class; ie, > > <input type="[[computeInputType_(enablePassword)]]" > placeholder="[[computeInputPlaceholder_(enablePassword)]]"> Done.
lgtm after comments are addressed https://codereview.chromium.org/2108813002/diff/100001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2108813002/diff/100001/chrome/app/generated_r... chrome/app/generated_resources.grd:13178: + <message name="IDS_PIN_KEYBOARD_HINT_TEXT_PIN" desc="Text to display in the pin field for user pod when no pin has been entered."> Is this in a cros-only section? If not, they should be wrapped in <if expr="chromeos"> ... </if> https://codereview.chromium.org/2108813002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2108813002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:104: * numerical. All three methods should have an @private annotation.
Description was changed from ========== Added strings of i18n and made the pin-keyboard work for rtl lang. BUG=622785 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Added strings of i18n and made the pin-keyboard work for rtl lang. BUG=622785, 623622 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
sammiequon@chromium.org changed reviewers: + xiyuan@chromium.org
xiyuan@ - Please take a look. https://codereview.chromium.org/2108813002/diff/100001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2108813002/diff/100001/chrome/app/generated_r... chrome/app/generated_resources.grd:13178: + <message name="IDS_PIN_KEYBOARD_HINT_TEXT_PIN" desc="Text to display in the pin field for user pod when no pin has been entered."> On 2016/06/30 18:22:12, jdufault wrote: > Is this in a cros-only section? If not, they should be wrapped in > > <if expr="chromeos"> > ... > </if> Done. https://codereview.chromium.org/2108813002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2108813002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:104: * numerical. On 2016/06/30 18:22:12, jdufault wrote: > All three methods should have an @private annotation. Done.
https://codereview.chromium.org/2108813002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2108813002/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:103: * @private nit: move this to the end of jsdoc, here and other places. https://codereview.chromium.org/2108813002/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:131: return enableRtl ? 'input-non-pin' : ''; I am not quite follow. What are we trying to do here? Switching to RTL when input is not a number? If so, what happens when Chrome uses a RTL lang such as "ar"?
https://codereview.chromium.org/2108813002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2108813002/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:131: return enableRtl ? 'input-non-pin' : ''; On 2016/06/30 22:54:14, xiyuan wrote: > I am not quite follow. What are we trying to do here? Switching to RTL when > input is not a number? If so, what happens when Chrome uses a RTL lang such as > "ar"? The complication here is that the PIN keyboard accepts both a password and a PIN. If the user is entering a PIN, input should always be LTR. If the user is entering a password, input should be RTL in RTL languages.
https://codereview.chromium.org/2108813002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2108813002/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:131: return enableRtl ? 'input-non-pin' : ''; On 2016/06/30 22:58:19, jdufault wrote: > On 2016/06/30 22:54:14, xiyuan wrote: > > I am not quite follow. What are we trying to do here? Switching to RTL when > > input is not a number? If so, what happens when Chrome uses a RTL lang such as > > "ar"? > > The complication here is that the PIN keyboard accepts both a password and a > PIN. > > If the user is entering a PIN, input should always be LTR. > > If the user is entering a password, input should be RTL in RTL languages. emm, would the default behavior of number input work properly? Think chrome should handle bidi and show numbers as LTR. Why we need to explicitly set to LTR for pin input? And with "direction: ltr" on #root, would that work properly for the digit buttons on rtl?
On 2016/06/30 23:09:34, xiyuan wrote: > https://codereview.chromium.org/2108813002/diff/120001/chrome/browser/resourc... > File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): > > https://codereview.chromium.org/2108813002/diff/120001/chrome/browser/resourc... > chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:131: return > enableRtl ? 'input-non-pin' : ''; > On 2016/06/30 22:58:19, jdufault wrote: > > On 2016/06/30 22:54:14, xiyuan wrote: > > > I am not quite follow. What are we trying to do here? Switching to RTL when > > > input is not a number? If so, what happens when Chrome uses a RTL lang such > as > > > "ar"? > > > > The complication here is that the PIN keyboard accepts both a password and a > > PIN. > > > > If the user is entering a PIN, input should always be LTR. > > > > If the user is entering a password, input should be RTL in RTL languages. > > emm, would the default behavior of number input work properly? Think chrome > should handle bidi and show numbers as LTR. Why we need to explicitly set to LTR > for pin input? > > And with "direction: ltr" on #root, would that work properly for the digit > buttons on rtl? ltr on root because we want the pin to always be ltr regardless on language. Since we still support them entering their password using the PIN method, we swap to RTL when we think it is a password, if the language is RTL.
On 2016/07/01 15:18:47, sammiequon wrote: > On 2016/06/30 23:09:34, xiyuan wrote: > > > https://codereview.chromium.org/2108813002/diff/120001/chrome/browser/resourc... > > File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): > > > > > https://codereview.chromium.org/2108813002/diff/120001/chrome/browser/resourc... > > chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:131: return > > enableRtl ? 'input-non-pin' : ''; > > On 2016/06/30 22:58:19, jdufault wrote: > > > On 2016/06/30 22:54:14, xiyuan wrote: > > > > I am not quite follow. What are we trying to do here? Switching to RTL > when > > > > input is not a number? If so, what happens when Chrome uses a RTL lang > such > > as > > > > "ar"? > > > > > > The complication here is that the PIN keyboard accepts both a password and a > > > PIN. > > > > > > If the user is entering a PIN, input should always be LTR. > > > > > > If the user is entering a password, input should be RTL in RTL languages. > > > > emm, would the default behavior of number input work properly? Think chrome > > should handle bidi and show numbers as LTR. Why we need to explicitly set to > LTR > > for pin input? > > > > And with "direction: ltr" on #root, would that work properly for the digit > > buttons on rtl? > > ltr on root because we want the pin to always be ltr regardless on language. > Since we > still support them entering their password using the PIN method, we swap to RTL > when we think > it is a password, if the language is RTL. Thanks for the clarification. LGTM
On 2016/07/01 15:18:47, sammiequon wrote: > ltr on root because we want the pin to always be ltr regardless on language. > Since we > still support them entering their password using the PIN method, we swap to RTL > when we think > it is a password, if the language is RTL. Can you add a comment explaining this?
The CQ bit was checked by sammiequon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jdufault@chromium.org, xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/2108813002/#ps160001 (title: "Added comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: closure_compilation on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
https://codereview.chromium.org/2108813002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2108813002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:131: // keyboard, we swap the input box to rtl when we think it is a password The comment I meant was along the lines of the why ltr is on the document root. You can revert this comment to the original. So add a cleaned up version of the below to pin_keyboard.html around line 14: Add ltr on the root element because the PIN keyboard should always be laid out ltr regardless of the language. However, if the user is entering a password, the password should be rtl, which is implemented by dynamically adding/removing the input-non-pin class. This also makes me think that input-non-pin should always be added and the rtl check should happen inside the CSS. Though if that's a lot of work it's fine as is.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
let me fix up some quick_unlock closure stuff real quickly
after this lands, your closure_compilation try runs should pass: https://codereview.chromium.org/2114803004/
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_gyp_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gyp_...)
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by sammiequon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, jdufault@chromium.org Link to the patchset: https://codereview.chromium.org/2108813002/#ps200001 (title: "Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Added strings of i18n and made the pin-keyboard work for rtl lang. BUG=622785, 623622 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Added strings of i18n and made the pin-keyboard work for rtl lang. BUG=622785, 623622 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Added strings of i18n and made the pin-keyboard work for rtl lang. BUG=622785, 623622 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Added strings of i18n and made the pin-keyboard work for rtl lang. BUG=622785, 623622 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/23157c0328600c84af5880c229c25d92a34816df Cr-Commit-Position: refs/heads/master@{#403945} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/23157c0328600c84af5880c229c25d92a34816df Cr-Commit-Position: refs/heads/master@{#403945} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
