23318 2009-01-14 08:21:00 -0800 Click event listener remains registered after onclick attribute is removed 2010-06-10 19:06:57 -0700 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) Other Linux NEW P2 Normal --- 17429 1 caryclark webkit-unassigned ap bolsinga oldest_to_newest 105670 0 caryclark 2009-01-14 08:21:00 -0800 The attached test removes the onClick() attribute on the second element when the first element is clicked. The test works correctly -- the second element no longer responds to the click when the onClick() element is removed. However, the EventTargetNode RegisteredEventListenerList still includes an entry for the click event. Android iterates through all nodes looking for ones that receive clicks, and computes focus rings for those nodes. Attached is a patch that removes the event listener when the attribute is removed. The patch works, but isn't elegant. What's the right approach to fixing this bug? 105671 1 26709 caryclark 2009-01-14 08:23:07 -0800 Created attachment 26709 demonstration of bug 105672 2 26710 caryclark 2009-01-14 08:26:07 -0800 Created attachment 26710 proof of concept patch -- not intended to be landed This patch fixes the problem for Android -- but what's the right way to do it? 105682 3 26710 darin 2009-01-14 09:32:55 -0800 Comment on attachment 26710 proof of concept patch -- not intended to be landed The first thing we need here is a test case to demonstrate the problem. The details of the problem could be different with different node types, for example, with the window. And the failure may be caused by some other code change or some port-specific issue. A test case cuts through all those issues and lets everyone see that something is really wrong. The function responsible for handling this sort of thing is EventTargetNode::removeInlineEventListenerForType. In the failing test case, you should figure out if the problem is that function is not being called, or if the that function is not working properly. Specifically, if you change the value of the onclick attribute on a <button> element to the empty string (to choose a specific example, because there are many different code paths), then: EventTargetNode::setOnclick will call EventTargetNode::setInlineEventListenerForType, which will call EventTargetNode::removeInlineEventListenerForType, which will remove the listener that was added 105683 4 darin 2009-01-14 09:35:04 -0800 (In reply to comment #3) > EventTargetNode::setOnclick will call > EventTargetNode::setInlineEventListenerForType, which will call > EventTargetNode::removeInlineEventListenerForType, which will remove the > listener that was added Oops, that's when the onclick JavaScript property is set. If you do removeAttribute, then it's different. I'll add comments about that now. 105684 5 darin 2009-01-14 09:38:19 -0800 (In reply to comment #4) > If you do removeAttribute, then it's different. I'll add comments about that > now. I'm almost certain that the bug here boils down to the fact that Element::removeAttribute doesn't call attributeChanged the way Element::setAttribute does. The attributeChanged function calls parseMappedAttribute, and then HTMLElement::parseMappedAttribute calls setInlineEventListenerForTypeAndAttribute. I see now the test case here. There's probably a much simpler way to demonstrate this problem that doesn't require user intervention. 117150 6 ap 2009-04-10 09:25:24 -0700 See also: bug 25130. 149105 7 ap 2009-09-22 10:39:18 -0700 Looks like this issue only affects Android currently. But once Web Inspector begins to show registered event listeners, it will likely be affected. 26709 2009-01-14 08:23:07 -0800 2009-01-14 08:23:07 -0800 demonstration of bug android-bug.html text/html 2838 caryclark PD94bWwgdmVyc2lvbj0iMS4wIiBlbmNvZGluZz0iVVRGLTgiPz4KPCFET0NUWVBFIGh0bWwgUFVC TElDICItLy9XM0MvL0RURCBYSFRNTCAxLjAgVHJhbnNpdGlvbmFsLy9FTiIgImh0dHA6Ly93d3cu dzMub3JnL1RSL3hodG1sMS9EVEQveGh0bWwxLXRyYW5zaXRpb25hbC5kdGQiPgo8aHRtbCB4bWxu cz0iaHR0cDovL3d3dy53My5vcmcvMTk5OS94aHRtbCI+IAogIDxoZWFkPiAKICAgIDxtZXRhIGh0 dHAtZXF1aXY9IkNvbnRlbnQtVHlwZSIgY29udGVudD0iYXBwbGljYXRpb24veGh0bWwreG1sOyBj aGFyc2V0PVVURi04Ii8+CiAgICA8c2NyaXB0IHR5cGU9InRleHQvamF2YXNjcmlwdCI+CiAgICAg IGZ1bmN0aW9uIHRvZ2dsZSgpIAogICAgICB7CiAgICAgICAgdmFyIGUgPSBkb2N1bWVudC5nZXRF bGVtZW50QnlJZCgidG90b2dnbGUiKTsKICAgICAgICB2YXIgZiA9IGRvY3VtZW50LmdldEVsZW1l bnRCeUlkKCJzZWNvbmQiKTsKICAgICAgICBpZiAoZS5zdHlsZS5kaXNwbGF5ICE9ICJub25lIikg ewogICAgICAgICAgZS5zdHlsZS5kaXNwbGF5ID0gIm5vbmUiOwogICAgICAgICAgZi5kaXNhYmxl ZCA9ICJkaXNhYmxlZCI7CiAgICAgICAgICBkaXNhYmxlRWxlbWVudCgidGhpcmQiLCB0cnVlKTsK ICAgICAgICAgIGRpc2FibGVFbGVtZW50KCJwZCIsIHRydWUpOwogICAgICAgIH0gZWxzZSB7CiAg ICAgICAgICBlLnN0eWxlLmRpc3BsYXkgPSAiIjsKICAgICAgICAgIGYuZGlzYWJsZWQgPSAiIjsK ICAgICAgICAgIGRpc2FibGVFbGVtZW50KCJ0aGlyZCIsIGZhbHNlKTsKICAgICAgICAgIGRpc2Fi bGVFbGVtZW50KCJwZCIsIGZhbHNlKTsKICAgICAgICB9CiAgICAgIH0KICAgICAgCiAgICAgIAog ICAgZnVuY3Rpb24gZGlzYWJsZUVsZW1lbnQob2JqSWQsIGRpc2FibGUpIAogICAgeyAKICAgICAg ICB2YXIgb2JqID0gZG9jdW1lbnQuZ2V0RWxlbWVudEJ5SWQob2JqSWQpOyAKICAgICAgICBpZiAo b2JqID09IG51bGwpIAogICAgICAgICAgICByZXR1cm47CiAgICAgICAgaWYoZGlzYWJsZSkgeyAK ICAgICAgICAgICAgdmFyIGhyZWYgPSBvYmouZ2V0QXR0cmlidXRlKCJocmVmIik7IAogICAgICAg ICAgICBpZiAoaHJlZiAmJiBocmVmICE9ICIiICYmIGhyZWYgIT0gbnVsbCkgewogICAgICAgICAg ICAgICAgb2JqLnNldEF0dHJpYnV0ZSgnaHJlZl9iYWsnLCBocmVmKTsgCiAgICAgICAgICAgICAg ICBvYmoucmVtb3ZlQXR0cmlidXRlKCdocmVmJyk7IAogICAgICAgICAgICB9IAogICAgICAgICAg ICB2YXIgb25jbGljayA9IG9iai5nZXRBdHRyaWJ1dGUoIm9uY2xpY2siKTsgCiAgICAgICAgICAg IGlmIChvbmNsaWNrICE9IG51bGwpIHsgCiAgICAgICAgICAgICAgICBvYmouc2V0QXR0cmlidXRl KCdvbmNsaWNrX2JhY2snLCBvbmNsaWNrKTsgCiAgICAgICAgICAgICAgICBvYmoucmVtb3ZlQXR0 cmlidXRlKCdvbmNsaWNrJyk7IAogICAgICAgICAgICB9IAogICAgICAgICAgICBvYmouc3R5bGUu Y29sb3I9ImdyYXkiOyAKICAgICAgICB9IGVsc2UgeyAKICAgICAgICAgICAgdmFyIG9uY2xpY2tC YWNrID0gb2JqLmdldEF0dHJpYnV0ZSgib25jbGlja19iYWNrIik7IAogICAgICAgICAgICBpZiAo b25jbGlja0JhY2sgIT0gbnVsbCkgIHsgCiAgICAgICAgICAgICAgICBvYmouc2V0QXR0cmlidXRl KCdvbmNsaWNrJywgb25jbGlja0JhY2spOyAKICAgICAgICAgICAgICAgIG9iai5yZW1vdmVBdHRy aWJ1dGUoJ29uY2xpY2tfYmFjaycpOyAKICAgICAgICAgICAgICAgIG9iai5zdHlsZS5jb2xvcj0i YmxhY2siOyAKICAgICAgICAgICAgfSAKICAgICAgICAgICAgdmFyIGhyZWZCYWNrID0gb2JqLmdl dEF0dHJpYnV0ZSgiaHJlZl9iYWsiKTsgCiAgICAgICAgICAgIGlmIChocmVmQmFjayAhPSBudWxs KSB7IAogICAgICAgICAgICAgICAgb2JqLnNldEF0dHJpYnV0ZSgnaHJlZicsIGhyZWZCYWNrKTsg CiAgICAgICAgICAgICAgICBvYmoucmVtb3ZlQXR0cmlidXRlKCdocmVmX2JhaycpOyAKICAgICAg ICAgICAgICAgIG9iai5zdHlsZS5jb2xvcj0iYmx1ZSI7IAogICAgICAgICAgICB9CiAgICAgICAg fSAKICAgIH0gCiAgICA8L3NjcmlwdD4KICA8L2hlYWQ+CiAgPGJvZHk+CiAgICA8cCBvbmNsaWNr PSJ0b2dnbGUoKSI+Q2xpY2sgbWUhPC9wPgogICAgPHAgaWQ9InBkIiBvbmNsaWNrPSJ0b2dnbGUo KSI+SSBjYW4gYmUgZGlzYWJsZWQhPC9wPgogICAgPHAgc3R5bGU9ImRpc3BsYXk6YmxvY2siIGlk PSJ0b3RvZ2dsZSI+QWxsIHdvcmsgYW5kIG5vIHBsYXkgbWFrZXMgSmFjayBhIGR1bGwgYm95Ljwv cD4KICAgIDxwPjxhIGlkPSJzZWNvbmQiIGhyZWY9Imh0dHA6Ly93d3cuZ29vZ2xlLmNvbS9tL3Vy bD9laT1ERGIzU01DWk51RDQ4SkFLMXNTc0xRJnE9aHR0cCUzQSUyRiUyRnd3dy55b3V0dWJlLmNv bSUyRndhdGNoJTNGdiUzREJrcDliREdEZDF3JnVzZz1BRlFqQ05FV0NkR0hweHUxZWU1a2xWcjdW YVp3c3FJc3BRIj5SZWRpcmVjdGVkIFlvdVR1YmUgbGluazwvYT48L3A+CiAgICA8cD48YSBpZD0i dGhpcmQiIGRpc2FibGVkPSJkaXNhYmxlZCIgaHJlZj0iaHR0cDovL3d3dy5nb29nbGUuY29tL20v dXJsP2VpPUREYjNTTUNaTnVENDhKQUsxc1NzTFEmcT1odHRwJTNBJTJGJTJGd3d3LnlvdXR1YmUu Y29tJTJGd2F0Y2glM0Z2JTNEQmtwOWJER0RkMXcmdXNnPUFGUWpDTkVXQ2RHSHB4dTFlZTVrbFZy N1ZhWndzcUlzcFEiPlJlZGlyZWN0ZWQgWW91VHViZSBsaW5rPC9hPjwvcD4KICAgIDxwPjxhIGhy ZWY9Imh0dHA6Ly93d3cueW91dHViZS5jb20vd2F0Y2g/dj1Ca3A5YkRHRGQxdyI+RGlyZWN0IFlv dVR1YmUgbGluazwvYT48L3A+CiAgPC9ib2R5Pgo8L2h0bWw+CgogICAgICAK 26710 2009-01-14 08:26:07 -0800 2010-06-10 19:06:57 -0700 proof of concept patch -- not intended to be landed patchForBug23318.txt text/plain 1342 caryclark SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiAzOTg4OSkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTUgQEAKKzIwMDktMDEtMTQgIENhcnkgQ2xhcmsgIDxjYXJ5Y2xhcmtAZ29vZ2xl LmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjMzMTgKKyAgICAgICAgVGhp cyBmaXggc29sdmVzIGEgcHJvYmxlbSBmb3IgQW5kcm9pZCBieSByZW1vdmluZyB0aGUgZXZlbnQg bGlzdGVuZXIKKyAgICAgICAgd2hlbiB0aGUgY29ycmVzcG9uZGluZyBhdHRyaWJ1dGUgaXMgcmVt b3ZlZCAtLSBidXQgb25seSB0byBkZW1vbnN0cmF0ZQorICAgICAgICB0aGF0IHRoZSBidWcgY2Fu IGJlIGZpeGVkLiBXaGF0J3MgdGhlIGNvcnJlY3Qgd2F5IHRvIGZpeCB0aGlzPworCisgICAgICAg ICogZG9tL0VsZW1lbnQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6RWxlbWVudDo6cmVtb3ZlQXR0 cmlidXRlKToKKwogMjAwOS0wMS0xNCAgQWxleGV5IFByb3NrdXJ5YWtvdiAgPGFwQHdlYmtpdC5v cmc+CiAKICAgICAgICAgUmVsZWFzZSBidWlsZCBmaXguCkluZGV4OiBXZWJDb3JlL2RvbS9FbGVt ZW50LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJDb3JlL2RvbS9FbGVtZW50LmNwcAkocmV2aXNpb24g Mzk4ODkpCisrKyBXZWJDb3JlL2RvbS9FbGVtZW50LmNwcAkod29ya2luZyBjb3B5KQpAQCAtMTAx Nyw2ICsxMDE3LDEwIEBAIHZvaWQgRWxlbWVudDo6cmVtb3ZlQXR0cmlidXRlKGNvbnN0IFN0cmkK ICAgICAgICAgaWYgKGVjID09IE5PVF9GT1VORF9FUlIpCiAgICAgICAgICAgICBlYyA9IDA7CiAg ICAgfQorICAgIGlmIChpc0V2ZW50VGFyZ2V0Tm9kZSgpICYmIGxvY2FsTmFtZS5sZWZ0KDIpID09 ICJvbiIpIHsKKyAgICAgICAgU3RyaW5nIGV2ZW50TmFtZSA9IGxvY2FsTmFtZS5zdWJzdHJpbmco Mik7CisgICAgICAgICgoRXZlbnRUYXJnZXROb2RlKikgdGhpcyktPnJlbW92ZUlubGluZUV2ZW50 TGlzdGVuZXJGb3JUeXBlKGV2ZW50TmFtZSk7CisgICAgfQogfQogCiB2b2lkIEVsZW1lbnQ6OnJl bW92ZUF0dHJpYnV0ZU5TKGNvbnN0IFN0cmluZyYgbmFtZXNwYWNlVVJJLCBjb25zdCBTdHJpbmcm IGxvY2FsTmFtZSwgRXhjZXB0aW9uQ29kZSYgZWMpCg==