# Event Bubbling Change Detection Coalescing
by Miško Hevery & Jia Li (inspired by Igor Minar)

## Overview

Assume that you have Angular template such as this:

<div (click)="doSomethingElse()">
  <button (click)="doSomething()">click me</button>
</div>

It turns out that as the `click` event bubbles from the `<button>` towards the root document it triggers change detection on each listener. This [example](https://stackblitz.com/edit/angular-kqjg8v) shows the phenomena in interactive form. Notice that clicking on the `<button>` will run the change detection twice. Once for the `<button>` and once for the `<div>`.

This is a common scenario especially for applications which use Material Components, because the Material Components often need to register events on interactive elements to update their internal state. Additionally the developer of the application also registers the event listener to perform application logic.

<input (keyup)="logic()" (input)="otherLogic()">

## Proposed Change

The proposal is to coalesce the change-detection as the event is bubbling and only execute one change detection for all of the events.

Technically this is a breaking change, however, we don't think there are many applications which rely on this behavior and so the coalescing should not have behavior impact on most of them. (Other than making them faster.) We propose that we introduce this change in `google3` and then depending on the impact make this either an opt-in or opt-out feature.

## Approach

The first thing we need to do is detect when the last listener has executed. One way to do this would be to put a global listener at the root of the application. Unfortunately this approach is error prone as there could be other listener which could cancel the bubbling and we could be in a detached tree. (This listener do not have to be registered in our current zone and so there is no easy way to know about them)

A more promising approach is to try to use `Promise.resolve()`, `requestAnimationFrame` and `setTimeout`. An example of each of the approaches is demonstrated in [here](https://stackblitz.com/edit/js-d84pnm)<!-- old link https://stackblitz.com/edit/js-ssfih6-->.

|Approach | Notes |
|:----------------------|:------ |
|`Promise.resolve()` | The promise resolves before the next listener is executed. |
|`setTimeout` | The `setTimeout` is executed after all of the event listeners are executed, but after `requestAnimationFrame`. |
|`requestAnimationFrame`| Executed after all of the event listener and before `setTimeout`. |

From the above table it seems that `requestAnimationFrame` is the best way to coalesce multiple event listeners into singe change detection. However, `requestAnimationFrame` will not work for tests because `element.click()` will expect the change detection to be synchronous.

### Hybrid Approach

Perhaps best way is to have a hybrid approach with `requestAnimationFrame` fallback. Here is the algorithm.
1. When the listener executes the first thing it does is to call `maybeDelayChangeDetection()` as described later.
2. `maybeDelayChangeDetection()` does three things.
- It walks to the parent most DOM element and registers a listener of the same type at that place. (This is where the bubbling ends). When this listener triggers we know it is time to run the change-detection.
- For the listeners which cancel the bubbling we do change detection right there.
- Just in case there was a listener which canceled the event and we were not aware of it, we can also register `requestAnimationFrame` as a fallback method.

## Implementation

The best place to hook into the event listeners is in the [`decoratePreventDefault`](https://github.com/angular/angular/blob/cb6dea473f7abaf1925ea34a3dfdbbfe36465689/packages/platform-browser/src/dom/dom_renderer.ts#L50-L59)

function decoratePreventDefault(eventHandler: Function): Function {
  return (event: any) => {
    // START: INSERT THIS CODE
    if (typeof Zone !== 'undefined') {
      const maybeDelayChangeDetection = Zone.current
                .get('maybeDelayChangeDetection');
      maybeDelayChangeDetection && maybeDelayChangeDetection();
    }
    // END: INSERT THIS CODE
    const allowDefaultBehavior = eventHandler(event);
    if (allowDefaultBehavior === false) {
      // TODO(tbosch): move preventDefault into event plugins...
      event.preventDefault();
      event.returnValue = false;
    }
  };
}

:::info
> [name=Jia Li] I checked the logic, `decoratePreventDefault` will only be called when user use `@Output`, but if user call `domElement.addEventListener`,
it this `decoratePreventDefault` will not be called, instead, the `DomEventsPlugin.addEventListener` will be called, so I think maybe we should add the logic there.
> [name=Miško Hevery] Sounds reasonable to me
:::

addEventListener(element: HTMLElement, eventName: string, handler: Function): Function {
  let callback = function(evt: Event) {
    // START: INSERT THIS CODE
    if (typeof Zone !== 'undefined') {
      const maybeDelayChangeDetection = Zone.current
                .get('maybeDelayChangeDetection');
      maybeDelayChangeDetection && maybeDelayChangeDetection();
    }
    // END: INSERT THIS CODE
    return handler.call(this, evt);
  }
  element.addEventListener(eventName, callback);
  return () => removeEventListener(element, eventName, callback);
}

:::info
> [name=Jia Li] and when I make the implementation in `DomEventsPlugin`, several test cases will fail, such as

:::

it ('should only successfully add one handler when try to add the same handler twice', () => {
  const handler = jasmine.createSpy();
  button.addEventListener('click', handler);
  button.addEventListener('click', handler);
  dispatchEvent(clickEvt);
  expect(handler.calls.count()).toBe(1);
});

This case will fail, because even the `handler` is the same, because it will be wrapped with `maybeDelayChangeDetection` callback when call `addEventListener`,
so from `Browser's perspective`, they are different callbacks. And we can do some tricks to resolve this issue, but the code will not look elegent.

So I think maybe this logic should be added to `NgZone` directly like this.

onInvokeTask: (delegate: ZoneDelegate, current: Zone, target: Zone, task: Task, applyThis: any,
                   applyArgs: any): any => {
      try {
        onEnter(zone);
        if (maybeDelayChangeDetection && task.type === 'eventTask') {
          maybeDelayChangeDetection();
        }
        return delegate.invokeTask(target, task, applyThis, applyArgs);
      } finally {
        onLeave(zone);
      }
    },

:::info
> [name=Misko Hevery]
> - How will the canceling of events be handled?
> - This seems like a good place because it would also be a good place for tests.
:::

We can then change the `NgZone` [implementation](https://github.com/angular/angular/blob/cb6dea473f7abaf1925ea34a3dfdbbfe36465689/packages/core/src/zone/ng_zone.ts#L254-L257) like so:

function forkInnerZoneWithAngularBehavior(zone: NgZonePrivate) {
  zone._inner = zone._inner.fork({
    name: 'angular',
    properties: <any>{
      'isAngularZone': true,
      'maybeDelayChangeDetection': shouldCoalesceEventChangeDetection 
          && delayChangeDetectionForEvents
    },

Possible implementation of `delayChangeDetectionForEvents`.

let nativeRequestAnimationFrame = requestAnimationFrame;
if (typeof Zone !== 'undefined') {
  // use unpatched version of requestAnimationFrame if possible
  // to avoid another Change detection
  const unpatchedRequestAnimationFrame = global[Zone.__symbol__('requestAnimationFrame')];
  if (unpatchedRequestAnimationFrame) {
    nativeRequestAnimationFrame = unpatchedRequestAnimationFrame;
  }
}
let lastRequestAnimationFrameId = null;
function delayChangeDetectionForEvents() {
  if (lastRequestAnimationFrameId !== null) {
    lastRequestAnimationFrameId = nativeRequestAnimationFrame(() => {
      lastRequestAnimationFrameId = null;
      change detection because the `Zone.js` has wrapped the `addEventListener` callback function with the task block. This behavior needs to be preserved.
      
      There are two ways to preserve it:
      1. We can patch the `click()` (zone); // Try to run change method to flussh change detection.
    });
  }
}

We would also need to update [`checkStable` implementation](https://github.com/angular/angular/blob/cb6dea473f7abaf1925ea34a3dfdbbfe36465689/packages/core/src/zone/ng_zone.ts#L236-L252) like so:

function checkStable(zone: NgZonePrivate) {
  if (zone._nesting == 0 && !zone.hasPendingMicrotasks && !zone.isStable) {
    try {
      zone._nesting++;
      zone.onMicrotaskEmpty.emit(null);
    } finally {
      zone._nesting--;
      if (!zone.hasPendingMicrotasks 
          && lastRequestAnimationFrameId == null) { // ADD THIS LINE
        try {
          zone.runOutsideAngular(() => zone.onStable.emit(null));
        } finally {
          zone.isStable = true;
        }
      }
    }
  }
}

:::info
> [name=Jia Li]: because the `requestAnimationFrame` do the same thing with `scheduleMicroTask` to do the `CD`, so currently my implementation didn't change the `checkStable function`, instead I change the `hasPendingMicrotasks` like this,
:::

get hasPendingMicrotasks(): boolean {
  return this.hasPendingZoneMicrotasks ||
        (this.shouldCoalesceEventChangeDetection && this.lastRequestAnimationFrameId !== -1);
}

Finally we need to update the bootstrap API to allow the application to opt-in/out of this behavior in [`BootstrapOptions`](https://github.com/angular/angular/blob/cb6dea473f7abaf1925ea34a3dfdbbfe36465689/packages/core/src/application_ref.ts#L188-L202).

/**
 * Provides additional options to the bootstraping process.
 *
 *
 */
export interface BootstrapOptions {
  /**
   * Optionally specify which `NgZone` should be used.
   *
   * - Provide your own `NgZone` instance.
   * - `zone.js` - Use default `NgZone` which requires `Zone.js`.
   * - `noop` - Use `NoopNgZone` which does nothing.
   */
  ngZone?: NgZone|'zone.js'|'noop';
  
  // START<<<<<<<<<
  /**
   * Optionally specify if `NgZone` should be configure to coalesce
   * events.
   */
  ngZoneEventCoalescing?: true|false;
  // END<<<<<<<<<<<
}

:::info
> [name=Jia Li], should `ngZoneEventCaolescing` by default `true`? to keep backward compatiblility, should we keep it `false`, and if it is false, we don't need to handle the existing test cases.
> We can start with `true` and see how many apps break in `google3` and based on that we can decide.
:::

## Test Considerations

Currently when tests programmatically fire events they have their change detection executed synchronously.

buttonDomElement.click();

In the above case the `click()` invocation will invoke change detection because the `Zone.js` has wrapped the `addEventListener` callback function with the task block. This behavior needs to be preserved.

There are two ways to preserve it:
1. We can patch the `click()` and `dispatchEvent` method to flush the change detection synchronously in tests.
2. When event happens we can walk the tree and find the highest parent element on which we can attach the same element listener type using `addEventListener` (and auto clean up). When the bubbling event gets there we flush the changed-detection.
3. Can we just add an switch in ngZone to let user be able to enable/disable this coalescing behavior? Such as this,

function forkInnerZoneWithAngularBehavior(zone: NgZonePrivate, shouldCoalesceEventChangeDetection?: boolean, runtimeEnableCoalesceEventChangeDetection?: boolean) {
  zone._inner = zone._inner.fork({
    name: 'angular',
    properties: <any>{
      'isAngularZone': true,
      'runtimeEnableCoalesceEventChangeDetection': {
        'enabled': !!runtimeEnableCoalesceEventChangeDetection
      },
      'maybeDelayChangeDetection': shouldCoalesceEventChangeDetection 
          && delayChangeDetectionForEvents
    },

function decoratePreventDefault(eventHandler: Function): Function {
  return (event: any) => {
    // START: INSERT THIS CODE
    if (typeof Zone !== 'undefined') {
      const maybeDelayChangeDetection = Zone.current
                .get('maybeDelayChangeDetection');
      const runtimeEnableCoalesceEventChangeDetection = Zone.current.get('runtimeEnableCoalesceEventChangeDetection');
      runtimeEnableCoalesceEventChangeDetection && !!runtimeEnableCoalesceEventChangeDetection.enabled && 
      maybeDelayChangeDetection && maybeDelayChangeDetection();
    }

And we can disable this `coalesceEvent` behaviour in two ways,
1. in `TestBed`, we initialize the `ngZone` with `runtimeEnableCoalesceEventChangeDetection = false`
2. Or we can disable the specified case inside `it`

it('test without coalesce event', () => {
  const runtimeEnableCoalesceEventChangeDetection = Zone.current.get('runtimeEnableCoalesceEventChangeDetection'); 
  if (runtimeEnableCoalesceEventChangeDetection) {
    runtimeEnableCoalesceEventChangeDetection.enabled = false;
  }
})

NOTE: In the current API if the test schedules a micro-task `Promise.resolve()` then the change detection will not run because the microtask will prevent it. Not sure how many tests rely on such behavior.

## Multiple Events Consideration

In browser an `<input>` text box will generate `keydown` followed by `keypress`. However when called programatically from tests a `keydown` will not generate `keypress`. I don't think there is anything special we should do in this case. We could try to coalesce `keydown` and `keyup` but getting it working correctly in tests seems like more trouble than it is worth

## Recommendation

Current recommendation is that we use the `requestAnimationFrame` for coalescing browser native events. This will also work with [Multiple Event Consideration](#Multiple-Events-Consideration) use case.

We patch `click()` and `dispatchEvent()` method in `@angular/core/testing` so that any programmatically dispatched events would auto change-detect just as before. (Internally the patch would also have to cancel the `requestAnimationFrame`)

----

## Notes
- All delay code needs to run outside of zone. (ie `requestAnimationFrame.__zone_symbol__requestAnimationFrame`)
- Related document [DOM listeners and change detection](https://docs.google.com/document/d/1w318lOfXnFgCy5WUyt_zlMYrSzRFgrl9f2C28CsrEa8/edit#)
- Related [WIP: performance tuning test by JiaLiPassion · Pull Request #29610 · angular/angular](https://github.com/angular/angular/pull/29610)

## Comments
> [name=Jia Li]
> And I think maybe the following case maybe the rare case(not sure how much `requestAnimationFrame` was used inside `Material`), if inside the `event handlers`, there are `requestAnimationFrame` calls, there will be still another `change detection` being triggered. In your new solution, we have take the `checkStable` code outside of the `eventHandler`, so maybe we can use `ngZone` to monitor if there are any `requestAnimationFrame` calls are `scheduled` during the `bubble phase`, we can `reschedule` those tasks into `<root> Zone`, so only one `change detection` happen finally.
>
> The case looks like:
> ```<div (click)="doSomethingElse()">
> <button (click)="doSomething()">click me</button>
> </div>```
> ```doSomethingElse() {
> requestAnimationFrame(() => {});
> }
> doSomething() {
> requestAnimationFrame(() => {});
> }```

> [name=Miško Hevery] I don't think we should coalesce that as now we would be in the business of coalescing across multiple macro tasks.

---
> [name=Pawel Kozlowski]
> @jialipassion @misko @igorminar glad that we are discussing this, although I wanted to highlight that this is not only about bubbling of the same event - there are more cases where multiple change detections can happen in response to one single action of a user:
>
> One target, different events:
> ```html
> <input (keypress)="noop()" (input)="noop()">
> ```
>
> Multiple targets, same event:
> ```html
> <button (click)="noop()" (document:click)="noop()"></button>
> ```
>
> Multiple targets, different events
> ```html
> <div (focusin)="noop()">
> <input (focus)="noop()">
> </div>
> ```

> [name=Miško Hevery] I don't think we should be doing anything special in this case, see [Multiple Event Consideration](#Multiple-Events-Consideration) section. Those really should be treated as different events for the purposes of testing.