Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
869813002:
Fix tabindex in chrome://settings/passwords (Closed)
|
Created:
5 years, 11 months ago by hcarmona Modified:
5 years, 10 months ago Reviewers:
Dan Beam CC:
chromium-reviews, dbeam+watch-options_chromium.org, gcasto+watchlist_chromium.org, arv+watch_chromium.org, mkwst+watchlist-passwords_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix tabindex in chrome://settings/passwords
Updated code so that the first row is focusable. This makes it possible to
tab into the first row making it more obvious that it is focused.
Applied Dan's feedback from previous CL (https://codereview.chromium.org/834323005).
BUG=449110
Committed: https://crrev.com/251f96fe57f3ae2453226d344656ed7b337a1c48
Cr-Commit-Position: refs/heads/master@{#313618}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Apply Feedback #
Total comments: 8
Patch Set 3 : Apply Feedback #
Total comments: 6
Patch Set 4 : Applied Feedback #
Total comments: 4
Patch Set 5 : Apply Feedback #Messages
Total messages: 16 (4 generated)
hcarmona@chromium.org changed reviewers: + dbeam@chromium.org
Applied feedback from previous CL and addressed focus issue that made this still reproducible.
https://codereview.chromium.org/869813002/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/869813002/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/password_manager_list.js:97: this.closeButtonElement_.removeEventListener(this.handleFocus, 'focus'); i'm confused. a) the order is 'eventName', eventHandlerFunction[1] where is the corresponding this.closeButtonElement_.addEventListener call or this.closeButtonElement_ even defined? is this supposed to say this.removeEventListener('focus', this.handleFocus); ? [1] https://developer.mozilla.org/en-US/docs/Web/API/EventTarget.addEventListener https://codereview.chromium.org/869813002/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/password_manager_list.js:98: this.addEventListener('focusin', this.handleFocus.bind(this)); why do we need to use focusin here? because it bubbles? https://codereview.chromium.org/869813002/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/password_manager_list.js:115: // Move the focus away from anything else that may have been focused. this comment is confusing. needed? https://codereview.chromium.org/869813002/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/password_manager_list.js:126: * @param {bool} focusable boolean https://codereview.chromium.org/869813002/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/password_manager_list.js:337: * up/down is pressed making it appear that the row items are not focusable. write this sentence using the word "focus" less https://codereview.chromium.org/869813002/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/password_manager_list.js:342: if (e.target == this && !this.selectedItem && this.items) if we're only looking for the case where e.target == this, why not just use focus instead?
Applied feedback https://codereview.chromium.org/869813002/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/869813002/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/password_manager_list.js:97: this.closeButtonElement_.removeEventListener(this.handleFocus, 'focus'); On 2015/01/23 00:00:37, Dan Beam wrote: > i'm confused. > > a) the order is 'eventName', eventHandlerFunction[1] > > where is the corresponding this.closeButtonElement_.addEventListener call or > this.closeButtonElement_ even defined? > > is this supposed to say > > this.removeEventListener('focus', this.handleFocus); > > ? > > [1] > https://developer.mozilla.org/en-US/docs/Web/API/EventTarget.addEventListener Done. https://codereview.chromium.org/869813002/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/password_manager_list.js:98: this.addEventListener('focusin', this.handleFocus.bind(this)); On 2015/01/23 00:00:37, Dan Beam wrote: > why do we need to use focusin here? because it bubbles? Yes, we need to focus event to bubble up from the elements in |this|. https://codereview.chromium.org/869813002/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/password_manager_list.js:115: // Move the focus away from anything else that may have been focused. On 2015/01/23 00:00:37, Dan Beam wrote: > this comment is confusing. needed? Done. https://codereview.chromium.org/869813002/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/password_manager_list.js:126: * @param {bool} focusable On 2015/01/23 00:00:37, Dan Beam wrote: > boolean Done. https://codereview.chromium.org/869813002/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/password_manager_list.js:337: * up/down is pressed making it appear that the row items are not focusable. On 2015/01/23 00:00:37, Dan Beam wrote: > write this sentence using the word "focus" less Reworded to make more clear. https://codereview.chromium.org/869813002/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/password_manager_list.js:342: if (e.target == this && !this.selectedItem && this.items) On 2015/01/23 00:00:37, Dan Beam wrote: > if we're only looking for the case where e.target == this, why not just use > focus instead? Done.
https://codereview.chromium.org/869813002/diff/20001/chrome/browser/resources... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/869813002/diff/20001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:98: this.closeButtonElement_.removeEventListener('focus', this.handleFocus); shouldn't this be: this.closeButtonElement_.removeEventListener( 'focus', this.closeButtonElement_.handleFocus); i don't think it'll work otherwise. is this really necessary? this seems like the second time I'm not sure this is even working, leading me to believe it isn't required. ultimately it's probably better to make some accessor on close buttons called .ignoreFocusEvents() or something that does this internally. https://codereview.chromium.org/869813002/diff/20001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:99: this.addEventListener('focusin', this.handleFocus); don't pass a 'focusin' event to a method that expects a 'focus' event, imo https://codereview.chromium.org/869813002/diff/20001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:336: * @param {Event} e The focusin event. this is no longer a focusin event https://codereview.chromium.org/869813002/diff/20001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:339: onFocusIn_: function(e) { onFocus_
Applied feedback https://codereview.chromium.org/869813002/diff/20001/chrome/browser/resources... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/869813002/diff/20001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:98: this.closeButtonElement_.removeEventListener('focus', this.handleFocus); On 2015/01/24 02:42:25, Dan Beam wrote: > shouldn't this be: > > this.closeButtonElement_.removeEventListener( > 'focus', this.closeButtonElement_.handleFocus); > > i don't think it'll work otherwise. > > is this really necessary? this seems like the second time I'm not sure this is > even working, leading me to believe it isn't required. > > ultimately it's probably better to make some accessor on close buttons called > .ignoreFocusEvents() or something that does this internally. Acknowledged. https://codereview.chromium.org/869813002/diff/20001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:99: this.addEventListener('focusin', this.handleFocus); On 2015/01/24 02:42:25, Dan Beam wrote: > don't pass a 'focusin' event to a method that expects a 'focus' event, imo Rather than adding a focusin event to the PasswordListItem, I've added listeners for the focus event to both of the additional controls in the PasswordListItem. This has the desired effect (highlight the row when an item is focused) without having to remove the listener from the close button. https://codereview.chromium.org/869813002/diff/20001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:336: * @param {Event} e The focusin event. On 2015/01/24 02:42:25, Dan Beam wrote: > this is no longer a focusin event Done. https://codereview.chromium.org/869813002/diff/20001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:339: onFocusIn_: function(e) { On 2015/01/24 02:42:24, Dan Beam wrote: > onFocus_ Done.
https://codereview.chromium.org/869813002/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/869813002/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:75: passwordInput.addEventListener('focus', this.handleFocus.bind(this)); i think this is still confusing, how about: var list = this; passwordInput.addEventListener('focus', function() { list.handleFocus(); }); so it's easy to tell what's going on here? https://codereview.chromium.org/869813002/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:122: * @param {boolean} focusable nit: make @private + add _ https://codereview.chromium.org/869813002/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:286: this.addEventListener('focus', this.onFocus_); nit: this.onFocus_.bind(this)
https://codereview.chromium.org/869813002/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/869813002/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:75: passwordInput.addEventListener('focus', this.handleFocus.bind(this)); On 2015/01/27 22:22:13, Dan Beam wrote: > i think this is still confusing, how about: > > var list = this; > passwordInput.addEventListener('focus', function() { > list.handleFocus(); > }); > > so it's easy to tell what's going on here? Done. https://codereview.chromium.org/869813002/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:122: * @param {boolean} focusable On 2015/01/27 22:22:12, Dan Beam wrote: > nit: make @private + add _ Done. https://codereview.chromium.org/869813002/diff/40001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:286: this.addEventListener('focus', this.onFocus_); On 2015/01/27 22:22:13, Dan Beam wrote: > nit: this.onFocus_.bind(this) Done.
lgtm https://codereview.chromium.org/869813002/diff/60001/chrome/browser/resources... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/869813002/diff/60001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:75: var list = this; sorry, s/list/item or deletableItem (I thought |this| was a list, but it's a DeletableItem) https://codereview.chromium.org/869813002/diff/60001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:77: list.handleFocus(e); handleFocus doesn't take an argument https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
The CQ bit was checked by hcarmona@chromium.org
The CQ bit was unchecked by hcarmona@chromium.org
Renamed list->deletableItem. Got rid of unused argument. Clicking commit. https://codereview.chromium.org/869813002/diff/60001/chrome/browser/resources... File chrome/browser/resources/options/password_manager_list.js (right): https://codereview.chromium.org/869813002/diff/60001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:75: var list = this; On 2015/01/28 01:49:01, Dan Beam wrote: > sorry, s/list/item or deletableItem (I thought |this| was a list, but it's a > DeletableItem) Done. https://codereview.chromium.org/869813002/diff/60001/chrome/browser/resources... chrome/browser/resources/options/password_manager_list.js:77: list.handleFocus(e); On 2015/01/28 01:49:01, Dan Beam wrote: > handleFocus doesn't take an argument > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... Done.
The CQ bit was checked by hcarmona@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/869813002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/251f96fe57f3ae2453226d344656ed7b337a1c48 Cr-Commit-Position: refs/heads/master@{#313618} |
