From 69ee4688af9e86572329b525b122debfaffb5dfd Mon Sep 17 00:00:00 2001 From: Peter Burns Date: Fri, 19 Jul 2019 18:13:36 -0700 Subject: [PATCH 1/5] Add additional externs (#5575) Upstreaming cl/259031976 --- externs/polymer-externs.js | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/externs/polymer-externs.js b/externs/polymer-externs.js index 0c7b8ea458..a1b9dae11a 100644 --- a/externs/polymer-externs.js +++ b/externs/polymer-externs.js @@ -143,6 +143,29 @@ Polymer.syncInitialRender; */ var PolymerElement = function() {}; +/** + * The tag name of the cutom element type. + * @type {string|undefined} + */ +PolymerElement.is; +/** + * The template to stamp when creating this element type. + * @type {!HTMLTemplateElement|undefined} + */ +PolymerElement.template; +/** + * The properties of the cutom element type. + * @type {!PolymerElementProperties|undefined} + */ +PolymerElement.properties; +/** + * The observers of this custom element type. + * @type {!Array|undefined} + */ +PolymerElement.observers; +/** @type {!PolymerInit|undefined} */ +PolymerElement.generatedFrom; + /** * On create callback. * @override From 61767da2c0f21ac40123900c5a52cf645b21d117 Mon Sep 17 00:00:00 2001 From: Daniel Freedman Date: Tue, 30 Jul 2019 14:03:45 -0700 Subject: [PATCH 2/5] Workaround bindings to textarea.placeholder in IE Textareas have a very weird bug in IE, where the text of the placeholder is copied to the content of the textarea when set. This a creates two bugs: 1. An unintended binding is made to the textContent of the textarea's text child node, meaning updates to the `placeholder` will be an unnecessary binding process in the best case, or an exception thrown when updating the text child node in the worst case. 2. When `legacyOptimizations` is enabled, the child node of the text area is removed when the binding for `placeholder` is processed and removed, leaving a binding to a `null` node, and throwing exceptions. Therefore, when we detect this placeholder behavior, we will remove the textnode before template processing, preventing both bugs. --- lib/mixins/template-stamp.js | 46 ++++++++++++++++++++++++ test/unit/property-effects-template.html | 40 +++++++++++++++++++-- 2 files changed, 83 insertions(+), 3 deletions(-) diff --git a/lib/mixins/template-stamp.js b/lib/mixins/template-stamp.js index e7a576ec2a..736091416a 100644 --- a/lib/mixins/template-stamp.js +++ b/lib/mixins/template-stamp.js @@ -22,6 +22,49 @@ const templateExtensions = { 'dom-if': true, 'dom-repeat': true }; + +let placeholderBugDetect = false; +let placeholderBug = false; + +function hasPlaceholderBug() { + if (!placeholderBugDetect) { + const t = document.createElement('textarea'); + t.placeholder = 'a'; + placeholderBug = t.placeholder === t.textContent; + } + return placeholderBug; +} + +/** + * Some browsers have a bug with textarea, where placeholder text is copied as + * a textnode child of the textarea. + * + * If the placeholder is a binding, this can break template stamping in two + * ways. + * + * One issue is that when the `placeholder` binding is removed, the textnode + * child of the textarea is deleted, and the template info tries to bind into + * that node. + * + * When `legacyOptimizations` is enabled, the node is removed from the textarea + * when the `placeholder` binding is processed, leaving an "undefined" cell in + * the binding metadata object. + * + * When `legacyOptimizations` is disabled, the template is cloned before + * processing, and has an extra binding to the textContent of the text node + * child of the textarea. This at best is an extra binding to process that has + * no useful effect, and at worst throws exceptions trying to update the text + * node. + * + * @param {!Node} node Check node for placeholder bug + * @return {boolean} True if placeholder is bugged + */ +function shouldFixPlaceholder(node) { + return hasPlaceholderBug() + && node.localName === 'textarea' && node.placeholder + && node.placeholder === node.textContent; +} + function wrapTemplateExtension(node) { let is = node.getAttribute('is'); if (is && templateExtensions[is]) { @@ -251,6 +294,9 @@ export const TemplateStamp = dedupingMixin( // For ShadyDom optimization, indicating there is an insertion point templateInfo.hasInsertionPoint = true; } + if (shouldFixPlaceholder(node)) { + node.textContent = null; + } if (element.firstChild) { this._parseTemplateChildNodes(element, templateInfo, nodeInfo); } diff --git a/test/unit/property-effects-template.html b/test/unit/property-effects-template.html index 2f761f672a..56327de127 100644 --- a/test/unit/property-effects-template.html +++ b/test/unit/property-effects-template.html @@ -13,7 +13,6 @@ - @@ -282,9 +281,10 @@ From ab49f51a669d173d9dcbdf38b6d57f5561033bb1 Mon Sep 17 00:00:00 2001 From: Daniel Freedman Date: Tue, 30 Jul 2019 17:23:27 -0700 Subject: [PATCH 3/5] Fix up comments based on feedback --- lib/mixins/template-stamp.js | 40 +++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/lib/mixins/template-stamp.js b/lib/mixins/template-stamp.js index 736091416a..e9e4a16da7 100644 --- a/lib/mixins/template-stamp.js +++ b/lib/mixins/template-stamp.js @@ -28,6 +28,7 @@ let placeholderBug = false; function hasPlaceholderBug() { if (!placeholderBugDetect) { + placeholderBugDetect = true; const t = document.createElement('textarea'); t.placeholder = 'a'; placeholderBug = t.placeholder === t.textContent; @@ -42,27 +43,30 @@ function hasPlaceholderBug() { * If the placeholder is a binding, this can break template stamping in two * ways. * - * One issue is that when the `placeholder` binding is removed, the textnode - * child of the textarea is deleted, and the template info tries to bind into - * that node. + * One issue is that when the `placeholder` attribute is removed when the + * binding is processed, the textnode child of the textarea is deleted, and the + * template info tries to bind into that node. * - * When `legacyOptimizations` is enabled, the node is removed from the textarea - * when the `placeholder` binding is processed, leaving an "undefined" cell in - * the binding metadata object. + * With `legacyOptimizations` in use, when the template is stamped and the + * `textarea.textContent` binding is processed, no corresponding node is found + * because it was removed during parsing. An exception is generated when this + * binding is updated. * - * When `legacyOptimizations` is disabled, the template is cloned before - * processing, and has an extra binding to the textContent of the text node - * child of the textarea. This at best is an extra binding to process that has - * no useful effect, and at worst throws exceptions trying to update the text - * node. + * With `legacyOptimizations` not in use, the template is cloned before + * processing and this changes the above behavior. The cloned template also has + * a value property set to the placeholder and textContent. This prevents the + * removal of the textContent when the placeholder attribute is removed. + * Therefore the exception does not occur. However, there is an extra + * unnecessary binding. * * @param {!Node} node Check node for placeholder bug - * @return {boolean} True if placeholder is bugged + * @return {void} */ -function shouldFixPlaceholder(node) { - return hasPlaceholderBug() - && node.localName === 'textarea' && node.placeholder - && node.placeholder === node.textContent; +function fixPlaceholder(node) { + if (hasPlaceholderBug() && node.localName === 'textarea' && node.placeholder + && node.placeholder === node.textContent) { + node.textContent = null; + } } function wrapTemplateExtension(node) { @@ -294,9 +298,7 @@ export const TemplateStamp = dedupingMixin( // For ShadyDom optimization, indicating there is an insertion point templateInfo.hasInsertionPoint = true; } - if (shouldFixPlaceholder(node)) { - node.textContent = null; - } + fixPlaceholder(node); if (element.firstChild) { this._parseTemplateChildNodes(element, templateInfo, nodeInfo); } From f050ce9ef5fd7341599f7885a2f1efa75541c3fb Mon Sep 17 00:00:00 2001 From: Daniel Freedman Date: Wed, 31 Jul 2019 12:04:20 -0700 Subject: [PATCH 4/5] Fix typing error in fixPlaceholder Upstream of change in http://cl/260969835 --- lib/mixins/template-stamp.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/mixins/template-stamp.js b/lib/mixins/template-stamp.js index e9e4a16da7..33717e65de 100644 --- a/lib/mixins/template-stamp.js +++ b/lib/mixins/template-stamp.js @@ -298,7 +298,7 @@ export const TemplateStamp = dedupingMixin( // For ShadyDom optimization, indicating there is an insertion point templateInfo.hasInsertionPoint = true; } - fixPlaceholder(node); + fixPlaceholder(element); if (element.firstChild) { this._parseTemplateChildNodes(element, templateInfo, nodeInfo); } From c65a58aeae81d253ab75df4e238ed953f67e5649 Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Mon, 26 Aug 2019 16:44:40 -0700 Subject: [PATCH 5/5] [ci-skip] Add comment explaining confusing check in _addPropertyToAttributeMap --- lib/mixins/properties-changed.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/mixins/properties-changed.js b/lib/mixins/properties-changed.js index 22766a3164..cc2801d415 100644 --- a/lib/mixins/properties-changed.js +++ b/lib/mixins/properties-changed.js @@ -134,6 +134,12 @@ export const PropertiesChanged = dedupingMixin( if (!this.hasOwnProperty('__dataAttributes')) { this.__dataAttributes = Object.assign({}, this.__dataAttributes); } + // This check is technically not correct; it's an optimization that + // assumes that if a _property_ name is already in the map (note this is + // an attr->property map), the property mapped directly to the attribute + // and it has already been mapped. This would fail if + // `attributeNameForProperty` were overridden such that this was not the + // case. let attr = this.__dataAttributes[property]; if (!attr) { attr = this.constructor.attributeNameForProperty(property);