Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(13)

Issue 2700663002: Adds keyboard functionality for videos. (Closed)

Created:
3 years, 10 months ago by CJ
Modified:
3 years, 6 months 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 Review-Url: https://codereview.chromium.org/2700663002 Cr-Commit-Position: refs/heads/master@{#475706} Committed: https://chromium.googlesource.com/chromium/src/+/f8ced1a33c4c82e224427dc7d7433582cc183600

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. #

Patch Set 12 : Add spatial nav check. #

Total comments: 7

Patch Set 13 : Format. #

Patch Set 14 : Now calls IsSpatialNavigationEnabled() directly. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+155 lines, -2 lines) 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
A third_party/WebKit/LayoutTests/media/controls/controls-video-keynav-no-controls.html View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +47 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/page/SpatialNavigation.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 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 9 10 11 12 13 3 chunks +20 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/media_controls/MediaControlsMediaEventListener.cpp View 1 2 3 4 5 6 7 8 9 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
M third_party/WebKit/Source/modules/media_controls/elements/MediaControlVolumeSliderElement.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 72 (31 generated)
CJ
3 years, 10 months ago (2017-02-16 00:52:18 UTC) #2
mlamouri (slow - plz ping)
A few general questions: - would the event still be propagated to the page? do ...
3 years, 10 months ago (2017-02-16 17:13:54 UTC) #5
CJ
+dahlke. Do you know anything about the a11y/UX guidance?
3 years, 10 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 years, 10 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 years, 10 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 years, 10 months ago (2017-02-17 02:45:50 UTC) #10
mlamouri (slow - plz ping)
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 years, 10 months ago (2017-02-17 10:03:51 UTC) #11
CJ
+lpalmaro for a11y
3 years, 10 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: > > > ...
3 years, 9 months ago (2017-03-16 02:43:30 UTC) #14
mlamouri (slow - plz ping)
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. ...
3 years, 9 months 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 ...
3 years, 9 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 ...
3 years, 9 months ago (2017-03-17 22:48:48 UTC) #17
mlamouri (slow - plz ping)
On 2017/03/17 at 22:48:48, lethalantidote wrote: > Also to clarify, I totally agree with keeping ...
3 years, 9 months ago (2017-03-20 15:08:25 UTC) #18
CJ
Updated as discussed offline. PTAL
3 years, 9 months ago (2017-03-22 16:56:49 UTC) #19
mlamouri (slow - plz ping)
In addition of the comments below, would be great to add tests. Handling the repaint ...
3 years, 9 months 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 years, 8 months ago (2017-04-08 00:31:27 UTC) #21
mlamouri (slow - plz ping)
I think you will have to do some rebase because the Blink codebase has been ...
3 years, 8 months 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 ...
3 years, 8 months ago (2017-04-12 23:21:06 UTC) #23
mlamouri (slow - plz ping)
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 ...
3 years, 8 months 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 ...
3 years, 8 months 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
3 years, 8 months 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, ...
3 years, 8 months 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
3 years, 8 months 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, ...
3 years, 8 months 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
3 years, 8 months 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)
3 years, 8 months ago (2017-04-21 06:12:29 UTC) #39
mlamouri (slow - plz ping)
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 ...
3 years, 7 months 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: > ...
3 years, 7 months ago (2017-05-18 00:03:34 UTC) #42
mlamouri (slow - plz ping)
still lgtm :)
3 years, 7 months ago (2017-05-18 07:52:02 UTC) #43
lethalantidote
On 2017/05/18 07:52:02, mlamouri (OOO up to 23-05) wrote: > still lgtm :) So question. ...
3 years, 7 months ago (2017-05-18 20:32:08 UTC) #44
lethalantidote
On 2017/05/18 20:32:08, lethalantidote wrote: > On 2017/05/18 07:52:02, mlamouri (OOO up to 23-05) wrote: ...
3 years, 7 months ago (2017-05-24 03:22:48 UTC) #45
lethalantidote
On 2017/05/18 20:32:08, lethalantidote wrote: > On 2017/05/18 07:52:02, mlamouri (OOO up to 23-05) wrote: ...
3 years, 7 months ago (2017-05-24 03:22:52 UTC) #46
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/220001
3 years, 7 months ago (2017-05-24 03:36:51 UTC) #54
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/445794)
3 years, 7 months ago (2017-05-24 03:46:26 UTC) #56
mlamouri (slow - plz ping)
https://codereview.chromium.org/2700663002/diff/220001/third_party/WebKit/Source/core/html/HTMLElement.cpp File third_party/WebKit/Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/2700663002/diff/220001/third_party/WebKit/Source/core/html/HTMLElement.cpp#newcode933 third_party/WebKit/Source/core/html/HTMLElement.cpp:933: return IsSpatialNavigationEnabled(GetDocument().GetFrame()); Maybe just make this call in MediaControlsImpl ...
3 years, 7 months ago (2017-05-24 08:49:15 UTC) #57
lethalantidote
https://codereview.chromium.org/2700663002/diff/220001/third_party/WebKit/Source/core/html/HTMLElement.cpp File third_party/WebKit/Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/2700663002/diff/220001/third_party/WebKit/Source/core/html/HTMLElement.cpp#newcode933 third_party/WebKit/Source/core/html/HTMLElement.cpp:933: return IsSpatialNavigationEnabled(GetDocument().GetFrame()); On 2017/05/24 08:49:15, mlamouri (OOO up to ...
3 years, 7 months ago (2017-05-24 18:46:09 UTC) #58
mlamouri (slow - plz ping)
https://codereview.chromium.org/2700663002/diff/220001/third_party/WebKit/Source/core/html/HTMLElement.cpp File third_party/WebKit/Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/2700663002/diff/220001/third_party/WebKit/Source/core/html/HTMLElement.cpp#newcode933 third_party/WebKit/Source/core/html/HTMLElement.cpp:933: return IsSpatialNavigationEnabled(GetDocument().GetFrame()); On 2017/05/24 at 18:46:08, lethalantidote wrote: > ...
3 years, 6 months ago (2017-05-30 10:22:41 UTC) #59
lethalantidote
+dtapuska To review changes to SpatialNavigation. Context: This CL allows the user to control the ...
3 years, 6 months ago (2017-05-30 19:48:12 UTC) #61
dtapuska
On 2017/05/30 19:48:12, lethalantidote wrote: > +dtapuska To review changes to SpatialNavigation. > > Context: ...
3 years, 6 months ago (2017-05-30 20:24:08 UTC) #64
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/260001
3 years, 6 months ago (2017-05-30 22:58:35 UTC) #69
commit-bot: I haz the power
3 years, 6 months ago (2017-05-31 00:39:03 UTC) #72
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/f8ced1a33c4c82e224427dc7d743...

Powered by Google App Engine
This is Rietveld 408576698