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

Fixes MdSidenav issue when closing a sidenav with [opened]="true" #1383

Closed
wants to merge 1 commit into from

Conversation

dima-gusyatiner
Copy link

Fixes MdSidenav issue with sidenav opened as default.
Setting the attribute on the html component creates _openPromise via toggle(), which is never resolved, like it's usually done with with _onTransitionEnd (issue #1382).

The reason is when MdSidenav component is initialized:

@Input()
  get opened(): boolean { return this._opened; }
  set opened(v: boolean) {
    // TODO(jelbourn): this coercion goes away when BooleanFieldValue is removed.
    let booleanValue = v != null && `${v}` !== 'false';
    this.toggle(booleanValue);
  } 

toggle() method creates _openPromiseReject and _openPromiseReject:

    if (isOpen) {
      if (this._openPromise == null) {
        this._openPromise = new Promise<void>((resolve, reject) => {
          this._openPromiseResolve = resolve;
          this._openPromiseReject = reject;
        });
      }
      return this._openPromise;
    } else {
      if (this._closePromise == null) {
        this._closePromise = new Promise<void>((resolve, reject) => {
          this._closePromiseResolve = resolve;
          this._closePromiseReject = reject;
        });
      }
      return this._closePromise;
    }

They are usually resolved at _onTransitionEnd, but since no transition occurs on application start, this method never gets called. When we click the close button, it rejects the open promise.

  _onTransitionEnd(transitionEvent: TransitionEvent) {
    if (transitionEvent.target == this._elementRef.nativeElement
        // Simpler version to check for prefixes.
        && transitionEvent.propertyName.endsWith('transform')) {
      this._transition = false;
      if (this._opened) {
        if (this._openPromise != null) {
          this._openPromiseResolve();
        }
        if (this._closePromise != null) {
          this._closePromiseReject();
        }

        this.onOpen.emit(null);
      } else {
        if (this._closePromise != null) {
          this._closePromiseResolve();
        }
        if (this._openPromise != null) {
          this._openPromiseReject();
        }

        this.onClose.emit(null);
      }

      this._openPromise = null;
      this._closePromise = null;
    }
  }

My current solution is to clear the promises when setting the propery:

  @Input()
  get opened(): boolean { return this._opened; }
  set opened(v: boolean) {
    // TODO(jelbourn): this coercion goes away when BooleanFieldValue is removed.
    let booleanValue = v != null && `${v}` !== 'false';
    this.toggle(booleanValue);

    this._openPromise = null;
    this._closePromise = null;
  }

…ribute on the html component creates _openPromise via toggle(), which is never resolved, like it's usually done with with _onTransitionEnd (issue angular#1382)
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@googlebot googlebot added the cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla label Sep 30, 2016
@dima-gusyatiner
Copy link
Author

I signed in!

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Sep 30, 2016
@dima-gusyatiner dima-gusyatiner changed the title Fixes MdSidenav issue closing sidenav opened as default Fixes MdSidenav issue when closing a sidenav with [opened]="true" Sep 30, 2016
@hansl
Copy link
Contributor

hansl commented Nov 1, 2016

This will introduce a bug and is unsafe. The bug is that when calling open twice the wrong promise will be returned, but never resolved.

I fixed this in a separate PR #1666 and added a unit test for avoiding a future regression.

Thanks!

@hansl hansl closed this Nov 1, 2016
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants