Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2389203003:
Move tab index and focus to radio button. (Closed)
|
Created:
4 years, 2 months ago by hcarmona Modified:
4 years, 2 months ago Reviewers:
Dan Beam 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/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove tab index and focus to radio button.
BUG=646284
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/3dd8aeb0481c8dd9f05e952c75cdb8d3c9a5eb53
Cr-Commit-Position: refs/heads/master@{#424575}
Patch Set 1 #
Total comments: 6
Dependent Patchsets: Messages
Total messages: 18 (8 generated)
Description was changed from ========== Move tab index and focus to radio button. BUG=646284 ========== to ========== Move tab index and focus to radio button. BUG=646284 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
hcarmona@chromium.org changed reviewers: + dbeam@chromium.org
PTAL, this makes it so focus moves to the next control when tabbing out of a radio group. Do you know of a radio group that's policy controlled? That's something that I didn't look at and want to make sure I didn't break focus. Also, how *should* focus work with a policy controlled radio button?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2389203003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/controls/controlled_radio_button.html (right): https://codereview.chromium.org/2389203003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/controls/controlled_radio_button.html:33: disabled="[[controlled_]]" tabindex$="[[tabindex]]"> how does this tabindex$= binding work?
https://codereview.chromium.org/2389203003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/controls/controlled_radio_button.html (right): https://codereview.chromium.org/2389203003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/controls/controlled_radio_button.html:33: disabled="[[controlled_]]" tabindex$="[[tabindex]]"> On 2016/10/05 23:39:34, Dan Beam wrote: > how does this tabindex$= binding work? The $= signifies that you're binding to an attribute. https://www.polymer-project.org/1.0/docs/devguide/data-binding It's binding to the tabindex attribute of the host b/c attributes are also polymer properties.
https://codereview.chromium.org/2389203003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/controls/controlled_radio_button.html (right): https://codereview.chromium.org/2389203003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/controls/controlled_radio_button.html:33: disabled="[[controlled_]]" tabindex$="[[tabindex]]"> On 2016/10/06 15:36:21, Hector Carmona wrote: > On 2016/10/05 23:39:34, Dan Beam wrote: > > how does this tabindex$= binding work? > > The $= signifies that you're binding to an attribute. > > https://www.polymer-project.org/1.0/docs/devguide/data-binding > > It's binding to the tabindex attribute of the host b/c attributes are > also polymer properties. Forgot to mention: The host element gets its tabindex from the paper-radio-group which takes care of managing the tab index for its children so that only one is tab-able at a time.
https://codereview.chromium.org/2389203003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/controls/controlled_radio_button.html (right): https://codereview.chromium.org/2389203003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/controls/controlled_radio_button.html:33: disabled="[[controlled_]]" tabindex$="[[tabindex]]"> On 2016/10/06 15:36:21, Hector Carmona wrote: > On 2016/10/05 23:39:34, Dan Beam wrote: > > how does this tabindex$= binding work? > > The $= signifies that you're binding to an attribute. > > https://www.polymer-project.org/1.0/docs/devguide/data-binding yep, get that > > It's binding to the tabindex attribute of the host b/c attributes are > also polymer properties. ehhhh, this is kinda odd imo, to use hostAttributes in this way, especially because I don't see where this value ever changes: https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro... if the behavior is specified i guess it's fine. i think all of this might be less cryptically expressed as simply... tabindex="0"
https://codereview.chromium.org/2389203003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/controls/controlled_radio_button.html (right): https://codereview.chromium.org/2389203003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/controls/controlled_radio_button.html:33: disabled="[[controlled_]]" tabindex$="[[tabindex]]"> On 2016/10/11 18:58:12, Dan Beam wrote: > On 2016/10/06 15:36:21, Hector Carmona wrote: > > On 2016/10/05 23:39:34, Dan Beam wrote: > > > how does this tabindex$= binding work? > > > > The $= signifies that you're binding to an attribute. > > > > https://www.polymer-project.org/1.0/docs/devguide/data-binding > > yep, get that > > > > > It's binding to the tabindex attribute of the host b/c attributes are > > also polymer properties. > > ehhhh, this is kinda odd imo, to use hostAttributes in this way, especially > because I don't see where this value ever changes: > https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro... > > if the behavior is specified i guess it's fine. > > i think all of this might be less cryptically expressed as simply... > tabindex="0" tabindex="0" is what we have now because each paper-radio-button is already focusable. This change transfers focusability from the host so that the radio button is only focusable when the row is selected. The tab index is set on the host by a parent element that has an IronMenuBehavior. The paper-radio-button is the element that should be focusable (not the host), so this change transfers focusability from the host to the radio button. Another option is to make this tabindex="-1", but then we won't get the ripple and focusable behavior from the paper-radio-button.
https://codereview.chromium.org/2389203003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/controls/controlled_radio_button.html (right): https://codereview.chromium.org/2389203003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/controls/controlled_radio_button.html:33: disabled="[[controlled_]]" tabindex$="[[tabindex]]"> On 2016/10/11 20:54:02, Hector Carmona wrote: > On 2016/10/11 18:58:12, Dan Beam wrote: > > On 2016/10/06 15:36:21, Hector Carmona wrote: > > > On 2016/10/05 23:39:34, Dan Beam wrote: > > > > how does this tabindex$= binding work? > > > > > > The $= signifies that you're binding to an attribute. > > > > > > https://www.polymer-project.org/1.0/docs/devguide/data-binding > > > > yep, get that > > > > > > > > It's binding to the tabindex attribute of the host b/c attributes are > > > also polymer properties. > > > > ehhhh, this is kinda odd imo, to use hostAttributes in this way, especially > > because I don't see where this value ever changes: > > > https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro... > > > > if the behavior is specified i guess it's fine. > > > > i think all of this might be less cryptically expressed as simply... > > tabindex="0" > > tabindex="0" is what we have now because each paper-radio-button is > already focusable. This change transfers focusability from the host > so that the radio button is only focusable when the row is selected. > > The tab index is set on the host by a parent element that has an > IronMenuBehavior. The paper-radio-button is the element that should be > focusable (not the host), so this change transfers focusability from the > host to the radio button. ok, this is what changes it. great. than this is definitely the best. lgtm > > Another option is to make this tabindex="-1", but then we won't get > the ripple and focusable behavior from the paper-radio-button. nah
The CQ bit was checked by hcarmona@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Move tab index and focus to radio button. BUG=646284 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Move tab index and focus to radio button. BUG=646284 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/3dd8aeb0481c8dd9f05e952c75cdb8d3c9a5eb53 Cr-Commit-Position: refs/heads/master@{#424575} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/3dd8aeb0481c8dd9f05e952c75cdb8d3c9a5eb53 Cr-Commit-Position: refs/heads/master@{#424575} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
