46949 2010-09-30 18:02:56 -0700 Event names should always match the registered handler 2010-09-30 20:37:02 -0700 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) All OS X 10.5 RESOLVED DUPLICATE 42580 InRadar P2 Normal --- 1 enrica enrica darin rniwa oldest_to_newest 287973 0 enrica 2010-09-30 18:02:56 -0700 When firing DOM Level 3 focusin/focusout events, we also fire DOM Level 2 DOMFocusIn/DOMFocusOut, but we fail to reflect it in the event object. The breaks any client who checks event.type. 287976 1 69407 enrica 2010-09-30 18:11:07 -0700 Created attachment 69407 Patch 287978 2 69407 adele 2010-09-30 18:17:11 -0700 Comment on attachment 69407 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69407&action=review > WebCore/dom/EventTarget.cpp:310 > + event->useAliasedType(); Would it make sense to have event->type() have this logic? Are there any other places where using the regular type for an event with an aliased type would cause problems? 287984 3 69407 rniwa 2010-09-30 18:22:44 -0700 Comment on attachment 69407 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69407&action=review > WebCore/dom/EventTarget.cpp:310 > + if (result != d->eventListenerMap.end()) { > + // When firing the event listener for the aliased event, the event type should > + // match the registered handler. > + event->useAliasedType(); It seems like alias is only used for focusin / focusout right now but are we sure we want to use the aliased type whenever an event is aliased? i.e. if we are adding another aliased event in the future, do we also want to use the aliased type for that? > LayoutTests/fast/events/focusinout.html:15 > - writePass('result1'); > + if (event.type == "focusin") > + writePass('result1'); Would it be possible to rename result1...result4 to something more meaningful? It'll be also nice if we could print out what failed. 287989 4 enrica 2010-09-30 18:33:51 -0700 (In reply to comment #2) > (From update of attachment 69407 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=69407&action=review > > > WebCore/dom/EventTarget.cpp:310 > > + event->useAliasedType(); > > Would it make sense to have event->type() have this logic? Are there any other places where using the regular type for an event with an aliased type would cause problems? I don't believe it is possible. Where we call useAliasedType is the only place we know we are firing an aliased type. 287990 5 69407 mitz 2010-09-30 18:36:23 -0700 Comment on attachment 69407 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69407&action=review >>> WebCore/dom/EventTarget.cpp:310 >>> + event->useAliasedType(); >> >> Would it make sense to have event->type() have this logic? Are there any other places where using the regular type for an event with an aliased type would cause problems? > > It seems like alias is only used for focusin / focusout right now but are we sure we want to use the aliased type whenever an event is aliased? i.e. if we are adding another aliased event in the future, do we also want to use the aliased type for that? I think it is wrong for the first target to change the event’s reported type for all subsequent targets that might handle the same event. 287992 6 enrica 2010-09-30 18:42:13 -0700 (In reply to comment #5) > (From update of attachment 69407 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=69407&action=review > > >>> WebCore/dom/EventTarget.cpp:310 > >>> + event->useAliasedType(); > >> > >> Would it make sense to have event->type() have this logic? Are there any other places where using the regular type for an event with an aliased type would cause problems? > > > > It seems like alias is only used for focusin / focusout right now but are we sure we want to use the aliased type whenever an event is aliased? i.e. if we are adding another aliased event in the future, do we also want to use the aliased type for that? > > I think it is wrong for the first target to change the event’s reported type for all subsequent targets that might handle the same event. I'm not sure I understand your concern. We are always firing the Level 3 event with its name for all the targets, then we change to the name to the aliased and we fire the aliased handlers with the aliased name. I apologize if I'm missing the point. 287999 7 enrica 2010-09-30 18:55:37 -0700 (In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 69407 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=69407&action=review > > > > >>> WebCore/dom/EventTarget.cpp:310 > > >>> + event->useAliasedType(); > > >> > > >> Would it make sense to have event->type() have this logic? Are there any other places where using the regular type for an event with an aliased type would cause problems? > > > > > > It seems like alias is only used for focusin / focusout right now but are we sure we want to use the aliased type whenever an event is aliased? i.e. if we are adding another aliased event in the future, do we also want to use the aliased type for that? > > > > I think it is wrong for the first target to change the event’s reported type for all subsequent targets that might handle the same event. > > I'm not sure I understand your concern. We are always firing the Level 3 event with its name for all the targets, then we change to the name to the aliased and we fire the aliased handlers with the aliased name. > I apologize if I'm missing the point. The concept of aliasedType was introduced to continue to support old style events even when using the DOM Level 3 ones. I believe it is correct for the event type to match the name of the registered handler. If a client registers a generic handler function and relies on the event.type to distinguish which event was received, it will fail if we don't have handler and event type in sync. 288005 8 rniwa 2010-09-30 19:08:21 -0700 (In reply to comment #7) > (In reply to comment #6) > > > I think it is wrong for the first target to change the event’s reported type for all subsequent targets that might handle the same event. > > > > I'm not sure I understand your concern. We are always firing the Level 3 event with its name for all the targets, then we change to the name to the aliased and we fire the aliased handlers with the aliased name. > > I apologize if I'm missing the point. > > The concept of aliasedType was introduced to continue to support old style events even when using the DOM Level 3 ones. I believe it is correct for the event type to match the name of the registered handler. > If a client registers a generic handler function and relies on the event.type to distinguish which event was received, it will fail if we don't have handler and event type in sync. I think Dan is more concerned about the timing in which we change the event type. Maybe we can do this where we call the first fireEventListeners? 288038 9 darin 2010-09-30 20:35:12 -0700 Longer term we definitely need to eliminate this concept of aliased types. I think it was not the right solution for the problem. 288039 10 darin 2010-09-30 20:35:38 -0700 There was another bug where we were trying to fix this same issue, and I had made a lot of comments there. 288040 11 darin 2010-09-30 20:37:02 -0700 *** This bug has been marked as a duplicate of bug 42580 *** 69407 2010-09-30 18:11:07 -0700 2010-09-30 18:36:22 -0700 Patch bug8107311.txt text/plain 4342 enrica SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA2ODg1NSkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMjYgQEAKKzIwMTAtMDktMzAgIEVucmljYSBDYXN1Y2NpICA8ZW5yaWNhQGFwcGxl LmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBF dmVudCBuYW1lcyBzaG91bGQgYWx3YXlzIG1hdGNoIHRoZSByZWdpc3RlcmVkIGhhbmRsZXIuCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD00Njk0OQorICAg ICAgICA8cmRhcjovL3Byb2JsZW0vODEwNzMxMT4KKyAgICAgICAgCisgICAgICAgIFdoZW4gZmly aW5nIERPTSBMZXZlbCAzIGZvY3VzaW4vZm9jdXNvdXQgZXZlbnRzLCB3ZSBhbHNvIGZpcmUKKyAg ICAgICAgRE9NIExldmVsIDIgRE9NRm9jdXNJbi9ET01Gb2N1c091dCwgYnV0IHdlIGZhaWwgdG8g cmVmbGVjdCBpdAorICAgICAgICBpbiB0aGUgZXZlbnQgb2JqZWN0LgorICAgICAgICBUaGlzIGJy ZWFrcyBhbnkgY2xpZW50IHdobyBjaGVja3MgZXZlbnQudHlwZS4KKyAgICAgICAgCisgICAgICAg IFRlc3Q6IHVwZGF0ZWQgZmFzdC9ldmVudHMvZm9jdXNpbm91dC5odG1sIHRvIGFkZCB0aGUgY2hl Y2sKKyAgICAgICAgb250aGUgZXZlbnQgdHlwZS4KKyAgICAgICAgCisgICAgICAgICogZG9tL0V2 ZW50LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkV2ZW50Ojp1c2VBbGlhc2VkVHlwZSk6IEFkZGVk LgorICAgICAgICAqIGRvbS9FdmVudC5oOgorICAgICAgICAqIGRvbS9FdmVudFRhcmdldC5jcHA6 CisgICAgICAgIChXZWJDb3JlOjpFdmVudFRhcmdldDo6ZmlyZUV2ZW50TGlzdGVuZXJzKTogTW9k aWZpZWQgdG8KKyAgICAgICAgY2hhbmdlIHRoZSBldmVudCB0eXBlIGluIHRoZSBldmVudCBvYmpl Y3QuCisKIDIwMTAtMDktMzAgIERhcmluIEFkbGVyICA8ZGFyaW5AYXBwbGUuY29tPgogCiAgICAg ICAgIFJldmlld2VkIGJ5IFNhbSBXZWluaWcuCkluZGV4OiBXZWJDb3JlL2RvbS9FdmVudC5jcHAK PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PQotLS0gV2ViQ29yZS9kb20vRXZlbnQuY3BwCShyZXZpc2lvbiA2ODgxOCkKKysr IFdlYkNvcmUvZG9tL0V2ZW50LmNwcAkod29ya2luZyBjb3B5KQpAQCAtNzIsNiArNzIsMTIgQEAg dm9pZCBFdmVudDo6aW5pdEV2ZW50KGNvbnN0IEF0b21pY1N0cmluZwogICAgIG1fY2FuY2VsYWJs ZSA9IGNhbmNlbGFibGVBcmc7CiB9CiAKK3ZvaWQgRXZlbnQ6OnVzZUFsaWFzZWRUeXBlKCkKK3sK KyAgICBpZiAoaGFzQWxpYXNlZFR5cGUoKSkKKyAgICAgICAgbV90eXBlID0gYWxpYXNlZFR5cGUo KTsKK30KKyAgICAKIGJvb2wgRXZlbnQ6OmlzQ3VzdG9tRXZlbnQoKSBjb25zdAogewogICAgIHJl dHVybiBmYWxzZTsKSW5kZXg6IFdlYkNvcmUvZG9tL0V2ZW50LmgKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2Vi Q29yZS9kb20vRXZlbnQuaAkocmV2aXNpb24gNjg4MTgpCisrKyBXZWJDb3JlL2RvbS9FdmVudC5o CSh3b3JraW5nIGNvcHkpCkBAIC03OCw2ICs3OCw3IEBAIG5hbWVzcGFjZSBXZWJDb3JlIHsKICAg ICAgICAgCiAgICAgICAgIGNvbnN0IEF0b21pY1N0cmluZyYgYWxpYXNlZFR5cGUoKSBjb25zdDsK ICAgICAgICAgYm9vbCBoYXNBbGlhc2VkVHlwZSgpIGNvbnN0OworICAgICAgICB2b2lkIHVzZUFs aWFzZWRUeXBlKCk7CiAKICAgICAgICAgRXZlbnRUYXJnZXQqIHRhcmdldCgpIGNvbnN0IHsgcmV0 dXJuIG1fdGFyZ2V0LmdldCgpOyB9CiAgICAgICAgIHZvaWQgc2V0VGFyZ2V0KFBhc3NSZWZQdHI8 RXZlbnRUYXJnZXQ+KTsKSW5kZXg6IFdlYkNvcmUvZG9tL0V2ZW50VGFyZ2V0LmNwcAo9PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09Ci0tLSBXZWJDb3JlL2RvbS9FdmVudFRhcmdldC5jcHAJKHJldmlzaW9uIDY4ODE4KQorKysg V2ViQ29yZS9kb20vRXZlbnRUYXJnZXQuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0zMDQsOCArMzA0 LDEyIEBAIGJvb2wgRXZlbnRUYXJnZXQ6OmZpcmVFdmVudExpc3RlbmVycyhFdmUKICAgICAvLyBu YW1lcyBpbiB0aGUgZnV0dXJlIGFuZCBzdGlsbCBtYWtlIHRoZW0gYmUgaW50ZXJvcGVyYWJsZS4K ICAgICBpZiAoZXZlbnQtPmhhc0FsaWFzZWRUeXBlKCkgJiYgIWV2ZW50LT5pbW1lZGlhdGVQcm9w YWdhdGlvblN0b3BwZWQoKSkgewogICAgICAgICBFdmVudExpc3RlbmVyTWFwOjppdGVyYXRvciBy ZXN1bHQgPSBkLT5ldmVudExpc3RlbmVyTWFwLmZpbmQoZXZlbnQtPmFsaWFzZWRUeXBlKCkpOwot ICAgICAgICBpZiAocmVzdWx0ICE9IGQtPmV2ZW50TGlzdGVuZXJNYXAuZW5kKCkpCisgICAgICAg IGlmIChyZXN1bHQgIT0gZC0+ZXZlbnRMaXN0ZW5lck1hcC5lbmQoKSkgeworICAgICAgICAgICAg Ly8gV2hlbiBmaXJpbmcgdGhlIGV2ZW50IGxpc3RlbmVyIGZvciB0aGUgYWxpYXNlZCBldmVudCwg dGhlIGV2ZW50IHR5cGUgc2hvdWxkCisgICAgICAgICAgICAvLyBtYXRjaCB0aGUgcmVnaXN0ZXJl ZCBoYW5kbGVyLgorICAgICAgICAgICAgZXZlbnQtPnVzZUFsaWFzZWRUeXBlKCk7CiAgICAgICAg ICAgICBmaXJlRXZlbnRMaXN0ZW5lcnMoZXZlbnQsIGQsICpyZXN1bHQtPnNlY29uZCk7CisgICAg ICAgIH0KICAgICB9CiAgICAgcmV0dXJuICFldmVudC0+ZGVmYXVsdFByZXZlbnRlZCgpOwogfQpJ bmRleDogTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIExheW91dFRlc3RzL0No YW5nZUxvZwkocmV2aXNpb24gNjg4NTUpCisrKyBMYXlvdXRUZXN0cy9DaGFuZ2VMb2cJKHdvcmtp bmcgY29weSkKQEAgLTEsMyArMSwxMyBAQAorMjAxMC0wOS0zMCAgRW5yaWNhIENhc3VjY2kgIDxl bnJpY2FAYXBwbGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgor CisgICAgICAgIEV2ZW50IG5hbWVzIHNob3VsZCBhbHdheXMgbWF0Y2ggdGhlIHJlZ2lzdGVyZWQg aGFuZGxlci4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lk PTQ2OTQ5CisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS84MTA3MzExPgorCisgICAgICAgICogZmFz dC9ldmVudHMvZm9jdXNpbm91dC5odG1sOiBBZGRlZCBjaGVjayBvbiB0aGUgZXZlbnQgdHlwZS4K KwogMjAxMC0wOS0zMCAgU2ltb24gRnJhc2VyICA8c2ltb24uZnJhc2VyQGFwcGxlLmNvbT4KIAog ICAgICAgICBSZXZpZXdlZCBieSBKb24gSG9uZXljdXR0LgpJbmRleDogTGF5b3V0VGVzdHMvZmFz dC9ldmVudHMvZm9jdXNpbm91dC5odG1sCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIExheW91dFRlc3RzL2Zhc3Qv ZXZlbnRzL2ZvY3VzaW5vdXQuaHRtbAkocmV2aXNpb24gNjg4MTgpCisrKyBMYXlvdXRUZXN0cy9m YXN0L2V2ZW50cy9mb2N1c2lub3V0Lmh0bWwJKHdvcmtpbmcgY29weSkKQEAgLTExLDIyICsxMSwy NiBAQCBmdW5jdGlvbiB3cml0ZVBhc3MoaWQpCiAKIGZ1bmN0aW9uIGZvY3VzSGFuZGxlcihldmVu dCkKIHsKLSAgICB3cml0ZVBhc3MoJ3Jlc3VsdDEnKTsKKyAgICBpZiAoZXZlbnQudHlwZSA9PSAi Zm9jdXNpbiIpCisgICAgICAgIHdyaXRlUGFzcygncmVzdWx0MScpOwogfQogCiBmdW5jdGlvbiBi bHVySGFuZGxlcihldmVudCkKIHsKLSAgICB3cml0ZVBhc3MoJ3Jlc3VsdDInKTsKKyAgICBpZiAo ZXZlbnQudHlwZSA9PSAiZm9jdXNvdXQiKQorICAgICAgICB3cml0ZVBhc3MoJ3Jlc3VsdDInKTsK IH0KIAogZnVuY3Rpb24gZm9jdXNIYW5kbGVyVHdvKGV2ZW50KQogewotICAgIHdyaXRlUGFzcygn cmVzdWx0MycpOworICAgIGlmIChldmVudC50eXBlID09ICJET01Gb2N1c0luIikKKyAgICAgICAg d3JpdGVQYXNzKCdyZXN1bHQzJyk7CiB9CiAKIGZ1bmN0aW9uIGJsdXJIYW5kbGVyVHdvKGV2ZW50 KQogewotICAgIHdyaXRlUGFzcygncmVzdWx0NCcpOworICAgIGlmIChldmVudC50eXBlID09ICJE T01Gb2N1c091dCIpCisgICAgICAgIHdyaXRlUGFzcygncmVzdWx0NCcpOwogfQogPC9zY3JpcHQ+ CiA8L2hlYWQ+Cg==