Add support for enter and space on amp-selector #9083
Conversation
| @@ -106,10 +106,7 @@ export class AmpSelector extends AMP.BaseElement { | |||
| this.init_(); | |||
| if (!this.isDisabled_) { | |||
| this.element.addEventListener('click', this.clickHandler_.bind(this)); | |||
| if (this.kbSelectMode_ != KEYBOARD_SELECT_MODES.NONE) { | |||
There was a problem hiding this comment.
why are we removing this?
There was a problem hiding this comment.
we definitely dont need this on mobile devices, seems to be unnecessary when some one does not set keyboard option
There was a problem hiding this comment.
Users who use a keyboard as a primary input device, on or off mobile, should be able to select an amp-selector option regardless of the keyboard-select-mode attribute.
There was a problem hiding this comment.
the viewport/platform i think has an api to figureout if keyboard is available, in-fact it uses that to add css classes to the body.
There was a problem hiding this comment.
I'm not seeing that. Could you link to it?
There was a problem hiding this comment.
touch does not mean !keyboard. Folks with dexterity use physical keyboard on their smartphones. We should always assume a physical keyboard exists. Adding a listener that may not be called is not something to optimize for anyway. It should have ~0 perf impact.
There was a problem hiding this comment.
@aghassemi - yes i do understand that, AMP has a way to detect if a device has a keyboard attached or not
@kmh287 - search for amp-mode-keyboard-active and you should be able to track it down.
There was a problem hiding this comment.
I believe that's for when the keyboard is presently in use i.e. user is typing, not for when there's a keyboard connected.
| * @private | ||
| */ | ||
| updateFocus_() { | ||
| updateFocus_(opt_focusOn) { |
| initialFocusedElement = this.options_[0]; | ||
| } else { | ||
| initialFocusedElement = this.selectedOptions_[0] || this.options_[0]; | ||
| let focusElement = opt_focusOn; |
There was a problem hiding this comment.
why not use the param variable?
There was a problem hiding this comment.
I would rather not redefine the parameter.
| selectionKeyDownHandler_(event) { | ||
| const keyCode = event.keyCode; | ||
| if (keyCode == Keycodes.SPACE || keyCode == Keycodes.ENTER) { | ||
| event.preventDefault(); |
There was a problem hiding this comment.
move preventDefault inside the if (this.options_.includes(event.target)) otherwise actionable elements inside option can be broken ( e.g. <div option> <button> </div>)
| } | ||
| } | ||
|
|
||
| navigationKeyDownHandler_(event) { |
There was a problem hiding this comment.
awesome. now with multi-select and keyboard-select-mode='focus' one can arrow through options and use SPACE to toggle them on/off. Exactly how it should work. I wish he had integration tests for amp-selector to constantly verify these end-to-end. May not be too much work if you find free time sometime.
|
I can definitely take a look in the near-future. 😄 |
|
@camelburrito Let me know if you have any further comments. |
|
Thanks! |
Adds support for enter and space on amp-selector.
keyboard-select-modeattributeAlso fixes a couple of bugs with arrow key navigation.
Partial for #8810
/to @aghassemi @camelburrito