Skip to content

Commit

Permalink
fix(ivy): various IE issues
Browse files Browse the repository at this point in the history
Since we haven't been running the Ivy tests against IE for a while, we've accumulated some errors and test failures. These changes fix the issues which come from a handful of root causes:

**i18n instructions thrown off by sanitizer:**

While sanitizing on browsers that don't support the `template` element (pretty much only IE), we create an inert document and we insert content into it via `document.body.innerHTML = unsafeHTML`. The problem is that IE appears to parse the HTML passed to `innerHTML` differently, depending on whether the element has been inserted into a document or not. In particular, it seems to split some strings into multiple text nodes, which would've otherwise been a single node. This ended up throwing off some of the i18n code down the line and causing a handful of failures. I've worked around it by creating a new inert `body` element into which the HTML would be inserted.

**Inheriting injectable definition from parent class not working in IE10 JIT mode:**

The way definitions are added in JIT mode is through `Object.defineProperty`, but the problem is that in IE10 properties defined through `defineProperty` won't be inherited which means that inheriting injectable definitions no longer works. These changes add a workaround only for JIT mode where we define a fallback method for retrieving the definition. This isn't ideal, but it should only be required until v10 where we'll no longer support inheriting injectable definitions from undecorated classes.

**Inheritance issues in IE10:**

It looks like most of the inheritance functionality in Ivy wasn't working in JIT mode because we weren't accessing things correctly. These changes fix all of the cases.

**Proxies being used in DebugElement.classes:**

We're currently using proxies to pick up changes to element classes coming outside of Angular. Proxies aren't supported in IE and error is always thrown. I've reworked the tests where possible and skipped the rest with a TODO to come back and re-enable them once we have som kind of fallback.

**Inconsistent `typeof Node` value:**

We have a couple of cases where we use something like `typeof Node === 'function'` to figure out whether we're in a worker context. This works in most browsers, but IE returns `object` instead of `function`. I've updated all the usages to account for it.

**IE saving the attribute case:**

In `DebugElement.attributes` we return all of the attributes from the underlying DOM node. Most browsers change the attribute names to lower case, but IE preserves the case and since we use camel-cased attributes, the return value was inconsitent. I've changed it to always lower case the attribute names.

**Unable to get own function name:**

When we log DI errors we get the name of the provider via `SomeClass.name`. In IE functions that inherit from other functions don't have their own `name`, but they take the `name` from the lowest parent in the chain, before `Function`. I've added some changes to fall back to parsing out the function name from the function's string form.

**Different attribute order in innerHTML:**

We've got some tests that assert that the generate DOM looks correct. The problem is that IE changes the attribute order in `innerHTML` which caused the tests to fail. I've reworked the relevant tests not to assert directly against `innerHTML`.

**__proto__ not supported:**

IE doesn't support `__proto__`. I've added a fallback based on #34279.
  • Loading branch information
crisbeto committed Dec 11, 2019
1 parent b72c7a8 commit ccbcd0f
Show file tree
Hide file tree
Showing 12 changed files with 166 additions and 83 deletions.
8 changes: 6 additions & 2 deletions packages/core/src/debug/debug_node.ts
Expand Up @@ -331,10 +331,14 @@ class DebugElement__POST_R3__ extends DebugNode__POST_R3__ implements DebugEleme
const eAttrs = element.attributes;
for (let i = 0; i < eAttrs.length; i++) {
const attr = eAttrs[i];
const lowercaseName = attr.name.toLowerCase();

// Make sure that we don't assign the same attribute both in its
// case-sensitive form and the lower-cased one from the browser.
if (lowercaseTNodeAttrs.indexOf(attr.name) === -1) {
attributes[attr.name] = attr.value;
if (lowercaseTNodeAttrs.indexOf(lowercaseName) === -1) {
// Save the lowercase name to align the behavior between browsers.
// IE preserves the case, while all other browser convert it to lower case.
attributes[lowercaseName] = attr.value;
}
}

Expand Down
34 changes: 31 additions & 3 deletions packages/core/src/di/interface/defs.ts
Expand Up @@ -218,19 +218,39 @@ function getOwnDefinition<T>(type: any, def: ɵɵInjectableDef<T>): ɵɵInjectab
* `ɵprov` on an ancestor only.
*/
export function getInheritedInjectableDef<T>(type: any): ɵɵInjectableDef<T>|null {
const def = type && (type[NG_PROV_DEF] || type[NG_INJECTABLE_DEF]);
// See `jit/injectable.ts#compileInjectable` for context on NG_PROV_DEF_FALLBACK.
const def = type && (type[NG_PROV_DEF] || type[NG_INJECTABLE_DEF] ||
(type[NG_PROV_DEF_FALLBACK] && type[NG_PROV_DEF_FALLBACK]()));

if (def) {
// TODO(FW-1307): Re-add ngDevMode when closure can handle it
// ngDevMode &&
const typeName = getTypeName(type);
console.warn(
`DEPRECATED: DI is instantiating a token "${type.name}" that inherits its @Injectable decorator but does not provide one itself.\n` +
`This will become an error in v10. Please add @Injectable() to the "${type.name}" class.`);
`DEPRECATED: DI is instantiating a token "${typeName}" that inherits its @Injectable decorator but does not provide one itself.\n` +
`This will become an error in v10. Please add @Injectable() to the "${typeName}" class.`);
return def;
} else {
return null;
}
}

