Bug 24955 – space bar does not play/pause when in standalone QuickTime movie

• Component: Platform
• Status: RESOLVED FIXED

[Dean Jackson]

Standalone MediaDocuments do not play/pause when you press space bar.
<rdar://problem/6635099>

[Dean Jackson]

Created attachment 29102 [details]
patch

Needs manual testing since it is a standalone media document.

[Simon Fraser (smfr)]

Comment on attachment 29102 [details]
patch


> diff --git a/WebCore/loader/MediaDocument.cpp b/WebCore/loader/MediaDocument.cpp
> index b89ac10..85aa48d 100644

> +    if (event->type() == eventNames().keydownEvent && event->isKeyboardEvent()) {
> +        KeyboardEvent* k = static_cast<KeyboardEvent*>(event);

Please use a better variable name than 'k'.

> +        HTMLVideoElement* video;
> +        if (targetNode) {
> +            if (targetNode->hasTagName(videoTag))
> +                video = static_cast<HTMLVideoElement*>(targetNode);
> +            else {
> +                RefPtr<NodeList> nodeList = targetNode->getElementsByTagName("video");
> +                if (nodeList.get()->length() > 0)
> +                    video = static_cast<HTMLVideoElement*>(nodeList.get()->item(0));

When does this happen? When the event target is the document? Since this is MediaDocument.cpp,
can you get to the media element directly?

> +            }
> +        }

What about standalone audio?

> +        if (video) {
> +            if (k->keyIdentifier() == "U+0020") { // space

This is convention used elsewhere?

r- for now, but I think it's generally sound.

[Dean Jackson]

> When does this happen? When the event target is the document? Since this is MediaDocument.cpp,
> can you get to the media element directly?

Yes, when the target is the document. The alternative would be to have the user focus the media element and then wait for the keyboard event, which is obviously not useful (and a regression).

And no, you can't get to the media element directly. The MediaDocument fires up a tokeniser which inserts the video directly. I could work it so the document holds a reference to the media - I wasn't sure which was the best way.

> What about standalone audio?

Standalone audio is standalone video. The MediaDocument tokeniser always makes a video element.

> This is convention used elsewhere?

I had a choice here between looking at the keyCode (would be 32 for space) or the keyIdentifier. Most of the other code I looked at uses keyIdentifier. I'm not sure why, since it is string comparison.

[Dean Jackson]

Looking again, it seems sometimes people use keyCode and sometimes they use keyIdentifier.

Obviously they use keyIdentifier for things like PageUp and Enter, but see HTMLButtonElement for an example checking for U+0020 like I do.

I don't care either way.

[Dean Jackson]

Keeping a reference to the media element would be a more intrusive change (eg. changing two class definitions rather than just this method). However, since we're in a standalone document it's only one extra reference - not a memory issue. Upside is that we don't search the document with every keypress.

Either way fine with me, but I'm tempted to keep the changes to a minimum.

[Dean Jackson]

Of course, I meant ie not eg.
See http://www.imdb.com/title/tt0113161/quotes

[Simon Fraser (smfr)]

Comment on attachment 29102 [details]
patch

OK, r+ but please use a better name than 'k' (suggest: "keyboardEvent").

[Dean Jackson]

Committed r42134
	M	WebCore/ChangeLog
	M	WebCore/loader/MediaDocument.cpp