1119049 - Keyboard shortcuts should work in MediaDocuments without explicitly focusing the media element

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Jesse Ruderman]

1. Load a video file as standalone
2. Press [space] to pause or [left] to seek backwards

Result: nothing happens

Workaround: click the <video> element to focus it, and don't subsequently click outside it, which would un-focus it.

Comment 1

[Michael Lowe]

I wonder if this is the same bug that affects the HTML 5 player? More info here. 
https://www.reddit.com/r/firefox/comments/3djzwp/firefox_and_netflix_cant_use_hotkeys_in_fullscreen/

I apologize if I'm completely off the mark here, been looking through bug reports for this issue.

Comment 2

[arni2033]

This will hopefully be implemented by the end of the decade, unlike malicious behavior in bug 1204836

Comment 3

[Steffen Wilberg]

I can write the javascript to make keypresses focus the <video> element, if somebody tells me (precisely) how to insert a <script> element in VideoDocument::SetScriptGlobalObject (which currently only adds two <link> elements to TopLevelVideoDocument.css):
https://dxr.mozilla.org/mozilla-central/source/dom/html/VideoDocument.cpp?offset=4400#68

Comment 4

[Steffen Wilberg]

Never mind, I figured it out.

Comment 5

[Steffen Wilberg]

Attachment: MozReview Request: Bug 1119049 - Keyboard shortcuts should work in MediaDocuments without explicitly focusing the media element. r?bz

Bug 1119049 - Keyboard shortcuts should work in MediaDocuments without explicitly focusing the media element.

Comment 6

[Steffen Wilberg]

Comment on attachment 8669433 [details]
MozReview Request: Bug 1119049 - Keyboard shortcuts should work in MediaDocuments without explicitly focusing the media element. r?bz

Bug 1119049 - Keyboard shortcuts should work in MediaDocuments without explicitly focusing the media element.

Comment 7

[Steffen Wilberg]

https://reviewboard.mozilla.org/r/21211/#review19063

::: dom/html/MediaDocument.cpp:348
(Diff revision 1)
> +MediaDocument::LinkScript(const nsAString& aScript)

This is copied from MediaDocument::LinkStylesheet above.

Comment 8

[Steffen Wilberg]

Comment on attachment 8669433 [details]
MozReview Request: Bug 1119049 - Keyboard shortcuts should work in MediaDocuments without explicitly focusing the media element. r?bz

Bug 1119049 - Keyboard shortcuts should work in MediaDocuments without explicitly focusing the media element.

Comment 9

[arni2033]

I can tell you right now: the approach of forcing focus to video is not perfect, because it prevents people from Shift-tabbing to search bar and location bar.

Comment 10

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8669433 [details]
MozReview Request: Bug 1119049 - Keyboard shortcuts should work in MediaDocuments without explicitly focusing the media element. r?bz

I don't know this code. Passing to Chris.

Comment 11

[Steffen Wilberg]

That's right. But you can press Tab twice, or Ctrl+l, or F6, to focus the location bar, and you can press ctrl+k to focus the search bar.

But when testing this, I discovered a problem: Clicking in the location bar and then in the content area around the video would not refocus the video element. The next patch fixes that.

Comment 12

[Steffen Wilberg]

Comment on attachment 8669433 [details]
MozReview Request: Bug 1119049 - Keyboard shortcuts should work in MediaDocuments without explicitly focusing the media element. r?bz

Bug 1119049 - Keyboard shortcuts should work in MediaDocuments without explicitly focusing the media element. r?cpearce

Comment 13

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/21211/#review19085

::: toolkit/content/TopLevelVideoDocument.js:18
(Diff revision 2)
> +  // and prevent it from losing focus
> +  videoElement.addEventListener("blur", () => {
> +    setTimeout(() => videoElement.focus(), 0);
> +  });

I think this will break tab navigation for keyboard/screenreader users in some cases, especially if we ever make the video controls tabbable. Isn't the document focus listener enough to do what we want here? I didn't think there were really other focusable things here besides the document and the video itself - why do we need the blur listener? :-)

Comment 14

[Steffen Wilberg]

https://reviewboard.mozilla.org/r/21211/#review19085

