Skip to content

Commit

Permalink
amp-bind: Remove BindValidator.canBind (#8033)
Browse files Browse the repository at this point in the history 
* remove BindValidator.canBind

* lint fix

* remove amp-source-origin check from bind-validator, fix array workaround for rewriteAttributeValue

* fix other tests
  • Loading branch information
William Chou authored Mar 9, 2017
1 parent 1792a0f commit ed8c2c4
Show file tree
Hide file tree
Showing 5 changed files with 29 additions and 365 deletions.
34 changes: 11 additions & 23 deletions extensions/amp-bind/0.1/bind-impl.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

import {BindExpressionResultDef} from './bind-expression';
import {BindingDef, BindEvaluator} from './bind-evaluator';
import {BindValidator} from './bind-validator';
import {chunk, ChunkPriority} from '../../../src/chunk';
import {dev, user} from '../../../src/log';
import {fromClassForDoc} from '../../../src/service';
Expand Down Expand Up @@ -97,9 +96,6 @@ export class Bind {
*/
this.expressionToElements_ = Object.create(null);

/** @const @private {!./bind-validator.BindValidator} */
this.validator_ = new BindValidator();

/** @const @private {!Object} */
this.scope_ = Object.create(null);

Expand Down Expand Up @@ -397,7 +393,7 @@ export class Bind {
const attrs = element.attributes;
for (let i = 0, numberOfAttrs = attrs.length; i < numberOfAttrs; i++) {
const attr = attrs[i];
const boundProperty = this.scanAttribute_(attr, element);
const boundProperty = this.scanAttribute_(attr);
if (boundProperty) {
boundProperties.push(boundProperty);
}
Expand All @@ -409,20 +405,14 @@ export class Bind {
* Returns the bound property and expression string within a given attribute,
* if it exists. Otherwise, returns null.
* @param {!Attr} attribute
* @param {!Element} element
* @return {?{property: string, expressionString: string}}
* @private
*/
scanAttribute_(attribute, element) {
scanAttribute_(attribute) {
const name = attribute.name;
if (name.length > 2 && name[0] === '[' && name[name.length - 1] === ']') {
const property = name.substr(1, name.length - 2);
if (this.validator_.canBind(element.tagName, property)) {
return {property, expressionString: attribute.value};
} else {
const err = user().createError(`Binding to [${property}] not allowed.`);
reportError(err, element);
}
return {property, expressionString: attribute.value};
}
return null;
}
Expand Down Expand Up @@ -495,7 +485,6 @@ export class Bind {
apply_(results) {
const applyPromises = this.boundElements_.map(boundElement => {
const {element, boundProperties} = boundElement;
const tagName = element.tagName;

const applyPromise = this.resources_.mutateElement(element, () => {
const mutations = {};
Expand All @@ -505,13 +494,7 @@ export class Bind {
const {property, expressionString, previousResult} =
boundProperty;

// TODO(choumx): Perform in worker with URL API.
// Rewrite attribute value if necessary. This is not done in the
// worker since it relies on `url#parseUrl`, which uses DOM APIs.
let newValue = results[expressionString];
if (typeof newValue === 'string') {
newValue = rewriteAttributeValue(tagName, property, newValue);
}
const newValue = results[expressionString];

// Don't apply if the result hasn't changed or is missing.
if (newValue === undefined ||
Expand All @@ -530,7 +513,7 @@ export class Bind {
mutations[mutation.name] = mutation.value;
}

switch (boundProperty.property) {
switch (property) {
case 'width':
width = isFiniteNumber(newValue) ? Number(newValue) : width;
break;
Expand Down Expand Up @@ -604,7 +587,12 @@ export class Bind {
attributeChanged = true;
}
} else if (newValue !== oldValue) {
element.setAttribute(property, String(newValue));
// TODO(choumx): Perform in worker with URL API.
// Rewrite attribute value if necessary. This is not done in the
// worker since it relies on `url#parseUrl`, which uses DOM APIs.
const rewrittenNewValue = rewriteAttributeValue(
element.tagName, property, String(newValue));
element.setAttribute(property, rewrittenNewValue);
attributeChanged = true;
}

Expand Down
174 changes: 2 additions & 172 deletions extensions/amp-bind/0.1/bind-validator.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ const TAG = 'amp-bind';
* @typedef {{
* allowedProtocols: (!Object<string,boolean>|undefined),
* alternativeName: (string|undefined),
* blockedURLs: (Array<string>|undefined),
* }}
*/
let PropertyRulesDef;
Expand All @@ -36,10 +35,6 @@ const GLOBAL_PROPERTY_RULES = {
'class': {
blacklistedValueRegex: '(^|\\W)i-amphtml-',
},
// ARIA accessibility attributes.
'aria-describedby': null,
'aria-label': null,
'aria-labelledby': null,
};

/**
Expand Down Expand Up @@ -67,18 +62,6 @@ const URL_PROPERTIES = {
* of the AMP validator's, selected with a focus on security and UX.
*/
export class BindValidator {
/**
* Returns true if (tag, property) binding is allowed.
* Otherwise, returns false.
* @note `tag` and `property` are case-sensitive.
* @param {!string} tag
* @param {!string} property
* @return {boolean}
*/
canBind(tag, property) {
return (this.rulesForTagAndProperty_(tag, property) !== undefined);
}

/**
* Returns true if `value` is a valid result for a (tag, property) binding.
* Otherwise, returns false.
Expand All @@ -95,13 +78,8 @@ export class BindValidator {
rules = this.rulesForTagAndProperty_(tag, rules.alternativeName);
}

// If binding to (tag, property) is not allowed, return false.
if (rules === undefined) {
return false;
}

// If binding is allowed but have no specific rules, return true.
if (rules === null) {
// If there are no rules governing this binding, return true.
if (!rules) {
return true;
}

Expand Down Expand Up @@ -163,22 +141,6 @@ export class BindValidator {
}
}

// @see validator/engine/validator.ParsedTagSpec.validateAttributes()
const blockedURLs = rules.blockedURLs;
if (blockedURLs && url) {
for (let i = 0; i < blockedURLs.length; i++) {
let decodedURL;
try {
decodedURL = decodeURIComponent(url);
} catch (e) {
decodedURL = unescape(url);
}
if (decodedURL.trim() === blockedURLs[i]) {
return false;
}
}
}

return true;
}

Expand Down Expand Up @@ -213,42 +175,23 @@ export class BindValidator {
function createElementRules_() {
// Initialize `rules` with tag-specific constraints.
const rules = {
'AMP-CAROUSEL': {
slide: null,
},
'AMP-IMG': {
alt: null,
referrerpolicy: null,
src: {
allowedProtocols: {
data: true,
http: true,
https: true,
},
blockedURLs: ['__amp_source_origin'],
},
srcset: {
alternativeName: 'src',
},
},
'AMP-SELECTOR': {
selected: null,
},
'AMP-VIDEO': {
alt: null,
attribution: null,
autoplay: null,
controls: null,
loop: null,
muted: null,
placeholder: null,
poster: null,
preload: null,
src: {
allowedProtocols: {
https: true,
},
blockedURLs: ['__amp_source_origin'],
},
},
A: {
Expand All @@ -270,141 +213,28 @@ function createElementRules_() {
viber: true,
whatsapp: true,
},
blockedURLs: ['__amp_source_origin'],
},
},
BUTTON: {
disabled: null,
type: null,
value: null,
},
FIELDSET: {
disabled: null,
},
INPUT: {
accept: null,
accesskey: null,
autocomplete: null,
checked: null,
disabled: null,
height: null,
inputmode: null,
max: null,
maxlength: null,
min: null,
minlength: null,
multiple: null,
name: {
blockedURLs: ['__amp_source_origin'],
},
pattern: null,
placeholder: null,
readonly: null,
required: null,
selectiondirection: null,
size: null,
spellcheck: null,
step: null,
type: {
blacklistedValueRegex: '(^|\\s)(button|file|image|password|)(\\s|$)',
},
value: null,
width: null,
},
OPTION: {
disabled: null,
label: null,
selected: null,
value: null,
},
OPTGROUP: {
disabled: null,
label: null,
},
SELECT: {
disabled: null,
multiple: null,
name: null,
required: null,
size: null,
},
SOURCE: {
src: {
allowedProtocols: {
https: true,
},
blockedURLs: ['__amp_source_origin'],
},
type: null,
},
TRACK: {
label: null,
src: {
allowedProtocols: {
https: true,
},
blockedURLs: ['__amp_source_origin'],
},
srclang: null,
},
TEXTAREA: {
autocomplete: null,
cols: null,
disabled: null,
maxlength: null,
minlength: null,
name: null,
placeholder: null,
readonly: null,
required: null,
rows: null,
selectiondirection: null,
selectionend: null,
selectionstart: null,
spellcheck: null,
wrap: null,
},
};

// Collate all standard elements that should support [text] binding
// and add them to `rules` object.
// 4.3 Sections
const sectionTags = ['ASIDE', 'H1', 'H2', 'H3', 'H4', 'H5', 'H6',
'HEADER', 'FOOTER', 'ADDRESS'];
// 4.4 Grouping content
const groupingTags = ['P', 'PRE', 'BLOCKQUOTE', 'LI', 'DT', 'DD',
'FIGCAPTION', 'DIV'];
// 4.5 Text-level semantics
const textTags = ['A', 'EM', 'STRONG', 'SMALL', 'S', 'CITE', 'Q',
'DFN', 'ABBR', 'DATA', 'TIME', 'CODE', 'VAR', 'SAMP', 'KBD',
'SUB', 'SUP', 'I', 'B', 'U', 'MARK', 'RUBY', 'RB', 'RT', 'RTC',
'RP', 'BDI', 'BDO', 'SPAN'];
// 4.6 Edits
const editTags = ['INS', 'DEL'];
// 4.9 Tabular data
const tabularTags = ['CAPTION', 'THEAD', 'TFOOT', 'TD'];
// 4.10 Forms
const formTags = ['BUTTON', 'LABEL', 'LEGEND', 'OPTION',
'OUTPUT', 'PROGRESS', 'TEXTAREA'];
const allTextTags = sectionTags.concat(groupingTags).concat(textTags)
.concat(editTags).concat(tabularTags).concat(formTags);
allTextTags.forEach(tag => {
if (rules[tag] === undefined) {
rules[tag] = {};
}
rules[tag]['text'] = null;
});

// AMP extensions support additional properties.
const ampExtensions = ['AMP-IMG'];
ampExtensions.forEach(tag => {
if (rules[tag] === undefined) {
rules[tag] = {};
}
const tagRule = rules[tag];
tagRule['width'] = null;
tagRule['height'] = null;
});

return rules;
}
Loading

0 comments on commit ed8c2c4

Please sign in to comment.