Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2269743002:
Profile Avatar Selector: Allow arrow keys to be used for moving between avatars. (Closed)
|
Created:
4 years, 4 months ago by Moe Modified:
4 years, 3 months ago Reviewers:
Dan Beam CC:
chromium-reviews, michaelpg+watch-elements_chromium.org, stevenjb+watch-md-settings_chromium.org, dbeam+watch-elements_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionProfile Avatar Selector: Allow arrow keys to be used for moving between avatars.
BUG=639843
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/41da613e2425d5cd726390dcecdb4102315900c1
Cr-Commit-Position: refs/heads/master@{#420355}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed comment #Patch Set 3 : Fixed broken test #Patch Set 4 : Make left and right arrow keys to act like up and down #
Total comments: 2
Patch Set 5 : Fixes the RTL issue #
Total comments: 1
Patch Set 6 : Up/Down keys focus the avatar directly above/below the currently focused one #
Total comments: 13
Patch Set 7 : Addressed comments #
Total comments: 20
Patch Set 8 : Addressed comments #
Total comments: 2
Patch Set 9 : Addressed comments #Messages
Total messages: 70 (46 generated)
Description was changed from ========== Profile Avatar Selector: Allow arrow keys to be used for moving between avatars. BUG=639843 ========== to ========== Profile Avatar Selector: Allow arrow keys to be used for moving between avatars. BUG=639843 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by mahmadi@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...
mahmadi@chromium.org changed reviewers: + dbeam@chromium.org
Hi Dan, This CL makes the profile avatar selector more accessible by allowing up/down arrow keys to be used to move between the avatars instead of tabbing through them. lpalmaro@'s ideal solution is to use left/right keys. I have another CL that does that but involves more changes: https://codereview.chromium.org/2263403003
https://codereview.chromium.org/2269743002/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html (right): https://codereview.chromium.org/2269743002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:18: background: transparent; iirc, using shorthands in mixins is kind of problematic for polymer's native CSS custom props shim. what are you trying to override here, specifically? background-color? background-image? https://codereview.chromium.org/2269743002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:47: .avatar.iron-selected { does this still apply? https://codereview.chromium.org/2269743002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:53: <paper-listbox id="selector" selected="{{selectedAvatarUrl}}" should id="selector" get updated?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mahmadi@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...
The CQ bit was checked by mahmadi@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...
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2269743002/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html (right): https://codereview.chromium.org/2269743002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:18: background: transparent; On 2016/08/22 21:28:11, Dan Beam wrote: > iirc, using shorthands in mixins is kind of problematic for polymer's native CSS > custom props shim. what are you trying to override here, specifically? > background-color? background-image? afaik it's not shorthands in mixins that cause things to break but the mismatch between shorthands and non-shorthands. meaning when we have a shorthand in mixin and a default non-shorthand property OR vice versa. for example https://jsfiddle.net/8hrtLdL6/ breaks while https://jsfiddle.net/ged0ck48/ doesn't. that's because when the properties match Polymer uses the default value as the fallback: :host { var(--mixin-defined-somewhere-else_-_background, green) } because paper-listbox uses the shorthand background property, using background-color here breaks things for other paper-listboxes https://codereview.chromium.org/2269743002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:47: .avatar.iron-selected { On 2016/08/22 21:28:11, Dan Beam wrote: > does this still apply? yes. this is still valid. https://codereview.chromium.org/2269743002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:53: <paper-listbox id="selector" selected="{{selectedAvatarUrl}}" On 2016/08/22 21:28:11, Dan Beam wrote: > should id="selector" get updated? Done. changed to "avatar-list"
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by mahmadi@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by mahmadi@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...
Hi Dan, Could you please take another look at this? I was waiting to hear back from lpalmaro@ to confirm the desired behavior. We would ideally like 'left' and 'up' arrow keys to go backward in the list of avatars and 'right' and 'down' to go forward, depending on the sketchiness of the solution. (knowing that _onUpKey and _onDownKey are not part of IronMenuBehavior's public API)
Patchset #4 (id:80001) has been deleted
The CQ bit was checked by mahmadi@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...
The CQ bit was checked by mahmadi@chromium.org to run a CQ dry run
Patchset #4 (id:100001) has been deleted
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2269743002/diff/120001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js (right): https://codereview.chromium.org/2269743002/diff/120001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js:41: avatarList.addOwnKeyBinding('left', '_onUpKey'); shouldn't left/right/up/down already work? (in LTR) left https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro... right https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro... up https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro... down https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro...
https://codereview.chromium.org/2269743002/diff/120001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js (right): https://codereview.chromium.org/2269743002/diff/120001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js:41: avatarList.addOwnKeyBinding('left', '_onUpKey'); On 2016/09/07 01:17:56, Dan Beam wrote: > shouldn't left/right/up/down already work? > > (in LTR) > > left > https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro... > > right > https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro... > > up > https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro... > > down > https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro... Oh I forgot about the RTL... in answer to your question, not really. paper-lisbox uses the IronMenuBehavior. Left/Right keys are handled in IronMenubarBehavior. However IronMenubarBehavior overrides the up/down key handlers to be used for selection rather than navigation. https://codereview.chromium.org/2319123002/ creates a new selector element that inherits both behaviors in a particular order which allows the four arrow keys to be used for navigation and all other keys for selection. I personally prefer it over this one especially because it handles RTL.
On 2016/09/07 15:50:50, moe wrote: > https://codereview.chromium.org/2269743002/diff/120001/ui/webui/resources/cr_... > File > ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js > (right): > > https://codereview.chromium.org/2269743002/diff/120001/ui/webui/resources/cr_... > ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js:41: > avatarList.addOwnKeyBinding('left', '_onUpKey'); > On 2016/09/07 01:17:56, Dan Beam wrote: > > shouldn't left/right/up/down already work? > > > > (in LTR) > > > > left > > > https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro... > > > > right > > > https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro... > > > > up > > > https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro... > > > > down > > > https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro... > > Oh I forgot about the RTL... > > in answer to your question, not really. paper-lisbox uses the IronMenuBehavior. > Left/Right keys are handled in IronMenubarBehavior. However IronMenubarBehavior > overrides the up/down key handlers to be used for selection rather than > navigation. that's weird. up/down for selection? is that a thing? i re-read this now more carefully. i can see IronMenubarBehavior's clobbering more clearly now. > https://codereview.chromium.org/2319123002/ creates a new selector element that > inherits both behaviors in a particular order which allows the four arrow keys > to be used for navigation and all other keys for selection. I personally prefer > it over this one especially because it handles RTL.
The CQ bit was checked by mahmadi@chromium.org to run a CQ dry run
Dan, Please take another look at this.
Dry run: 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
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
https://codereview.chromium.org/2269743002/diff/140001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js (right): https://codereview.chromium.org/2269743002/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js:42: avatarList.addOwnKeyBinding('left', '_onDownKey'); i think this code would be more understandable if it was written in terms of '_focusPrevious', '_focusNext' instead of "left means down" and "up means right". can we rewrite in those terms? additionally, does it make sense to just ... not use paper-listbox here and instead just use IronMenuBehavior or IronSelectableBehavior directly from this element's behaviors list?
On 2016/09/15 20:57:23, Dan Beam wrote: > https://codereview.chromium.org/2269743002/diff/140001/ui/webui/resources/cr_... > File > ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js > (right): > > https://codereview.chromium.org/2269743002/diff/140001/ui/webui/resources/cr_... > ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js:42: > avatarList.addOwnKeyBinding('left', '_onDownKey'); > i think this code would be more understandable if it was written in terms of > '_focusPrevious', '_focusNext' instead of "left means down" and "up means > right". > > can we rewrite in those terms? > > additionally, does it make sense to just ... not use paper-listbox here and > instead just use IronMenuBehavior or IronSelectableBehavior directly from this > element's behaviors list? The second part is not quite possible if we are to encapsulate the avatars in the element's local dom. The selectable "items" are the element's light dom children and not its local dom children: https://github.com/PolymerElements/iron-selector/blob/master/iron-selectable.... What you're suggesting can be done with an extra element. Basically, what we had before expect with the relevant parts copied from IronMenubarBehavior instead of using the behavior. It addresses part 1 of your comment too: https://codereview.chromium.org/2349773002/
On 2016/09/16 16:13:54, moe wrote: > On 2016/09/15 20:57:23, Dan Beam wrote: > > > https://codereview.chromium.org/2269743002/diff/140001/ui/webui/resources/cr_... > > File > > > ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js > > (right): > > > > > https://codereview.chromium.org/2269743002/diff/140001/ui/webui/resources/cr_... > > > ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js:42: > > avatarList.addOwnKeyBinding('left', '_onDownKey'); > > i think this code would be more understandable if it was written in terms of > > '_focusPrevious', '_focusNext' instead of "left means down" and "up means > > right". > > > > can we rewrite in those terms? > > > > additionally, does it make sense to just ... not use paper-listbox here and > > instead just use IronMenuBehavior or IronSelectableBehavior directly from this > > element's behaviors list? > > The second part is not quite possible if we are to encapsulate the avatars in > the element's local dom. The selectable "items" are the element's light dom > children and not its local dom children: > https://github.com/PolymerElements/iron-selector/blob/master/iron-selectable.... what if we just changed _updateItems? > > What you're suggesting can be done with an extra element. Basically, what we had > before expect with the relevant parts copied from IronMenubarBehavior instead of > using the behavior. It addresses part 1 of your comment too: > > https://codereview.chromium.org/2349773002/
On 2016/09/16 16:58:39, Dan Beam wrote: > On 2016/09/16 16:13:54, moe wrote: > > On 2016/09/15 20:57:23, Dan Beam wrote: > > > > > > https://codereview.chromium.org/2269743002/diff/140001/ui/webui/resources/cr_... > > > File > > > > > > ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js > > > (right): > > > > > > > > > https://codereview.chromium.org/2269743002/diff/140001/ui/webui/resources/cr_... > > > > > > ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js:42: > > > avatarList.addOwnKeyBinding('left', '_onDownKey'); > > > i think this code would be more understandable if it was written in terms of > > > '_focusPrevious', '_focusNext' instead of "left means down" and "up means > > > right". > > > > > > can we rewrite in those terms? > > > > > > additionally, does it make sense to just ... not use paper-listbox here and > > > instead just use IronMenuBehavior or IronSelectableBehavior directly from > this > > > element's behaviors list? > > > > The second part is not quite possible if we are to encapsulate the avatars in > > the element's local dom. The selectable "items" are the element's light dom > > children and not its local dom children: > > > https://github.com/PolymerElements/iron-selector/blob/master/iron-selectable.... > > what if we just changed _updateItems? > > > > > What you're suggesting can be done with an extra element. Basically, what we > had > > before expect with the relevant parts copied from IronMenubarBehavior instead > of > > using the behavior. It addresses part 1 of your comment too: > > > > https://codereview.chromium.org/2349773002/ Changing _updateItems isn't sufficient. Selection doesn't work either as well as some styling issues. IMO it's easiest to introduce a extra element that uses the IronMenubarBehavior and overrides _onUpKey and _onDownKey to focus an item from the previous/next logical row. I tested it and it works well. wdyt?
On 2016/09/19 22:24:18, moe wrote: > On 2016/09/16 16:58:39, Dan Beam wrote: > > On 2016/09/16 16:13:54, moe wrote: > > > On 2016/09/15 20:57:23, Dan Beam wrote: > > > > > > > > > > https://codereview.chromium.org/2269743002/diff/140001/ui/webui/resources/cr_... > > > > File > > > > > > > > > > ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2269743002/diff/140001/ui/webui/resources/cr_... > > > > > > > > > > ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js:42: > > > > avatarList.addOwnKeyBinding('left', '_onDownKey'); > > > > i think this code would be more understandable if it was written in terms > of > > > > '_focusPrevious', '_focusNext' instead of "left means down" and "up means > > > > right". > > > > > > > > can we rewrite in those terms? > > > > > > > > additionally, does it make sense to just ... not use paper-listbox here > and > > > > instead just use IronMenuBehavior or IronSelectableBehavior directly from > > this > > > > element's behaviors list? > > > > > > The second part is not quite possible if we are to encapsulate the avatars > in > > > the element's local dom. The selectable "items" are the element's light dom > > > children and not its local dom children: > > > > > > https://github.com/PolymerElements/iron-selector/blob/master/iron-selectable.... > > > > what if we just changed _updateItems? > > > > > > > > What you're suggesting can be done with an extra element. Basically, what we > > had > > > before expect with the relevant parts copied from IronMenubarBehavior > instead > > of > > > using the behavior. It addresses part 1 of your comment too: > > > > > > https://codereview.chromium.org/2349773002/ > > Changing _updateItems isn't sufficient. Selection doesn't work either as well as > some styling issues. IMO it's easiest to introduce a extra element that uses the > IronMenubarBehavior and overrides _onUpKey and _onDownKey to focus an item from > the previous/next logical row. I tested it and it works well. wdyt? that sounds fine
Dan, Please take another look.
https://codereview.chromium.org/2269743002/diff/160001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html (right): https://codereview.chromium.org/2269743002/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:18: --cr-profile-avatar-selector-listbox: { use a var if you only need to change 1 thing https://codereview.chromium.org/2269743002/diff/160001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js (right): https://codereview.chromium.org/2269743002/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:26: event.detail.keyboardEvent.preventDefault(); do we want to call preventDefault() only if there was actually a valid row to move? probably https://codereview.chromium.org/2269743002/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:43: rowSize_: function() { nit: calculateAvatarsPerRow_ https://codereview.chromium.org/2269743002/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:45: getComputedStyle(this).getPropertyValue('--avatar-spacing'), 10); call getComputedStyle() only once https://codereview.chromium.org/2269743002/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:56: focusItemInPreviousRow_: function() { why can't we make a moveFocusRow_(rowOffset) where we pass 1 for down and -1 for up? then we could just call: _onDownKey: function(event) { if (this.moveFocusRow_(1)) event.detail.keyboardEvent.preventDefault(); }, _onUpKey: function(event) { if (this.moveFocusRow_(-1)) event.detail.keyboardEvent.preventDefault(); }, and we could put rowSize_ within moveFocusRow_ as it wouldn't need to be shared
The CQ bit was checked by mahmadi@chromium.org to run a CQ dry run
https://codereview.chromium.org/2269743002/diff/160001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html (right): https://codereview.chromium.org/2269743002/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:18: --cr-profile-avatar-selector-listbox: { On 2016/09/21 03:40:59, Dan Beam wrote: > use a var if you only need to change 1 thing The grid's negative margin is a function of the avatar's size. So are the avatar's width and height. https://codereview.chromium.org/2269743002/diff/160001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js (right): https://codereview.chromium.org/2269743002/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:26: event.detail.keyboardEvent.preventDefault(); On 2016/09/21 03:40:59, Dan Beam wrote: > do we want to call preventDefault() only if there was actually a valid row to > move? probably currently the focus wraps vertically (more straightforward to implement it that way). Therefore we always end up focusing something. Do you have any reservations about the focus wrapping? https://codereview.chromium.org/2269743002/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:43: rowSize_: function() { On 2016/09/21 03:40:59, Dan Beam wrote: > nit: calculateAvatarsPerRow_ Done. https://codereview.chromium.org/2269743002/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:45: getComputedStyle(this).getPropertyValue('--avatar-spacing'), 10); On 2016/09/21 03:40:59, Dan Beam wrote: > call getComputedStyle() only once Done. https://codereview.chromium.org/2269743002/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:56: focusItemInPreviousRow_: function() { On 2016/09/21 03:40:59, Dan Beam wrote: > why can't we make a moveFocusRow_(rowOffset) where we pass 1 for down and -1 for > up? > > then we could just call: > > _onDownKey: function(event) { > if (this.moveFocusRow_(1)) > event.detail.keyboardEvent.preventDefault(); > }, > > _onUpKey: function(event) { > if (this.moveFocusRow_(-1)) > event.detail.keyboardEvent.preventDefault(); > }, > > and we could put rowSize_ within moveFocusRow_ as it wouldn't need to be shared Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2269743002/diff/160001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html (right): https://codereview.chromium.org/2269743002/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:18: --cr-profile-avatar-selector-listbox: { On 2016/09/21 16:52:57, moe wrote: > On 2016/09/21 03:40:59, Dan Beam wrote: > > use a var if you only need to change 1 thing > > The grid's negative margin is a function of the avatar's size. So are the > avatar's width and height. i mean this can be changed from a mixin to a variable until we need to change more than one style property? --cr-profile-avatar-selector-listbox-margin: calc(var(--avatar-spacing) / -2); https://codereview.chromium.org/2269743002/diff/160001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js (right): https://codereview.chromium.org/2269743002/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:26: event.detail.keyboardEvent.preventDefault(); On 2016/09/21 16:52:57, moe wrote: > On 2016/09/21 03:40:59, Dan Beam wrote: > > do we want to call preventDefault() only if there was actually a valid row to > > move? probably > > currently the focus wraps vertically (more straightforward to implement it that > way). Therefore we always end up focusing something. Do you have any > reservations about the focus wrapping? ah OK https://codereview.chromium.org/2269743002/diff/180001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js (right): https://codereview.chromium.org/2269743002/diff/180001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:10: (function() { what's the point of this immediately invoked function expression if there's no locals? https://codereview.chromium.org/2269743002/diff/180001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:42: * necessary. @param {number} offset ... https://codereview.chromium.org/2269743002/diff/180001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:53: }.bind(this); it makes no sense that this is wrapped in a function with .bind(this) just... put it in the same level as the rest of the code var length = this.items.length; var style = getComputedStyle(this); var avatarSpacing = parseInt(style.getPropertyValue('--avatar-spacing'), 10); var avatarSize = parseInt(style.getPropertyValue('--avatar-size'), 10); var rowSize = Math.floor(this.offsetWidth / (avatarSpacing + avatarSize)); https://codereview.chromium.org/2269743002/diff/180001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:59: var curFocusIndex = Number(this.indexOf(this.focusedItem)); why does this need to be cast to a number? https://codereview.chromium.org/2269743002/diff/180001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:61: var nextFocusIndex = (curFocusIndex + i * rowSize + gridSize) % gridSize; why does the "+ gridSize" matter if we do % gridSize? is this just to ensure the number is positive? https://codereview.chromium.org/2269743002/diff/180001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:64: var item = this.items[nextFocusIndex]; nit: maybe var item = this.items[(curFocusIndex + i * rowSize) % gridSize]; if (!item) continue; instead of the length check? https://codereview.chromium.org/2269743002/diff/180001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:66: var owner = Polymer.dom(item).getOwnerRoot() || document; do we need the || document? https://codereview.chromium.org/2269743002/diff/180001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:70: if (Polymer.dom(owner).activeElement == item) { can't we assert() this in our code? https://codereview.chromium.org/2269743002/diff/180001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:72: } no curlies
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mahmadi@chromium.org to run a CQ dry run
Thank you for the feedback. Please take another look. https://codereview.chromium.org/2269743002/diff/160001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html (right): https://codereview.chromium.org/2269743002/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:18: --cr-profile-avatar-selector-listbox: { On 2016/09/21 17:54:25, Dan Beam wrote: > On 2016/09/21 16:52:57, moe wrote: > > On 2016/09/21 03:40:59, Dan Beam wrote: > > > use a var if you only need to change 1 thing > > > > The grid's negative margin is a function of the avatar's size. So are the > > avatar's width and height. > > i mean this can be changed from a mixin to a variable until we need to change > more than one style property? > > --cr-profile-avatar-selector-listbox-margin: calc(var(--avatar-spacing) / -2); ok got it. or I can have margin: calc(var(--avatar-spacing) / -2); on the cr-profile-avatar-selector-grid element itself. https://codereview.chromium.org/2269743002/diff/180001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js (right): https://codereview.chromium.org/2269743002/diff/180001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:10: (function() { On 2016/09/21 17:54:26, Dan Beam wrote: > what's the point of this immediately invoked function expression if there's no > locals? a habit more than anything. Removed. https://codereview.chromium.org/2269743002/diff/180001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:42: * necessary. On 2016/09/21 17:54:25, Dan Beam wrote: > @param {number} offset ... Done. strange the closure compiler doesn't nag about missing @param. https://codereview.chromium.org/2269743002/diff/180001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:53: }.bind(this); On 2016/09/21 17:54:25, Dan Beam wrote: > it makes no sense that this is wrapped in a function with .bind(this) > > just... put it in the same level as the rest of the code > > var length = this.items.length; > var style = getComputedStyle(this); > var avatarSpacing = parseInt(style.getPropertyValue('--avatar-spacing'), 10); > var avatarSize = parseInt(style.getPropertyValue('--avatar-size'), 10); > var rowSize = Math.floor(this.offsetWidth / (avatarSpacing + avatarSize)); Done. https://codereview.chromium.org/2269743002/diff/180001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:59: var curFocusIndex = Number(this.indexOf(this.focusedItem)); On 2016/09/21 17:54:25, Dan Beam wrote: > why does this need to be cast to a number? Good question. btw, most of this code was copied/pasted from IronMenuBehavior. I thought they must've had a good reason for casting it. But it looks like it simply calls Array.prototype.indexOf() https://github.com/PolymerElements/iron-selector/blob/master/iron-selectable.... https://codereview.chromium.org/2269743002/diff/180001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:61: var nextFocusIndex = (curFocusIndex + i * rowSize + gridSize) % gridSize; On 2016/09/21 17:54:26, Dan Beam wrote: > why does the "+ gridSize" matter if we do % gridSize? is this just to ensure > the number is positive? % returns a negative number if the first operand is negative. https://codereview.chromium.org/2269743002/diff/180001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:64: var item = this.items[nextFocusIndex]; On 2016/09/21 17:54:25, Dan Beam wrote: > nit: maybe > > var item = this.items[(curFocusIndex + i * rowSize) % gridSize]; > if (!item) > continue; > > instead of the length check? Done. https://codereview.chromium.org/2269743002/diff/180001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:66: var owner = Polymer.dom(item).getOwnerRoot() || document; On 2016/09/21 17:54:25, Dan Beam wrote: > do we need the || document? in shadow dom, not really. Again, IronMenuBehavior copy/paste... https://codereview.chromium.org/2269743002/diff/180001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:70: if (Polymer.dom(owner).activeElement == item) { On 2016/09/21 17:54:25, Dan Beam wrote: > can't we assert() this in our code? why assert? if the item isn't focusable we should try the next one until we find something that is. https://codereview.chromium.org/2269743002/diff/180001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:72: } On 2016/09/21 17:54:25, Dan Beam wrote: > no curlies Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2269743002/diff/180001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js (right): https://codereview.chromium.org/2269743002/diff/180001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:70: if (Polymer.dom(owner).activeElement == item) { On 2016/09/22 00:25:09, moe wrote: > On 2016/09/21 17:54:25, Dan Beam wrote: > > can't we assert() this in our code? > > why assert? if the item isn't focusable we should try the next one until we find > something that is. which of the current items aren't focusable?
lgtm https://codereview.chromium.org/2269743002/diff/200001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.html (right): https://codereview.chromium.org/2269743002/diff/200001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.html:12: @apply(--cr-profile-avatar-selector-grid); ^ still used?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mahmadi@chromium.org to run a CQ dry run
Thank you. https://codereview.chromium.org/2269743002/diff/180001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js (right): https://codereview.chromium.org/2269743002/diff/180001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:70: if (Polymer.dom(owner).activeElement == item) { On 2016/09/22 01:01:43, Dan Beam wrote: > On 2016/09/22 00:25:09, moe wrote: > > On 2016/09/21 17:54:25, Dan Beam wrote: > > > can't we assert() this in our code? > > > > why assert? if the item isn't focusable we should try the next one until we > find > > something that is. > > which of the current items aren't focusable? ah I see your point now. https://codereview.chromium.org/2269743002/diff/200001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.html (right): https://codereview.chromium.org/2269743002/diff/200001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.html:12: @apply(--cr-profile-avatar-selector-grid); On 2016/09/22 01:03:25, Dan Beam wrote: > ^ still used? Removed. My bad.
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mahmadi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2269743002/#ps220001 (title: "Addressed comments")
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 #9 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Profile Avatar Selector: Allow arrow keys to be used for moving between avatars. BUG=639843 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Profile Avatar Selector: Allow arrow keys to be used for moving between avatars. BUG=639843 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/41da613e2425d5cd726390dcecdb4102315900c1 Cr-Commit-Position: refs/heads/master@{#420355} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/41da613e2425d5cd726390dcecdb4102315900c1 Cr-Commit-Position: refs/heads/master@{#420355} |
