Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2700663002: Adds keyboard functionality for videos.

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months ago by CJ
Modified:
3 days, 22 hours ago
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 #

Total comments: 2

Patch Set 10 : Addresses mlamouri's #40 comment. #

Patch Set 11 : Forgot the test. #

Messages

Total messages: 44 (14 generated)
CJ
3 months ago (2017-02-16 00:52:18 UTC) #2
mlamouri (OOO up to 23-05)
A few general questions: - would the event still be propagated to the page? do ...
3 months ago (2017-02-16 17:13:54 UTC) #5
CJ
+dahlke. Do you know anything about the a11y/UX guidance?
3 months 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? ...
3 months 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 ...
3 months 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: > ...
3 months ago (2017-02-17 02:45:50 UTC) #10
mlamouri (OOO up to 23-05)
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 ...
3 months ago (2017-02-17 10:03:51 UTC) #11
CJ
+lpalmaro for a11y
2 months, 4 weeks 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: > > > ...
2 months, 1 week ago (2017-03-16 02:43:30 UTC) #14
mlamouri (OOO up to 23-05)
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. ...
2 months, 1 week 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 ...
2 months 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 ...
2 months ago (2017-03-17 22:48:48 UTC) #17
mlamouri (OOO up to 23-05)
On 2017/03/17 at 22:48:48, lethalantidote wrote: > Also to clarify, I totally agree with keeping ...
2 months ago (2017-03-20 15:08:25 UTC) #18
CJ
Updated as discussed offline. PTAL
2 months ago (2017-03-22 16:56:49 UTC) #19
mlamouri (OOO up to 23-05)
In addition of the comments below, would be great to add tests. Handling the repaint ...
1 month, 3 weeks 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 ...
1 month, 2 weeks ago (2017-04-08 00:31:27 UTC) #21
mlamouri (OOO up to 23-05)
I think you will have to do some rebase because the Blink codebase has been ...
1 month, 1 week 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 ...
1 month, 1 week ago (2017-04-12 23:21:06 UTC) #23
mlamouri (OOO up to 23-05)
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 ...
1 month, 1 week 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 ...
1 month, 1 week 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 month 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 month 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 month 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 month 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 month ago (2017-04-21 04:29:16 UTC) #37
commit-bot: I haz the power
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_rel_ng/builds/436168)
1 month ago (2017-04-21 06:12:29 UTC) #39
mlamouri (OOO up to 23-05)
https://codereview.chromium.org/2700663002/diff/160001/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/160001/third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp#newcode771 third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:771: if (event->IsKeyboardEvent()) { I realised the other day (not ...
1 week, 6 days ago (2017-05-09 17:32:11 UTC) #40
lethalantidote
https://codereview.chromium.org/2700663002/diff/160001/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/160001/third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp#newcode771 third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:771: if (event->IsKeyboardEvent()) { On 2017/05/09 17:32:10, mlamouri wrote: > ...
4 days, 19 hours ago (2017-05-18 00:03:34 UTC) #42
mlamouri (OOO up to 23-05)
still lgtm :)
4 days, 11 hours ago (2017-05-18 07:52:02 UTC) #43
lethalantidote
3 days, 22 hours ago (2017-05-18 20:32:08 UTC) #44
On 2017/05/18 07:52:02, mlamouri (OOO up to 23-05) wrote:
> still lgtm :)

So question. Do you know anything about
third_party/WebKit/LayoutTests/fast/spatial-navigation/snav-media-elements.html
? I am not even sure how it is supposed to pass in a clean build. I sort of
understand how it would fail with the added changes, but I think I need more
information on how it would even succeed in the first place to work.
Sign in to reply to this message.

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