|
|
Created:
8 years ago by Rune Fevang Modified:
7 years, 11 months ago Reviewers:
csilv CC:
chromium-reviews, dbeam+watch-options_chromium.org, arv (Not doing code reviews) Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionMake hitting "Enter" submit the add/change profile dialog.
BUG=102172
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175586
Patch Set 1 #Patch Set 2 : Respect OK button disabled state #
Total comments: 2
Patch Set 3 : Share event registration code between 'create' and 'manage' #Patch Set 4 : Black border around OK buttons #
Messages
Total messages: 21 (0 generated)
Hey csilv, I added you since you were in the OWNERS file. If you're the wrong person to review could you point me to the right one?
One issue I foresee is that this issue will not work if the user clicks on the dialog, causing the text field to lose focus. It'd be nice if you could catch the keydown event for the any keydown within the overlay although I don't believe there's a way to do that other than installing a key handler on the document while the dialog is visible. dbeam, suggestions? https://codereview.chromium.org/11552029/diff/3001/chrome/browser/resources/o... File chrome/browser/resources/options/manage_profile_overlay.js (right): https://codereview.chromium.org/11552029/diff/3001/chrome/browser/resources/o... chrome/browser/resources/options/manage_profile_overlay.js:90: } These two methods are identical, so it would be nice to put the implementation into a method that can be shared.
On 2012/12/15 01:05:12, csilv wrote: > One issue I foresee is that this issue will not work if the user clicks on the > dialog, causing the text field to lose focus. It'd be nice if you could catch > the keydown event for the any keydown within the overlay although I don't > believe there's a way to do that other than installing a key handler on the > document while the dialog is visible. I had it on manage-profile-overlay-manage/create initially, the issue with that was that it also triggered when enter was pressed on the cancel button. I suppose you could check for that case and exclude it though?
Registering for events on the overlay itself didn't help in the 'lost focus' case, so I guess adding a listener on the document is needed for that case. That said, I don't think the current behavior is necessarily wrong, when I experimented with it I thought it seemed pretty natural that enter didn't work when there was no visible focus. Isn't that consistent with how forms work in general? https://codereview.chromium.org/11552029/diff/3001/chrome/browser/resources/o... File chrome/browser/resources/options/manage_profile_overlay.js (right): https://codereview.chromium.org/11552029/diff/3001/chrome/browser/resources/o... chrome/browser/resources/options/manage_profile_overlay.js:90: } On 2012/12/15 01:05:12, csilv wrote: > These two methods are identical, so it would be nice to put the implementation > into a method that can be shared. Done.
Ping, any chance of getting this looked at before the holiday break?
this is a lot of changes, can't you just wrap with a <form> and say form.onsubmit = function(e) { e.preventDefault(); saveTheUserName(); }; ?
or perhaps something more generically useful using event delegation?
On 2012/12/21 21:42:42, Dan Beam wrote: > this is a lot of changes, can't you just wrap with a <form> and say > form.onsubmit = function(e) { e.preventDefault(); saveTheUserName(); }; ? Doing that doesn't work when focus is on one of the avatars, it also needs special-case handling for the cancel button, like when setting the listener to one of the outer divs. Most of the changes were in response to the comment about sharing implementation between 'manage' and 'create' listeners. I figured that if those methods should be combined, then so should the other listener methods that were common across the two modes.
On 2012/12/21 22:59:19, Rune Fevang wrote: > On 2012/12/21 21:42:42, Dan Beam wrote: > > this is a lot of changes, can't you just wrap with a <form> and say > > form.onsubmit = function(e) { e.preventDefault(); saveTheUserName(); }; ? > > Doing that doesn't work when focus is on one of the avatars, It does if they're <input type=image> > it also needs > special-case handling for the cancel button, like when setting the listener to > one of the outer divs. <form> would only improve the automagic submitting, not the canceling, but I don't think it degrades it either. > > Most of the changes were in response to the comment about sharing implementation > between 'manage' and 'create' listeners. I figured that if those methods should > be combined, then so should the other listener methods that were common across > the two modes. that's fine, it just seemed more than necessary. csilv@ is your reviewer, I just happened to see the on a watchlist and thought this might be more addressible globally (and could be done more semantically).
lgtm Rune, sorry for neglecting this issue for so long. The change itself looks good. However, I have two concerns about this issue: 1. In Settings (and other parts of WebUI), we have numerous dialogs all with submit buttons. But this change only addresses one of those dialogs. Personally, I'd rather we have a solution to address all dialogs rather than doing one at a time (and in the process, creating a lot of duplicate code). 2. The submit button doesn't currently have any visual styling to indicate that hitting enter will activate the button. The task of solving this issue universally is covered by: http://crbug.com/116859 Also note comment #7 of that issue for recommended styling for default submit buttons. So, I leave the decision with you. I'm find with this CL as-is... but I really hope someone volunteers to take on 116859 as a more complete solution.
On 2013/01/04 20:20:43, csilv wrote: > lgtm > > Rune, sorry for neglecting this issue for so long. The change itself looks > good. However, I have two concerns about this issue: > > 1. In Settings (and other parts of WebUI), we have numerous dialogs all with > submit buttons. But this change only addresses one of those dialogs. > Personally, I'd rather we have a solution to address all dialogs rather than > doing one at a time (and in the process, creating a lot of duplicate code). > > 2. The submit button doesn't currently have any visual styling to indicate that > hitting enter will activate the button. > > The task of solving this issue universally is covered by: > http://crbug.com/116859 > > Also note comment #7 of that issue for recommended styling for default submit > buttons. So, I leave the decision with you. I'm find with this CL as-is... but > I really hope someone volunteers to take on 116859 as a more complete solution. Added styling around the OK buttons. I picked up this bug from the fixit-hotlist and wasn't aware it was a wider issue. I don't really have time to look at a more complete solution right now, but I could probably work on that the next time I have some free time.
Thanks for adding the border, slgtm.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rfevang@chromium.org/11552029/19001
Retried try job too often on ios_dbg_simulator for step(s) sync_unit_tests, unit_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rfevang@chromium.org/11552029/19001
Retried try job too often on win_aura for step(s) interactive_ui_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rfevang@chromium.org/11552029/19001
Step "update" is always a major failure. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rfevang@chromium.org/11552029/19001
Message was sent while issue was closed.
Change committed as 175586 |