Skip to content

Commit

Permalink
fix(core): should fake a top event task when coalescing events to pre…
Browse files Browse the repository at this point in the history
…vent draining microTaskQueue too early. (#36841)

Close #36839.

This is a known issue of zone.js,

```
(window as any)[(Zone as any).__symbol__('setTimeout')](() => {
  let log = '';
  button.addEventListener('click', () => {
    Zone.current.scheduleMicroTask('test', () => log += 'microtask;');
    log += 'click;';
  });
  button.click();
  expect(log).toEqual('click;microtask;');
  done();
});
```

Since in this case, we use native `setTimeout` which is not a ZoneTask,
so zone.js consider the button click handler as the top Task then drain the
microTaskQueue after the click at once, which is not correct(too early).

This case was an edge case and not reported by the users, until we have the
new option ngZoneEventCoalescing, since the event coalescing will happen
in native requestAnimationFrame, so it will not be a ZoneTask, and zone.js will
consider any Task happen in the change detection stage as the top task, and if
there are any microTasks(such as Promise.then) happen in the process, it may be
drained earlier than it should be, so to prevent this situation, we need to schedule
a fake event task and run the change detection check in this fake event task,
so the Task happen in the change detection stage will not be
considered as top ZoneTask.

PR Close #36841
  • Loading branch information
JiaLiPassion authored and subratpalhar92 committed Aug 15, 2020
1 parent a1aa091 commit 448fb2a
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 3 deletions.
24 changes: 21 additions & 3 deletions packages/core/src/zone/ng_zone.ts
Expand Up @@ -243,6 +243,10 @@ interface NgZonePrivate extends NgZone {
isStable: boolean;
shouldCoalesceEventChangeDetection: boolean;
nativeRequestAnimationFrame: (callback: FrameRequestCallback) => number;

// Cache of "fake" top eventTask. This is done so that we don't need to schedule a new task every
// time we want to run a `checkStable`.
fakeTopEventTask: Task;
}

function checkStable(zone: NgZonePrivate) {
Expand All @@ -268,9 +272,23 @@ function delayChangeDetectionForEvents(zone: NgZonePrivate) {
return;
}
zone.lastRequestAnimationFrameId = zone.nativeRequestAnimationFrame.call(global, () => {
zone.lastRequestAnimationFrameId = -1;
updateMicroTaskStatus(zone);
checkStable(zone);
// This is a work around for https://github.com/angular/angular/issues/36839.
// The core issue is that when event coalescing is enabled it is possible for microtasks
// to get flushed too early (As is the case with `Promise.then`) between the
// coalescing eventTasks.
//
// To workaround this we schedule a "fake" eventTask before we process the
// coalescing eventTasks. The benefit of this is that the "fake" container eventTask
// will prevent the microtasks queue from getting drained in between the coalescing
// eventTask execution.
if (!zone.fakeTopEventTask) {
zone.fakeTopEventTask = Zone.root.scheduleEventTask('fakeTopEventTask', () => {
zone.lastRequestAnimationFrameId = -1;
updateMicroTaskStatus(zone);
checkStable(zone);
}, undefined, () => {}, () => {});
}
zone.fakeTopEventTask.invoke();
});
updateMicroTaskStatus(zone);
}
Expand Down
46 changes: 46 additions & 0 deletions packages/platform-browser/test/dom/events/event_manager_spec.ts
Expand Up @@ -370,6 +370,52 @@ describe('EventManager', () => {
done();
});
});

it('should not drain micro tasks queue too early with shouldCoalesceEventChangeDetection=true',
(done: DoneFn) => {
doc = getDOM().supportsDOMEvents() ? document : getDOM().createHtmlDocument();
zone = new NgZone({shouldCoalesceEventChangeDetection: true});
domEventPlugin = new DomEventsPlugin(doc);
const element = el('<div></div>');
const child = el('<div></div>');
doc.body.appendChild(element);
const dispatchedClickEvent = createMouseEvent('click');
const dispatchedBlurEvent: FocusEvent =
getDOM().getDefaultDocument().createEvent('FocusEvent');
dispatchedBlurEvent.initEvent('blur', true, true);
let logs: any = [];
const handler = () => {};

const blurHandler = (e: any) => {
logs.push('blur');
};
const manager = new EventManager([domEventPlugin], zone);
let removerParent: any;
let removerChildFocus: any;

zone.run(() => {
removerParent = manager.addEventListener(element, 'click', handler);
removerChildFocus = manager.addEventListener(child, 'blur', blurHandler);
});
const sub = zone.onStable.subscribe(() => {
logs.push('begin');
Promise.resolve().then(() => {
logs.push('promise resolved');
});
element.appendChild(child);
getDOM().dispatchEvent(child, dispatchedBlurEvent);
sub.unsubscribe();
logs.push('end');
});
getDOM().dispatchEvent(element, dispatchedClickEvent);
requestAnimationFrame(() => {
expect(logs).toEqual(['begin', 'blur', 'end', 'promise resolved']);

removerParent && removerParent();
removerChildFocus && removerChildFocus();
done();
});
});
});
})();

Expand Down

0 comments on commit 448fb2a

Please sign in to comment.