> I think this will break tab navigation for keyboard/screenreader users in some cases, especially if we ever make the video controls tabbable. Isn't the document focus listener enough to do what we want here? I didn't think there were really other focusable things here besides the document and the video itself - why do we need the blur listener? :-)

The focus listener catches clicking outside the content area (e.g. the location bar) and then somewhere in the content area, but outside the video.
However, if the video has focus, it doesn't catch clicking outside the video element. The body element receives focus, but the focus listener doesn't fire. Any ideas?

For testing, you can add
  video:focus { border: 5px solid green; }
to http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/media/TopLevelVideoDocument.css .

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8669433 [details]
MozReview Request: Bug 1119049 - Keyboard shortcuts should work in MediaDocuments without explicitly focusing the media element. r?bz

Behaviour change is fine with me (modulo breaking screen readers?), but I think this patch needs a dom peer review...

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8669433 [details]
MozReview Request: Bug 1119049 - Keyboard shortcuts should work in MediaDocuments without explicitly focusing the media element. r?bz

https://reviewboard.mozilla.org/r/21211/#review19297

Did you test tab navigation with this setup?  I assume that these focus() calls don't move windows up/down or change which tab is focused, right?

Comment 17

[Steffen Wilberg]

https://reviewboard.mozilla.org/r/21211/#review19297

I didn't notice any unusual behavior, no. You can still press Tab to focus the current tab, and press the left/right arrow keys to select the previous/next tab, or press again to focus the location bar.
Of course, you can just press ctrl+PageUp/PageDown to switch tabs, or press Ctrl+l or F6 to focus the location bar.

You can press ctrl+b to open the bookmarks sidebar, and its search box is focused.
You can press ctrl+f to open the find bar, and it is focused.
You can press ctrl+j to open the library, and it is focused.
You can press ctrl+p to open the print dialog, and it is focused.

The only thing that doesn't work anymore is pressing shift+tab twice to focus the search bar. Forward-tabbing works, and of course you can just press ctrl+k.

You can click on any browser chrome element to interact with it.
You just can't focus the document body anymore, which is the purpose of this patch.

You can right-click (or press the context key) to open the context menu of the page or of the video element, and navigate those with the arrow keys, and press enter to select the focused menu item.

I don't know how I could interact with windows which are not in the front, to test whether they would move up. 
I don't know how I could interact with a background tab either.

According to the debugger, the blur event listener does fire when you click e.g. in the location bar, but the focus() call doesn't steal back the focus (I also tried a 500ms timeout). And of course it shouldn't, as this page is not privileged.

The focus event listener fires when clicking in the content area, if the content was not focused before.
It also fires when switching tabs to the page with the video element, and the content area had focus there before.

Anything else I should test?

Comment 18

[arni2033]

I only have a few notes, aka "feedback".

1) This patch removes the possibility to open context menu for page (Shift+F10 on Win)
2) This patch isn't complete: I've tried your script and have to say that if I focus searchbar,
   then press Tab (that focuses document), then press Spacebar - nothing happens.
3) [could be easily fixed, but...]   Pressing Tab does nothing when <video> is focused AND video
   controls are shown (they show up when I move mouse), because fullscreen button is already tabbable.

4) This patch removes Shift+Tab navigation - people could get used to it.
   STR [given that (3) is fixed]:
     0. Open a video in firefox with your patch applied
     1. Open bookmarks sidebar, select one of folders with keyboard (e.g. 11th folder from the start)
     2. Now Press Tab
Result:
   Now the quickest way to get back to bookmarks is not Shift+Tab like it was before, but (user is
   forced to use this tricky workaround) Ctrl+K -> Tab(x3) = 1+4 pressings instead of 2+2 currently,
   and instead of 1+1 if the script only listened and transmitted 'keypress'es from body to video.
   ("2+2" - means that currently user needs to press 2 keys to focus video, and 2 keys to go back)

5) Would you also like to make a patch for _audio_? If not, open separate bug for that.

Comment 19

[arni2033]

* Sorry, it's (2) that could be easily fixed. I don't know how 3 could be fixed.

Comment 20

[Steffen Wilberg]

