Skip to content

Commit

Permalink
fix prop normalization
Browse files Browse the repository at this point in the history
This (along with some changes to Ember that I will also PR) solves emberjs/ember.js#16311 and emberjs/ember.js#16477

(cherry picked from commit 014ed47)
  • Loading branch information
ef4 authored and rwjblue committed Apr 21, 2018
1 parent df23d0f commit 1b6dc0a
Show file tree
Hide file tree
Showing 5 changed files with 31 additions and 34 deletions.
1 change: 0 additions & 1 deletion packages/@glimmer/runtime/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ export { PublicVM as VM, VM as LowLevelVM, UpdatingVM, RenderResult, IteratorRes

export {
SimpleDynamicAttribute,
DynamicAttributeFactory,
DynamicAttribute
} from './lib/vm/attributes/dynamic';

Expand Down
6 changes: 3 additions & 3 deletions packages/@glimmer/runtime/lib/environment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -283,8 +283,8 @@ export abstract class Environment {
transaction.commit();
}

attributeFor(element: Simple.Element, attr: string, _isTrusting: boolean, _namespace: Option<string> = null): DynamicAttributeFactory {
return defaultDynamicAttributes(element, attr);
attributeFor(element: Simple.Element, attr: string, _isTrusting: boolean, namespace: Option<string> = null): DynamicAttribute {
return dynamicAttribute(element, attr, namespace);
}
}

Expand Down
47 changes: 24 additions & 23 deletions packages/@glimmer/runtime/lib/vm/attributes/dynamic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string>): 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 {
Expand Down Expand Up @@ -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();
Expand All @@ -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);
}
}
}
Expand Down
5 changes: 1 addition & 4 deletions packages/@glimmer/runtime/lib/vm/element-builder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -405,11 +405,8 @@ export class NewElementBuilder implements ElementBuilder {

setDynamicAttribute(name: string, value: Opaque, trusting: boolean, namespace: Option<string>): 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;
}
}
Expand Down
6 changes: 3 additions & 3 deletions packages/@glimmer/runtime/test/style-warning-test.ts
Original file line number Diff line number Diff line change
@@ -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";

Expand Down Expand Up @@ -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<string>): DynamicAttributeFactory {
attributeFor(element: Simple.Element, attr: string, isTrusting: boolean, namespace: Option<string>): DynamicAttribute {
if (attr === 'style' && !isTrusting) {
return StyleAttribute;
return new StyleAttribute({ element, name, namespace });
}

return super.attributeFor(element, attr, isTrusting, namespace);
Expand Down

0 comments on commit 1b6dc0a

Please sign in to comment.