Bug 23318 – Click event listener remains registered after onclick attribute is removed

• Component: DOM
• Status: NEW

[Cary Clark]

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?

[Cary Clark]

Created attachment 26709 [details]
demonstration of bug

[Cary Clark]

Created attachment 26710 [details]
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?

[Darin Adler]

Comment on attachment 26710 [details]
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

[Darin Adler]

(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.

[Darin Adler]

(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.

[Alexey Proskuryakov]

See also: bug 25130.

[Alexey Proskuryakov]

Looks like this issue only affects Android currently. But once Web Inspector begins to show registered event listeners, it will likely be affected.