> 1) This patch removes the possibility to open context menu for page
> (Shift+F10 on Win)
Correct. Pressing Shift+F10 or the menu key (left of the right ctrl key) opens the context menu for the focused element. So as long as the content area has focus, it would always be the video element.
And that's the right thing to do, because the context menu of the video has all the relevant items like Play, Mute, Play Speed, Show/Hide Controls, Show Statistics, Full Screen, Copy Video Location, Save Video As, Save Snapshot as, Email Video, Send Video to device.
What would you do with the page context menu? Go back? Reload? Bookmark? Those have keyboard shortcuts. View Page Info?

> 2) This patch isn't complete: I've tried your script and have to say that if
> I focus searchbar, then press Tab (that focuses document), then press Spacebar - nothing
> happens.
This works for me (on Windows).

> 3) Pressing Tab does nothing when <video>
> is focused AND video controls are shown (they show up when I move mouse), because fullscreen
> button is already tabbable.
WFM as well.

> 4) This patch removes Shift+Tab navigation - people could get used to it.
That's true. This is because of the blur event listener. Pressing shift+tab would focus the body element, so the blur event listener fires. Pressing tab moves focus outside the content area, which works.

>  if the script only listened and transmitted 'keypress'es from body to video.
That would work, but I couldn't get it to work yet. It tried this:

  document.addEventListener("keypress", ev => {
    if (document.activeElement == videoElement)
      return;

    console.log("key pressed: "+ev.key);
    let syntheticEvent = new KeyboardEvent("keydown", {key: ev.key, code: ev.code, charCode: ev.charCode});
    videoElement.dispatchEvent(syntheticEvent);
  });


> 5) Would you also like to make a patch for _audio_? If not, open separate
> bug for that.
This already works with this patch because <video> is used for top-level audio documents as well.
You can try e.g. http://www.vorbis.com/music/Hydrate-Kenny_Beltrey.ogg

Comment 21

[arni2033]

Hm. Sorry for (5) - it's invalid. (2) only happens in non-e10s mode. I was going to suggest you to check active element inside that function with timeout 0ms, not before setting the timeout.
(3) always happens for me on Win7. The console says that <video> lost focus, and <body> becomes active element, so <video> is being focused again. It's weird, but since video controls are anonymous...

