ng-matero / extensions

Angular Material Extensions Library.
https://ng-matero.github.io/extensions/
MIT License
393 stars 48 forks source link

[datetimepicker] on closing datetimepicker with onPush changeDetection is not detected #144

Closed JelleBruisten closed 1 year ago

JelleBruisten commented 1 year ago

I noticed that when using the changeDetection strategy 'onPush' whenever the datetimepicker is closed it is not detected, therefore the:

<mtx-datetimepicker-toggle [for]="datetimePicker2" matSuffix></mtx-datetimepicker-toggle>

Would still have the color when it was opened, when you do another click in the same component somewhere maybe on a button the change would be detected. I tracked this down due to:

  close(): void {
    if (!this._opened) {
      return;
    }

    if (this._componentRef) {
      this._destroyOverlay();
    }

    const completeClose = () => {
      // The `_opened` could've been reset already if
      // we got two events in quick succession.
      if (this._opened) {
        this._opened = false;
        this.closedStream.emit();
        this._focusedElementBeforeOpen = null;
      }
    };

    if (
      this._restoreFocus &&
      this._focusedElementBeforeOpen &&
      typeof this._focusedElementBeforeOpen.focus === 'function'
    ) {
      // Because IE moves focus asynchronously, we can't count on it being restored before we've
      // marked the datetimepicker as closed. If the event fires out of sequence and the element
      // that we're refocusing opens the datetimepicker on focus, the user could be stuck with not
      // being able to close the calendar at all. We work around it by making the logic, that marks
      // the datetimepicker as closed, async as well.
      this._focusedElementBeforeOpen.focus();
      setTimeout(completeClose);
    } else {
      completeClose();
    }
  }

the setTimeout would make the completeClose to be run in a new macrotask, but changeDetection would already have been run and therefore would not be picked up. A small workaround for now is setting the

[restoreFocus]="false"

on the datetimepicker or using the closed event:

<mtx-datetimepicker #datetimePicker
  [type]="'datetime'"
  [startView]="'clock'"
  [timeInterval]="5"
  (closed)="cdr.markForCheck()"
></mtx-datetimepicker>

to explicity call changeDetection when the datetimepicker has closed

JelleBruisten commented 1 year ago

@nzbin Do we want to support IE? seeing how angular too already dropped support for this.

Personally I don't need this fixed, seeing how there is a work around.

nzbin commented 1 year ago

Maybe you can try the 12.10.0 of extensions, it can support IE 11 and the features are same with v14.

JelleBruisten commented 1 year ago

@nzbin I mean related to this bug, where we currently have a fix specific for ie that does block something with onPush strategy while current version of angular does not support ie anymore.

project where I am working on that is gonna be using this datetime picker is on latest version of angular so no need to have support for ie myself, just wondering if we should keep specific ie code.

nzbin commented 1 year ago

I'm sorry, I misunderstood. There has also some compatible codes for IE in the Material library. Maybe I should fix this issue.

nzbin commented 1 year ago

14.5.0 has fixed this issue, please have a try.