Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Event handlers-created event listener need to be removed sometimes #3836

Closed
TimothyGu opened this issue Jul 21, 2018 · 0 comments
Closed

Event handlers-created event listener need to be removed sometimes #3836

TimothyGu opened this issue Jul 21, 2018 · 0 comments
Labels
interop Implementations are not interoperable with each other topic: events

Comments

@TimothyGu
Copy link
Member

The way event handlers are spec'd right now implies that once an event listener running event handler processing algorithm is added (under the "When an event handler H of an EventTarget object T is first set to a non-null value" steps), it will never get removed. However, this doesn't seem to reflect implementations' actual behavior.

Exhibit A.1:

<button id="test">Start Demo</button>
<script>
 var button = document.getElementById('test');
 button.onclick = function () { alert('ONE'); }; // 1
 button.addEventListener('click', function () { alert('TWO') }, false); // 2
 button.onclick = null; // 3
 button.onclick = function () { alert('ONE'); }; // 4
</script>

Exhibit A.2:

<button id="test">Start Demo</button>
<script>
 var button = document.getElementById('test');
 button.setAttribute("onclick", "alert('ONE');"); // 1
 button.addEventListener('click', function () { alert('TWO') }, false); // 2
 button.removeAttribute("onclick"); // 3
 button.setAttribute("onclick", "alert('ONE');"); // 4
</script>

In both of these cases, one would expect step 1 to register an event listener running the event handler processing algorithm to be registered as the first event listener. That is, after 1 and 2 if the button were to be clicked, "ONE TWO" would result.

After 3, the event handler for the click event would be set to null, but per current spec the event handler processing algorithm should still be the first event listener in the list. After 4, the event handler is restored, so "ONE TWO" would be alerted in order just like after 2.

However, in all four implementations tested (Chrome, Edge, Firefox, Safari) the order is "TWO ONE" for these two test cases.

Exhibit B:

<button id="test">Start Demo</button>
<script>
 var button = document.getElementById('test');
 button.setAttribute("onclick", "alert('ONE');"); // 1
 button.addEventListener('click', function () { alert('TWO') }, false); // 2
 button.onclick = null; // 3
 button.setAttribute("onclick", "alert('ONE');"); // 4
</script>

Similar to above, the spec behavior would be "ONE TWO". However, Chrome, Edge, and Safari all alert "TWO ONE", while Firefox alerts "TWO" only (presumably because of the fact that the second setAttribute didn't actually change the value of the attribute, so Firefox doesn't bother re-compiling the attribute).

These three test cases show that in all implementations step 3 actually removes the event listener, contrary to what the spec claims. We need to fix that.


In particular, Blink's implementation of IDL attribute setter, where the new value is newValue, can be summarized as:

  1. If newValue is null, then:

    1. If an event handler processing algorithm event listener has been registered, then remove that listener.

    2. Return.

  2. Otherwise, if an event handler processing algorithm event listener has been registered, replace the current event handler with newValue, then return.

  3. Otherwise, add an event listener with the event handler processing algorithm as its operation. (Basically the "When an event handler H of an EventTarget object T is first set to a non-null value" steps).

The steps are pretty similar in WebKit.

Compare with current spec:

  1. Set the current event handler to newValue.

  2. If newValue is null, then return.

  3. Otherwise, if an event handler processing algorithm event listener has been registered, then return.

  4. Otherwise, add an event listener with the event handler processing algorithm as its operation. (Basically the "When an event handler H of an EventTarget object T is first set to a non-null value" steps).


