feat: initial keyboard shortcut support #263
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| /** | ||
| * Media Controller should not mimic the HTMLMediaElement API. | ||
| * @see https://github.com/muxinc/media-chrome/pull/182#issuecomment-1067370339 | ||
| */ |
There was a problem hiding this comment.
moved from the bottom of the class here, which seems more appropriate.
src/js/media-controller.js
Outdated
| * @see https://github.com/muxinc/media-chrome/pull/182#issuecomment-1067370339 | ||
| */ | ||
| keyboardShortcutHandler(e) { | ||
| if (this.contains(e.target) || this.shadowRoot.contains(e.target)) { |
There was a problem hiding this comment.
question(non-blocking): Do we need these checks?
There was a problem hiding this comment.
Maybe not, since I guess the event bubbling should guarantee that it's coming from a child element?
There was a problem hiding this comment.
yup that's what I was thinking. We may also get into some weirdness with contains checks for nested web component use cases.
src/js/media-controller.js
Outdated
| // keysUsed is either an attribute or a property. | ||
| // The attribute is a DOM array and the property is a JS array | ||
| // In the attribute Space represents the space key and gets convered to ' ' | ||
| const keysUsed = (e.target.getAttribute('keysused')?.split(' ') ?? e.target?.keysUsed ?? []) |
There was a problem hiding this comment.
thought(non-blocking): As a followup, we'll need to figure out how this should work for non-nested components based on the current architecture (See code/architecture for associatedElements and how event (re)propagation works).
There was a problem hiding this comment.
yup, this got called out in the discussion #257
src/js/media-controller.js
Outdated
| return; | ||
| } | ||
|
|
||
| if (!this.hasAttribute('nohotkeys')) { |
There was a problem hiding this comment.
suggestion(non-blocking): Instead of continuing to listen to irrelevant keyboard presses when "nohotkeys" is set, we should probably:
- add
"nohotkeys"toobservedAttributes. - add/remove event handlers for
"keyup"/"keydown"based onattributeChangedCallback- This entails moving the event callbacks to methods instead of inline.
docs/media-controller.md
Outdated
| | ⬅ | Seek back 10s | | ||
| | ➡ | Seek forward 10s | | ||
|
|
||
| If you are implementing an interactive element that uses any of these keys, you can stopPropagation in your `keyup` handler. Alternatively, you can add a `keysUsed` property on the element or a `keysused` attribute. |
There was a problem hiding this comment.
chore(blocking):
- Discuss and link to https://developer.mozilla.org/en-US/docs/Web/API/UI_Events/Keyboard_event_key_values
- Callout the
"Space"keyword mapping
| If you are implementing an interactive element that uses any of these keys, you can stopPropagation in your `keyup` handler. Alternatively, you can add a `keysUsed` property on the element or a `keysused` attribute. The values are those that match the `key` property on the KeyboardEvent. You can find a list of those values [on mdn](https://developer.mozilla.org/en-US/docs/Web/API/UI_Events/Keyboard_event_key_values). Additionally, since the DOM list can't have the Space key represented as `" "`, we will accept `Space` as an alternative name for it. | ||
| Example (`keysused` attribute): | ||
| ```html | ||
| <media-time-range keysused="ArrowLeft ArrowRight Space"></media-time-range> |
src/js/media-controller.js
Outdated
| #keyUpHandler(e) { | ||
| const { key } = e; | ||
| if (!ButtonPressedKeys.includes(key)) { | ||
| this.removeEventListener('keyup', keyUpHandler); |
There was a problem hiding this comment.
issue(blocker): missed this.#keyUpHandler refactor.
| Example (hotkeys disabled): | ||
|
|
||
| ```html | ||
| <media-controller nohotkeys> |
Kicking off the work for #257