angular / components

Component infrastructure and Material Design components for Angular
https://material.angular.io
MIT License
24.32k stars 6.72k forks source link

Sidenav issues in safari with Breakpoint Observer. #12479

Open Trent-Matthias opened 6 years ago

Trent-Matthias commented 6 years ago

Bug, feature request, or proposal:

Bug

What is the expected behavior?

For the sidenav to properly respond to the breakpoint observer. It should hide the toggle sidenav button in the toolbar when the breakpoint is false.

What is the current behavior?

Does not hide the toggle sidenav button in Safari only.

What are the steps to reproduce?

Providing a StackBlitz reproduction is the best way to share your issue.
I wanted to update this with 2 StackBlitzs that show the bug happening in safari and the fix.

Here is the StackBlitz with Angular updated to version 7 and using the Breakpoint Observer.

Here is the workaround I found using flex-layout.

The only difference in the code is the observable implementation. The flex-layout one works in safari while the breakpoint observer seems to still be broken.

Simply generate a new project using the cli. run ng add @angular/material run npm install @angular/cdk run ng generate @angular/material:material-nav --name side-navigation add <app-side-navigation></app-side-navigation> to app.component.html run the app in safari and change the screen size to trigger the breakpoint observer

What is the use-case or motivation for changing an existing behavior?

This is just broken in Safari as far as I can tell.

Which versions of Angular, Material, OS, TypeScript, browsers are affected?

Latest versions as of now.

Angular: 7.1.3, Material: 7.1.1, Mac OS Sierra 10.12.6, TypeScript: ~3.1.6

Tested in Safari 11.0.2

Is there anything else we should know?

Was throwing this error on my larger project:

Error: ExpressionChangedAfterItHasBeenCheckedError: Expression has changed after it was checked. Previous value: 'mat-drawer-shown: true'. Current value: 'mat-drawer-shown: false'

This error doesn't usually get thrown with the above implementation.

The easiest way to see the bug is by paying attention to the button that opens the drawer. It should disappear when the breakpoint is not triggered and appear when the drawer is closed. Depending on the window size when first loading the app, the button may never appear or never disappear, whichever state it starts in.

RenatoFranca commented 6 years ago

I was having the same problem. Here is the solution: https://blog.angularindepth.com/everything-you-need-to-know-about-the-expressionchangedafterithasbeencheckederror-error-e3fd9ce7dbb4

Now I'm using setTimeout:

breakpointObserver.observe(Breakpoints.Handset).subscribe(result => {
  if (result.matches) {
    setTimeout(() => {
      // content
    });
  }
});

In the end, this is not a bug. It's just the right behavior of Angular.

mmalerba commented 6 years ago

This appears to be something wrong in Angular animations. I made a repro that shows the issue still happening after removing the styles and start/end listeners from the animation, then working properly after I remove the animation completely. https://github.com/mmalerba/material2/tree/12479-repro @matsko

Trent-Matthias commented 6 years ago

An update on this, I believe the issue is in the BreakpointObserver. The new StackBlitz I created uses the flex layout API instead to get media changes. I push that result to the isHandset$ Observable and without changing the template it works in Safari. @mmalerba

After trying this out in my larger project the error in my first post still gets thrown, but everything seems to work despite that (the button appears and toggles the sidenav). The error only gets thrown when the drawer opens after a breakpoint check.

https://stackblitz.com/edit/angular-material2-issue-uulez4

jbojcic1 commented 5 years ago

I also think the issue is with BreakPointObserver. I don't use sidenav at all and have this problem. Using setTimeout like @RenatoFranca has suggested fixed it for me. I guess it happens only on Safari due to the differences in handling micro and macro tasks.

Trent-Matthias commented 5 years ago

Work around for ExpressionChangedAfterItHasBeenCheckedError for async pipe usage is to add tap(() => this.changeDetectorRef.detectChanges()) after returning the media query result. Would rather not have to mark for change detection though (and only for Safari), but it at least fixes my issue.

My whole observable composed with rxjs looks like this

private _isHandset$: Observable<boolean> = this.media.asObservable().pipe( map( () => this.media.isActive('xs') || this.media.isActive('sm') || this.media.isActive('lt-md') ), tap(() => this.changeDetectorRef.detectChanges()), distinctUntilChanged(), takeUntil(this.destroy$) );

Trent-Matthias commented 5 years ago

I wanted to update this with 2 stackblitzs that show the bug happening in safari and the fix.

Here is the stackblitz with Angular updated to version 7 and using the Breakpoint Observer.

Here is the workaround I found using flex-layout.

The only difference in the code is the observable implementation. The flex-layout one works in safari while the breakpoint observer seems to still be broken.

I've updated the issue with this information, I believe the problem is in the breakpoint observer implementation.