Skip to content

Commit

Permalink
fix(menu,toolbar): avoid potential server-side rendering errors (#9423)
Browse files Browse the repository at this point in the history
Avoids a couple of potential server-side rendering errors due to the menu item and toolbar components referring to the `Node` global variable directly.
  • Loading branch information
crisbeto authored and jelbourn committed Jan 24, 2018
1 parent a1db4e4 commit dfa68db
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 5 deletions.
13 changes: 11 additions & 2 deletions src/lib/menu/menu-item.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
ElementRef,
OnDestroy,
ViewEncapsulation,
Inject,
} from '@angular/core';
import {
CanDisable,
Expand All @@ -21,6 +22,7 @@ import {
mixinDisableRipple
} from '@angular/material/core';
import {Subject} from 'rxjs/Subject';
import {DOCUMENT} from '@angular/common';

// Boilerplate for applying mixins to MatMenuItem.
/** @docs-private */
Expand Down Expand Up @@ -55,6 +57,8 @@ export const _MatMenuItemMixinBase = mixinDisableRipple(mixinDisabled(MatMenuIte
export class MatMenuItem extends _MatMenuItemMixinBase
implements FocusableOption, CanDisable, CanDisableRipple, OnDestroy {

private _document: Document;

/** Stream that emits when the menu item is hovered. */
_hovered: Subject<MatMenuItem> = new Subject<MatMenuItem>();

Expand All @@ -66,8 +70,10 @@ export class MatMenuItem extends _MatMenuItemMixinBase

constructor(
private _elementRef: ElementRef,
// TODO(crisbeto): switch to a required param when doing breaking changes.
@Inject(DOCUMENT) document?: any,
private _focusMonitor?: FocusMonitor) {

// @deletion-target 6.0.0 make `_focusMonitor` and `document` required params.
super();

if (_focusMonitor) {
Expand All @@ -76,6 +82,8 @@ export class MatMenuItem extends _MatMenuItemMixinBase
// mouse or touch interaction.
_focusMonitor.monitor(this._getHostElement(), false);
}

this._document = document;
}

/** Focuses the menu item. */
Expand Down Expand Up @@ -123,6 +131,7 @@ export class MatMenuItem extends _MatMenuItemMixinBase
/** Gets the label to be used when determining whether the option should be focused. */
getLabel(): string {
const element: HTMLElement = this._elementRef.nativeElement;
const textNodeType = this._document ? this._document.TEXT_NODE : 3;
let output = '';

if (element.childNodes) {
Expand All @@ -132,7 +141,7 @@ export class MatMenuItem extends _MatMenuItemMixinBase
// We skip anything that's not a text node to prevent the text from
// being thrown off by something like an icon.
for (let i = 0; i < length; i++) {
if (element.childNodes[i].nodeType === Node.TEXT_NODE) {
if (element.childNodes[i].nodeType === textNodeType) {
output += element.childNodes[i].textContent;
}
}
Expand Down
15 changes: 12 additions & 3 deletions src/lib/toolbar/toolbar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,12 @@ import {
ElementRef,
isDevMode,
QueryList,
ViewEncapsulation
ViewEncapsulation,
Inject,
} from '@angular/core';
import {CanColor, mixinColor} from '@angular/material/core';
import {Platform} from '@angular/cdk/platform';
import {DOCUMENT} from '@angular/common';

// Boilerplate for applying mixins to MatToolbar.
/** @docs-private */
Expand Down Expand Up @@ -51,12 +53,19 @@ export class MatToolbarRow {}
preserveWhitespaces: false,
})
export class MatToolbar extends _MatToolbarMixinBase implements CanColor, AfterViewInit {
private _document: Document;

/** Reference to all toolbar row elements that have been projected. */
@ContentChildren(MatToolbarRow) _toolbarRows: QueryList<MatToolbarRow>;

constructor(elementRef: ElementRef, private _platform: Platform) {
constructor(
elementRef: ElementRef,
private _platform: Platform,
@Inject(DOCUMENT) document?: any) {

This comment has been minimized.

Copy link
@rafaelss95

rafaelss95 Apr 8, 2018

Contributor

@crisbeto I know this PR has already been merged, but shouldn't this have @deletion-target x.y.z as in others constructor parameters?

super(elementRef);

// TODO: make the document a required param when doing breaking changes.
this._document = document;
}

ngAfterViewInit() {
Expand All @@ -80,7 +89,7 @@ export class MatToolbar extends _MatToolbarMixinBase implements CanColor, AfterV
// a <mat-toolbar-row> element.
const isCombinedUsage = [].slice.call(this._elementRef.nativeElement.childNodes)
.filter(node => !(node.classList && node.classList.contains('mat-toolbar-row')))
.filter(node => node.nodeType !== Node.COMMENT_NODE)
.filter(node => node.nodeType !== (this._document ? this._document.COMMENT_NODE : 8))
.some(node => node.textContent.trim());

if (isCombinedUsage) {
Expand Down

0 comments on commit dfa68db

Please sign in to comment.