Skip to content

Commit

Permalink
fix(nav): avoid doing double reflow for fade in transition (#3936)
Browse files Browse the repository at this point in the history
Double reflow issue caused styles + layout calculation with tab pane hidden (without `.active`).
This caused window scroll positition adjustment as if tab pane wasn't there.

```
Before:
- reflow() in ngbRunTransition
- add('.active') in ngbNavFadeInTransition
- reflow() in ngbNavFadeInTransition
- add('.show') in ngbNavFadeInTransition

After:
- add('.active') in NgbNavOutlet
- reflow() in ngbRunTransition
- add('.show') in ngbNavFadeInTransition

```

Fixes #3900
  • Loading branch information
maxokorokov committed Jan 12, 2021
1 parent 50a0ea1 commit 76e38ee
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 11 deletions.
8 changes: 5 additions & 3 deletions src/nav/nav-outlet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
} from '@angular/core';
import {distinctUntilChanged, skip, startWith, takeUntil} from 'rxjs/operators';

import {ngbNavFadeInNoReflowTransition, ngbNavFadeInTransition, ngbNavFadeOutTransition} from './nav-transition';
import {ngbNavFadeInTransition, ngbNavFadeOutTransition} from './nav-transition';
import {ngbRunTransition, NgbTransitionOptions} from '../util/transition/ngbTransition';
import {NgbNav, NgbNavItem} from './nav';

Expand Down Expand Up @@ -98,8 +98,10 @@ export class NgbNavOutlet implements AfterViewInit {

// fading in
if (this._activePane) {
const fadeInTransition = this.nav.animation ? ngbNavFadeInTransition : ngbNavFadeInNoReflowTransition;
ngbRunTransition(this._activePane.elRef.nativeElement, fadeInTransition, options).subscribe(() => {
// we have to add the '.active' class before running the transition,
// because it should be in place before `ngbRunTransition` does `reflow()`
this._activePane.elRef.nativeElement.classList.add('active');
ngbRunTransition(this._activePane.elRef.nativeElement, ngbNavFadeInTransition, options).subscribe(() => {
if (nextItem) {
nextItem.shown.emit();
this.nav.shown.emit(nextItem.id);
Expand Down
8 changes: 0 additions & 8 deletions src/nav/nav-transition.ts
Original file line number Diff line number Diff line change
@@ -1,18 +1,10 @@
import {NgbTransitionStartFn} from '../util/transition/ngbTransition';
import {reflow} from '../util/util';

export const ngbNavFadeOutTransition: NgbTransitionStartFn = ({classList}) => {
classList.remove('show');
return () => classList.remove('active');
};

export const ngbNavFadeInTransition: NgbTransitionStartFn = (element: HTMLElement) => {
element.classList.add('active');
reflow(element);
element.classList.add('show');
};

export const ngbNavFadeInNoReflowTransition: NgbTransitionStartFn = (element: HTMLElement) => {
element.classList.add('active');
element.classList.add('show');
};
31 changes: 31 additions & 0 deletions src/nav/nav.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -685,6 +685,37 @@ describe('nav', () => {
});
});

it(`should not change container scroll position after switching navs`, () => {
const fixture = createTestComponent(`
<div class="container" style="overflow: scroll; height: 5rem; border: 1px solid gray; padding-top: 2rem;">
<ul ngbNav #n="ngbNav" class="nav-tabs">
<li ngbNavItem>
<a ngbNavLink>link 1</a>
<ng-template ngbNavContent>content 1</ng-template>
</li>
<li ngbNavItem>
<a ngbNavLink>link 2</a>
<ng-template ngbNavContent>content 2</ng-template>
</li>
</ul>
<div [ngbNavOutlet]="n"></div>
</div>
`);

const links = getLinks(fixture);
const container = fixture.nativeElement.querySelector('.container');

// scroll to bottom
container.scrollTop = container.scrollHeight;
const {scrollTop} = container;
expect(container.scrollTop).toBe(scrollTop);

// staying at the same position after changing the nav
links[1].click();
fixture.detectChanges();
expect(container.scrollTop).toBe(scrollTop);
});

describe(`(navChange) preventDefault()`, () => {

let fixture: ComponentFixture<TestComponent>;
Expand Down

0 comments on commit 76e38ee

Please sign in to comment.