Added support for arrow keys in media play [chromium/src : main]

SHREY PATEL (Gerrit)

unread,

Sep 22, 2023, 4:22:45 AM9/22/23

to Dale Curtis, blink-rev...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org

Attention is currently required from: Dale Curtis.

SHREY PATEL would like Dale Curtis to review this change.

Added support for arrow keys in media play

Need of this change:
When opening audio/video with chrome (using chrome as a media player)
arrow keys dosent serve a purpose.

Change Description:
Added support for arrow keys to control the timeline and volume
in the media element. This enhancement allows users to easily
navigate within media content and adjust the volume using keyboard
shortcuts.

chromium-discuss:
https://groups.google.com/a/chromium.org/g/chromium-discuss/c/t7SgVV9N6lk
Change-Id: I1fa5bfb8e27af06e01e341434599bcc12c1a0749
---
M AUTHORS
M third_party/blink/renderer/core/html/media/media_document.cc
2 files changed, 33 insertions(+), 0 deletions(-)

To view, visit change 4881903. To unsubscribe, or for help writing mail filters, visit settings.

Dale Curtis (Gerrit)

unread,

Sep 22, 2023, 1:00:57 PM9/22/23

to Frank Liberato, blink-rev...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, SHREY PATEL

Attention is currently required from: Frank Liberato, SHREY PATEL.

Dale Curtis would like Frank Liberato to review this change authored by SHREY PATEL.

Dale Curtis (Gerrit)

unread,

Sep 22, 2023, 1:01:05 PM9/22/23

to SHREY PATEL, blink-rev...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, Frank Liberato, chromium...@chromium.org

Attention is currently required from: Frank Liberato, SHREY PATEL.

View Change

1 comment:

Patchset:
- Patch Set #1:
  =>media ux folks for initial discussion.

To view, visit change 4881903. To unsubscribe, or for help writing mail filters, visit settings.

Dale Curtis (Gerrit)

unread,

Sep 22, 2023, 1:01:58 PM9/22/23

to SHREY PATEL, blink-rev...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, Frank Liberato, chromium...@chromium.org

Attention is currently required from: Frank Liberato, SHREY PATEL.

View Change

1 comment:

Patchset:
- Patch Set #1:
  (Generally seems fine to me since this only affects MediaDocument and not the normal HTMLMediaElement).

To view, visit change 4881903. To unsubscribe, or for help writing mail filters, visit settings.

Frank Liberato (Gerrit)

unread,

Sep 22, 2023, 1:16:44 PM9/22/23

to Tommy Steimel, blink-rev...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, SHREY PATEL, Frank Liberato, Dale Curtis

Attention is currently required from: Frank Liberato, SHREY PATEL, Tommy Steimel.

Frank Liberato would like Tommy Steimel to review this change authored by SHREY PATEL.

View Change

Added support for arrow keys in media play

Need of this change:
When opening audio/video with chrome (using chrome as a media player)
arrow keys dosent serve a purpose.

Change Description:
Added support for arrow keys to control the timeline and volume
in the media element. This enhancement allows users to easily
navigate within media content and adjust the volume using keyboard
shortcuts.

chromium-discuss:
https://groups.google.com/a/chromium.org/g/chromium-discuss/c/t7SgVV9N6lk
Change-Id: I1fa5bfb8e27af06e01e341434599bcc12c1a0749
---
M AUTHORS
M third_party/blink/renderer/core/html/media/media_document.cc
2 files changed, 33 insertions(+), 0 deletions(-)

To view, visit change 4881903. To unsubscribe, or for help writing mail filters, visit settings.

Frank Liberato (Gerrit)

unread,

Sep 22, 2023, 1:16:58 PM9/22/23

to SHREY PATEL, blink-rev...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, Tommy Steimel, Frank Liberato, Dale Curtis, chromium...@chromium.org

Attention is currently required from: Frank Liberato, SHREY PATEL, Tommy Steimel.

View Change

1 comment:

