Chromium Code Reviews
Help | Chromium Project | Sign in
(22)

Issue 2700663002: Adds keyboard functionality for videos.

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 1 week ago by CJ
Modified:
1 week, 1 day ago
Reviewers:
dahlke, lpalmaro, mlamouri - OOO until May 8th
CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, mlamouri+watch-blink_chromium.org, nessy, Srirama, Wez
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Adds keyboard functionality for videos. With this CL, in focus videos can be controlled using the up/down keys (volume), left/right keys (scrubbing), and space/enter (play/pause). This improves upon the existing functionality, where one must tab over to the individual control to manipulate it. Instead, the video now only needs to be in focus to be able to control it. BUG=135661

Patch Set 1 #

Total comments: 2

Patch Set 2 : Addresses mlamouri's #11 comment. #

Total comments: 8

Patch Set 3 : Updates according the mlamouri's suggestion. #

Patch Set 4 : Clean up. #

Total comments: 12

Patch Set 5 : Addresses mlamouri's #20 comments. #

Total comments: 7

Patch Set 6 : Ignore. #

Patch Set 7 : Addresses mlamouri's #22, adds tests. #

Total comments: 8

Patch Set 8 : Addresses mlamouri's #24 comments. #

Patch Set 9 : Merge branch 'real_master' into keypress #

