From 1b6dc0aa7a69adcbb906c3317339b9aedc0faaf0 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Fri, 20 Apr 2018 22:49:56 -0400 Subject: [PATCH] fix prop normalization This (along with some changes to Ember that I will also PR) solves https://github.com/emberjs/ember.js/issues/16311 and https://github.com/emberjs/ember.js/issues/16477 (cherry picked from commit 014ed47b3ff32e69dd99a03f40554c347e601d59) --- packages/@glimmer/runtime/index.ts | 1 - packages/@glimmer/runtime/lib/environment.ts | 6 +-- .../runtime/lib/vm/attributes/dynamic.ts | 47 ++++++++++--------- .../runtime/lib/vm/element-builder.ts | 5 +- .../runtime/test/style-warning-test.ts | 6 +-- 5 files changed, 31 insertions(+), 34 deletions(-) diff --git a/packages/@glimmer/runtime/index.ts b/packages/@glimmer/runtime/index.ts index d602f3458e..dbe870ea82 100644 --- a/packages/@glimmer/runtime/index.ts +++ b/packages/@glimmer/runtime/index.ts @@ -18,7 +18,6 @@ export { PublicVM as VM, VM as LowLevelVM, UpdatingVM, RenderResult, IteratorRes export { SimpleDynamicAttribute, - DynamicAttributeFactory, DynamicAttribute } from './lib/vm/attributes/dynamic'; diff --git a/packages/@glimmer/runtime/lib/environment.ts b/packages/@glimmer/runtime/lib/environment.ts index 12d6613981..ed94e2de3b 100644 --- a/packages/@glimmer/runtime/lib/environment.ts +++ b/packages/@glimmer/runtime/lib/environment.ts @@ -15,7 +15,7 @@ import { DOMChanges, DOMTreeConstruction } from './dom/helper'; import { PublicVM } from './vm/append'; import { IArguments } from './vm/arguments'; import { UNDEFINED_REFERENCE, ConditionalReference } from './references'; -import { DynamicAttributeFactory, defaultDynamicAttributes } from './vm/attributes/dynamic'; +import { DynamicAttribute, dynamicAttribute } from './vm/attributes/dynamic'; import { ModifierManager, Modifier } from './modifier/interfaces'; @@ -283,8 +283,8 @@ export abstract class Environment { transaction.commit(); } - attributeFor(element: Simple.Element, attr: string, _isTrusting: boolean, _namespace: Option = null): DynamicAttributeFactory { - return defaultDynamicAttributes(element, attr); + attributeFor(element: Simple.Element, attr: string, _isTrusting: boolean, namespace: Option = null): DynamicAttribute { + return dynamicAttribute(element, attr, namespace); } } diff --git a/packages/@glimmer/runtime/lib/vm/attributes/dynamic.ts b/packages/@glimmer/runtime/lib/vm/attributes/dynamic.ts index d8f13e0cb8..9ea021ad71 100644 --- a/packages/@glimmer/runtime/lib/vm/attributes/dynamic.ts +++ b/packages/@glimmer/runtime/lib/vm/attributes/dynamic.ts @@ -7,48 +7,46 @@ import { SVG_NAMESPACE } from '../../dom/helper'; import { Attribute, AttributeOperation } from './index'; import { normalizeStringValue } from '../../dom/normalize'; -export interface DynamicAttributeFactory { - new(attribute: Attribute): DynamicAttribute; -} - -export function defaultDynamicAttributes(element: Simple.Element, attr: string): DynamicAttributeFactory { +export function dynamicAttribute(element: Simple.Element, attr: string, namespace: Option): DynamicAttribute { let { tagName, namespaceURI } = element; + let attribute = { element, name: attr, namespace }; if (namespaceURI === SVG_NAMESPACE) { - return defaultDynamicAttribute(tagName, attr); + return buildDynamicAttribute(tagName, attr, attribute); } let { type, normalized } = normalizeProperty(element, attr); if (type === 'attr') { - return defaultDynamicAttribute(tagName, normalized); + return buildDynamicAttribute(tagName, normalized, attribute); } else { - return defaultDynamicProperty(tagName, normalized); + return buildDynamicProperty(tagName, normalized, attribute); } + } -export function defaultDynamicAttribute(tagName: string, name: string): DynamicAttributeFactory { +function buildDynamicAttribute(tagName: string, name: string, attribute: Attribute): DynamicAttribute { if (requiresSanitization(tagName, name)) { - return SafeDynamicAttribute; + return new SafeDynamicAttribute(attribute); } else { - return SimpleDynamicAttribute; + return new SimpleDynamicAttribute(attribute); } } -export function defaultDynamicProperty(tagName: string, name: string): DynamicAttributeFactory { +function buildDynamicProperty(tagName: string, name: string, attribute: Attribute): DynamicAttribute { if (requiresSanitization(tagName, name)) { - return SafeDynamicProperty; + return new SafeDynamicProperty(name, attribute); } if (isUserInputValue(tagName, name)) { - return InputValueDynamicAttribute; + return new InputValueDynamicAttribute(name, attribute); } if (isOptionSelected(tagName, name)) { - return OptionSelectedDynamicAttribute; + return new OptionSelectedDynamicAttribute(name, attribute); } - return DefaultDynamicProperty; + return new DefaultDynamicProperty(name, attribute); } export abstract class DynamicAttribute implements AttributeOperation { @@ -81,20 +79,23 @@ export class SimpleDynamicAttribute extends DynamicAttribute { } export class DefaultDynamicProperty extends DynamicAttribute { + constructor(private normalizedName: string, attribute: Attribute) { + super(attribute); + } + value: Opaque; set(dom: ElementBuilder, value: Opaque, _env: Environment): void { if (value !== null && value !== undefined) { - let { name } = this.attribute; this.value = value; - dom.__setProperty(name, value); + dom.__setProperty(this.normalizedName, value); } } update(value: Opaque, _env: Environment): void { - let { element, name } = this.attribute; + let { element } = this.attribute; if (this.value !== value) { - element[name] = this.value = value; + element[this.normalizedName] = this.value = value; if (value === null || value === undefined) { this.removeAttribute(); @@ -106,12 +107,12 @@ export class DefaultDynamicProperty extends DynamicAttribute { protected removeAttribute() { // TODO this sucks but to preserve properties first and to meet current // semantics we must do this. - let { element, name, namespace } = this.attribute; + let { element, namespace } = this.attribute; if (namespace) { - element.removeAttributeNS(namespace, name); + element.removeAttributeNS(namespace, this.normalizedName); } else { - element.removeAttribute(name); + element.removeAttribute(this.normalizedName); } } } diff --git a/packages/@glimmer/runtime/lib/vm/element-builder.ts b/packages/@glimmer/runtime/lib/vm/element-builder.ts index a8062ebf2d..90a19089e1 100644 --- a/packages/@glimmer/runtime/lib/vm/element-builder.ts +++ b/packages/@glimmer/runtime/lib/vm/element-builder.ts @@ -405,11 +405,8 @@ export class NewElementBuilder implements ElementBuilder { setDynamicAttribute(name: string, value: Opaque, trusting: boolean, namespace: Option): DynamicAttribute { let element = this.constructing!; - let DynamicAttribute = this.env.attributeFor(element, name, trusting, namespace); - let attribute = new DynamicAttribute({ element, name, namespace: namespace || null }); - + let attribute = this.env.attributeFor(element, name, trusting, namespace); attribute.set(this, value, this.env); - return attribute; } } diff --git a/packages/@glimmer/runtime/test/style-warning-test.ts b/packages/@glimmer/runtime/test/style-warning-test.ts index 6db7e6451c..b6efd441ae 100644 --- a/packages/@glimmer/runtime/test/style-warning-test.ts +++ b/packages/@glimmer/runtime/test/style-warning-test.ts @@ -1,6 +1,6 @@ import { TestEnvironment, equalTokens } from "@glimmer/test-helpers"; import { test, module } from "@glimmer/runtime/test/support"; -import { RenderResult, DynamicAttributeFactory, SimpleDynamicAttribute, ElementBuilder, clientBuilder } from "@glimmer/runtime"; +import { RenderResult, DynamicAttribute, SimpleDynamicAttribute, ElementBuilder, clientBuilder } from "@glimmer/runtime"; import { UpdatableReference } from "@glimmer/object-reference"; import { Option, Simple, Opaque, Template } from "@glimmer/interfaces"; @@ -35,9 +35,9 @@ function render(template: Template, self: any) { module('Style attributes', { beforeEach() { class StyleEnv extends TestEnvironment { - attributeFor(element: Simple.Element, attr: string, isTrusting: boolean, namespace: Option): DynamicAttributeFactory { + attributeFor(element: Simple.Element, attr: string, isTrusting: boolean, namespace: Option): DynamicAttribute { if (attr === 'style' && !isTrusting) { - return StyleAttribute; + return new StyleAttribute({ element, name, namespace }); } return super.attributeFor(element, attr, isTrusting, namespace);