24955 2009-03-30 18:07:17 -0700 space bar does not play/pause when in standalone QuickTime movie 2009-03-31 12:51:19 -0700 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) Mac OS X 10.5 RESOLVED FIXED InRadar P2 Normal --- 1 dino webkit-unassigned cmarrin eric.carlson simon.fraser oldest_to_newest 115949 0 dino 2009-03-30 18:07:17 -0700 Standalone MediaDocuments do not play/pause when you press space bar. <rdar://problem/6635099> 115950 1 29102 dino 2009-03-30 18:10:15 -0700 Created attachment 29102 patch Needs manual testing since it is a standalone media document. 115953 2 29102 simon.fraser 2009-03-30 18:16:59 -0700 Comment on attachment 29102 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. 115957 3 dino 2009-03-30 18:30:49 -0700 > 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. 115958 4 dino 2009-03-30 18:35:17 -0700 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. 115959 5 dino 2009-03-30 18:38:00 -0700 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. 115961 6 dino 2009-03-30 18:40:35 -0700 Of course, I meant ie not eg. See http://www.imdb.com/title/tt0113161/quotes 115979 7 29102 simon.fraser 2009-03-30 20:04:48 -0700 Comment on attachment 29102 patch OK, r+ but please use a better name than 'k' (suggest: "keyboardEvent"). 116056 8 dino 2009-03-31 12:51:19 -0700 Committed r42134 M WebCore/ChangeLog M WebCore/loader/MediaDocument.cpp 29102 2009-03-30 18:10:15 -0700 2009-03-30 20:04:48 -0700 patch spacebar.patch text/plain 2100 dino ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg MWE5NDJmMS4uYjAwOTJhYiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNCBAQAorMjAwOS0wMy0zMCAgRGVhbiBKYWNrc29u ICA8ZGlub0BhcHBsZS5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISku CisKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTI0OTU1 CisgICAgICAgIAorICAgICAgICBTcGFjZWJhciBkaWRuJ3QgcGxheS9wYXVzZSBpbiBzdGFuZGFs b25lIE1lZGlhRG9jdW1lbnQKKyAgICAgICAgCisgICAgICAgICogbG9hZGVyL01lZGlhRG9jdW1l bnQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6TWVkaWFEb2N1bWVudDo6ZGVmYXVsdEV2ZW50SGFu ZGxlcik6CisKIDIwMDktMDMtMzAgIFRpbW90aHkgSGF0Y2hlciAgPHRpbW90aHlAYXBwbGUuY29t PgogCiAgICAgICAgIDxyZGFyOi8vcHJvYmxlbS81ODM4ODcxPiBDcmFzaFRyYWNlcjogMTQ4MyBj cmFzaGVzIFF1aWNrbG9va2luZwpkaWZmIC0tZ2l0IGEvV2ViQ29yZS9sb2FkZXIvTWVkaWFEb2N1 bWVudC5jcHAgYi9XZWJDb3JlL2xvYWRlci9NZWRpYURvY3VtZW50LmNwcAppbmRleCBiODlhYzEw Li44NWFhNDhkIDEwMDY0NAotLS0gYS9XZWJDb3JlL2xvYWRlci9NZWRpYURvY3VtZW50LmNwcAor KysgYi9XZWJDb3JlL2xvYWRlci9NZWRpYURvY3VtZW50LmNwcApAQCAtMzgsNyArMzgsOSBAQAog I2luY2x1ZGUgIkhUTUxFbWJlZEVsZW1lbnQuaCIKICNpbmNsdWRlICJIVE1MTmFtZXMuaCIKICNp bmNsdWRlICJIVE1MVmlkZW9FbGVtZW50LmgiCisjaW5jbHVkZSAiS2V5Ym9hcmRFdmVudC5oIgog I2luY2x1ZGUgIk1haW5SZXNvdXJjZUxvYWRlci5oIgorI2luY2x1ZGUgIk5vZGVMaXN0LmgiCiAj aW5jbHVkZSAiUGFnZS5oIgogI2luY2x1ZGUgIlNlZ21lbnRlZFN0cmluZy5oIgogI2luY2x1ZGUg IlNldHRpbmdzLmgiCkBAIC0xNjEsNiArMTYzLDMwIEBAIHZvaWQgTWVkaWFEb2N1bWVudDo6ZGVm YXVsdEV2ZW50SGFuZGxlcihFdmVudCogZXZlbnQpCiAgICAgICAgICAgICB9CiAgICAgICAgIH0K ICAgICB9CisKKyAgICBpZiAoZXZlbnQtPnR5cGUoKSA9PSBldmVudE5hbWVzKCkua2V5ZG93bkV2 ZW50ICYmIGV2ZW50LT5pc0tleWJvYXJkRXZlbnQoKSkgeworICAgICAgICBLZXlib2FyZEV2ZW50 KiBrID0gc3RhdGljX2Nhc3Q8S2V5Ym9hcmRFdmVudCo+KGV2ZW50KTsKKyAgICAgICAgSFRNTFZp ZGVvRWxlbWVudCogdmlkZW87CisgICAgICAgIGlmICh0YXJnZXROb2RlKSB7CisgICAgICAgICAg ICBpZiAodGFyZ2V0Tm9kZS0+aGFzVGFnTmFtZSh2aWRlb1RhZykpCisgICAgICAgICAgICAgICAg dmlkZW8gPSBzdGF0aWNfY2FzdDxIVE1MVmlkZW9FbGVtZW50Kj4odGFyZ2V0Tm9kZSk7CisgICAg ICAgICAgICBlbHNlIHsKKyAgICAgICAgICAgICAgICBSZWZQdHI8Tm9kZUxpc3Q+IG5vZGVMaXN0 ID0gdGFyZ2V0Tm9kZS0+Z2V0RWxlbWVudHNCeVRhZ05hbWUoInZpZGVvIik7CisgICAgICAgICAg ICAgICAgaWYgKG5vZGVMaXN0LmdldCgpLT5sZW5ndGgoKSA+IDApCisgICAgICAgICAgICAgICAg ICAgIHZpZGVvID0gc3RhdGljX2Nhc3Q8SFRNTFZpZGVvRWxlbWVudCo+KG5vZGVMaXN0LmdldCgp LT5pdGVtKDApKTsKKyAgICAgICAgICAgIH0KKyAgICAgICAgfQorICAgICAgICBpZiAodmlkZW8p IHsKKyAgICAgICAgICAgIGlmIChrLT5rZXlJZGVudGlmaWVyKCkgPT0gIlUrMDAyMCIpIHsgLy8g c3BhY2UKKyAgICAgICAgICAgICAgICBpZiAodmlkZW8tPnBhdXNlZCgpKSB7CisgICAgICAgICAg ICAgICAgICAgIGlmICh2aWRlby0+Y2FuUGxheSgpKQorICAgICAgICAgICAgICAgICAgICAgICAg dmlkZW8tPnBsYXkoKTsKKyAgICAgICAgICAgICAgICB9IGVsc2UKKyAgICAgICAgICAgICAgICAg ICAgdmlkZW8tPnBhdXNlKCk7CisgICAgICAgICAgICAgICAgZXZlbnQtPnNldERlZmF1bHRIYW5k bGVkKCk7CisgICAgICAgICAgICB9CisgICAgICAgIH0KKyAgICB9CiB9CiAKIH0K