Another interesting way to think about this problem is through the lens of document.open(), which as part of its operation removes all event listeners of all nodes, including the ones associated with IDL or content attribute event handlers (in fact that's the only way to remove them, since removeEventListener() can never get to the spec-created event listener). Under the current spec, once the event listener associated with the event handler is removed, it can never be brought back, since the algorithm that adds the listener is only executed "When an event handler H of an EventTarget object T is first set to a non-null value." This doesn't exactly sound like what the developer would expect.

@TimothyGu TimothyGu added topic: events interop Implementations are not interoperable with each other labels Jul 21, 2018
domenic pushed a commit that referenced this issue Aug 7, 2018
* Move the "When an event handler H of an EventTarget object T is first
  set to a non-null value" steps into a dedicated "activate an event
  handler" algorithm. Create a corresponding "deactivate an event
  handler" algorithm, called in the circumstances outlined in #3836,
  thereby fixing #3836.

* Define "When an event handler content attribute is set" and "When an
  event handler content attribute is removed" steps rigorously using
  the attribute change steps hook in DOM. Care is taken to clarify
  certain edge cases, such as calling removeAttribute() on a nonexistent
  event handler content attribute but with a corresponding event handler
  IDL attribute defined, and calling setAttribute() an attribute with
  the same value as it currently possesses but with the event handler
  deactivated (exhibit B in #3836).

* Formalize multiple definitions:
  * event handler as a struct containing a value and an event listener
  * location of event handlers and the concept of event handler target
  * rigorous set of steps on how to obtain the target of an event
    handler through the "determine the target of an event handler"
    algorithm. The latter algorithm supersedes the handwavy and
    inconsistently-applied "If an event handler IDL attribute exposes an
    event handler of an object that doesn't exist" steps.
  * storage of event handlers in the event handler map

* More descriptive variable names: H → eventHandler, T → eventTarget,
  and E → event, in various algorithms.

Tests: web-platform-tests/wpt#12201

Follow-up issue to improve clarity by breaking this up into smaller
sections: #3861.
fserb pushed a commit to fserb/html that referenced this issue Aug 9, 2018
* Move the "When an event handler H of an EventTarget object T is first
  set to a non-null value" steps into a dedicated "activate an event
  handler" algorithm. Create a corresponding "deactivate an event
  handler" algorithm, called in the circumstances outlined in whatwg#3836,
  thereby fixing whatwg#3836.

* Define "When an event handler content attribute is set" and "When an
  event handler content attribute is removed" steps rigorously using
  the attribute change steps hook in DOM. Care is taken to clarify
  certain edge cases, such as calling removeAttribute() on a nonexistent
  event handler content attribute but with a corresponding event handler
  IDL attribute defined, and calling setAttribute() an attribute with
  the same value as it currently possesses but with the event handler
  deactivated (exhibit B in whatwg#3836).

* Formalize multiple definitions:
  * event handler as a struct containing a value and an event listener
  * location of event handlers and the concept of event handler target
  * rigorous set of steps on how to obtain the target of an event
    handler through the "determine the target of an event handler"
    algorithm. The latter algorithm supersedes the handwavy and
    inconsistently-applied "If an event handler IDL attribute exposes an
    event handler of an object that doesn't exist" steps.
  * storage of event handlers in the event handler map

* More descriptive variable names: H → eventHandler, T → eventTarget,
  and E → event, in various algorithms.

Tests: web-platform-tests/wpt#12201

Follow-up issue to improve clarity by breaking this up into smaller
sections: whatwg#3861.
TimothyGu added a commit to TimothyGu/html that referenced this issue Aug 9, 2018
This change aligns the event listener removal behavior with
implementations, specifically in two aspects:

- The event handler's value should be set to null (i.e., deactivated).
- Event listeners and handlers should be removed from the Window object
  as well.

See prior investigation around deactivation of event handlers in whatwg#3836
and whatwg#3850. See investigation around `document.open()`'s behavior in
whatwg#3818.

Tests: web-platform-tests/wpt#12122
domenic pushed a commit that referenced this issue Aug 9, 2018
This change aligns the event listener removal behavior with
implementations, specifically in two aspects:

- The event handler's value should be set to null (i.e., deactivated).
- Event listeners and handlers should be removed from the Window object
  as well.

See prior investigation around deactivation of event handlers in #3836
and #3850. See investigation around document.open()'s behavior in
#3818.

Tests: web-platform-tests/wpt#12122
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
* Move the "When an event handler H of an EventTarget object T is first
  set to a non-null value" steps into a dedicated "activate an event
  handler" algorithm. Create a corresponding "deactivate an event
  handler" algorithm, called in the circumstances outlined in whatwg#3836,
  thereby fixing whatwg#3836.

* Define "When an event handler content attribute is set" and "When an
  event handler content attribute is removed" steps rigorously using
  the attribute change steps hook in DOM. Care is taken to clarify
  certain edge cases, such as calling removeAttribute() on a nonexistent
  event handler content attribute but with a corresponding event handler
  IDL attribute defined, and calling setAttribute() an attribute with
  the same value as it currently possesses but with the event handler
  deactivated (exhibit B in whatwg#3836).

* Formalize multiple definitions:
  * event handler as a struct containing a value and an event listener
  * location of event handlers and the concept of event handler target
  * rigorous set of steps on how to obtain the target of an event
    handler through the "determine the target of an event handler"
    algorithm. The latter algorithm supersedes the handwavy and
    inconsistently-applied "If an event handler IDL attribute exposes an
    event handler of an object that doesn't exist" steps.
  * storage of event handlers in the event handler map

* More descriptive variable names: H → eventHandler, T → eventTarget,
  and E → event, in various algorithms.

Tests: web-platform-tests/wpt#12201

Follow-up issue to improve clarity by breaking this up into smaller
sections: whatwg#3861.
mustaqahmed pushed a commit to mustaqahmed/html that referenced this issue Feb 15, 2019
* Move the "When an event handler H of an EventTarget object T is first
  set to a non-null value" steps into a dedicated "activate an event
  handler" algorithm. Create a corresponding "deactivate an event
  handler" algorithm, called in the circumstances outlined in whatwg#3836,
  thereby fixing whatwg#3836.

* Define "When an event handler content attribute is set" and "When an
  event handler content attribute is removed" steps rigorously using
  the attribute change steps hook in DOM. Care is taken to clarify
  certain edge cases, such as calling removeAttribute() on a nonexistent
  event handler content attribute but with a corresponding event handler
  IDL attribute defined, and calling setAttribute() an attribute with
  the same value as it currently possesses but with the event handler
  deactivated (exhibit B in whatwg#3836).

* Formalize multiple definitions:
  * event handler as a struct containing a value and an event listener
  * location of event handlers and the concept of event handler target
  * rigorous set of steps on how to obtain the target of an event
    handler through the "determine the target of an event handler"
    algorithm. The latter algorithm supersedes the handwavy and
    inconsistently-applied "If an event handler IDL attribute exposes an
    event handler of an object that doesn't exist" steps.
  * storage of event handlers in the event handler map

* More descriptive variable names: H → eventHandler, T → eventTarget,
  and E → event, in various algorithms.

Tests: web-platform-tests/wpt#12201

Follow-up issue to improve clarity by breaking this up into smaller
sections: whatwg#3861.
mustaqahmed pushed a commit to mustaqahmed/html that referenced this issue Feb 15, 2019
This change aligns the event listener removal behavior with
implementations, specifically in two aspects:

- The event handler's value should be set to null (i.e., deactivated).
- Event listeners and handlers should be removed from the Window object
  as well.

See prior investigation around deactivation of event handlers in whatwg#3836
and whatwg#3850. See investigation around document.open()'s behavior in
whatwg#3818.

Tests: web-platform-tests/wpt#12122
mustaqahmed pushed a commit to mustaqahmed/html that referenced this issue Feb 15, 2019
* Move the "When an event handler H of an EventTarget object T is first
  set to a non-null value" steps into a dedicated "activate an event
  handler" algorithm. Create a corresponding "deactivate an event
  handler" algorithm, called in the circumstances outlined in whatwg#3836,
  thereby fixing whatwg#3836.

* Define "When an event handler content attribute is set" and "When an
  event handler content attribute is removed" steps rigorously using
  the attribute change steps hook in DOM. Care is taken to clarify
  certain edge cases, such as calling removeAttribute() on a nonexistent
  event handler content attribute but with a corresponding event handler
  IDL attribute defined, and calling setAttribute() an attribute with
  the same value as it currently possesses but with the event handler
  deactivated (exhibit B in whatwg#3836).

* Formalize multiple definitions:
  * event handler as a struct containing a value and an event listener
  * location of event handlers and the concept of event handler target
  * rigorous set of steps on how to obtain the target of an event
    handler through the "determine the target of an event handler"
    algorithm. The latter algorithm supersedes the handwavy and
    inconsistently-applied "If an event handler IDL attribute exposes an
    event handler of an object that doesn't exist" steps.
  * storage of event handlers in the event handler map

* More descriptive variable names: H → eventHandler, T → eventTarget,
  and E → event, in various algorithms.

Tests: web-platform-tests/wpt#12201

Follow-up issue to improve clarity by breaking this up into smaller
sections: whatwg#3861.
mustaqahmed pushed a commit to mustaqahmed/html that referenced this issue Feb 15, 2019
This change aligns the event listener removal behavior with
implementations, specifically in two aspects:

- The event handler's value should be set to null (i.e., deactivated).
- Event listeners and handlers should be removed from the Window object
  as well.

See prior investigation around deactivation of event handlers in whatwg#3836
and whatwg#3850. See investigation around document.open()'s behavior in
whatwg#3818.

Tests: web-platform-tests/wpt#12122
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interop Implementations are not interoperable with each other topic: events
Development

No branches or pull requests

1 participant