|
|
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. |
DescriptionAdds 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. #Messages
Total messages: 72 (31 generated)
lethalantidote@chromium.org changed reviewers: + mlamouri@chromium.org
Description was changed from ========== Adds keyboard funcationality 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 ========== to ========== Adds keyboard funcationality 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 ==========
Description was changed from ========== Adds keyboard funcationality 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 ========== to ========== 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 ==========
A few general questions: - would the event still be propagated to the page? do we would like them to be? - do we have a11y guidance? - do we have UX guidance? Also, we need tests :) https://codereview.chromium.org/2700663002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right): https://codereview.chromium.org/2700663002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/shadow/MediaControls.h:119: friend class HTMLMediaElement; We have an ongoing project to split the close relationship between MediaControls and HTMLMediaElement. I don't think we should add friends here :) MediaControls should only access public methods from HTMLMediaElement. Ideally, only methods available from HTMLMediaElement.idl.
lethalantidote@chromium.org changed reviewers: + dahlke@chromium.org
+dahlke. Do you know anything about the a11y/UX guidance?
On 2017/02/16 21:53:25, CJ wrote: > +dahlke. Do you know anything about the a11y/UX guidance? This is the first I have heard of the feature (sounds like a good one). I don't know what impact this would have on accessibility. You should reach out to lpalmaro@ for that question. As for UX, let me bring this up with our designer and see how best to handle it.
On 2017/02/16 17:13:54, mlamouri wrote: > A few general questions: > - would the event still be propagated to the page? do we would like them to be? > - do we have a11y guidance? > - do we have UX guidance? > > Also, we need tests :) > > https://codereview.chromium.org/2700663002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right): > > https://codereview.chromium.org/2700663002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/html/shadow/MediaControls.h:119: friend class > HTMLMediaElement; > We have an ongoing project to split the close relationship between MediaControls > and HTMLMediaElement. I don't think we should add friends here :) MediaControls > should only access public methods from HTMLMediaElement. Ideally, only methods > available from HTMLMediaElement.idl. The event is not propagated to the page because it causes the page to jump around (space and up/down scroll vertically, left/right horizontally), hence the early exits in each of the cases. I assume this is what your a referring to?
https://codereview.chromium.org/2700663002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right): https://codereview.chromium.org/2700663002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/shadow/MediaControls.h:119: friend class HTMLMediaElement; On 2017/02/16 17:13:54, mlamouri wrote: > We have an ongoing project to split the close relationship between MediaControls > and HTMLMediaElement. I don't think we should add friends here :) MediaControls > should only access public methods from HTMLMediaElement. Ideally, only methods > available from HTMLMediaElement.idl. I figured, I just wanted to start the conversation. It was either this or making the startHideMediaControlsTimer() public (it is the only reason for friends here). Would making startHideMediaControlsTimer() public be better? I did not find a good public method that would reach into this functionality. I am open to other suggestions though.
On 2017/02/17 at 02:45:50, lethalantidote wrote: > https://codereview.chromium.org/2700663002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right): > > https://codereview.chromium.org/2700663002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/html/shadow/MediaControls.h:119: friend class HTMLMediaElement; > On 2017/02/16 17:13:54, mlamouri wrote: > > We have an ongoing project to split the close relationship between MediaControls > > and HTMLMediaElement. I don't think we should add friends here :) MediaControls > > should only access public methods from HTMLMediaElement. Ideally, only methods > > available from HTMLMediaElement.idl. > > I figured, I just wanted to start the conversation. It was either this or making the startHideMediaControlsTimer() public (it is the only reason for friends here). Would making startHideMediaControlsTimer() public be better? I did not find a good public method that would reach into this functionality. I am open to other suggestions though. Could you forword the keyboard event to the MediaControls and have the MediaControls deal with its own logic? I think that would be a more web-y way of doing things.
lethalantidote@chromium.org changed reviewers: + lpalmaro@google.com
+lpalmaro for a11y
On 2017/02/17 10:03:51, mlamouri wrote: > On 2017/02/17 at 02:45:50, lethalantidote wrote: > > > https://codereview.chromium.org/2700663002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right): > > > > > https://codereview.chromium.org/2700663002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/html/shadow/MediaControls.h:119: friend class > HTMLMediaElement; > > On 2017/02/16 17:13:54, mlamouri wrote: > > > We have an ongoing project to split the close relationship between > MediaControls > > > and HTMLMediaElement. I don't think we should add friends here :) > MediaControls > > > should only access public methods from HTMLMediaElement. Ideally, only > methods > > > available from HTMLMediaElement.idl. > > > > I figured, I just wanted to start the conversation. It was either this or > making the startHideMediaControlsTimer() public (it is the only reason for > friends here). Would making startHideMediaControlsTimer() public be better? I > did not find a good public method that would reach into this functionality. I am > open to other suggestions though. > > Could you forword the keyboard event to the MediaControls and have the > MediaControls deal with its own logic? I think that would be a more web-y way of > doing things. Updated it. Please take a look. Tests will be coming in the next patch. Right now things work, but there is a small bug with the blue line on the volume controls. Volume adjusts and the blue circle moves accordingly, but there is a lag on update for the blue line. Any clue what the line is called so I can figure out who owns it?
https://codereview.chromium.org/2700663002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2700663002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3724: m_mediaControls->defaultEventHandler(event); We are working on decoupling HTMLMediaElement and MediaControls. Would it make sense to have MediaControlsMediaEventListener listen to some keyboard events on HTMLMediaElement and transfer back the information to MediaControls instead? https://codereview.chromium.org/2700663002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2700663002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:772: onTimeUpdate(); Shouldn't this be called automatically? https://codereview.chromium.org/2700663002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:776: volumeSliderElement()->defaultEventHandler(event); For the issue you mention, is onVolueChange called? What happens if you manually call it? https://codereview.chromium.org/2700663002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:835: startHideMediaControlsTimer(); Why are you hiding this?
Hey, before I keep reworking this, can we sit down and talk about the decoupling effort, and the solution you proposed? I think I may be missing something. https://codereview.chromium.org/2700663002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2700663002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3724: m_mediaControls->defaultEventHandler(event); On 2017/03/16 12:17:22, mlamouri wrote: > We are working on decoupling HTMLMediaElement and MediaControls. Would it make > sense to have MediaControlsMediaEventListener listen to some keyboard events on > HTMLMediaElement and transfer back the information to MediaControls instead? I don't think I know how these classes relate to each other fully, because it would seem that doesn't make much sense. The MediaControlsMediaEventListener seems to listen for events after the MediaEvent has happened. I'd be rather happy if we had a moment to sit down and talk about this next week. https://codereview.chromium.org/2700663002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2700663002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:772: onTimeUpdate(); On 2017/03/16 12:17:22, mlamouri wrote: > Shouldn't this be called automatically? Didn't seem to work without it. Will double check. https://codereview.chromium.org/2700663002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:776: volumeSliderElement()->defaultEventHandler(event); On 2017/03/16 12:17:22, mlamouri wrote: > For the issue you mention, is onVolueChange called? What happens if you manually > call it? I tried that. Instead of refreshing the blue line after reshowing, it starts the refresh after it fades away. https://codereview.chromium.org/2700663002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:835: startHideMediaControlsTimer(); On 2017/03/16 12:17:22, mlamouri wrote: > Why are you hiding this? Not sure why that is there. Removing.
On 2017/03/16 20:08:29, CJ wrote: > Hey, before I keep reworking this, can we sit down and talk about the decoupling > effort, and the solution you proposed? I think I may be missing something. > > https://codereview.chromium.org/2700663002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/2700663002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3724: > m_mediaControls->defaultEventHandler(event); > On 2017/03/16 12:17:22, mlamouri wrote: > > We are working on decoupling HTMLMediaElement and MediaControls. Would it make > > sense to have MediaControlsMediaEventListener listen to some keyboard events > on > > HTMLMediaElement and transfer back the information to MediaControls instead? > > I don't think I know how these classes relate to each other fully, because it > would seem that doesn't make much sense. The MediaControlsMediaEventListener > seems to listen for events after the MediaEvent has happened. I'd be rather > happy if we had a moment to sit down and talk about this next week. > > https://codereview.chromium.org/2700663002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): > > https://codereview.chromium.org/2700663002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:772: > onTimeUpdate(); > On 2017/03/16 12:17:22, mlamouri wrote: > > Shouldn't this be called automatically? > > Didn't seem to work without it. Will double check. > > https://codereview.chromium.org/2700663002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:776: > volumeSliderElement()->defaultEventHandler(event); > On 2017/03/16 12:17:22, mlamouri wrote: > > For the issue you mention, is onVolueChange called? What happens if you > manually > > call it? > > I tried that. Instead of refreshing the blue line after reshowing, it starts the > refresh after it fades away. > > https://codereview.chromium.org/2700663002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:835: > startHideMediaControlsTimer(); > On 2017/03/16 12:17:22, mlamouri wrote: > > Why are you hiding this? > > Not sure why that is there. Removing. Also to clarify, I totally agree with keeping things private. I am just not totally convinced that MediaControlsMediaEventListner is the best place for listening to InputEvents, and would love to discuss this further.
On 2017/03/17 at 22:48:48, lethalantidote wrote: > Also to clarify, I totally agree with keeping things private. I am just not totally convinced that MediaControlsMediaEventListner is the best place for listening to InputEvents, and would love to discuss this further. Let's discuss this f2f this week :)
Updated as discussed offline. PTAL
In addition of the comments below, would be great to add tests. Handling the repaint bug would be good too :) Regarding the repaint bug, the issue is that when you change the volume using the keyboard, the volume change is sent to the media element who then notifies the media controls using the volumechange event. However, the media controls usually makes sure that the slider has a repaint but in this case, the volume would look like it did not change (see setVolume()). You will need to call setShouldDoFullPaintInvalidation() on the slider's layoutObject. Maybe when the input event is handled or maybe after the media controls sends the keyboard event to the slider. Not sure which one would be the best. Doing this on the input event handling would be better but it might be noisy too. https://codereview.chromium.org/2700663002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2700663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:45: #include "core/events/KeyboardEvent.h" I guess you no longer need this include? https://codereview.chromium.org/2700663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:89: #include "public/platform/UserMetricsAction.h" ... and this one? https://codereview.chromium.org/2700663002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2700663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:756: if (event->type() == EventTypeNames::input) { stupid question: why is that? https://codereview.chromium.org/2700663002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right): https://codereview.chromium.org/2700663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.h:122: void onMediaKeyboardEvent(Event* event) { defaultEventHandler(event); } Can you move this in the block that starts with the "// Methods called by MediaControlsMediaEventListener" comment? Also maybe call it "onKeyPress" to make it generic. MediaControls could re-use this for other stuff. Finally, what about having our own handler for this with the `key` sent back. https://codereview.chromium.org/2700663002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp (right): https://codereview.chromium.org/2700663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp:34: mediaElement().addEventListener(EventTypeNames::keyup, this, false); I think keypress should be enough here. https://codereview.chromium.org/2700663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp:129: EventTypeNames::keyup) { ditto
Tests coming in next patch. https://codereview.chromium.org/2700663002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2700663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:45: #include "core/events/KeyboardEvent.h" On 2017/03/27 13:02:12, mlamouri wrote: > I guess you no longer need this include? Done. https://codereview.chromium.org/2700663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:89: #include "public/platform/UserMetricsAction.h" On 2017/03/27 13:02:11, mlamouri wrote: > ... and this one? Done. https://codereview.chromium.org/2700663002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2700663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:756: if (event->type() == EventTypeNames::input) { On 2017/03/27 13:02:12, mlamouri wrote: > stupid question: why is that? So we don't hide media controls while manipulating the volume/timeline. It partially just does what steimel@ is doing in the mediacontrols bug he is working on. https://codereview.chromium.org/2700663002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right): https://codereview.chromium.org/2700663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControls.h:122: void onMediaKeyboardEvent(Event* event) { defaultEventHandler(event); } On 2017/03/27 13:02:12, mlamouri wrote: > Can you move this in the block that starts with the "// Methods called by > MediaControlsMediaEventListener" comment? > > Also maybe call it "onKeyPress" to make it generic. MediaControls could re-use > this for other stuff. > > Finally, what about having our own handler for this with the `key` sent back. Done. https://codereview.chromium.org/2700663002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp (right): https://codereview.chromium.org/2700663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp:34: mediaElement().addEventListener(EventTypeNames::keyup, this, false); On 2017/03/27 13:02:12, mlamouri wrote: > I think keypress should be enough here. Keypress only works for the Enter button. All other controls are not registered. Need to check if keyup has any baring in the blue line error once the fix for that makes its way into the repo. https://codereview.chromium.org/2700663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp:129: EventTypeNames::keyup) { On 2017/03/27 13:02:12, mlamouri wrote: > ditto Acknowledged.
I think you will have to do some rebase because the Blink codebase has been changed to match the Chromium coding style. Just had to rebase 4 CLs this morning :( Otherwise, for the tests, you can either add unit tests in MediaControlsImplTest.cpp or add LayoutTests in media/controls/. LayoutTests are more like integration tests but I think they shouldn't be too hard to do and it might be better than unit tests in this case. You will need to use the eventSender object. Feel free to ping me if you have any question. https://codereview.chromium.org/2700663002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp (right): https://codereview.chromium.org/2700663002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:772: } I think steimel@'s CL doing this has landed. https://codereview.chromium.org/2700663002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:782: timelineElement()->onMediaKeyboardEvent(event); nit: use the member directly https://codereview.chromium.org/2700663002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:786: volumeSliderElement()->onMediaKeyboardEvent(event); ditto
https://codereview.chromium.org/2700663002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp (right): https://codereview.chromium.org/2700663002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:772: } On 2017/04/10 13:14:02, mlamouri wrote: > I think steimel@'s CL doing this has landed. Done. https://codereview.chromium.org/2700663002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:772: } On 2017/04/10 13:14:02, mlamouri wrote: > I think steimel@'s CL doing this has landed. Done. https://codereview.chromium.org/2700663002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:782: timelineElement()->onMediaKeyboardEvent(event); On 2017/04/10 13:14:02, mlamouri wrote: > nit: use the member directly Done. https://codereview.chromium.org/2700663002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:786: volumeSliderElement()->onMediaKeyboardEvent(event); On 2017/04/10 13:14:02, mlamouri wrote: > ditto Done.
lgtm with the comments applied. The tests look great! :) https://codereview.chromium.org/2700663002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/controls-video-keynav.html (right): https://codereview.chromium.org/2700663002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/controls-video-keynav.html:2: <title>Test media controls video keyboard navigation</title> Can you move this file to media/controls/ (so we have all the tests for the media controls in one place) https://codereview.chromium.org/2700663002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/controls-video-keynav.html:15: video.src = findMediaFile("video", "content/test"); Note: when you move to media/controls/, you will have to change this to "../content/test". I and others spent a long time debugging issues like these :) https://codereview.chromium.org/2700663002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/controls-video-keynav.html:20: style: drop the empty line https://codereview.chromium.org/2700663002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/media/MediaControls.h (right): https://codereview.chromium.org/2700663002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/media/MediaControls.h:89: virtual void OnMediaKeyboardEvent(Event*) = 0; I don't think you need this.
Will commit once I get ok from UX. https://codereview.chromium.org/2700663002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/controls-video-keynav.html (right): https://codereview.chromium.org/2700663002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/controls-video-keynav.html:2: <title>Test media controls video keyboard navigation</title> On 2017/04/13 14:28:21, mlamouri wrote: > Can you move this file to media/controls/ (so we have all the tests for the > media controls in one place) Done. https://codereview.chromium.org/2700663002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/controls-video-keynav.html:15: video.src = findMediaFile("video", "content/test"); On 2017/04/13 14:28:21, mlamouri wrote: > Note: when you move to media/controls/, you will have to change this to > "../content/test". I and others spent a long time debugging issues like these :) Done. https://codereview.chromium.org/2700663002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/controls-video-keynav.html:20: On 2017/04/13 14:28:21, mlamouri wrote: > style: drop the empty line Done. https://codereview.chromium.org/2700663002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/media/MediaControls.h (right): https://codereview.chromium.org/2700663002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/media/MediaControls.h:89: virtual void OnMediaKeyboardEvent(Event*) = 0; On 2017/04/13 14:28:21, mlamouri wrote: > I don't think you need this. Done.
The CQ bit was checked by lethalantidote@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2700663002/#ps140001 (title: "Addresses mlamouri's #24 comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_presub...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lethalantidote@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2700663002/#ps160001 (title: "Merge branch 'real_master' into keypress")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by lethalantidote@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
https://codereview.chromium.org/2700663002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp (right): https://codereview.chromium.org/2700663002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:771: if (event->IsKeyboardEvent()) { I realised the other day (not sure why I was thinking about this) that this might conflict with websites keyboard event handling. Should we add a check like `MediaElement().ShouldShowControls()`? And add a test checking that these shortcuts do not apply if `controls` isn't set?
lethalantidote@google.com changed reviewers: + lethalantidote@google.com
https://codereview.chromium.org/2700663002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp (right): https://codereview.chromium.org/2700663002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:771: if (event->IsKeyboardEvent()) { On 2017/05/09 17:32:10, mlamouri wrote: > I realised the other day (not sure why I was thinking about this) that this > might conflict with websites keyboard event handling. Should we add a check like > `MediaElement().ShouldShowControls()`? And add a test checking that these > shortcuts do not apply if `controls` isn't set? Done.
still lgtm :)
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.
On 2017/05/18 20:32:08, lethalantidote wrote: > 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. Ok added some stuff to deal with Spatial Nav. Please let me know if there is an issue with how I did this, as Build dependencies made the way I chose the easiest.
On 2017/05/18 20:32:08, lethalantidote wrote: > 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. Ok added some stuff to deal with Spatial Nav. Please let me know if there is an issue with how I did this, as Build dependencies made the way I chose the easiest.
The CQ bit was checked by lethalantidote@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by lethalantidote@google.com
The CQ bit was checked by lethalantidote@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2700663002/#ps220001 (title: "Add spatial nav check.")
The CQ bit was unchecked by lethalantidote@google.com
The CQ bit was checked by lethalantidote@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_presub...)
https://codereview.chromium.org/2700663002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/2700663002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLElement.cpp:933: return IsSpatialNavigationEnabled(GetDocument().GetFrame()); Maybe just make this call in MediaControlsImpl instead of adding it to HTMLElement? https://codereview.chromium.org/2700663002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp (right): https://codereview.chromium.org/2700663002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:857: if (event->IsKeyboardEvent() && !IsSpatialNavigationActive()) { Would it make sense to allow "enter" and "space" to be used with spatial navigation enabled?
https://codereview.chromium.org/2700663002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/2700663002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLElement.cpp:933: return IsSpatialNavigationEnabled(GetDocument().GetFrame()); On 2017/05/24 08:49:15, mlamouri (OOO up to 23-05) wrote: > Maybe just make this call in MediaControlsImpl instead of adding it to > HTMLElement? Tried doing that. It's not directly accessible due to BUILD visibility. Wasnt sure if I should be messing with that (also tried and didn't figure out how to get to visible. Willing to give it another go if this is preferable.) https://codereview.chromium.org/2700663002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp (right): https://codereview.chromium.org/2700663002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:857: if (event->IsKeyboardEvent() && !IsSpatialNavigationActive()) { On 2017/05/24 08:49:15, mlamouri (OOO up to 23-05) wrote: > Would it make sense to allow "enter" and "space" to be used with spatial > navigation enabled? Maybe, but wouldn't that cause inconsistency? It's only one more button press to get the same effect, and it would be the same expectation for all controls (User has to navigate to the control versus operate on the video itself). I think that this might be something we would want Rachel to have input on.
https://codereview.chromium.org/2700663002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/2700663002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLElement.cpp:933: return IsSpatialNavigationEnabled(GetDocument().GetFrame()); On 2017/05/24 at 18:46:08, lethalantidote wrote: > On 2017/05/24 08:49:15, mlamouri (OOO up to 23-05) wrote: > > Maybe just make this call in MediaControlsImpl instead of adding it to > > HTMLElement? > > Tried doing that. It's not directly accessible due to BUILD visibility. Wasnt sure if I should be messing with that (also tried and didn't figure out how to get to visible. Willing to give it another go if this is preferable.) I think you would simply need to add CORE_EXPORT to `bool IsSpatialNavigationEnabled(const LocalFrame*);` in SpatialNavigation.h and include "core/CoreExport.h". This should work: `CORE_EXPORT bool IsSpatialNavigationEnabled(const LocalFrame*);` You can find an example here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Range... https://codereview.chromium.org/2700663002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp (right): https://codereview.chromium.org/2700663002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:857: if (event->IsKeyboardEvent() && !IsSpatialNavigationActive()) { On 2017/05/24 at 18:46:08, lethalantidote wrote: > On 2017/05/24 08:49:15, mlamouri (OOO up to 23-05) wrote: > > Would it make sense to allow "enter" and "space" to be used with spatial > > navigation enabled? > > Maybe, but wouldn't that cause inconsistency? It's only one more button press to get the same effect, and it would be the same expectation for all controls (User has to navigate to the control versus operate on the video itself). I think that this might be something we would want Rachel to have input on. I have no strong opinion on this. Whatever sounds right to you. I see two valid options: 1. we drop all the keyboards when spatial navigation is enabled 2. we drop all the arrow keys when spatial navigation is enabled
lethalantidote@google.com changed reviewers: + dtapuska@chromium.org
+dtapuska To review changes to SpatialNavigation. Context: This CL allows the user to control the video (volume/timeline scrubbing, etc using arrow keys and a few others) using the keyboard while the video is in focus. This clashed with SpatialNavigation, which expects to be able to use arrow keys to move from the video, whereas in this CL the video is eating that input. Thus we had to do a check, and export the IsSpatialNavigationEnabled function. https://codereview.chromium.org/2700663002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/2700663002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLElement.cpp:933: return IsSpatialNavigationEnabled(GetDocument().GetFrame()); On 2017/05/30 10:22:41, mlamouri wrote: > On 2017/05/24 at 18:46:08, lethalantidote wrote: > > On 2017/05/24 08:49:15, mlamouri (OOO up to 23-05) wrote: > > > Maybe just make this call in MediaControlsImpl instead of adding it to > > > HTMLElement? > > > > Tried doing that. It's not directly accessible due to BUILD visibility. Wasnt > sure if I should be messing with that (also tried and didn't figure out how to > get to visible. Willing to give it another go if this is preferable.) > > I think you would simply need to add CORE_EXPORT to `bool > IsSpatialNavigationEnabled(const LocalFrame*);` in SpatialNavigation.h and > include "core/CoreExport.h". > > This should work: > `CORE_EXPORT bool IsSpatialNavigationEnabled(const LocalFrame*);` > > You can find an example here: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Range... Done. The example was very helpful to me.
The CQ bit was checked by lethalantidote@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/30 19:48:12, lethalantidote wrote: > +dtapuska To review changes to SpatialNavigation. > > Context: This CL allows the user to control the video (volume/timeline > scrubbing, etc using arrow keys and a few others) using the keyboard while the > video is in focus. This clashed with SpatialNavigation, which expects to be able > to use arrow keys to move from the video, whereas in this CL the video is eating > that input. Thus we had to do a check, and export the IsSpatialNavigationEnabled > function. > > https://codereview.chromium.org/2700663002/diff/220001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/html/HTMLElement.cpp (right): > > https://codereview.chromium.org/2700663002/diff/220001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLElement.cpp:933: return > IsSpatialNavigationEnabled(GetDocument().GetFrame()); > On 2017/05/30 10:22:41, mlamouri wrote: > > On 2017/05/24 at 18:46:08, lethalantidote wrote: > > > On 2017/05/24 08:49:15, mlamouri (OOO up to 23-05) wrote: > > > > Maybe just make this call in MediaControlsImpl instead of adding it to > > > > HTMLElement? > > > > > > Tried doing that. It's not directly accessible due to BUILD visibility. > Wasnt > > sure if I should be messing with that (also tried and didn't figure out how to > > get to visible. Willing to give it another go if this is preferable.) > > > > I think you would simply need to add CORE_EXPORT to `bool > > IsSpatialNavigationEnabled(const LocalFrame*);` in SpatialNavigation.h and > > include "core/CoreExport.h". > > > > This should work: > > `CORE_EXPORT bool IsSpatialNavigationEnabled(const LocalFrame*);` > > > > You can find an example here: > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Range... > > Done. The example was very helpful to me. lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by lethalantidote@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2700663002/#ps260001 (title: "Now calls IsSpatialNavigationEnabled() directly.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 260001, "attempt_start_ts": 1496185071640400, "parent_rev": "7d18c710d2a6aa00cf8692e83598c7b9ee7733b0", "commit_rev": "f8ced1a33c4c82e224427dc7d7433582cc183600"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/f8ced1a33c4c82e224427dc7d743... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/chromium/src/+/f8ced1a33c4c82e224427dc7d743... |