Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10796115:
Enabled Selection (ctrl-A) on login password (Closed)
|
Created:
8 years, 5 months ago by Harry McCleave Modified:
8 years, 5 months ago CC:
chromium-reviews, arv (Not doing code reviews), stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, oshima+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionEnabled Selection (ctrl-A) on login password
BUG=125863
TEST=Tested change on linux build /w chromeos=1
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148441
Patch Set 1 #
Total comments: 3
Patch Set 2 : Links + Oobe #
Total comments: 20
Patch Set 3 : Code cleanup #
Total comments: 5
Patch Set 4 : More cleanup #Patch Set 5 : Generalized solution #
Total comments: 2
Patch Set 6 : Comment update #
Messages
Total messages: 17 (0 generated)
Please take a look at this when you get the chance.
lgtm with couple of minor comments thanks! https://chromiumcodereview.appspot.com/10796115/diff/1/chrome/browser/resourc... File chrome/browser/resources/chromeos/login/login.js (right): https://chromiumcodereview.appspot.com/10796115/diff/1/chrome/browser/resourc... chrome/browser/resources/chromeos/login/login.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Makes sense to update chrome/browser/resources/chromeos/login/oobe.js as well. login.js is a stripped down version of it used after OOBE is completed. https://chromiumcodereview.appspot.com/10796115/diff/1/chrome/browser/resourc... chrome/browser/resources/chromeos/login/login.js:183: // bug (125863) nit: Please add http://crosbug.com/ so that link is clickable (at least in code search etc.)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/harrym@chromium.org/10796115/5001
Presubmit check for 10796115-5001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome/browser/resources
Presubmit checks took 1.3s to calculate.
https://chromiumcodereview.appspot.com/10796115/diff/1/chrome/browser/resourc... File chrome/browser/resources/chromeos/login/login.js (right): https://chromiumcodereview.appspot.com/10796115/diff/1/chrome/browser/resourc... chrome/browser/resources/chromeos/login/login.js:183: // bug (125863) On 2012/07/24 20:12:32, Nikita Kostylev wrote: > nit: Please add http://crosbug.com/ so that link is clickable (at least in code > search etc.) Done.
On 2012/07/24 21:18:04, Harry McCleave wrote: > https://chromiumcodereview.appspot.com/10796115/diff/1/chrome/browser/resourc... > File chrome/browser/resources/chromeos/login/login.js (right): > > https://chromiumcodereview.appspot.com/10796115/diff/1/chrome/browser/resourc... > chrome/browser/resources/chromeos/login/login.js:183: // bug (125863) > On 2012/07/24 20:12:32, Nikita Kostylev wrote: > > nit: Please add http://crosbug.com/ so that link is clickable (at least in > code > > search etc.) > > Done. Dan, could you take a look over chrome/browser/resources/shared/js/util.js, thanks.
https://chromiumcodereview.appspot.com/10796115/diff/5001/chrome/browser/reso... File chrome/browser/resources/chromeos/login/login.js (right): https://chromiumcodereview.appspot.com/10796115/diff/5001/chrome/browser/reso... chrome/browser/resources/chromeos/login/login.js:184: function ReturnTrueOnInputElement(e) { nit: returnTrueOnInputElement if you must keep this name, I think this could be more usefully named isInputElement or something along these lines, additionally, why not just inline this, i.e.: disableTextSelectAndDrag(function(e) { return e.target instanceof HTMLInputElement; }); (srcElement is the non-standard IE name, btw...) https://chromiumcodereview.appspot.com/10796115/diff/5001/chrome/browser/reso... chrome/browser/resources/chromeos/login/login.js:185: return (e.srcElement instanceof HTMLInputElement); nit: no () https://chromiumcodereview.appspot.com/10796115/diff/5001/chrome/browser/reso... File chrome/browser/resources/chromeos/login/oobe.js (right): https://chromiumcodereview.appspot.com/10796115/diff/5001/chrome/browser/reso... chrome/browser/resources/chromeos/login/oobe.js:339: function ReturnTrueOnInputElement(e) { same nits, also why not combine these identical callbacks into one and put them in a namespace? right now if you built the code together, the last one defined would override the first and could cause subtle bugs (as these are global names and are "hoisted" as they're function declarations) https://chromiumcodereview.appspot.com/10796115/diff/5001/chrome/browser/reso... File chrome/browser/resources/shared/js/util.js (right): https://chromiumcodereview.appspot.com/10796115/diff/5001/chrome/browser/reso... chrome/browser/resources/shared/js/util.js:110: * Disables text selection and dragging, with optional whitelist callbacks. nit: no extra \n here in jsdoc https://chromiumcodereview.appspot.com/10796115/diff/5001/chrome/browser/reso... chrome/browser/resources/shared/js/util.js:112: * @param {function(Event) : boolean} allowSelectStart if this function is * @param {function(Event):boolean=} opt_allowSelectStart If this function is https://chromiumcodereview.appspot.com/10796115/diff/5001/chrome/browser/reso... chrome/browser/resources/shared/js/util.js:114: * @param {function(Event) : boolean} allowDragStart if this funcion is defined * @param {function(Event):boolean=} opt_allowDragStart If this function is https://chromiumcodereview.appspot.com/10796115/diff/5001/chrome/browser/reso... chrome/browser/resources/shared/js/util.js:120: if (!(allowSelectStart && allowSelectStart(e))) { nit: no curlies, should probably actually modify this as well with allowSelectStart.call(this, e) https://chromiumcodereview.appspot.com/10796115/diff/5001/chrome/browser/reso... chrome/browser/resources/shared/js/util.js:123: } }; https://chromiumcodereview.appspot.com/10796115/diff/5001/chrome/browser/reso... chrome/browser/resources/shared/js/util.js:127: if (!(allowDragStart && allowDragStart(e))) { nit: no curlies, should probably actually modify this as well with allowDragStart.call(this, e) https://chromiumcodereview.appspot.com/10796115/diff/5001/chrome/browser/reso... chrome/browser/resources/shared/js/util.js:130: } };
https://chromiumcodereview.appspot.com/10796115/diff/5001/chrome/browser/reso... File chrome/browser/resources/chromeos/login/login.js (right): https://chromiumcodereview.appspot.com/10796115/diff/5001/chrome/browser/reso... chrome/browser/resources/chromeos/login/login.js:184: function ReturnTrueOnInputElement(e) { On 2012/07/24 21:56:27, Dan Beam wrote: > nit: returnTrueOnInputElement if you must keep this name, I think this could be > more usefully named isInputElement or something along these lines, additionally, > why not just inline this, i.e.: > > disableTextSelectAndDrag(function(e) { > return e.target instanceof HTMLInputElement; > }); > > (srcElement is the non-standard IE name, btw...) Done. https://chromiumcodereview.appspot.com/10796115/diff/5001/chrome/browser/reso... chrome/browser/resources/chromeos/login/login.js:185: return (e.srcElement instanceof HTMLInputElement); On 2012/07/24 21:56:27, Dan Beam wrote: > nit: no () Done. https://chromiumcodereview.appspot.com/10796115/diff/5001/chrome/browser/reso... File chrome/browser/resources/chromeos/login/oobe.js (right): https://chromiumcodereview.appspot.com/10796115/diff/5001/chrome/browser/reso... chrome/browser/resources/chromeos/login/oobe.js:339: function ReturnTrueOnInputElement(e) { On 2012/07/24 21:56:27, Dan Beam wrote: > same nits, also why not combine these identical callbacks into one and put them > in a namespace? right now if you built the code together, the last one defined > would override the first and could cause subtle bugs (as these are global names > and are "hoisted" as they're function declarations) Done. https://chromiumcodereview.appspot.com/10796115/diff/5001/chrome/browser/reso... File chrome/browser/resources/shared/js/util.js (right): https://chromiumcodereview.appspot.com/10796115/diff/5001/chrome/browser/reso... chrome/browser/resources/shared/js/util.js:110: * Disables text selection and dragging, with optional whitelist callbacks. On 2012/07/24 21:56:27, Dan Beam wrote: > nit: no extra \n here in jsdoc Done. https://chromiumcodereview.appspot.com/10796115/diff/5001/chrome/browser/reso... chrome/browser/resources/shared/js/util.js:112: * @param {function(Event) : boolean} allowSelectStart if this function is On 2012/07/24 21:56:27, Dan Beam wrote: > * @param {function(Event):boolean=} opt_allowSelectStart If this function is Done. https://chromiumcodereview.appspot.com/10796115/diff/5001/chrome/browser/reso... chrome/browser/resources/shared/js/util.js:114: * @param {function(Event) : boolean} allowDragStart if this funcion is defined On 2012/07/24 21:56:27, Dan Beam wrote: > * @param {function(Event):boolean=} opt_allowDragStart If this function is Done. https://chromiumcodereview.appspot.com/10796115/diff/5001/chrome/browser/reso... chrome/browser/resources/shared/js/util.js:120: if (!(allowSelectStart && allowSelectStart(e))) { On 2012/07/24 21:56:27, Dan Beam wrote: > nit: no curlies, should probably actually modify this as well with > allowSelectStart.call(this, e) Done. https://chromiumcodereview.appspot.com/10796115/diff/5001/chrome/browser/reso... chrome/browser/resources/shared/js/util.js:123: } On 2012/07/24 21:56:27, Dan Beam wrote: > }; Done. https://chromiumcodereview.appspot.com/10796115/diff/5001/chrome/browser/reso... chrome/browser/resources/shared/js/util.js:127: if (!(allowDragStart && allowDragStart(e))) { On 2012/07/24 21:56:27, Dan Beam wrote: > nit: no curlies, should probably actually modify this as well with > allowDragStart.call(this, e) Done. https://chromiumcodereview.appspot.com/10796115/diff/5001/chrome/browser/reso... chrome/browser/resources/shared/js/util.js:130: } On 2012/07/24 21:56:27, Dan Beam wrote: > }; Done.
https://chromiumcodereview.appspot.com/10796115/diff/4004/chrome/browser/reso... File chrome/browser/resources/chromeos/login/login.js (right): https://chromiumcodereview.appspot.com/10796115/diff/4004/chrome/browser/reso... chrome/browser/resources/chromeos/login/login.js:185: return e.target instanceof HTMLInputElement; Copy pasta detected, remove two spaces from this line and below in each file. Also, do you only care about password or text inputs? If so, you should probably check e.target.type is 'password' or 'text' or 'search' or has no type or something... https://chromiumcodereview.appspot.com/10796115/diff/4004/chrome/browser/reso... File chrome/browser/resources/shared/js/util.js (right): https://chromiumcodereview.appspot.com/10796115/diff/4004/chrome/browser/reso... chrome/browser/resources/shared/js/util.js:111: * @param {function(Event):boolean=} opt_allowSelectStart if this function is Capitalize the first word after each parameter name (both "if"s in this case).
https://chromiumcodereview.appspot.com/10796115/diff/4004/chrome/browser/reso... File chrome/browser/resources/chromeos/login/login.js (right): https://chromiumcodereview.appspot.com/10796115/diff/4004/chrome/browser/reso... chrome/browser/resources/chromeos/login/login.js:185: return e.target instanceof HTMLInputElement; On 2012/07/24 23:07:49, Dan Beam wrote: > Copy pasta detected, remove two spaces from this line and below in each file. > Also, do you only care about password or text inputs? If so, you should probably > check e.target.type is 'password' or 'text' or 'search' or has no type or > something... The behavior identified in the bug was on a password field however any text input field would be unintuitive without this change (page identified only has password field) so I think not limiting the type is correct. https://chromiumcodereview.appspot.com/10796115/diff/4004/chrome/browser/reso... File chrome/browser/resources/shared/js/util.js (right): https://chromiumcodereview.appspot.com/10796115/diff/4004/chrome/browser/reso... chrome/browser/resources/shared/js/util.js:111: * @param {function(Event):boolean=} opt_allowSelectStart if this function is On 2012/07/24 23:07:49, Dan Beam wrote: > Capitalize the first word after each parameter name (both "if"s in this case). Done.
https://chromiumcodereview.appspot.com/10796115/diff/4004/chrome/browser/reso... File chrome/browser/resources/chromeos/login/login.js (right): https://chromiumcodereview.appspot.com/10796115/diff/4004/chrome/browser/reso... chrome/browser/resources/chromeos/login/login.js:185: return e.target instanceof HTMLInputElement; On 2012/07/24 23:21:15, Harry McCleave wrote: > On 2012/07/24 23:07:49, Dan Beam wrote: > > Copy pasta detected, remove two spaces from this line and below in each file. > > Also, do you only care about password or text inputs? If so, you should > probably > > check e.target.type is 'password' or 'text' or 'search' or has no type or > > something... > > The behavior identified in the bug was on a password field however any text > input field would be unintuitive without this change (page identified only has > password field) so I think not limiting the type is correct. You may want to allow <textarea>s as well then, perhaps something like: var src = e.target; return src instanceof HTMLTextAreaElement || src instanceof HTMLInputElement && /text|password|search/.test(src.type);
On 2012/07/25 00:04:59, Dan Beam wrote: > https://chromiumcodereview.appspot.com/10796115/diff/4004/chrome/browser/reso... > File chrome/browser/resources/chromeos/login/login.js (right): > > https://chromiumcodereview.appspot.com/10796115/diff/4004/chrome/browser/reso... > chrome/browser/resources/chromeos/login/login.js:185: return e.target instanceof > HTMLInputElement; > On 2012/07/24 23:21:15, Harry McCleave wrote: > > On 2012/07/24 23:07:49, Dan Beam wrote: > > > Copy pasta detected, remove two spaces from this line and below in each > file. > > > Also, do you only care about password or text inputs? If so, you should > > probably > > > check e.target.type is 'password' or 'text' or 'search' or has no type or > > > something... > > > > The behavior identified in the bug was on a password field however any text > > input field would be unintuitive without this change (page identified only has > > password field) so I think not limiting the type is correct. > > You may want to allow <textarea>s as well then, perhaps something like: > > var src = e.target; > return src instanceof HTMLTextAreaElement || > src instanceof HTMLInputElement && > /text|password|search/.test(src.type); Done
lgtm w/nits https://chromiumcodereview.appspot.com/10796115/diff/10003/chrome/browser/res... File chrome/browser/resources/chromeos/login/oobe.js (right): https://chromiumcodereview.appspot.com/10796115/diff/10003/chrome/browser/res... chrome/browser/resources/chromeos/login/oobe.js:337: // Allow selection events specifically on InputElements (password field) nit: probably ought to update these comments now https://chromiumcodereview.appspot.com/10796115/diff/10003/chrome/browser/res... File chrome/browser/resources/shared/js/util.js (right): https://chromiumcodereview.appspot.com/10796115/diff/10003/chrome/browser/res... chrome/browser/resources/shared/js/util.js:112: * defined and returns true, will allow the SelectStart event to be processed nit: you need a subject in this sentence and need to end it with a period, e.g.: If this function is defined and returns true, selectstart events will be allowed. Otherwise, the default action will be prevented. something like that...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/harrym@chromium.org/10796115/18002
Try job failure for 10796115-18002 (retry) on linux_clang for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/harrym@chromium.org/10796115/18002
Change committed as 148441 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