Patchset:
- Patch Set #1:
  good catch -- i never noticed that the media document keyboard behavior doesn't allow this.
  the main concern i have is that the media controls try to do this already [1], and it works fine outside of the media document if the media element has the focus. i'm not sure why any of the media document keyboard handling is needed -- maybe it should route keys to the media element, so the behavior stays in sync. is it just so that focus isn't required?
  +steimel@: do you have any idea why it's like this?
  thanks
  -fl
  [1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/media_controls/media_controls_impl.cc;drc=f765f883246ae3e1ea83c5c92410e4af497a4ab1;l=1543

To view, visit change 4881903. To unsubscribe, or for help writing mail filters, visit settings.

Tommy Steimel (Gerrit)

unread,

Sep 22, 2023, 3:12:54 PM9/22/23

to SHREY PATEL, blink-rev...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, Frank Liberato, Frank Liberato, Dale Curtis, chromium...@chromium.org

Attention is currently required from: Frank Liberato, Frank Liberato, SHREY PATEL.

View Change

1 comment:

Patchset:
- Patch Set #1:
  good catch -- i never noticed that the media document keyboard behavior doesn't allow this. […]
  I think we considered automatically focusing the media element before, but for some reason it wasn't ideal (I can't recall the details off the top of my head since this was many years ago). Routing these keys directly to the media element so we don't need to duplicate the logic makes sense to me

To view, visit change 4881903. To unsubscribe, or for help writing mail filters, visit settings.

SHREY PATEL (Gerrit)

unread,

Sep 22, 2023, 3:25:36 PM9/22/23

to blink-rev...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, Tommy Steimel, Frank Liberato, Frank Liberato, Dale Curtis, chromium...@chromium.org

Attention is currently required from: Frank Liberato, Frank Liberato, Tommy Steimel.

View Change

1 comment:

Patchset:
- Patch Set #1:
  I think we considered automatically focusing the media element before, but for some reason it wasn't […]
  @Frank I was thinking the same, Why wouldn't the media controls work.
  Then I found the code which I edited. Initially it only contained the space to control the media. So my straight forward thought was to add arrow keys to it.
  but now as @Tommy Steimel mentioned routing these makes more sense.
  can you help me by referring to other code where Routing controls may be used.
  thanks.

To view, visit change 4881903. To unsubscribe, or for help writing mail filters, visit settings.

Frank Liberato (Gerrit)

unread,

Sep 22, 2023, 3:43:06 PM9/22/23

to SHREY PATEL, blink-rev...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, Tommy Steimel, Frank Liberato, Dale Curtis, chromium...@chromium.org

Attention is currently required from: Frank Liberato, SHREY PATEL, Tommy Steimel.

View Change

1 comment:

Patchset:
- Patch Set #1:
  @Frank I was thinking the same, Why wouldn't the media controls work. […]
  one might be able to `video.dispatchEvent(event)` or similar.

To view, visit change 4881903. To unsubscribe, or for help writing mail filters, visit settings.

SHREY PATEL (Gerrit)

unread,

Sep 22, 2023, 6:14:13 PM9/22/23

to blink-rev...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org

Attention is currently required from: Frank Liberato, SHREY PATEL, Tommy Steimel.

SHREY PATEL uploaded patch set #2 to this change.

View Change

Added support for arrow keys in media play

Need of this change:
When opening audio/video with chrome (using chrome as a media player)
arrow keys dosent serve a purpose.

Change Description:
Added support for arrow keys to control the timeline and volume
in the media element. This enhancement allows users to easily
navigate within media content and adjust the volume using keyboard
shortcuts.

chromium-discuss:
https://groups.google.com/a/chromium.org/g/chromium-discuss/c/t7SgVV9N6lk
Change-Id: I1fa5bfb8e27af06e01e341434599bcc12c1a0749
---
M AUTHORS
M third_party/blink/renderer/core/html/media/media_document.cc

2 files changed, 4 insertions(+), 0 deletions(-)

To view, visit change 4881903. To unsubscribe, or for help writing mail filters, visit settings.

SHREY PATEL (Gerrit)

unread,

Sep 25, 2023, 4:04:50 PM9/25/23

to blink-rev...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, Tommy Steimel, Frank Liberato, Frank Liberato, Dale Curtis, chromium...@chromium.org

Attention is currently required from: Frank Liberato, Frank Liberato, Tommy Steimel.

View Change

1 comment:

Patchset:
- Patch Set #1:
  one might be able to `video.dispatchEvent(event)` or similar.
  @libe...@chromium.org I have changed the code as specified.

To view, visit change 4881903. To unsubscribe, or for help writing mail filters, visit settings.

Frank Liberato (Gerrit)

