From 1a0aa55fb1e6b6eb6bc115ba307b19e389297556 Mon Sep 17 00:00:00 2001 From: Bill Keese Date: Thu, 19 Mar 2015 20:49:49 +0900 Subject: [PATCH] Call deliver() automatically when widgets created. Runs both for programatically and declarative widgets, after the user-specified parameters have been applied. Note that when widgets are created programatically, deliver() will be called even if the widget isn't yet attached to the DOM. Avoids widgets having a stale state (i.e. DOM nodes not updated or computed properties not computed) when applications access the widget immediately after it has been created, before the widget is updated automatically due to the timer firing. Fixes #385. --- Bidi.js | 10 +- CustomElement.js | 99 +++++++++++--------- register.js | 3 + tests/unit/CssState.js | 1 - tests/unit/FormValueWidget.js | 1 - tests/unit/FormWidget.js | 51 ++++++----- tests/unit/Scrollable.js | 2 - tests/unit/Widget.js | 12 +-- tests/unit/handlebars.js | 107 +++++++--------------- tests/unit/resources/Scrollable-shared.js | 9 +- 10 files changed, 129 insertions(+), 166 deletions(-) diff --git a/Bidi.js b/Bidi.js index 932c528a2e..a876c1fd6b 100644 --- a/Bidi.js +++ b/Bidi.js @@ -37,12 +37,20 @@ define([ * @protected */ getInheritedDir: function () { - if (has("inherited-dir")) { + if (this.attached && has("inherited-dir")) { return window.getComputedStyle(this, null).direction; } return this.ownerDocument.body.dir || this.ownerDocument.documentElement.dir || "ltr"; }, + attachedCallback: function () { + if (has("inherited-dir")) { + // Now that the widget is attached to the DOM, need to retrigger computation of effectiveDir. + this.notifyCurrentValue("dir"); + this.deliver(); + } + }, + /** * Returns the right direction of text. * diff --git a/CustomElement.js b/CustomElement.js index e16cfcb786..0c5f6576d8 100644 --- a/CustomElement.js +++ b/CustomElement.js @@ -162,6 +162,50 @@ define([ this[pa.prop] = pa.value; } }, this); + + if (!has("setter-on-native-prop")) { + var setterMap = this._nativePropSetterMap, + attrs = Object.keys(setterMap); + + // Call custom setters for initial values of attributes with shadow properties (dir, tabIndex, etc) + attrs.forEach(function (attrName) { + if (this.hasAttribute(attrName)) { // initial value was specified + var value = this.getAttribute(attrName); + this.removeAttribute(attrName); + setterMap[attrName].call(this, value); // call custom setter + } + }, this); + + // Begin watching for changes to those DOM attributes. + // Note that (at least on Chrome) I could use attributeChangedCallback() instead, which is + // synchronous, so Widget#deliver() will work as expected, but OTOH gets lots of notifications + // that I don't care about. + // If Polymer is loaded, use MutationObserver rather than WebKitMutationObserver + // to avoid error about "referencing a Node in a context where it does not exist". + /* global WebKitMutationObserver */ + var MO = window.MutationObserver || WebKitMutationObserver; // for jshint + var observer = new MO(function (records) { + records.forEach(function (mr) { + var attrName = mr.attributeName, + setter = setterMap[attrName], + newValue = this.getAttribute(attrName); + if (newValue !== null) { + this.removeAttribute(attrName); + setter.call(this, newValue); + } + }, this); + }.bind(this)); + observer.observe(this, { + subtree: false, + attributeFilter: attrs, + attributes: true + }); + } + + // TODO: deliver() should be called from Widget but I can't get dcl.advise() w/around advice to work. + if (this.deliver) { + this.deliver(); + } } }), @@ -182,52 +226,17 @@ define([ * @method * @fires module:delite/CustomElement#customelement-attached */ - attachedCallback: dcl.after(function () { - this.attached = true; - this.emit("customelement-attached", { - bubbles: false, - cancelable: false - }); - - // Prevent against repeated calls - this.attachedCallback = nop; - - if (!has("setter-on-native-prop")) { - var setterMap = this._nativePropSetterMap, - attrs = Object.keys(setterMap); - - // Call custom setters for initial values of attributes with shadow properties (dir, tabIndex, etc) - attrs.forEach(function (attrName) { - if (this.hasAttribute(attrName)) { // initial value was specified - var value = this.getAttribute(attrName); - this.removeAttribute(attrName); - setterMap[attrName].call(this, value); // call custom setter - } - }, this); + attachedCallback: dcl.advise({ + before: function () { + this.attached = true; - // Begin watching for changes to those DOM attributes. - // Note that (at least on Chrome) I could use attributeChangedCallback() instead, which is synchronous, - // so Widget#deliver() will work as expected, but OTOH gets lots of notifications - // that I don't care about. - // If Polymer is loaded, use MutationObserver rather than WebKitMutationObserver - // to avoid error about "referencing a Node in a context where it does not exist". - /* global WebKitMutationObserver */ - var MO = window.MutationObserver || WebKitMutationObserver; // for jshint - var observer = new MO(function (records) { - records.forEach(function (mr) { - var attrName = mr.attributeName, - setter = setterMap[attrName], - newValue = this.getAttribute(attrName); - if (newValue !== null) { - this.removeAttribute(attrName); - setter.call(this, newValue); - } - }, this); - }.bind(this)); - observer.observe(this, { - subtree: false, - attributeFilter: attrs, - attributes: true + // Prevent against repeated calls + this.attachedCallback = nop; + }, + after: function () { + this.emit("customelement-attached", { + bubbles: false, + cancelable: false }); } }), diff --git a/register.js b/register.js index e0202be6c5..6af95c1616 100644 --- a/register.js +++ b/register.js @@ -254,6 +254,9 @@ define([ node[name] = params[name]; } } + if (node.deliver) { + node.deliver(); + } return node; }; diff --git a/tests/unit/CssState.js b/tests/unit/CssState.js index cecf628e10..7bb35cd1f1 100644 --- a/tests/unit/CssState.js +++ b/tests/unit/CssState.js @@ -24,7 +24,6 @@ define([ disabled: true, checked: true }); - widget.deliver(); assert($(widget).hasClass("d-error"), "error state"); assert($(widget).hasClass("d-disabled"), "disabled"); diff --git a/tests/unit/FormValueWidget.js b/tests/unit/FormValueWidget.js index ac6bd9c709..ce555ad395 100644 --- a/tests/unit/FormValueWidget.js +++ b/tests/unit/FormValueWidget.js @@ -112,7 +112,6 @@ define([ myWidget.deliver(); assert(myWidget.focusNode.readOnly, "readOnly set on focusNode"); - myWidget.readOnly = false; myWidget.deliver(); assert.isFalse(myWidget.focusNode.readOnly, "readOnly not set on focusNode"); diff --git a/tests/unit/FormWidget.js b/tests/unit/FormWidget.js index 7b7f1c974a..f5077a311e 100644 --- a/tests/unit/FormWidget.js +++ b/tests/unit/FormWidget.js @@ -70,7 +70,6 @@ define([ assert(myWidget.valueNode.disabled, "disabled set on valueNode"); assert(myWidget.focusNode.disabled, "disabled set on focusNode"); - myWidget.disabled = false; myWidget.deliver(); assert.isFalse(myWidget.valueNode.disabled, "disabled not set on valueNode"); @@ -78,36 +77,41 @@ define([ }, "#tabIndex": function () { + // When !has("setter-on-native-prop"), tabIndex changes reported asynchronously even if you call + // this.deliver(). See code in CustomElement.js. + var d = this.async(1000); + // default tabIndex var myWidget = new FormWidgetTest(); - myWidget.deliver(); - assert.strictEqual(myWidget.focusNode.getAttribute("tabindex"), "0", "default tabIndex"); - assert.isFalse(HTMLElement.prototype.hasAttribute.call(myWidget, "tabindex"), "no tabIndex on root 1"); - - // specify initial tabIndex - myWidget = new FormWidgetTest({ - tabIndex: "3" - }); - myWidget.placeAt(container); - //myWidget.setAttribute("tabindex", 3); - myWidget.deliver(); - assert.strictEqual(myWidget.focusNode.getAttribute("tabindex"), "3", "specified tabIndex"); - assert.isFalse(HTMLElement.prototype.hasAttribute.call(myWidget, "tabindex"), "no tabIndex on root 2"); - - // Change tabIndex. At least on Chrome, this always happens asynchronously even if you call deliver(). - myWidget.tabIndex = 4; - setTimeout(this.async().callback(function () { - assert.strictEqual(myWidget.focusNode.getAttribute("tabindex"), "4", "changed tabIndex"); + setTimeout(d.rejectOnError(function () { + assert.strictEqual(myWidget.focusNode.getAttribute("tabindex"), "0", "default tabIndex"); assert.isFalse(HTMLElement.prototype.hasAttribute.call(myWidget, "tabindex"), - "no tabIndex on root 3"); - }), 100); + "no tabIndex on root 1"); + + // specify initial tabIndex + myWidget = new FormWidgetTest({ + tabIndex: "3" + }); + setTimeout(d.rejectOnError(function () { + assert.strictEqual(myWidget.focusNode.getAttribute("tabindex"), "3", "specified tabIndex"); + assert.isFalse(HTMLElement.prototype.hasAttribute.call(myWidget, "tabindex"), + "no tabIndex on root 2"); + + // Change tabIndex. + myWidget.tabIndex = 4; + setTimeout(d.callback(function () { + assert.strictEqual(myWidget.focusNode.getAttribute("tabindex"), "4", "changed tabIndex"); + assert.isFalse(HTMLElement.prototype.hasAttribute.call(myWidget, "tabindex"), + "no tabIndex on root 3"); + }), 10); + }), 10); + }), 10); }, "#alt": function () { var myWidget = new FormWidgetTest({ alt: "hello world" }); - myWidget.deliver(); assert.strictEqual(myWidget.focusNode.getAttribute("alt"), "hello world"); }, @@ -115,7 +119,6 @@ define([ var myWidget = new FormWidgetTest({ name: "bob" }); - myWidget.deliver(); assert.strictEqual(myWidget.valueNode.getAttribute("name"), "bob"); } }, @@ -155,7 +158,6 @@ define([ register.parse(container); var myWidget = container.firstChild; - myWidget.deliver(); // tabIndex assert.strictEqual(myWidget.field1.getAttribute("tabindex"), "0", "field1 default tabIndex"); @@ -204,7 +206,6 @@ define([ assert(myWidget.field1.disabled, "disabled set on field1"); assert(myWidget.field2.disabled, "disabled set on field2"); - myWidget.disabled = false; myWidget.deliver(); assert.isFalse(myWidget.valueNode.disabled, "disabled not set on valueNode"); diff --git a/tests/unit/Scrollable.js b/tests/unit/Scrollable.js index 11ad95c48c..d02924413b 100644 --- a/tests/unit/Scrollable.js +++ b/tests/unit/Scrollable.js @@ -42,7 +42,6 @@ define([ "Default CSS": function () { var w = (new MyScrollableWidget({id: "mysw"})).placeAt(container); - w.deliver(); assert.strictEqual(w.scrollableNode, w, "The scrollableNode should be the widget itself!"); @@ -71,7 +70,6 @@ define([ }) }); var w = (new ScrollableWithCustomScrollableNode()).placeAt(container); - w.deliver(); assert.strictEqual(w.scrollableNode, w.createdScrollableNode, "The scrollableNode property has changed since it was set in render!"); // Test that the CSS class d-scrollable has been added by the mixin delite/Scrollable diff --git a/tests/unit/Widget.js b/tests/unit/Widget.js index b62b257278..6f2745f8c2 100644 --- a/tests/unit/Widget.js +++ b/tests/unit/Widget.js @@ -115,14 +115,7 @@ define([ assert.isFalse(declarative.hasAttribute("tabindex"), "tabindex attr removed"); assert.isTrue(declarative.isrange, "isrange set"); assert.isTrue(declarative.isbool, "isbool set"); - - var d = this.async(1000); - - setTimeout(d.callback(function () { - assert.strictEqual(declarative.value, "5", "value"); - }), 10); - - return d; + assert.strictEqual(declarative.value, "5", "value"); }, notInPrototype: function () { @@ -199,8 +192,6 @@ define([ }, declarative: function () { - // And also test for declarative widgets, to make sure the tabIndex property is - // removed from the root node, to prevent an extra tab stop container.innerHTML = ""; var declarative = document.getElementById("dirTest"); register.parse(container); @@ -233,7 +224,6 @@ define([ var myWidget = new TestWidget(); container.appendChild(myWidget); myWidget.attachedCallback(); - myWidget.deliver(); assert($(myWidget).hasClass("base2"), "baseClass is base2"); diff --git a/tests/unit/handlebars.js b/tests/unit/handlebars.js index ef4372294d..858f3eb961 100644 --- a/tests/unit/handlebars.js +++ b/tests/unit/handlebars.js @@ -31,7 +31,6 @@ define([ }); var myWidget = new MyWidget({}); - myWidget.deliver(); assert.strictEqual(myWidget.textContent.trim(), "number 0, boolean false, string hi, null"); }, @@ -44,7 +43,6 @@ define([ template: buttonHBTmpl }); myButton = new TestButton(); - myButton.deliver(); assert.strictEqual(myButton.tagName.toLowerCase(), "button", "root node exists"); assert.strictEqual(myButton.firstChild.tagName.toLowerCase(), "span", "icon node exists too"); assert.strictEqual(myButton.firstChild.className, "d-reset originalClass", "icon class set"); @@ -93,7 +91,6 @@ define([ ) }); var mySpecialPropsWidget = new SpecialPropsWidget(); - mySpecialPropsWidget.deliver(); var input = mySpecialPropsWidget.children[0]; assert.strictEqual(input.value, "original value", "value set as property"); @@ -130,16 +127,10 @@ define([ multiple: true }); - var d = this.async(1000); - - setTimeout(d.callback(function () { - var select = myWidget.select; - assert.strictEqual(select.getAttribute("foo"), "2", "foo"); - assert.strictEqual(select.size, 2, "size"); - assert.strictEqual(select.multiple, true, "multiple"); - }), 100); - - return d; + var select = myWidget.select; + assert.strictEqual(select.getAttribute("foo"), "2", "foo"); + assert.strictEqual(select.size, 2, "size"); + assert.strictEqual(select.multiple, true, "multiple"); }, autocorrect: function () { @@ -166,7 +157,6 @@ define([ "") }); var myList = new TestList(); - myList.deliver(); assert.strictEqual(myList.tagName.toLowerCase(), "ul", "root node exists"); assert.strictEqual(myList.firstChild.tagName.toLowerCase(), "li", "child exists"); @@ -241,26 +231,19 @@ define([ assert.ok(headingWidget.attached, "heading widget was attached"); assert.ok(buttonWidget.attached, "button widget was attached"); - var d = this.async(1000); - - setTimeout(d.rejectOnError(function () { - assert.strictEqual(headingWidget.textContent, "original heading", - "heading widget got title from main widget"); - assert.strictEqual(buttonWidget.textContent.trim(), "original button label", - "button widget got label from main widget"); - - myComplexWidget.mix({ - heading: "new heading", - buttonLabel: "new button label" - }); + assert.strictEqual(headingWidget.textContent, "original heading", + "heading widget got title from main widget"); + assert.strictEqual(buttonWidget.textContent.trim(), "original button label", + "button widget got label from main widget"); - setTimeout(d.callback(function () { - assert.strictEqual(headingWidget.textContent, "new heading", "heading changed"); - assert.strictEqual(buttonWidget.textContent.trim(), "new button label", "button changed"); - }), 100); - }), 100); + myComplexWidget.mix({ + heading: "new heading", + buttonLabel: "new button label" + }); + myComplexWidget.deliver(); - return d; + assert.strictEqual(headingWidget.textContent, "new heading", "heading changed"); + assert.strictEqual(buttonWidget.textContent.trim(), "new button label", "button changed"); }, buttons: function () { @@ -275,7 +258,6 @@ define([ }); var testWidget = new ButtonWidget(); testWidget.placeAt(container); - testWidget.deliver(); // Make sure all the buttons are siblings of each other, not parent-child. assert.strictEqual(testWidget.children.length, 3, "direct children"); @@ -301,7 +283,6 @@ define([ }); var myComplexWidget = new ComplexWidget(); - myComplexWidget.deliver(); assert.strictEqual(myComplexWidget.firstElementChild.vertical, false, "prop is false not 'false'"); assert.strictEqual(myComplexWidget.firstElementChild.className, "horizontal", "className"); @@ -318,7 +299,6 @@ define([ }); var myComplexWidget = new ComplexWidget(); - myComplexWidget.deliver(); assert.strictEqual(myComplexWidget.firstElementChild.vertical, false, "prop is false not 'false'"); assert.strictEqual(myComplexWidget.firstElementChild.className, "horizontal", "className"); @@ -487,7 +467,6 @@ define([ }); var node = new TestUndefined(); - node.deliver(); assert.strictEqual(node.className, "", "class #1"); assert.strictEqual(node.textContent.trim(), "Hello Bob !", "textContent #1"); @@ -512,35 +491,25 @@ define([ "Hello {{item.first}} {{item.last}}!") }); - var d = this.async(1000); - var node = new TestNested(); + assert.strictEqual(node.className, "", "class #1"); + assert.strictEqual(node.textContent.trim(), "Hello Bob !", "textContent #1"); + node.item = { + first: "Tom" + }; + node.deliver(); + assert.strictEqual(node.className, "", "class #2"); + assert.strictEqual(node.textContent.trim(), "Hello Tom !", "textContent #2"); - setTimeout(d.rejectOnError(function () { - assert.strictEqual(node.className, "", "class #1"); - assert.strictEqual(node.textContent.trim(), "Hello Bob !", "textContent #1"); - - node.item = { - first: "Tom" - }; - setTimeout(d.rejectOnError(function () { - assert.strictEqual(node.className, "", "class #2"); - assert.strictEqual(node.textContent.trim(), "Hello Tom !", "textContent #2"); - - node.item = { - first: "Fred", - last: "Smith", - className: "blue" - }; - setTimeout(d.callback(function () { - assert.strictEqual(node.className, "blue", "class #3"); - assert.strictEqual(node.textContent.trim(), "Hello Fred Smith!", "textContent #3"); - }), 100); - }), 100); - }), 100); - - return d; + node.item = { + first: "Fred", + last: "Smith", + className: "blue" + }; + node.deliver(); + assert.strictEqual(node.className, "blue", "class #3"); + assert.strictEqual(node.textContent.trim(), "Hello Fred Smith!", "textContent #3"); }, aria: function () { @@ -558,7 +527,6 @@ define([ // Initial values var node = new TestAria(); node.placeAt(container); - node.deliver(); assert(node.hasAttribute("aria-valuenow"), "aria-valuenow exists"); assert.strictEqual(node.getAttribute("aria-valuenow"), "", "aria-valuenow initial value"); assert.strictEqual(node.getAttribute("aria-selected"), "true", "aria-selected initial value"); @@ -604,7 +572,6 @@ define([ var node = new TestNested(); node.placeAt(container); - node.deliver(); assert.strictEqual(getComputedStyle(node).display, "inline", "not hidden"); node.showSpan = false; @@ -666,7 +633,6 @@ define([ // Initial values var node = new TextExpr(); node.placeAt(container); - node.deliver(); assert.strictEqual(node.textContent.trim(), "1 + 1 = 2"); assert.strictEqual(node.getAttribute("d-shown"), "true", "d-shown"); @@ -707,7 +673,6 @@ define([ var node = new ImgWidget(); node.placeAt(container); - node.deliver(); assert.strictEqual(node.firstElementChild.nodeName.toLowerCase(), "img", "tag name"); assert(/plus.gif/.test(node.firstElementChild.src, "img src set")); @@ -740,7 +705,6 @@ define([ container.innerHTML += ""; var cwdc = container.lastChild; register.upgrade(cwdc); - cwdc.deliver(); assert.strictEqual(cwdc.className, "userDefinedClass constClass myBaseClass", "declarative const"); @@ -748,7 +712,6 @@ define([ container.innerHTML += ""; var cwdv = container.lastChild; register.upgrade(cwdv); - cwdv.deliver(); assert.strictEqual(cwdv.className, "userDefinedClass myTemplateClass1 myBaseClass", "declarative var, before update"); @@ -761,21 +724,19 @@ define([ var cwpc = new ClassConstWidget({ className: "userDefinedClass" }); - cwpc.deliver(); - assert.strictEqual(cwpc.className, "constClass userDefinedClass myBaseClass", + assert.strictEqual(cwpc.className, "constClass myBaseClass userDefinedClass", "programmatic const"); // Test programmatic with class={{foo}} var cwpv = new ClassVarWidget({ className: "userDefinedClass" }); - cwpv.deliver(); - assert.strictEqual(cwpv.className, "userDefinedClass myTemplateClass1 myBaseClass", + assert.strictEqual(cwpv.className, "myTemplateClass1 myBaseClass userDefinedClass", "programmatic var, before update"); cwpv.templateClass = "myTemplateClass2"; cwpv.deliver(); - assert.strictEqual(cwpv.className, "userDefinedClass myBaseClass myTemplateClass2", + assert.strictEqual(cwpv.className, "myBaseClass userDefinedClass myTemplateClass2", "programmatic var, after update"); }, diff --git a/tests/unit/resources/Scrollable-shared.js b/tests/unit/resources/Scrollable-shared.js index 987f3278b5..0e8329436f 100755 --- a/tests/unit/resources/Scrollable-shared.js +++ b/tests/unit/resources/Scrollable-shared.js @@ -5,7 +5,7 @@ define([ ], function (registerSuite, assert, $) { // Test cases for both delite/Scrollable and deliteful/ScrollableContainer. - // Since we can not reuse test files accross projects, there are two copies of + // Since we can not reuse test files across projects, there are two copies of // this file, one in delite/unit/tests/resources and another in // deliteful/unit/tests/resources. They need to be kept in sync. @@ -18,14 +18,12 @@ define([ shared.testCases = { "Default CSS": function () { var w = document.getElementById("sc1"); - w.deliver(); assert.isTrue($(w).hasClass(shared.containerCSSClassName), "Expecting " + shared.containerCSSClassName + " CSS class! (id='sc1')"); assert.isTrue($(w).hasClass("d-scrollable"), // class added by the mixin delite/Scrollable "Expecting d-scrollable CSS class! (id='sc1')"); w = document.getElementById("sc2"); // with scrollDirection == "none" - w.deliver(); assert.strictEqual(w.scrollDirection, "none", "wrong scroll direction for id=sc2!"); assert.isTrue($(w).hasClass(shared.containerCSSClassName), "Expecting " + shared.containerCSSClassName + " CSS class! (id='sc2')"); @@ -34,7 +32,6 @@ define([ "Not expecting d-scrollable CSS class! (id='sc2')"); w = document.getElementById("mysc1"); - w.deliver(); assert.isTrue($(w).hasClass(shared.containerCSSClassName), "Expecting " + shared.containerCSSClassName + " CSS class! (id='mysc1')"); assert.isTrue($(w).hasClass("d-scrollable"), // class added by the mixin delite/Scrollable @@ -43,7 +40,6 @@ define([ "CSS class dependency on scrollDirection": function () { var w = document.getElementById("sc1"); - w.deliver(); assert.isTrue($(w).hasClass(shared.containerCSSClassName), "Expecting " + shared.containerCSSClassName + " CSS class! (id='sc1')"); assert.isTrue($(w).hasClass("d-scrollable"), // class added by the mixin delite/Scrollable @@ -89,8 +85,7 @@ define([ "scrollableNode": function () { var w = document.getElementById("sc1"); - w.deliver(); - assert.isTrue(w.scrollableNode === w, "Wrong scrollableNode!"); + assert.strictEqual(w.scrollableNode, w, "Wrong scrollableNode!"); }, "scrollTop/scrollLeft": function () {