From be9aa3cebd06ad7ecaededb71df581a8ac005091 Mon Sep 17 00:00:00 2001 From: Steven Orvell Date: Wed, 27 May 2015 10:11:59 -0700 Subject: [PATCH 1/9] Correct race condition accessing textContent in created when upgrading custom-style --- src/lib/custom-style.html | 22 +++------------------- src/lib/style-defaults.html | 12 +++++------- 2 files changed, 8 insertions(+), 26 deletions(-) diff --git a/src/lib/custom-style.html b/src/lib/custom-style.html index 41424ecfd5..a422a04e20 100644 --- a/src/lib/custom-style.html +++ b/src/lib/custom-style.html @@ -79,13 +79,11 @@ extends: 'style', created: function() { - this._appliesToDocument = (this.parentNode.localName !== 'dom-module'); + this._appliesToDocument = (this.parentNode && + (this.parentNode.localName !== 'dom-module')); if (this._appliesToDocument) { // used applied element from HTMLImports polyfill or this - var e = this.__appliedElement || this; - var rules = styleUtil.rulesForStyle(e); - propertyUtils.decorateStyles([e]); - this._rulesToDefaultProperties(rules); + styleDefaults.addStyle(this.__appliedElement || this); // NOTE: go async to give a chance to collect properties into // the StyleDefaults before applying this.async(this._applyStyle); @@ -119,20 +117,6 @@ } } ); - }, - - _rulesToDefaultProperties: function(rules) { - // produce css containing only property assignments. - styleUtil.forEachStyleRule(rules, function(rule) { - if (!rule.propertyInfo.properties) { - rule.cssText = ''; - } - }); - // tell parser to emit css that includes properties. - var cssText = styleUtil.parser.stringify(rules, true); - if (cssText) { - styleDefaults.applyCss(cssText); - } } }); diff --git a/src/lib/style-defaults.html b/src/lib/style-defaults.html index 8d4789ce79..abce23d8b7 100644 --- a/src/lib/style-defaults.html +++ b/src/lib/style-defaults.html @@ -15,18 +15,16 @@ var styleProperties = Polymer.StyleProperties; var styleUtil = Polymer.StyleUtil; - var style = document.createElement('style') - var api = { - style: style, - _styles: [style], + _styles: [], _properties: null, - applyCss: function(cssText) { - this.style.textContent += cssText; - styleUtil.clearStyleRules(this.style); + addStyle: function(style) { + this._styles.push(style); this._properties = null; + // TODO(sorvell): needed here? + //this._styles._scopeStyleProperties = null; }, // NOTE: this object can be used as a styling scope so it has an api From 36edb76f406e6ad1e3927168e6ecdae48b31e04b Mon Sep 17 00:00:00 2001 From: Steven Orvell Date: Wed, 27 May 2015 15:31:21 -0700 Subject: [PATCH 2/9] defer processing style only when it has no textContent. --- src/lib/custom-style.html | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/lib/custom-style.html b/src/lib/custom-style.html index a422a04e20..f6031a1c4b 100644 --- a/src/lib/custom-style.html +++ b/src/lib/custom-style.html @@ -83,10 +83,20 @@ (this.parentNode.localName !== 'dom-module')); if (this._appliesToDocument) { // used applied element from HTMLImports polyfill or this - styleDefaults.addStyle(this.__appliedElement || this); - // NOTE: go async to give a chance to collect properties into - // the StyleDefaults before applying - this.async(this._applyStyle); + var e = this.__appliedElement || this; + styleDefaults.addStyle(e); + // we may not have any textContent yet due to parser yielding + // if so, wait until we do... + if (e.textContent) { + this._applyStyle(); + } else { + var observer = new MutationObserver(function() { + observer.disconnect(); + this._applyStyle(); + }.bind(this)); + observer.observe(e, {characterData: true, childList: true, + subtree: true}); + } } }, From c463f8aec2aacb6ad45f971c75e203652edacf29 Mon Sep 17 00:00:00 2001 From: Steven Orvell Date: Wed, 27 May 2015 18:17:08 -0700 Subject: [PATCH 3/9] test styles explicitly instead of stylesheets --- test/unit/styling-cross-scope-var.html | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/unit/styling-cross-scope-var.html b/test/unit/styling-cross-scope-var.html index b3b67e54a8..23ba60fdce 100644 --- a/test/unit/styling-cross-scope-var.html +++ b/test/unit/styling-cross-scope-var.html @@ -375,16 +375,16 @@ test('updateStyles changes property values and using style cache', function() { styled.$.child.classList.add('special'); - var l = document.styleSheets.length; + var l = document.querySelectorAll('style').length; styled.updateStyles(); if (styled.shadyRoot) { - assert.equal(document.styleSheets.length, l+4); + assert.equal(document.querySelectorAll('style').length, l+4); } assertComputed(styled.$.child.$.me, '12px'); styled.$.child.classList.remove('special'); styled.updateStyles(); if (styled.shadyRoot) { - assert.equal(document.styleSheets.length, l); + assert.equal(document.querySelectorAll('style').length, l); } assertComputed(styled.$.child.$.me, '2px'); }); From 3d1d6cc8481a976feeeec342c528db2819b9e932 Mon Sep 17 00:00:00 2001 From: Steven Orvell Date: Mon, 8 Jun 2015 12:00:13 -0700 Subject: [PATCH 4/9] avoid caching rules for styles without textContent --- src/lib/style-util.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib/style-util.html b/src/lib/style-util.html index 742064eff0..6af6a5f208 100644 --- a/src/lib/style-util.html +++ b/src/lib/style-util.html @@ -35,8 +35,8 @@ }, rulesForStyle: function(style) { - if (!style.__cssRules) { - style.__cssRules = this.parser.parse(style.textContent); + if (!style.__cssRules && style.textContent) { + style.__cssRules = this.parser.parse(style.textContent); } return style.__cssRules; }, From 94daa17ad991bc5627d8bc6626e07bbd89a527c8 Mon Sep 17 00:00:00 2001 From: Steven Orvell Date: Mon, 8 Jun 2015 12:01:55 -0700 Subject: [PATCH 5/9] Allow dynamic creation of `custom-style`; `Polymer.updateStyles` now correctly updates `custom-style` elements. --- src/lib/custom-style.html | 66 ++++++++++++++++++++++------------ src/lib/style-transformer.html | 8 +++++ src/standard/x-styling.html | 5 +++ test/unit/custom-style.html | 23 +++++++++++- 4 files changed, 78 insertions(+), 24 deletions(-) diff --git a/src/lib/custom-style.html b/src/lib/custom-style.html index b689f892b3..130d2d92f3 100644 --- a/src/lib/custom-style.html +++ b/src/lib/custom-style.html @@ -72,6 +72,7 @@ var propertyUtils = Polymer.StyleProperties; var styleUtil = Polymer.StyleUtil; var styleDefaults = Polymer.StyleDefaults; + var styleTransformer = Polymer.StyleTransformer; Polymer({ @@ -79,30 +80,41 @@ extends: 'style', created: function() { - this._appliesToDocument = (this.parentNode && - (this.parentNode.localName !== 'dom-module')); - if (this._appliesToDocument) { - // used applied element from HTMLImports polyfill or this - var e = this.__appliedElement || this; - styleDefaults.addStyle(e); - // we may not have any textContent yet due to parser yielding - // if so, wait until we do... - if (e.textContent) { - this._applyStyle(); - } else { - var observer = new MutationObserver(function() { - observer.disconnect(); - this._applyStyle(); - }.bind(this)); - observer.observe(e, {characterData: true, childList: true, - subtree: true}); + this._tryApply(); + }, + + attached: function() { + this._tryApply(); + }, + + _tryApply: function() { + if (!this._hasApplied) { + // only apply variables iff this style is not inside a dom-module + this._shouldApply = (this.parentNode && + (this.parentNode.localName !== 'dom-module')); + if (this._shouldApply) { + this._hasApplied = true; + // used applied element from HTMLImports polyfill or this + var e = this.__appliedElement || this; + styleDefaults.addStyle(e); + // we may not have any textContent yet due to parser yielding + // if so, wait until we do... + if (e.textContent) { + this._apply(); + } else { + var observer = new MutationObserver(function() { + observer.disconnect(); + this._apply(); + }.bind(this)); + observer.observe(e, {childList: true}); + } } } }, // polyfill this style with root scoping and // apply custom properties! - _applyStyle: function() { + _apply: function() { // used applied element from HTMLImports polyfill or this var e = this.__appliedElement || this; this._computeStyleProperties(); @@ -110,10 +122,6 @@ var self = this; e.textContent = styleUtil.toCssText(styleUtil.rulesForStyle(e), function(rule) { - // polyfill lack of support for :root - if (rule.selector === ':root') { - rule.selector = 'body'; - } var css = rule.cssText = rule.parsedCssText; if (rule.propertyInfo && rule.propertyInfo.cssText) { // TODO(sorvell): factor better @@ -122,11 +130,23 @@ // replace with reified properties, scenario is same as mixin rule.cssText = propertyUtils.valueForProperties(css, props); } + // TODO(sorvell): generalize this; we need to reset the selector + // here for transformation + rule.selector = rule.parsedSelector; if (!nativeShadow) { - Polymer.StyleTransformer.rootRule(rule); + styleTransformer.rootRule(rule); + } else { + // polyfill lack of support for :root + styleTransformer._normalizeRootSelector(rule); } } ); + }, + + _rootRuleToBody: function(rule) { + if (rule.selector === ':root') { + rule.selector = 'body'; + } } }); diff --git a/src/lib/style-transformer.html b/src/lib/style-transformer.html index f7f2dbc564..b052799a95 100644 --- a/src/lib/style-transformer.html +++ b/src/lib/style-transformer.html @@ -207,9 +207,16 @@ }, rootRule: function(rule) { + this._normalizeRootSelector(rule); this._transformRule(rule, this._transformRootSelector); }, + _normalizeRootSelector: function(rule) { + if (rule.selector === ROOT) { + rule.selector = 'body'; + } + }, + _transformRootSelector: function(selector) { return selector.match(SCOPE_JUMP) ? this._transformComplexSelector(selector) : @@ -225,6 +232,7 @@ var COMPLEX_SELECTOR_SEP = ','; var SIMPLE_SELECTOR_SEP = /(^|[\s>+~]+)([^\s>+~]+)/g; var HOST = ':host'; + var ROOT = ':root'; // NOTE: this supports 1 nested () pair for things like // :host(:not([selected]), more general support requires // parsing which seems like overkill diff --git a/src/standard/x-styling.html b/src/standard/x-styling.html index 42f44db3f8..d14c98c490 100644 --- a/src/standard/x-styling.html +++ b/src/standard/x-styling.html @@ -235,6 +235,11 @@ */ Polymer.updateStyles = function() { styleDefaults._styleCache.clear(); + // update custom styles + styleDefaults._styles.forEach(function(s) { + s = s.__importElement || s; + s._apply(); + }); Polymer.Base._updateRootStyles(document); }; diff --git a/test/unit/custom-style.html b/test/unit/custom-style.html index 5d5b1e3f09..091f7c2789 100644 --- a/test/unit/custom-style.html +++ b/test/unit/custom-style.html @@ -35,7 +35,9 @@ border: 1px solid black; }; - margin: 10px; + /* mocha.css in the testing environment is hosing this test; + use !important as a workaround */ + margin: 10px !important; width: auto; --special: var(--primary); @@ -64,6 +66,10 @@ .import-var { border: var(--import-var); } + + .dynamic { + border: var(--dynamic); + } @@ -71,6 +77,8 @@
bag
mix
+
dynamic
+
import-mixin
import-var
@@ -161,6 +169,19 @@ assertComputed(m, '4px'); }); + test('dynamic custom-styles apply', function(done) { + var dynamic = document.querySelector('.dynamic'); + assertComputed(dynamic, '0px'); + var ds = document.createElement('style', 'custom-style'); + ds.textContent = ':root { --dynamic: 11px solid orange; }'; + document.head.appendChild(ds); + setTimeout(function() { + Polymer.updateStyles(); + assertComputed(dynamic, '11px'); + done(); + }, 0); + }); + test('custom-styles apply normal and property values to elements', function() { var e = document.querySelector('x-foo').$.me; assertComputed(e, '1px'); From eb4f2d97d3e415962e1cc04fdea6ff66d10b4f68 Mon Sep 17 00:00:00 2001 From: Steven Orvell Date: Mon, 8 Jun 2015 17:14:25 -0700 Subject: [PATCH 6/9] slight factoring and naming changes based on review --- src/lib/custom-style.html | 13 ++----------- src/lib/style-defaults.html | 13 +++++++++++-- src/lib/style-transformer.html | 8 +++++--- src/standard/x-styling.html | 9 +++------ 4 files changed, 21 insertions(+), 22 deletions(-) diff --git a/src/lib/custom-style.html b/src/lib/custom-style.html index 130d2d92f3..e55cf1278a 100644 --- a/src/lib/custom-style.html +++ b/src/lib/custom-style.html @@ -130,23 +130,14 @@ // replace with reified properties, scenario is same as mixin rule.cssText = propertyUtils.valueForProperties(css, props); } - // TODO(sorvell): generalize this; we need to reset the selector - // here for transformation - rule.selector = rule.parsedSelector; if (!nativeShadow) { - styleTransformer.rootRule(rule); + styleTransformer.documentRule(rule); } else { // polyfill lack of support for :root - styleTransformer._normalizeRootSelector(rule); + styleTransformer.normalizeRootSelector(rule); } } ); - }, - - _rootRuleToBody: function(rule) { - if (rule.selector === ':root') { - rule.selector = 'body'; - } } }); diff --git a/src/lib/style-defaults.html b/src/lib/style-defaults.html index abce23d8b7..84390bd34b 100644 --- a/src/lib/style-defaults.html +++ b/src/lib/style-defaults.html @@ -23,8 +23,6 @@ addStyle: function(style) { this._styles.push(style); this._properties = null; - // TODO(sorvell): needed here? - //this._styles._scopeStyleProperties = null; }, // NOTE: this object can be used as a styling scope so it has an api @@ -46,6 +44,17 @@ _computeStyleProperties: function() { return this._styleProperties; + }, + + updateStyles: function() { + // invalidate the cache + this._styleCache.clear(); + // update any custom-styles we are tracking + for (var i=0, s; i < this._styles.length; i++) { + s = this._styles[i]; + s = s.__importElement || s; + s._apply(); + } } }; diff --git a/src/lib/style-transformer.html b/src/lib/style-transformer.html index b052799a95..9b57bc9e84 100644 --- a/src/lib/style-transformer.html +++ b/src/lib/style-transformer.html @@ -206,12 +206,14 @@ return p$.join(PSEUDO_PREFIX); }, - rootRule: function(rule) { - this._normalizeRootSelector(rule); + documentRule: function(rule) { + // reset selector in case this is redone. + rule.selector = rule.parsedSelector; + this.normalizeRootSelector(rule); this._transformRule(rule, this._transformRootSelector); }, - _normalizeRootSelector: function(rule) { + normalizeRootSelector: function(rule) { if (rule.selector === ROOT) { rule.selector = 'body'; } diff --git a/src/standard/x-styling.html b/src/standard/x-styling.html index d14c98c490..6364da51df 100644 --- a/src/standard/x-styling.html +++ b/src/standard/x-styling.html @@ -234,12 +234,9 @@ * to update styling. */ Polymer.updateStyles = function() { - styleDefaults._styleCache.clear(); - // update custom styles - styleDefaults._styles.forEach(function(s) { - s = s.__importElement || s; - s._apply(); - }); + // update default/custom styles + styleDefaults.updateStyles(); + // search the document for elements to update Polymer.Base._updateRootStyles(document); }; From 26b5015e28039ec68c88cba47ba54a06aa570733 Mon Sep 17 00:00:00 2001 From: Steven Orvell Date: Mon, 8 Jun 2015 17:25:53 -0700 Subject: [PATCH 7/9] track state of applying style more explicitly. --- src/lib/custom-style.html | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/lib/custom-style.html b/src/lib/custom-style.html index e55cf1278a..cdcfd3f660 100644 --- a/src/lib/custom-style.html +++ b/src/lib/custom-style.html @@ -80,6 +80,8 @@ extends: 'style', created: function() { + // NOTE: we cannot just check attached because custom elements in + // HTMLImports do not get attached. this._tryApply(); }, @@ -88,12 +90,10 @@ }, _tryApply: function() { - if (!this._hasApplied) { + if (!(this._hasApplied || this._isApplying)) { // only apply variables iff this style is not inside a dom-module - this._shouldApply = (this.parentNode && - (this.parentNode.localName !== 'dom-module')); - if (this._shouldApply) { - this._hasApplied = true; + if (this.parentNode && + (this.parentNode.localName !== 'dom-module')) { // used applied element from HTMLImports polyfill or this var e = this.__appliedElement || this; styleDefaults.addStyle(e); @@ -102,8 +102,10 @@ if (e.textContent) { this._apply(); } else { + this._isApplying = true; var observer = new MutationObserver(function() { observer.disconnect(); + this._isApplying = false; this._apply(); }.bind(this)); observer.observe(e, {childList: true}); @@ -115,6 +117,7 @@ // polyfill this style with root scoping and // apply custom properties! _apply: function() { + this._hasApplied = true; // used applied element from HTMLImports polyfill or this var e = this.__appliedElement || this; this._computeStyleProperties(); From 1df4076bac4e63fd3e1a42c1195facfaa993331b Mon Sep 17 00:00:00 2001 From: Steven Orvell Date: Mon, 8 Jun 2015 18:00:27 -0700 Subject: [PATCH 8/9] slight simplification --- src/lib/custom-style.html | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/lib/custom-style.html b/src/lib/custom-style.html index cdcfd3f660..1fb6ddbbc7 100644 --- a/src/lib/custom-style.html +++ b/src/lib/custom-style.html @@ -90,10 +90,11 @@ }, _tryApply: function() { - if (!(this._hasApplied || this._isApplying)) { + if (!this._appliesToDocument) { // only apply variables iff this style is not inside a dom-module if (this.parentNode && (this.parentNode.localName !== 'dom-module')) { + this._appliesToDocument = true; // used applied element from HTMLImports polyfill or this var e = this.__appliedElement || this; styleDefaults.addStyle(e); @@ -102,10 +103,8 @@ if (e.textContent) { this._apply(); } else { - this._isApplying = true; var observer = new MutationObserver(function() { observer.disconnect(); - this._isApplying = false; this._apply(); }.bind(this)); observer.observe(e, {childList: true}); @@ -117,7 +116,6 @@ // polyfill this style with root scoping and // apply custom properties! _apply: function() { - this._hasApplied = true; // used applied element from HTMLImports polyfill or this var e = this.__appliedElement || this; this._computeStyleProperties(); From bb95be8e838d8d3669c79afd0e7d2713265ec79d Mon Sep 17 00:00:00 2001 From: Steven Orvell Date: Mon, 8 Jun 2015 18:04:06 -0700 Subject: [PATCH 9/9] move shadow root knowledge to style transformer. --- src/lib/custom-style.html | 7 +------ src/lib/style-transformer.html | 4 +++- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/lib/custom-style.html b/src/lib/custom-style.html index 1fb6ddbbc7..1eb97416ac 100644 --- a/src/lib/custom-style.html +++ b/src/lib/custom-style.html @@ -131,12 +131,7 @@ // replace with reified properties, scenario is same as mixin rule.cssText = propertyUtils.valueForProperties(css, props); } - if (!nativeShadow) { - styleTransformer.documentRule(rule); - } else { - // polyfill lack of support for :root - styleTransformer.normalizeRootSelector(rule); - } + styleTransformer.documentRule(rule); } ); } diff --git a/src/lib/style-transformer.html b/src/lib/style-transformer.html index 9b57bc9e84..e443015d5a 100644 --- a/src/lib/style-transformer.html +++ b/src/lib/style-transformer.html @@ -210,7 +210,9 @@ // reset selector in case this is redone. rule.selector = rule.parsedSelector; this.normalizeRootSelector(rule); - this._transformRule(rule, this._transformRootSelector); + if (!nativeShadow) { + this._transformRule(rule, this._transformRootSelector); + } }, normalizeRootSelector: function(rule) {