/** Gets the name of a type, accounting for some cross-browser differences. */
function getTypeName(type: any): string {
// `Function.prototype.name` behaves differently between IE and other browsers. In most browsers
// it'll always return the name of the function itself, no matter how many other functions it
// inherits from. On IE the function doesn't have its own `name` property, but it takes it from
// the lowest level in the prototype chain. E.g. if we have `class Foo extends Parent` most
// browsers will evaluate `Foo.name` to `Foo` while IE will return `Parent`. We work around
// the issue by converting the function to a string and parsing its name out that way via a regex.
if (type.hasOwnProperty('name')) {
return type.name;
}

const match = ('' + type).match(/^function\s*([^\s(]+)/);
return match === null ? '' : match[1];
}

/**
* Read the injector def type in a way which is immune to accidentally reading inherited value.
*
Expand All @@ -245,6 +265,14 @@ export function getInjectorDef<T>(type: any): ɵɵInjectorDef<T>|null {
export const NG_PROV_DEF = getClosureSafeProperty({ɵprov: getClosureSafeProperty});
export const NG_INJ_DEF = getClosureSafeProperty({ɵinj: getClosureSafeProperty});

// On IE10 properties defined via `defineProperty` won't be inherited by child classes,
// which will break inheriting the injectable definition from a grandparent through an
// undecorated parent class. We work around it by defining a fallback method which will be
// used to retrieve the definition. This should only be a problem in JIT mode, because in
// AOT TypeScript seems to have a workaround for static properties. When inheriting from an
// undecorated parent is no longer supported in v10, this can safely be removed.
export const NG_PROV_DEF_FALLBACK = getClosureSafeProperty({ɵprovFallback: getClosureSafeProperty});

// We need to keep these around so we can read off old defs if new defs are unavailable
export const NG_INJECTABLE_DEF = getClosureSafeProperty({ngInjectableDef: getClosureSafeProperty});
export const NG_INJECTOR_DEF = getClosureSafeProperty({ngInjectorDef: getClosureSafeProperty});
12 changes: 11 additions & 1 deletion packages/core/src/di/jit/injectable.ts
Expand Up @@ -12,7 +12,7 @@ import {NG_FACTORY_DEF} from '../../render3/fields';
import {getClosureSafeProperty} from '../../util/property';
import {resolveForwardRef} from '../forward_ref';
import {Injectable} from '../injectable';
import {NG_PROV_DEF} from '../interface/defs';
import {NG_PROV_DEF, NG_PROV_DEF_FALLBACK} from '../interface/defs';
import {ClassSansProvider, ExistingSansProvider, FactorySansProvider, ValueProvider, ValueSansProvider} from '../interface/provider';

import {angularCoreDiEnv} from './environment';
Expand Down Expand Up @@ -40,6 +40,16 @@ export function compileInjectable(type: Type<any>, srcMeta?: Injectable): void {
return ngInjectableDef;
},
});

// On IE10 properties defined via `defineProperty` won't be inherited by child classes,
// which will break inheriting the injectable definition from a grandparent through an
// undecorated parent class. We work around it by defining a method which should be used
// as a fallback. This should only be a problem in JIT mode, because in AOT TypeScript
// seems to have a workaround for static properties. When inheriting from an undecorated
// parent is no longer supported in v10, this can safely be removed.
if (!type.hasOwnProperty(NG_PROV_DEF_FALLBACK)) {
(type as any)[NG_PROV_DEF_FALLBACK] = () => (type as any)[NG_PROV_DEF];
}
}

// if NG_FACTORY_DEF is already defined on this class then don't overwrite it
Expand Down
12 changes: 8 additions & 4 deletions packages/core/src/di/jit/util.ts
Expand Up @@ -48,13 +48,17 @@ function reflectDependency(compiler: CompilerFacade, dep: any | any[]): R3Depend
if (param === undefined) {
// param may be undefined if type of dep is not set by ngtsc
continue;
} else if (param instanceof Optional || param.__proto__.ngMetadataName === 'Optional') {
}

const proto = Object.getPrototypeOf(param);

if (param instanceof Optional || proto.ngMetadataName === 'Optional') {
meta.optional = true;
} else if (param instanceof SkipSelf || param.__proto__.ngMetadataName === 'SkipSelf') {
} else if (param instanceof SkipSelf || proto.ngMetadataName === 'SkipSelf') {
meta.skipSelf = true;
} else if (param instanceof Self || param.__proto__.ngMetadataName === 'Self') {
} else if (param instanceof Self || proto.ngMetadataName === 'Self') {
meta.self = true;
} else if (param instanceof Host || param.__proto__.ngMetadataName === 'Host') {
} else if (param instanceof Host || proto.ngMetadataName === 'Host') {
meta.host = true;
} else if (param instanceof Inject) {
meta.token = param.token;
Expand Down
5 changes: 4 additions & 1 deletion packages/core/src/render3/instructions/element.ts
Expand Up @@ -271,7 +271,10 @@ function validateElement(
// as a custom element. Note that unknown elements with a dash in their name won't be instances
// of HTMLUnknownElement in browsers that support web components.
const isUnknown =
(typeof HTMLUnknownElement === 'function' && element instanceof HTMLUnknownElement) ||
// Note that we can't check for `typeof HTMLUnknownElement === 'function'`,
// because while most browsers return 'function', IE returns 'object'.
(typeof HTMLUnknownElement !== 'undefined' && HTMLUnknownElement &&
element instanceof HTMLUnknownElement) ||
(typeof customElements !== 'undefined' && tagName.indexOf('-') > -1 &&
!customElements.get(tagName));

Expand Down
10 changes: 8 additions & 2 deletions packages/core/src/render3/instructions/shared.ts
Expand Up @@ -1011,8 +1011,14 @@ function validateProperty(
hostView: LView, element: RElement | RComment, propName: string, tNode: TNode): boolean {
// The property is considered valid if the element matches the schema, it exists on the element
// or it is synthetic, and we are in a browser context (web worker nodes should be skipped).
return matchingSchemas(hostView, tNode.tagName) || propName in element ||
isAnimationProp(propName) || typeof Node !== 'function' || !(element instanceof Node);
if (matchingSchemas(hostView, tNode.tagName) || propName in element ||
isAnimationProp(propName)) {
return true;
}

// Note: `typeof Node` returns 'function' in most browsers, but on IE it is 'object' so we
// need to account for both here, while being careful for `typeof null` also returning 'object'.
return typeof Node === 'undefined' || Node === null || !(element instanceof Node);
}

export function matchingSchemas(hostView: LView, tagName: string | null): boolean {
Expand Down
18 changes: 8 additions & 10 deletions packages/core/src/render3/jit/directive.ts
Expand Up @@ -189,7 +189,8 @@ export function extendsDirectlyFromObject(type: Type<any>): boolean {
*/
export function directiveMetadata(type: Type<any>, metadata: Directive): R3DirectiveMetadataFacade {
// Reflect inputs and outputs.
const propMetadata = getReflect().ownPropMetadata(type);
const reflect = getReflect();
const propMetadata = reflect.ownPropMetadata(type);

return {
name: type.name,
Expand All @@ -202,7 +203,7 @@ export function directiveMetadata(type: Type<any>, metadata: Directive): R3Direc
inputs: metadata.inputs || EMPTY_ARRAY,
outputs: metadata.outputs || EMPTY_ARRAY,
queries: extractQueriesMetadata(type, propMetadata, isContentQuery),
lifecycle: {usesOnChanges: usesLifecycleHook(type, 'ngOnChanges')},
lifecycle: {usesOnChanges: reflect.hasLifecycleHook(type, 'ngOnChanges')},
typeSourceSpan: null !,
usesInheritance: !extendsDirectlyFromObject(type),
exportAs: extractExportAs(metadata.exportAs),
Expand All @@ -216,7 +217,7 @@ export function directiveMetadata(type: Type<any>, metadata: Directive): R3Direc
*/
function addDirectiveDefToUndecoratedParents(type: Type<any>) {
const objPrototype = Object.prototype;
let parent = Object.getPrototypeOf(type);
let parent = Object.getPrototypeOf(type.prototype).constructor;

// Go up the prototype until we hit `Object`.
while (parent && parent !== objPrototype) {
Expand Down Expand Up @@ -291,22 +292,19 @@ function splitByComma(value: string): string[] {
return value.split(',').map(piece => piece.trim());
}

function usesLifecycleHook(type: Type<any>, name: string): boolean {
const prototype = type.prototype;
return prototype && prototype.hasOwnProperty(name);
}

const LIFECYCLE_HOOKS = [
'ngOnChanges', 'ngOnInit', 'ngOnDestroy', 'ngDoCheck', 'ngAfterViewInit', 'ngAfterViewChecked',
'ngAfterContentInit', 'ngAfterContentChecked'
];

function shouldAddAbstractDirective(type: Type<any>): boolean {
if (LIFECYCLE_HOOKS.some(hookName => usesLifecycleHook(type, hookName))) {
const reflect = getReflect();

if (LIFECYCLE_HOOKS.some(hookName => reflect.hasLifecycleHook(type, hookName))) {
return true;
}

const propMetadata = getReflect().ownPropMetadata(type);
const propMetadata = reflect.propMetadata(type);

for (const field in propMetadata) {
const annotations = propMetadata[field];
Expand Down
32 changes: 19 additions & 13 deletions packages/core/src/sanitization/inert_body.ts
Expand Up @@ -15,33 +15,31 @@
* Default: InertDocument strategy
*/
export class InertBodyHelper {
private inertBodyElement: HTMLElement;
private inertDocument: Document;

constructor(private defaultDoc: Document) {
this.inertDocument = this.defaultDoc.implementation.createHTMLDocument('sanitization-inert');
this.inertBodyElement = this.inertDocument.body;
let inertBodyElement = this.inertDocument.body;

if (this.inertBodyElement == null) {
if (inertBodyElement == null) {
// usually there should be only one body element in the document, but IE doesn't have any, so
// we need to create one.
const inertHtml = this.inertDocument.createElement('html');
this.inertDocument.appendChild(inertHtml);
this.inertBodyElement = this.inertDocument.createElement('body');
inertHtml.appendChild(this.inertBodyElement);
inertBodyElement = this.inertDocument.createElement('body');
inertHtml.appendChild(inertBodyElement);
}

this.inertBodyElement.innerHTML = '<svg><g onload="this.parentNode.remove()"></g></svg>';
if (this.inertBodyElement.querySelector && !this.inertBodyElement.querySelector('svg')) {
inertBodyElement.innerHTML = '<svg><g onload="this.parentNode.remove()"></g></svg>';
if (inertBodyElement.querySelector && !inertBodyElement.querySelector('svg')) {
// We just hit the Safari 10.1 bug - which allows JS to run inside the SVG G element
// so use the XHR strategy.
this.getInertBodyElement = this.getInertBodyElement_XHR;
return;
}

this.inertBodyElement.innerHTML =
'<svg><p><style><img src="</style><img src=x onerror=alert(1)//">';
if (this.inertBodyElement.querySelector && this.inertBodyElement.querySelector('svg img')) {
inertBodyElement.innerHTML = '<svg><p><style><img src="</style><img src=x onerror=alert(1)//">';
if (inertBodyElement.querySelector && inertBodyElement.querySelector('svg img')) {
// We just hit the Firefox bug - which prevents the inner img JS from being sanitized
// so use the DOMParser strategy, if it is available.
// If the DOMParser is not available then we are not in Firefox (Server/WebWorker?) so we
Expand Down Expand Up @@ -122,15 +120,23 @@ export class InertBodyHelper {
return templateEl;
}

this.inertBodyElement.innerHTML = html;
// Note that previously we used to do something like `this.inertDocument.body.innerHTML = html`
// and we returned the inert `body` node. This was changed, because IE seems to treat setting
// `innerHTML` on an inserted element differently, compared to one that hasn't been inserted
// yet. In particular, IE appears to split some of the text into multiple text nodes rather
// than keeping them in a single one which ends up messing with Ivy's i18n parsing further
// down the line. This has been worked around by creating a new inert `body` and using it as
// the root node in which we insert the HTML.
const inertBody = this.inertDocument.createElement('body');
inertBody.innerHTML = html;

// Support: IE 9-11 only
// strip custom-namespaced attributes on IE<=11
if ((this.defaultDoc as any).documentMode) {
this.stripCustomNsAttrs(this.inertBodyElement);
this.stripCustomNsAttrs(inertBody);
}

return this.inertBodyElement;
return inertBody;
}

/**
Expand Down
36 changes: 25 additions & 11 deletions packages/core/test/acceptance/i18n_spec.ts
Expand Up @@ -1451,13 +1451,13 @@ onlyInIvy('Ivy i18n logic').describe('runtime i18n', () => {
@Component({
selector: `my-app`,
template: `
<div i18n test i18n-title title="start {{exp1}} middle {{exp2}} end">
<div i18n test i18n-title title="start {{exp1}} middle {{exp2}} end" outer>
trad: {exp1, plural,
=0 {no <b title="none">emails</b>!}
=1 {one <i>email</i>}
other {{{exp1}} emails}
}
</div><div test></div>`
</div><div test inner></div>`
})
class MyApp {
exp1 = 1;
Expand All @@ -1476,19 +1476,27 @@ onlyInIvy('Ivy i18n logic').describe('runtime i18n', () => {
});
const fixture = TestBed.createComponent(MyApp);
fixture.detectChanges();
expect(fixture.nativeElement.innerHTML)
.toEqual(
`<div test="" title="début 2 milieu 1 fin" class="foo"> traduction: un <i>email</i><!--ICU 21--> ` +
`</div><div test="" class="foo"></div>`);

const outerDiv: HTMLElement = fixture.nativeElement.querySelector('div[outer]');
const innerDiv: HTMLElement = fixture.nativeElement.querySelector('div[inner]');

// Note that ideally we'd just compare the innerHTML here, but different browsers return
// the order of attributes differently. E.g. most browsers preserve the declaration order,
// but IE does not.
expect(outerDiv.getAttribute('title')).toBe('début 2 milieu 1 fin');
expect(outerDiv.getAttribute('class')).toBe('foo');
expect(outerDiv.textContent !.trim()).toBe('traduction: un email');
expect(innerDiv.getAttribute('class')).toBe('foo');

directiveInstances.forEach(instance => instance.klass = 'bar');
fixture.componentRef.instance.exp1 = 2;
fixture.componentRef.instance.exp2 = 3;
fixture.detectChanges();
expect(fixture.nativeElement.innerHTML)
.toEqual(
`<div test="" title="début 3 milieu 2 fin" class="bar"> traduction: 2 emails<!--ICU 21--> ` +
`</div><div test="" class="bar"></div>`);

expect(outerDiv.getAttribute('title')).toBe('début 3 milieu 2 fin');
expect(outerDiv.getAttribute('class')).toBe('bar');
expect(outerDiv.textContent !.trim()).toBe('traduction: 2 emails');
expect(innerDiv.getAttribute('class')).toBe('bar');
});

it('should handle i18n attribute with directive inputs', () => {
Expand Down Expand Up @@ -2033,9 +2041,15 @@ onlyInIvy('Ivy i18n logic').describe('runtime i18n', () => {

const fixture = TestBed.createComponent(ContentElementDialog);
fixture.detectChanges();

// Remove the reflect attribute, because the attribute order in innerHTML
// isn't guaranteed in different browsers so it could throw off our assertions.
const button = fixture.nativeElement.querySelector('button');
button.removeAttribute('ng-reflect-dialog-result');

expect(fixture.nativeElement.innerHTML).toEqual(`<div dialog=""><!--bindings={
"ng-reflect-ng-if": "false"
}--></div><button ng-reflect-dialog-result="true" title="Close dialog">Button label</button>`);
}--></div><button title="Close dialog">Button label</button>`);
});
});

Expand Down

0 comments on commit ccbcd0f

Please sign in to comment.