Bug 24955 - space bar does not play/pause when in standalone QuickTime movie
Summary: space bar does not play/pause when in standalone QuickTime movie
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2009-03-30 18:07 PDT by Dean Jackson
Modified: 2009-03-31 12:51 PDT (History)
3 users (show)

See Also:


Attachments
patch (2.05 KB, patch)
2009-03-30 18:10 PDT, Dean Jackson
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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