unread,

Sep 25, 2023, 9:18:44 PM9/25/23

to SHREY PATEL, blink-rev...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, Tommy Steimel, Frank Liberato, Dale Curtis, chromium...@chromium.org

Attention is currently required from: Frank Liberato, SHREY PATEL, Tommy Steimel.

View Change

2 comments:

File third_party/blink/renderer/core/html/media/media_document.cc:
- Patch Set #2, Line 151: VKEY_MEDIA_PLAY_PAUSE
  It looks like the media controls code doesn't handle `VKEY_MEDIA_PLAY_PAUSE`, so we can't get rid of this case yet.
  @steimel -- it's surprising to me that media keys are checked here, instead of just being a media session thing. Do you know if this is important?
- Patch Set #2, Line 158: event.SetDefaultHandled();
  I'm not sure that you should call this here. I think that `video->DispatchEvent()` will set it if needed.

To view, visit change 4881903. To unsubscribe, or for help writing mail filters, visit settings.

SHREY PATEL (Gerrit)

unread,

Sep 26, 2023, 9:20:42 AM9/26/23

to blink-rev...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org

Attention is currently required from: Frank Liberato, SHREY PATEL, Tommy Steimel.

SHREY PATEL uploaded patch set #3 to this change.

View Change

Added support for arrow keys in media play

Need of this change:
When opening audio/video with chrome (using chrome as a media player)
arrow keys dosent serve a purpose.

Change Description:
Added support for arrow keys to control the timeline and volume
in the media element. This enhancement allows users to easily
navigate within media content and adjust the volume using keyboard
shortcuts.

chromium-discuss:
https://groups.google.com/a/chromium.org/g/chromium-discuss/c/t7SgVV9N6lk
Change-Id: I1fa5bfb8e27af06e01e341434599bcc12c1a0749
---
M AUTHORS
M third_party/blink/renderer/core/html/media/media_document.cc

2 files changed, 3 insertions(+), 0 deletions(-)

To view, visit change 4881903. To unsubscribe, or for help writing mail filters, visit settings.

SHREY PATEL (Gerrit)

unread,

Sep 26, 2023, 9:21:53 AM9/26/23

to blink-rev...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, Tommy Steimel, Frank Liberato, Frank Liberato, Dale Curtis, chromium...@chromium.org

Attention is currently required from: Frank Liberato, Frank Liberato, Tommy Steimel.

View Change

1 comment:

File third_party/blink/renderer/core/html/media/media_document.cc:
- Patch Set #2, Line 158: event.SetDefaultHandled();
  I'm not sure that you should call this here. […]
  done.

To view, visit change 4881903. To unsubscribe, or for help writing mail filters, visit settings.

SHREY PATEL (Gerrit)

unread,

Sep 26, 2023, 9:24:54 AM9/26/23

to blink-rev...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, Tommy Steimel, Frank Liberato, Frank Liberato, Dale Curtis, chromium...@chromium.org

Attention is currently required from: Frank Liberato, Frank Liberato, Tommy Steimel.

View Change

1 comment:

File third_party/blink/renderer/core/html/media/media_document.cc:
- Patch Set #2, Line 151: VKEY_MEDIA_PLAY_PAUSE
  It looks like the media controls code doesn't handle `VKEY_MEDIA_PLAY_PAUSE`, so we can't get rid of […]
  Yes, as you said removing this will remove the functionality of play and pause.

To view, visit change 4881903. To unsubscribe, or for help writing mail filters, visit settings.

Frank Liberato (Gerrit)

unread,

Sep 26, 2023, 11:14:41 AM9/26/23

to SHREY PATEL, blink-rev...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, Tommy Steimel, Frank Liberato, Dale Curtis, chromium...@chromium.org

Attention is currently required from: Dale Curtis, Frank Liberato, SHREY PATEL, Tommy Steimel.

View Change

3 comments:

Patchset:

- Patch Set #1:
  =>media ux folks for initial discussion.

- Acknowledged
File third_party/blink/renderer/core/html/media/media_document.cc:
- Patch Set #2, Line 158: event.SetDefaultHandled();
  done.
  thanks! as an aside, there's a checkbox at the bottom of each comment block that's labelled 'resolved'. checking it when you fill in your reply will also mark the comment thread as finished. That way, it doesn't show up in the list of active / unaddressed comments.
