Skip to content

fix(material/sidenav): enable hydration #28340

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 2, 2024
Merged

fix(material/sidenav): enable hydration #28340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d1e7734
build: update angular version
crisbeto Dec 28, 2023
2f187bf
fix(material/sidenav): enable hydration
crisbeto Dec 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 16 additions & 16 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,12 @@
},
"version": "17.1.0-next.3",
"dependencies": {
"@angular/animations": "^17.1.0-next.1",
"@angular/common": "^17.1.0-next.1",
"@angular/compiler": "^17.1.0-next.1",
"@angular/core": "^17.1.0-next.1",
"@angular/forms": "^17.1.0-next.1",
"@angular/platform-browser": "^17.1.0-next.1",
"@angular/animations": "^17.1.0-next.5",
"@angular/common": "^17.1.0-next.5",
"@angular/compiler": "^17.1.0-next.5",
"@angular/core": "^17.1.0-next.5",
"@angular/forms": "^17.1.0-next.5",
"@angular/platform-browser": "^17.1.0-next.5",
"@types/google.maps": "^3.52.4",
"@types/youtube": "^0.0.46",
"rxjs": "^6.6.7",
Expand All @@ -72,18 +72,18 @@
"zone.js": "~0.14.0"
},
"devDependencies": {
"@angular-devkit/build-angular": "^17.1.0-next.0",
"@angular-devkit/core": "^17.1.0-next.0",
"@angular-devkit/schematics": "^17.1.0-next.0",
"@angular-devkit/build-angular": "17.1.0-next.1",
"@angular-devkit/core": "17.1.0-next.1",
"@angular-devkit/schematics": "17.1.0-next.1",
"@angular/bazel": "https://github.com/angular/bazel-builds.git#bac9c1abe1e6ac1801fbbccb53353a1ed7126469",
"@angular/build-tooling": "https://github.com/angular/dev-infra-private-build-tooling-builds.git#3069be882e3e41cdb3dad58788d878e31d7d82e8",
"@angular/cli": "^17.1.0-next.0",
"@angular/compiler-cli": "^17.1.0-next.1",
"@angular/localize": "^17.1.0-next.1",
"@angular/cli": "17.1.0-next.1",
"@angular/compiler-cli": "^17.1.0-next.5",
"@angular/localize": "^17.1.0-next.5",
"@angular/ng-dev": "https://github.com/angular/dev-infra-private-ng-dev-builds.git#6f2e63fcda1f645ba3c6709c14a8417f921f9e5c",
"@angular/platform-browser-dynamic": "^17.1.0-next.1",
"@angular/platform-server": "^17.1.0-next.1",
"@angular/router": "^17.1.0-next.1",
"@angular/platform-browser-dynamic": "^17.1.0-next.5",
"@angular/platform-server": "^17.1.0-next.5",
"@angular/router": "^17.1.0-next.5",
"@babel/core": "^7.16.12",
"@babel/helper-explode-assignable-expression": "^7.18.6",
"@babel/helper-string-parser": "^7.22.5",
Expand Down Expand Up @@ -149,7 +149,7 @@
"@octokit/rest": "18.3.5",
"@rollup/plugin-commonjs": "^21.0.0",
"@rollup/plugin-node-resolve": "^13.1.3",
"@schematics/angular": "^17.1.0-next.0",
"@schematics/angular": "17.1.0-next.1",
"@types/babel__core": "^7.1.18",
"@types/browser-sync": "^2.26.3",
"@types/fs-extra": "^9.0.13",
Expand Down
6 changes: 4 additions & 2 deletions src/cdk/portal/portal.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -734,8 +734,10 @@ class Chocolate {
class ChocolateInjector {
constructor(public parentInjector: Injector) {}

get(token: any) {
return token === Chocolate ? new Chocolate() : this.parentInjector.get<any>(token);
get(token: any, notFoundValue?: any) {
return token === Chocolate
? new Chocolate()
: this.parentInjector.get<any>(token, notFoundValue);
}
}

Expand Down
26 changes: 15 additions & 11 deletions src/material/sidenav/drawer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,6 @@ export function MAT_DRAWER_DEFAULT_AUTOSIZE_FACTORY(): boolean {
'class': 'mat-drawer-content',
'[style.margin-left.px]': '_container._contentMargins.left',
'[style.margin-right.px]': '_container._contentMargins.right',
'ngSkipHydration': '',
},
changeDetection: ChangeDetectionStrategy.OnPush,
encapsulation: ViewEncapsulation.None,
Expand Down Expand Up @@ -152,15 +151,14 @@ export class MatDrawerContent extends CdkScrollable implements AfterContentInit
'[@transform]': '_animationState',
'(@transform.start)': '_animationStarted.next($event)',
'(@transform.done)': '_animationEnd.next($event)',
'ngSkipHydration': '',
},
changeDetection: ChangeDetectionStrategy.OnPush,
encapsulation: ViewEncapsulation.None,
standalone: true,
imports: [CdkScrollable],
})
export class MatDrawer implements AfterViewInit, AfterContentChecked, OnDestroy {
private _focusTrap: FocusTrap;
private _focusTrap: FocusTrap | null = null;
private _elementFocusedBeforeDrawerWasOpened: HTMLElement | null = null;

/** Whether the drawer is initialized. Used for disabling the initial animation. */
Expand Down Expand Up @@ -480,14 +478,19 @@ export class MatDrawer implements AfterViewInit, AfterContentChecked, OnDestroy

ngAfterViewInit() {
this._isAttached = true;
this._focusTrap = this._focusTrapFactory.create(this._elementRef.nativeElement);
this._updateFocusTrapState();

// Only update the DOM position when the sidenav is positioned at
// the end since we project the sidenav before the content by default.
if (this._position === 'end') {
this._updatePositionInParent('end');
}

// Needs to happen after the position is updated
// so the focus trap anchors are in the right place.
if (this._platform.isBrowser) {
this._focusTrap = this._focusTrapFactory.create(this._elementRef.nativeElement);
this._updateFocusTrapState();
}
}

ngAfterContentChecked() {
Expand All @@ -501,10 +504,7 @@ export class MatDrawer implements AfterViewInit, AfterContentChecked, OnDestroy
}

ngOnDestroy() {
if (this._focusTrap) {
this._focusTrap.destroy();
}

this._focusTrap?.destroy();
this._anchor?.remove();
this._anchor = null;
this._animationStarted.complete();
Expand Down Expand Up @@ -610,7 +610,12 @@ export class MatDrawer implements AfterViewInit, AfterContentChecked, OnDestroy
* matches the tab order. We also need to be able to move it back to `start` if the sidenav
* started off as `end` and was changed to `start`.
*/
private _updatePositionInParent(newPosition: 'start' | 'end') {
private _updatePositionInParent(newPosition: 'start' | 'end'): void {
// Don't move the DOM node around on the server, because it can throw off hydration.
if (!this._platform.isBrowser) {
return;
}

const element = this._elementRef.nativeElement;
const parent = element.parentNode!;

Expand Down Expand Up @@ -641,7 +646,6 @@ export class MatDrawer implements AfterViewInit, AfterContentChecked, OnDestroy
host: {
'class': 'mat-drawer-container',
'[class.mat-drawer-container-explicit-backdrop]': '_backdropOverride',
'ngSkipHydration': '',
},
changeDetection: ChangeDetectionStrategy.OnPush,
encapsulation: ViewEncapsulation.None,
Expand Down
3 changes: 0 additions & 3 deletions src/material/sidenav/sidenav.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ import {ScrollDispatcher, CdkScrollable} from '@angular/cdk/scrolling';
'class': 'mat-drawer-content mat-sidenav-content',
'[style.margin-left.px]': '_container._contentMargins.left',
'[style.margin-right.px]': '_container._contentMargins.right',
'ngSkipHydration': '',
},
changeDetection: ChangeDetectionStrategy.OnPush,
encapsulation: ViewEncapsulation.None,
Expand Down Expand Up @@ -79,7 +78,6 @@ export class MatSidenavContent extends MatDrawerContent {
'[class.mat-sidenav-fixed]': 'fixedInViewport',
'[style.top.px]': 'fixedInViewport ? fixedTopGap : null',
'[style.bottom.px]': 'fixedInViewport ? fixedBottomGap : null',
'ngSkipHydration': '',
},
changeDetection: ChangeDetectionStrategy.OnPush,
encapsulation: ViewEncapsulation.None,
Expand Down Expand Up @@ -132,7 +130,6 @@ export class MatSidenav extends MatDrawer {
host: {
'class': 'mat-drawer-container mat-sidenav-container',
'[class.mat-drawer-container-explicit-backdrop]': '_backdropOverride',
'ngSkipHydration': '',
},
changeDetection: ChangeDetectionStrategy.OnPush,
encapsulation: ViewEncapsulation.None,
Expand Down
17 changes: 11 additions & 6 deletions src/universal-app/kitchen-sink/kitchen-sink.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,18 +257,23 @@ <h2>Select</h2>
<h2>Sidenav</h2>
<mat-sidenav-container>
<!--
TODO: add sample with multiple sidenavs which can be problematic for hydration.
Currently blocked on https://github.com/angular/angular/issues/53246
Place the end sidenav first to test the behavior where we manually change the DOM order.
-->
<mat-sidenav #endSidenav position="end" mode="push">
End sidenav
<button (click)="endSidenav.toggle()">Close</button>
</mat-sidenav>

<!--
Only attempt to capture focus when automated, otherwise it makes the page jump around.
-->
<mat-sidenav #sidenav opened [autoFocus]="isAutomated ? 'first-tabbable' : false">
On the side
<button (click)="sidenav.toggle()">Button for testing focus trapping</button>
<mat-sidenav #startSidenav opened [autoFocus]="isAutomated ? 'first-tabbable' : false">
Start sidenav
<button (click)="startSidenav.toggle()">Button for testing focus trapping</button>
</mat-sidenav>
Main content
<button (click)="sidenav.toggle()">Click me</button>
<button (click)="startSidenav.toggle()">Toggle start</button>
<button (click)="endSidenav.toggle()">Toggle end</button>
</mat-sidenav-container>

<h2>Slide-toggle</h2>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW: One thing I did play with in the past was to use Pupeteer and capture a screenshot of the SSR structure to ensure the initial "boilerplate" looks as expected. I wonder, do we also bother about this with these PRs or are we just ensuring no errors?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're just ensuring that there's no errors, but hydration errors actually aren't captured on the CI yet. Eventually it would be nice to add something (potentially Puppeteer or Webdriver) that opens the client-side-rendered page and checks that there are no JS errors.

Expand Down
Loading