Unified diffs Side-by-side diffs Delta from patch set Stats (+104 lines, -1 line) Patch
A third_party/WebKit/LayoutTests/media/controls/controls-video-keynav.html View 1 2 3 4 5 6 7 1 chunk +67 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/html/shadow/MediaControlElements.h View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.h View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp View 1 2 3 4 5 6 7 8 2 chunks +18 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/media_controls/MediaControlsMediaEventListener.cpp View 1 2 3 4 5 6 2 chunks +12 lines, -1 line 0 comments Download
M third_party/WebKit/Source/modules/media_controls/elements/MediaControlPlayButtonElement.h View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/media_controls/elements/MediaControlTimelineElement.h View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 39 (13 generated)
CJ
2 months, 1 week ago (2017-02-16 00:52:18 UTC) #2
mlamouri - OOO until May 8th
A few general questions: - would the event still be propagated to the page? do ...
2 months, 1 week ago (2017-02-16 17:13:54 UTC) #5
CJ
+dahlke. Do you know anything about the a11y/UX guidance?
2 months, 1 week ago (2017-02-16 21:53:25 UTC) #7
dahlke
On 2017/02/16 21:53:25, CJ wrote: > +dahlke. Do you know anything about the a11y/UX guidance? ...
2 months, 1 week ago (2017-02-16 22:33:15 UTC) #8
CJ
On 2017/02/16 17:13:54, mlamouri wrote: > A few general questions: > - would the event ...
2 months, 1 week ago (2017-02-17 02:40:31 UTC) #9
CJ
https://codereview.chromium.org/2700663002/diff/1/third_party/WebKit/Source/core/html/shadow/MediaControls.h File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right): https://codereview.chromium.org/2700663002/diff/1/third_party/WebKit/Source/core/html/shadow/MediaControls.h#newcode119 third_party/WebKit/Source/core/html/shadow/MediaControls.h:119: friend class HTMLMediaElement; On 2017/02/16 17:13:54, mlamouri wrote: > ...
2 months, 1 week ago (2017-02-17 02:45:50 UTC) #10
mlamouri - OOO until May 8th
On 2017/02/17 at 02:45:50, lethalantidote wrote: > https://codereview.chromium.org/2700663002/diff/1/third_party/WebKit/Source/core/html/shadow/MediaControls.h > File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right): > > https://codereview.chromium.org/2700663002/diff/1/third_party/WebKit/Source/core/html/shadow/MediaControls.h#newcode119 ...
2 months, 1 week ago (2017-02-17 10:03:51 UTC) #11
CJ
+lpalmaro for a11y
2 months ago (2017-02-21 22:49:18 UTC) #13
CJ
On 2017/02/17 10:03:51, mlamouri wrote: > On 2017/02/17 at 02:45:50, lethalantidote wrote: > > > ...
1 month, 2 weeks ago (2017-03-16 02:43:30 UTC) #14
mlamouri - OOO until May 8th
https://codereview.chromium.org/2700663002/diff/20001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2700663002/diff/20001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode3724 third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3724: m_mediaControls->defaultEventHandler(event); We are working on decoupling HTMLMediaElement and MediaControls. ...
1 month, 2 weeks ago (2017-03-16 12:17:22 UTC) #15
CJ
Hey, before I keep reworking this, can we sit down and talk about the decoupling ...
1 month, 1 week ago (2017-03-16 20:08:29 UTC) #16
CJ
On 2017/03/16 20:08:29, CJ wrote: > Hey, before I keep reworking this, can we sit ...
1 month, 1 week ago (2017-03-17 22:48:48 UTC) #17
mlamouri - OOO until May 8th
On 2017/03/17 at 22:48:48, lethalantidote wrote: > Also to clarify, I totally agree with keeping ...
1 month, 1 week ago (2017-03-20 15:08:25 UTC) #18
CJ
Updated as discussed offline. PTAL
1 month, 1 week ago (2017-03-22 16:56:49 UTC) #19
mlamouri - OOO until May 8th
In addition of the comments below, would be great to add tests. Handling the repaint ...
1 month ago (2017-03-27 13:02:12 UTC) #20
CJ
Tests coming in next patch. https://codereview.chromium.org/2700663002/diff/60001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2700663002/diff/60001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode45 third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:45: #include "core/events/KeyboardEvent.h" On 2017/03/27 ...
3 weeks ago (2017-04-08 00:31:27 UTC) #21
mlamouri - OOO until May 8th
I think you will have to do some rebase because the Blink codebase has been ...
2 weeks, 5 days ago (2017-04-10 13:14:02 UTC) #22
CJ
https://codereview.chromium.org/2700663002/diff/80001/third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp File third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp (right): https://codereview.chromium.org/2700663002/diff/80001/third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp#newcode772 third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:772: } On 2017/04/10 13:14:02, mlamouri wrote: > I think ...
2 weeks, 2 days ago (2017-04-12 23:21:06 UTC) #23
mlamouri - OOO until May 8th
lgtm with the comments applied. The tests look great! :) https://codereview.chromium.org/2700663002/diff/120001/third_party/WebKit/LayoutTests/media/controls-video-keynav.html File third_party/WebKit/LayoutTests/media/controls-video-keynav.html (right): https://codereview.chromium.org/2700663002/diff/120001/third_party/WebKit/LayoutTests/media/controls-video-keynav.html#newcode2 ...
2 weeks, 2 days ago (2017-04-13 14:28:22 UTC) #24
CJ
Will commit once I get ok from UX. https://codereview.chromium.org/2700663002/diff/120001/third_party/WebKit/LayoutTests/media/controls-video-keynav.html File third_party/WebKit/LayoutTests/media/controls-video-keynav.html (right): https://codereview.chromium.org/2700663002/diff/120001/third_party/WebKit/LayoutTests/media/controls-video-keynav.html#newcode2 third_party/WebKit/LayoutTests/media/controls-video-keynav.html:2: <title>Test ...
2 weeks ago (2017-04-14 21:56:45 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2700663002/140001
1 week, 1 day ago (2017-04-20 21:47:54 UTC) #28
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/416929) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, ...
1 week, 1 day ago (2017-04-20 21:51:51 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2700663002/160001
1 week, 1 day ago (2017-04-20 22:13:20 UTC) #33
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_compile_dbg_ng/builds/394392) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, ...
1 week, 1 day ago (2017-04-20 22:16:02 UTC) #35
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2700663002/160001
1 week, 1 day ago (2017-04-21 04:29:16 UTC) #37
commit-bot: I haz the power
1 week, 1 day ago (2017-04-21 06:12:29 UTC) #39
Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46