File third_party/blink/renderer/core/html/media/media_document.cc:
- Patch Set #3, Line 154: event.SetDefaultHandled();
  I just noticed that it probably shouldn't this return here, rather than falling through to @157. i'm surprised that ' ' works as intended without it. maybe setting it to 'DefaultHandled' skips the second dispatch or something.
  Sorry about that -- I should have noticed that yesterday and saved a round-trip.

To view, visit change 4881903. To unsubscribe, or for help writing mail filters, visit settings.

Tommy Steimel (Gerrit)

unread,

Sep 26, 2023, 2:30:51 PM9/26/23

to SHREY PATEL, blink-rev...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, Frank Liberato, Frank Liberato, Dale Curtis, chromium...@chromium.org

Attention is currently required from: Frank Liberato, Frank Liberato, SHREY PATEL.

View Change

1 comment:

File third_party/blink/renderer/core/html/media/media_document.cc:
- Patch Set #2, Line 151: VKEY_MEDIA_PLAY_PAUSE

- Yes, as you said removing this will remove the functionality of play and pause.

- I think this predates when we had system media controls handle it, but it's probably fine to keep this, especially if it's not working without. I assume if we're getting it here then it's not getting passed to system media controls anyway, so we may as well use it. Either way, it's not something to worry about for this CL

To view, visit change 4881903. To unsubscribe, or for help writing mail filters, visit settings.

SHREY PATEL (Gerrit)

unread,

Sep 26, 2023, 3:21:03 PM9/26/23

to blink-rev...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org

Attention is currently required from: Frank Liberato, Frank Liberato, SHREY PATEL.

SHREY PATEL uploaded patch set #4 to this change.

View Change

Added support for arrow keys in media play

Need of this change:
When opening audio/video with chrome (using chrome as a media player)
arrow keys dosent serve a purpose.

Change Description:
Added support for arrow keys to control the timeline and volume
in the media element. This enhancement allows users to easily
navigate within media content and adjust the volume using keyboard
shortcuts.

chromium-discuss:
https://groups.google.com/a/chromium.org/g/chromium-discuss/c/t7SgVV9N6lk
Change-Id: I1fa5bfb8e27af06e01e341434599bcc12c1a0749
---
M AUTHORS
M third_party/blink/renderer/core/html/media/media_document.cc

2 files changed, 5 insertions(+), 0 deletions(-)

To view, visit change 4881903. To unsubscribe, or for help writing mail filters, visit settings.

SHREY PATEL (Gerrit)

unread,

Sep 26, 2023, 3:49:15 PM9/26/23

to blink-rev...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, Tommy Steimel, Frank Liberato, Frank Liberato, Dale Curtis, chromium...@chromium.org

Attention is currently required from: Frank Liberato, Frank Liberato, Tommy Steimel.

View Change

1 comment:

File third_party/blink/renderer/core/html/media/media_document.cc:

- Patch Set #2, Line 151: VKEY_MEDIA_PLAY_PAUSE

- I think this predates when we had system media controls handle it, but it's probably fine to keep th […]
  thanks!

To view, visit change 4881903. To unsubscribe, or for help writing mail filters, visit settings.

SHREY PATEL (Gerrit)

unread,

Sep 26, 2023, 3:50:18 PM9/26/23

to blink-rev...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, Tommy Steimel, Frank Liberato, Frank Liberato, Dale Curtis, chromium...@chromium.org

Attention is currently required from: Frank Liberato, Frank Liberato, Tommy Steimel.

View Change

1 comment:

File third_party/blink/renderer/core/html/media/media_document.cc:
- Patch Set #3, Line 154: event.SetDefaultHandled();
  I just noticed that it probably shouldn't this return here, rather than falling through to @157. […]
  Added return statements to terminate the code before further execution.

To view, visit change 4881903. To unsubscribe, or for help writing mail filters, visit settings.

Frank Liberato (Gerrit)

unread,

Sep 26, 2023, 4:00:25 PM9/26/23

to SHREY PATEL, blink-rev...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, Tommy Steimel, Frank Liberato, Dale Curtis, chromium...@chromium.org

Attention is currently required from: Frank Liberato, SHREY PATEL, Tommy Steimel.

Patch set 5:Code-Review +1

View Change

