Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2930683003:
MD Settings: fix focus-row-behavior allowing invisible focus on rows. (Closed)
|
Created:
3 years, 6 months ago by scottchen Modified:
3 years, 6 months ago Reviewers:
hcarmona CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: fix focus-row-behavior allowing invisible focus on rows.
In some scenarios when a row's control is focused, the row remained tabindex="0" (i.e. focusable) so shift-tabbing was moving the focus back to the row which is undesired. This CL makes sure that when the control is focused, shift-tabbing would never go back to the containing row.
BUG=725463
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2930683003
Cr-Commit-Position: refs/heads/master@{#478458}
Committed: https://chromium.googlesource.com/chromium/src/+/6d30a69cd0eba07bbbff2e8efc9b0ab2e335dae4
Patch Set 1 #
Total comments: 7
Patch Set 2 : add clarifying comments #Messages
Total messages: 18 (8 generated)
Description was changed from ========== MD Settings: fix focus-row-behavior allowing invisible focus on rows. BUG=725463 ========== to ========== MD Settings: fix focus-row-behavior allowing invisible focus on rows. BUG=725463 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: fix focus-row-behavior allowing invisible focus on rows. BUG=725463 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: fix focus-row-behavior allowing invisible focus on rows. In some scenarios when a row's control is focused, the row remained tabindex="0" (i.e. focusable) so shift-tabbing was moving the focus back to the row which is undesired. This CL makes sure that when the control is focused, shift-tabbing would never go back to the containing row. BUG=725463 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
scottchen@chromium.org changed reviewers: + hcarmona@chromium.org
https://codereview.chromium.org/2930683003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/focus_row_behavior.js (right): https://codereview.chromium.org/2930683003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/focus_row_behavior.js:21: onFocus: function(row, e) { When does this |onFocus| get called? https://codereview.chromium.org/2930683003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/focus_row_behavior.js:142: onFocus_: function() { Vs this |onFocus_|? IIUC, they both set tabIndex = -1.
https://codereview.chromium.org/2930683003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/focus_row_behavior.js (right): https://codereview.chromium.org/2930683003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/focus_row_behavior.js:21: onFocus: function(row, e) { On 2017/06/08 18:57:50, hcarmona wrote: > When does this |onFocus| get called? When a control element (i,e, [focus-row-control]) within the list gets focused. https://codereview.chromium.org/2930683003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/focus_row_behavior.js:142: onFocus_: function() { On 2017/06/08 18:57:50, hcarmona wrote: > Vs this |onFocus_|? > > IIUC, they both set tabIndex = -1. This is when the row gets focused. This function essentially forwards the focus to the first focusable child and then set the row itself to be unfocusable. However, there are some scenarios in which the contained control element get focused programmatically (e.g. when cr-action-menu). In such scenario, this function is not invoked, so when you shift-tab from the child, the focus *would* go to the row because its tabindex is not set to -1 yet, which is undesired.
https://codereview.chromium.org/2930683003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/focus_row_behavior.js (right): https://codereview.chromium.org/2930683003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/focus_row_behavior.js:21: onFocus: function(row, e) { On 2017/06/08 22:55:35, scottchen wrote: > On 2017/06/08 18:57:50, hcarmona wrote: > > When does this |onFocus| get called? > > When a control element (i,e, [focus-row-control]) within the list gets focused. Sounds good. Let's add a short comment to both focus functions to remove ambiguity about when they are called. https://codereview.chromium.org/2930683003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/focus_row_behavior.js:142: onFocus_: function() { On 2017/06/08 22:55:35, scottchen wrote: > On 2017/06/08 18:57:50, hcarmona wrote: > > Vs this |onFocus_|? > > > > IIUC, they both set tabIndex = -1. > > This is when the row gets focused. This function essentially forwards the focus > to the first focusable child and then set the row itself to be unfocusable. > > However, there are some scenarios in which the contained control element get > focused programmatically (e.g. when cr-action-menu). > > In such scenario, this function is not invoked, so when you shift-tab from the > child, the focus *would* go to the row because its tabindex is not set to -1 > yet, which is undesired. Acknowledged.
https://codereview.chromium.org/2930683003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/focus_row_behavior.js (right): https://codereview.chromium.org/2930683003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/focus_row_behavior.js:21: onFocus: function(row, e) { On 2017/06/08 23:21:31, hcarmona wrote: > On 2017/06/08 22:55:35, scottchen wrote: > > On 2017/06/08 18:57:50, hcarmona wrote: > > > When does this |onFocus| get called? > > > > When a control element (i,e, [focus-row-control]) within the list gets > focused. > > Sounds good. Let's add a short comment to both focus functions to remove > ambiguity about when they are called. Done.
LGTM
The CQ bit was checked by scottchen@chromium.org
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by scottchen@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1497044453706840,
"parent_rev": "b353680e28a5a8288b95cd797772a6a08618b284", "commit_rev":
"6d30a69cd0eba07bbbff2e8efc9b0ab2e335dae4"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: fix focus-row-behavior allowing invisible focus on rows. In some scenarios when a row's control is focused, the row remained tabindex="0" (i.e. focusable) so shift-tabbing was moving the focus back to the row which is undesired. This CL makes sure that when the control is focused, shift-tabbing would never go back to the containing row. BUG=725463 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: fix focus-row-behavior allowing invisible focus on rows. In some scenarios when a row's control is focused, the row remained tabindex="0" (i.e. focusable) so shift-tabbing was moving the focus back to the row which is undesired. This CL makes sure that when the control is focused, shift-tabbing would never go back to the containing row. BUG=725463 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2930683003 Cr-Commit-Position: refs/heads/master@{#478458} Committed: https://chromium.googlesource.com/chromium/src/+/6d30a69cd0eba07bbbff2e8efc9b... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/6d30a69cd0eba07bbbff2e8efc9b... |
