Bug 24955

Summary: space bar does not play/pause when in standalone QuickTime movie
Product: WebKit Reporter: Dean Jackson <dino>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarrin, eric.carlson, simon.fraser
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Attachments:
Description Flags
patch simon.fraser: review+

Description Dean Jackson 2009-03-30 18:07:17 PDT
Standalone MediaDocuments do not play/pause when you press space bar.
<rdar://problem/6635099>
Comment 1 Dean Jackson 2009-03-30 18:10:15 PDT
Created attachment 29102 [details]
patch

Needs manual testing since it is a standalone media document.
Comment 2 Simon Fraser (smfr) 2009-03-30 18:16:59 PDT
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.
Comment 3 Dean Jackson 2009-03-30 18:30:49 PDT
> 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.

Comment 4 Dean Jackson 2009-03-30 18:35:17 PDT
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.
Comment 5 Dean Jackson 2009-03-30 18:38:00 PDT
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.
Comment 6 Dean Jackson 2009-03-30 18:40:35 PDT
Of course, I meant ie not eg.
See http://www.imdb.com/title/tt0113161/quotes
Comment 7 Simon Fraser (smfr) 2009-03-30 20:04:48 PDT
Comment on attachment 29102 [details]
patch

OK, r+ but please use a better name than 'k' (suggest: "keyboardEvent").
Comment 8 Dean Jackson 2009-03-31 12:51:19 PDT
Committed r42134
	M	WebCore/ChangeLog
	M	WebCore/loader/MediaDocument.cpp