Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2822733002:
md settings: Small changes to lock screen. (Closed)
|
Created:
3 years, 8 months ago by sammiequon Modified:
3 years, 8 months ago CC:
chromium-reviews, vabr+watchlistpasswordmanager_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, gcasto+watchlist_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionmd settings: Small changes to lock screen.
A couple of minor touchups.
1) Focus the input element when opening the password prompt for the first time.
2) "Setup PIN" link gets focused after closing the setup pin dialog and shows a large blue border around the whole link box; removed border and bolded the text to indicate focus.
3) Setup pin dialog will bounce up after entering the first number. This may be annoying if someone trying to click/touch 88 and accidently enter 84 instead. Makes the problem div invisible instead. Alternatively we can just show "PIN should be N length" right away.
TEST=manual
BUG=703998
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Patch Set 1 #
Total comments: 12
Patch Set 2 : Fixed patch set 1 errors. #Patch Set 3 : Removed link related css. #
Messages
Total messages: 11 (3 generated)
Description was changed from ========== md settings: Small changes to lock screen. A couple of minor touchups. 1) Focus the input element when opening the password prompt for the first time. 2) "Setup PIN" link gets focused after closing the setup pin dialog and shows a large blue border around the whole link box; removed border and bolded the text to indicate focus. 3) Setup pin dialog will bounce up after entering the first number. This may be annoying if someone trying to click/touch 88 and accidently enter 84 instead. Makes the problem div invisible instead. Alternatively we can just show "PIN should be N length" right away. TEST=manual BUG=703998 ========== to ========== md settings: Small changes to lock screen. A couple of minor touchups. 1) Focus the input element when opening the password prompt for the first time. 2) "Setup PIN" link gets focused after closing the setup pin dialog and shows a large blue border around the whole link box; removed border and bolded the text to indicate focus. 3) Setup pin dialog will bounce up after entering the first number. This may be annoying if someone trying to click/touch 88 and accidently enter 84 instead. Makes the problem div invisible instead. Alternatively we can just show "PIN should be N length" right away. TEST=manual BUG=703998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== md settings: Small changes to lock screen. A couple of minor touchups. 1) Focus the input element when opening the password prompt for the first time. 2) "Setup PIN" link gets focused after closing the setup pin dialog and shows a large blue border around the whole link box; removed border and bolded the text to indicate focus. 3) Setup pin dialog will bounce up after entering the first number. This may be annoying if someone trying to click/touch 88 and accidently enter 84 instead. Makes the problem div invisible instead. Alternatively we can just show "PIN should be N length" right away. TEST=manual BUG=703998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== md settings: Small changes to lock screen. A couple of minor touchups. 1) Focus the input element when opening the password prompt for the first time. 2) "Setup PIN" link gets focused after closing the setup pin dialog and shows a large blue border around the whole link box; removed border and bolded the text to indicate focus. 3) Setup pin dialog will bounce up after entering the first number. This may be annoying if someone trying to click/touch 88 and accidently enter 84 instead. Makes the problem div invisible instead. Alternatively we can just show "PIN should be N length" right away. TEST=manual BUG=703998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
sammiequon@chromium.org changed reviewers: + jdufault@chromium.org, stevenjb@chromium.org
jdufault@, stevenjb@ - Please take a look. Thanks! https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/password_prompt_dialog.js (right): https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/password_prompt_dialog.js:114: this.$.passwordInput.shadowRoot.querySelector('#input').focus(); This is kind of messy but the JS guide doesn't allow this.$.passwordInput.$.input.focus(); and this.$.passwordInput.focus() doesn't do the job.
+dpapad@ For question about PaperInputElement.focus(). https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/lock_screen.html:55: on-tap="onConfigurePin_"> Why is this a link instead of a button like the rest of our UI? Can you double check with UX... having custom formatting like is difficult to maintain. https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/password_prompt_dialog.js (right): https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/password_prompt_dialog.js:114: this.$.passwordInput.shadowRoot.querySelector('#input').focus(); On 2017/04/14 17:22:07, sammiequon wrote: > This is kind of messy but the JS guide doesn't allow > this.$.passwordInput.$.input.focus(); and this.$.passwordInput.focus() doesn't > do the job. If this.$.passwordInput.focus() doesn't work that sounds like a bug. dpapad@ - Do you know anything about that? https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/setup_pin_dialog.html (right): https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/setup_pin_dialog.html:38: } A simpler approach might just be to hide the iron-icon with hidden-[[!problemMessage]]. (I don't think you need hasProblem_)
https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/password_prompt_dialog.js (right): https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/password_prompt_dialog.js:114: this.$.passwordInput.shadowRoot.querySelector('#input').focus(); On 2017/04/14 21:47:20, stevenjb wrote: > On 2017/04/14 17:22:07, sammiequon wrote: > > This is kind of messy but the JS guide doesn't allow > > this.$.passwordInput.$.input.focus(); and this.$.passwordInput.focus() doesn't > > do the job. > > If this.$.passwordInput.focus() doesn't work that sounds like a bug. > > dpapad@ - Do you know anything about that? Oh, actually, I think your problem may be that this.$.foo doesn't work when |foo| is inside a <template>, maybe <dialog> has the same problem. Try this instead: this.$$('#passwordInput').focus(); Also, I don't think you need the async() since the dialog is not templated out.
https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/lock_screen.html:55: on-tap="onConfigurePin_"> On 2017/04/14 21:47:20, stevenjb wrote: > Why is this a link instead of a button like the rest of our UI? Can you double > check with UX... having custom formatting like is difficult to maintain. Yeah, it is a button in the newest mocks, but for this CL I am trying not to bring anything new. https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/password_prompt_dialog.js (right): https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/password_prompt_dialog.js:114: this.$.passwordInput.shadowRoot.querySelector('#input').focus(); On 2017/04/14 21:51:30, stevenjb wrote: > On 2017/04/14 21:47:20, stevenjb wrote: > > On 2017/04/14 17:22:07, sammiequon wrote: > > > This is kind of messy but the JS guide doesn't allow > > > this.$.passwordInput.$.input.focus(); and this.$.passwordInput.focus() > doesn't > > > do the job. > > > > If this.$.passwordInput.focus() doesn't work that sounds like a bug. > > > > dpapad@ - Do you know anything about that? > > Oh, actually, I think your problem may be that this.$.foo doesn't work when > |foo| is inside a <template>, maybe <dialog> has the same problem. Try this > instead: > > this.$$('#passwordInput').focus(); > > Also, I don't think you need the async() since the dialog is not templated out. > That doesn't seem to work, it has the same behaviour as this.$.passwordInput.focus() in that the focus is there the first time but consequent opens of this dialog the focus is missing. It also does not work without async. https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/setup_pin_dialog.html (right): https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/setup_pin_dialog.html:38: } On 2017/04/14 21:47:20, stevenjb wrote: > A simpler approach might just be to hide the iron-icon with > hidden-[[!problemMessage]]. (I don't think you need hasProblem_) Done.
https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/lock_screen.html:55: on-tap="onConfigurePin_"> On 2017/04/14 22:11:43, sammiequon wrote: > On 2017/04/14 21:47:20, stevenjb wrote: > > Why is this a link instead of a button like the rest of our UI? Can you double > > check with UX... having custom formatting like is difficult to maintain. > > Yeah, it is a button in the newest mocks, but for this CL I am trying not to > bring anything new. Well, you're adding new CSS, which I'd really rather not :P I think if you change this to a button it will just work and will look better, without the extra CSS> https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/password_prompt_dialog.js (right): https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/password_prompt_dialog.js:114: this.$.passwordInput.shadowRoot.querySelector('#input').focus(); On 2017/04/14 22:11:43, sammiequon wrote: > On 2017/04/14 21:51:30, stevenjb wrote: > > On 2017/04/14 21:47:20, stevenjb wrote: > > > On 2017/04/14 17:22:07, sammiequon wrote: > > > > This is kind of messy but the JS guide doesn't allow > > > > this.$.passwordInput.$.input.focus(); and this.$.passwordInput.focus() > > doesn't > > > > do the job. > > > > > > If this.$.passwordInput.focus() doesn't work that sounds like a bug. > > > > > > dpapad@ - Do you know anything about that? > > > > Oh, actually, I think your problem may be that this.$.foo doesn't work when > > |foo| is inside a <template>, maybe <dialog> has the same problem. Try this > > instead: > > > > this.$$('#passwordInput').focus(); > > > > Also, I don't think you need the async() since the dialog is not templated > out. > > > > That doesn't seem to work, it has the same behaviour as > this.$.passwordInput.focus() in that the focus is there the first time but > consequent opens of this dialog the focus is missing. It also does not work > without async. Hmm, I think we need to investigate that then. Querying the shadowRoot like this is not something we want to be doing.
https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/lock_screen.html:55: on-tap="onConfigurePin_"> On 2017/04/14 22:29:02, stevenjb wrote: > On 2017/04/14 22:11:43, sammiequon wrote: > > On 2017/04/14 21:47:20, stevenjb wrote: > > > Why is this a link instead of a button like the rest of our UI? Can you > double > > > check with UX... having custom formatting like is difficult to maintain. > > > > Yeah, it is a button in the newest mocks, but for this CL I am trying not to > > bring anything new. > > Well, you're adding new CSS, which I'd really rather not :P > > I think if you change this to a button it will just work and will look better, > without the extra CSS> > Removed. Switching to button in another CL which i just sent you to review :-). https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/password_prompt_dialog.js (right): https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/password_prompt_dialog.js:114: this.$.passwordInput.shadowRoot.querySelector('#input').focus(); On 2017/04/14 22:29:02, stevenjb wrote: > On 2017/04/14 22:11:43, sammiequon wrote: > > On 2017/04/14 21:51:30, stevenjb wrote: > > > On 2017/04/14 21:47:20, stevenjb wrote: > > > > On 2017/04/14 17:22:07, sammiequon wrote: > > > > > This is kind of messy but the JS guide doesn't allow > > > > > this.$.passwordInput.$.input.focus(); and this.$.passwordInput.focus() > > > doesn't > > > > > do the job. > > > > > > > > If this.$.passwordInput.focus() doesn't work that sounds like a bug. > > > > > > > > dpapad@ - Do you know anything about that? > > > > > > Oh, actually, I think your problem may be that this.$.foo doesn't work when > > > |foo| is inside a <template>, maybe <dialog> has the same problem. Try this > > > instead: > > > > > > this.$$('#passwordInput').focus(); > > > > > > Also, I don't think you need the async() since the dialog is not templated > > out. > > > > > > > That doesn't seem to work, it has the same behaviour as > > this.$.passwordInput.focus() in that the focus is there the first time but > > consequent opens of this dialog the focus is missing. It also does not work > > without async. > > Hmm, I think we need to investigate that then. Querying the shadowRoot like this > is not something we want to be doing. Acknowledged. Let me see if I can find a better approach.
On 2017/04/14 23:48:13, sammiequon wrote: > https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... > File chrome/browser/resources/settings/people_page/lock_screen.html (right): > > https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/people_page/lock_screen.html:55: > on-tap="onConfigurePin_"> > On 2017/04/14 22:29:02, stevenjb wrote: > > On 2017/04/14 22:11:43, sammiequon wrote: > > > On 2017/04/14 21:47:20, stevenjb wrote: > > > > Why is this a link instead of a button like the rest of our UI? Can you > > double > > > > check with UX... having custom formatting like is difficult to maintain. > > > > > > Yeah, it is a button in the newest mocks, but for this CL I am trying not to > > > bring anything new. > > > > Well, you're adding new CSS, which I'd really rather not :P > > > > I think if you change this to a button it will just work and will look better, > > without the extra CSS> > > > > Removed. Switching to button in another CL which i just sent you to review :-). > > https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... > File chrome/browser/resources/settings/people_page/password_prompt_dialog.js > (right): > > https://codereview.chromium.org/2822733002/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/people_page/password_prompt_dialog.js:114: > this.$.passwordInput.shadowRoot.querySelector('#input').focus(); > On 2017/04/14 22:29:02, stevenjb wrote: > > On 2017/04/14 22:11:43, sammiequon wrote: > > > On 2017/04/14 21:51:30, stevenjb wrote: > > > > On 2017/04/14 21:47:20, stevenjb wrote: > > > > > On 2017/04/14 17:22:07, sammiequon wrote: > > > > > > This is kind of messy but the JS guide doesn't allow > > > > > > this.$.passwordInput.$.input.focus(); and this.$.passwordInput.focus() > > > > doesn't > > > > > > do the job. > > > > > > > > > > If this.$.passwordInput.focus() doesn't work that sounds like a bug. > > > > > > > > > > dpapad@ - Do you know anything about that? > > > > > > > > Oh, actually, I think your problem may be that this.$.foo doesn't work > when > > > > |foo| is inside a <template>, maybe <dialog> has the same problem. Try > this > > > > instead: > > > > > > > > this.$$('#passwordInput').focus(); > > > > > > > > Also, I don't think you need the async() since the dialog is not templated > > > out. > > > > > > > > > > That doesn't seem to work, it has the same behaviour as > > > this.$.passwordInput.focus() in that the focus is there the first time but > > > consequent opens of this dialog the focus is missing. It also does not work > > > without async. > > > > Hmm, I think we need to investigate that then. Querying the shadowRoot like > this > > is not something we want to be doing. > > Acknowledged. Let me see if I can find a better approach. Can this CL be closed now?
Yes, closing. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