Actually, I modified your last code and the following woked for me (it doesn't even break anything):
>   var videoElement = document.querySelector('video');
>   
>   function alwaysTransmitEvent(type){
>     document.addEventListener(type, ev => {
>       if (document.activeElement == videoElement)
>         return;
>       // console.log(ev); // <--- delete this
>       let syntheticEvent = new KeyboardEvent(type, {
>         key:      ev.key,
>         code:     ev.code,
>         keyCode:  ev.keyCode,
>         charCode: ev.charCode,
>         which:    ev.which,
>         ctrlKey:  ev.ctrlKey,
>         altKey:   ev.altKey,
>         shiftKey: ev.shiftKey,
>         metaKey:  ev.metaKey
>       });
>       videoElement.dispatchEvent(syntheticEvent);
>     });
>   }
>   
>   alwaysTransmitEvent('keydown');
>   alwaysTransmitEvent('keypress');
>   alwaysTransmitEvent('keyup');
It doesn't even break context menus. The only bug I encountered is also presented currently (it's connected to fullscreen button. I'll file it later, maybe next week)
The good thing is that here we're not dealing with 'blur' events and for video controls we always have
document.activeElement == videoElement   [compare it to (3) above]

Comment 22

[Steffen Wilberg]

Thanks! I missed keyCode, which is deprecated, but the respective code in videocontrols.xml is from 2009...
Sending keydown couldn't work either since videocontrols.xml only listens for keypress.

Keys to test are:

Space = Play/Pause

ArrowUp = Volume increase
ArrowDown = Volume decrease
Accel+ArrowDown = Mute
Accel+ArrowUp = Unmute

ArrowLeft = Seek back 15 seconds
ArrowRight = Seek forward 15 seconds
Accel+ArrowLeft = Seek back 10%
Accel+ArrowRight = Seek forward 10%
Home = Seek to beginning
End = Seek to end

Surprisingly, there's no key for fullscreen yet. F11 doesn't maximize the video. But that's easy to fix as a follow-up.

Comment 23

[Steffen Wilberg]

Comment on attachment 8669433 [details]
MozReview Request: Bug 1119049 - Keyboard shortcuts should work in MediaDocuments without explicitly focusing the media element. r?bz

Bug 1119049 - Keyboard shortcuts should work in MediaDocuments without explicitly focusing the media element. r?bz

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8669433 [details]
MozReview Request: Bug 1119049 - Keyboard shortcuts should work in MediaDocuments without explicitly focusing the media element. r?bz

https://reviewboard.mozilla.org/r/21211/#review19475

::: toolkit/content/TopLevelVideoDocument.js:7
(Diff revision 3)
> +var videoElement;

Is there a reason this needs to go into the global scope and not just be a var inside the load listener  The latter seems like it would be a better idea.

::: toolkit/content/TopLevelVideoDocument.js:16
(Diff revision 3)
> +    if (document.activeElement == videoElement)

It took me a bit to figure out why this check is here, since it's not enough to prevent recursion from firing the keypress on its own.  Please document this check clearly; if I understand the intent, you want to catch cases when the video is focused, no matter what the event target is (which may be the video itself or controls inside it or whatever)?

::: toolkit/content/TopLevelVideoDocument.js:19
(Diff revision 3)
> +    let newEvent = new KeyboardEvent("keypress", {key:      ev.key,

So the key reason that this doesn't end up reentering our keypress listener is that the default value of "bubbles" in EventInit is false, right?

Is there a reason that creating a new event here is preferable to doing some sort of recursion guard and then just redispatching "ev" to the videoElement?  If so, please document that reason.  If not, consider doing things that way, so you don't have to adjust this code every time mode state that needs to be copied gets added to key events.

r=me with those addressed; I like this approach a lot more than messing with focus.

Comment 25

[Steffen Wilberg]

https://reviewboard.mozilla.org/r/21211/#review19475

> So the key reason that this doesn't end up reentering our keypress listener is that the default value of "bubbles" in EventInit is false, right?
> 
> Is there a reason that creating a new event here is preferable to doing some sort of recursion guard and then just redispatching "ev" to the videoElement?  If so, please document that reason.  If not, consider doing things that way, so you don't have to adjust this code every time mode state that needs to be copied gets added to key events.

Yes, I think so. I didn't have problems with recursion with that patch.
I can't just dispatch the event on another target, that throws a 
"InvalidStateError: An attempt was made to use an object that is not, or is no longer, usable"

But I can use the event as a prototype for the new one:
    let newEvent = new KeyboardEvent("keypress", ev);
    videoElement.dispatchEvent(newEvent);
After adding a recursion check, this works fine.

Comment 26

[Steffen Wilberg]

Comment on attachment 8669433 [details]
MozReview Request: Bug 1119049 - Keyboard shortcuts should work in MediaDocuments without explicitly focusing the media element. r?bz

Bug 1119049 - Keyboard shortcuts should work in MediaDocuments without explicitly focusing the media element. r?bz

Comment 27

[Boris Zbarsky [:bzbarsky]]

> I can't just dispatch the event on another target

Ah, right, not until the original dispatch is done...

Comment 28

[Boris Zbarsky [:bzbarsky]]

https://reviewboard.mozilla.org/r/21211/#review19483

r=me

Comment 29

[Steffen Wilberg]

https://hg.mozilla.org/integration/fx-team/rev/642713addeae753ba19848769818fb44f688b3c3
Bug 1119049 - Keyboard shortcuts should work in MediaDocuments without explicitly focusing the media element. r=bz

Comment 30

[Steffen Wilberg]

Thanks for the good comments and the quick review!

Comment 31

[Steffen Wilberg]

(In reply to Steffen Wilberg from comment #22)
> Surprisingly, there's no key for fullscreen yet. F11 doesn't maximize the
> video. But that's easy to fix as a follow-up.

Filed bug 1213568.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/642713addeae

Comment 33

[arni2033]

This doesn't work with local media documents. I  think there's no 'load' event for local docs.

Comment 34

[Boris Zbarsky [:bzbarsky]]

There should be a load event.

The question is whether it's managing to fire before the script runs....

Comment 35

[arni2033]

Well, you know better. Is it possible (and useful) to use the same solution as that is currently used on error page (to paste <script> after <video>) ??

Comment 36

[Boris Zbarsky [:bzbarsky]]

> Well, you know better.

I actually don't.  Please file a new bug with steps to reproduce for whatever you're seeing, and I can try to tale a look if Steffen doesn't get to it.