To view, visit change 4881903. To unsubscribe, or for help writing mail filters, visit settings.

Frank Liberato (Gerrit)

unread,

Sep 26, 2023, 4:07:33 PM9/26/23

to SHREY PATEL, blink-rev...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, Tommy Steimel, Frank Liberato, Dale Curtis, chromium...@chromium.org

Attention is currently required from: Frank Liberato, SHREY PATEL, Tommy Steimel.

Patch set 5:Commit-Queue +1

View Change

To view, visit change 4881903. To unsubscribe, or for help writing mail filters, visit settings.

Tommy Steimel (Gerrit)

unread,

Sep 26, 2023, 5:01:11 PM9/26/23

to SHREY PATEL, blink-rev...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, Chromium LUCI CQ, Frank Liberato, Frank Liberato, Dale Curtis, chromium...@chromium.org

Attention is currently required from: Frank Liberato, SHREY PATEL.

Patch set 5:Code-Review +1

View Change

To view, visit change 4881903. To unsubscribe, or for help writing mail filters, visit settings.

Dale Curtis (Gerrit)

unread,

Sep 26, 2023, 6:32:40 PM9/26/23

to SHREY PATEL, blink-rev...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, Tommy Steimel, Chromium LUCI CQ, Frank Liberato, Frank Liberato, chromium...@chromium.org

Dale Curtis (Gerrit)

unread,

Sep 26, 2023, 6:32:43 PM9/26/23

to SHREY PATEL, blink-rev...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, Tommy Steimel, Chromium LUCI CQ, Frank Liberato, Frank Liberato, chromium...@chromium.org

Attention is currently required from: Frank Liberato, SHREY PATEL.

Patch set 5:Commit-Queue +2

View Change

To view, visit change 4881903. To unsubscribe, or for help writing mail filters, visit settings.

Chromium LUCI CQ (Gerrit)

unread,

Sep 26, 2023, 7:02:09 PM9/26/23

to SHREY PATEL, blink-rev...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, Dale Curtis, Tommy Steimel, Frank Liberato, Frank Liberato, chromium...@chromium.org

Chromium LUCI CQ submitted this change.

View Change

Approvals:
  Dale Curtis: Looks good to me; Commit
  Tommy Steimel: Looks good to me
  Frank Liberato: Looks good to me

Added support for arrow keys in media play

Need of this change:
When opening audio/video with chrome (using chrome as a media player)
arrow keys dosent serve a purpose.

Change Description:
Added support for arrow keys to control the timeline and volume
in the media element. This enhancement allows users to easily
navigate within media content and adjust the volume using keyboard
shortcuts.

chromium-discuss: 
https: //groups.google.com/a/chromium.org/g/chromium-discuss/c/t7SgVV9N6lk
Change-Id: I1fa5bfb8e27af06e01e341434599bcc12c1a0749

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4881903
Reviewed-by: Dale Curtis <dalec...@chromium.org>
Reviewed-by: Frank Liberato <libe...@chromium.org>
Reviewed-by: Tommy Steimel <ste...@chromium.org>
Commit-Queue: Dale Curtis <dalec...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1201855}


---
M AUTHORS
M third_party/blink/renderer/core/html/media/media_document.cc
2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/AUTHORS b/AUTHORS
index 6d5492a..3b62601 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -1216,6 +1216,7 @@
 Shouqun Liu <liush...@xiaomi.com>
 Shouqun Liu <shouq...@intel.com>
 Shreeram Kushwaha <shree...@samsung.com>
+Shrey Patel <shrey1...@gmail.com>
 Shreyas Gopal <shre...@samsung.com>
 Shreyas VA <v.a.s...@gmail.com>
 Shubham Agrawal <shu...@amazon.com>
diff --git a/third_party/blink/renderer/core/html/media/media_document.cc b/third_party/blink/renderer/core/html/media/media_document.cc
index dee5f6d..9de3e2a 100644
--- a/third_party/blink/renderer/core/html/media/media_document.cc
+++ b/third_party/blink/renderer/core/html/media/media_document.cc
@@ -152,7 +152,11 @@
       // space or media key (play/pause)
       video->TogglePlayState();
       event.SetDefaultHandled();
+      return;
     }
+    // Route the keyboard events directly to the media element
+    video->DispatchEvent(event);
+    return;
   }
 }

To view, visit change 4881903. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward