Skip to content

Commit

Permalink
Don't run html parser against the default contentBox generated from
Browse files Browse the repository at this point in the history 
CONTENT_TEMPLATE. Only run it if the user (or component) provided a
srcNode (or contentBox) node reference.

Fixes #2532074
  • Loading branch information
@sdesai
sdesai committed May 15, 2012
1 parent bf36b96 commit 712600e
Show file tree
Hide file tree
Showing 4 changed files with 85 additions and 16 deletions.
7 changes: 7 additions & 0 deletions src/widget/HISTORY.md
View file
@@ -1,6 +1,13 @@
Widget Change History
=====================

3.6.0
-----

* Widget no longer runs html parser against the default contentBox created from
CONTENT_TEMPLATE, so that html parser implementations don't need to check for
null results for missing nodes.

3.5.1
-----

Expand Down
22 changes: 18 additions & 4 deletions src/widget/js/Widget.js
View file
Expand Up @@ -763,7 +763,7 @@ Y.extend(Widget, Y.Base, {
* @return Node
*/
_setBB: function(node) {
return this._setBox(this.get(ID), node, this.BOUNDING_TEMPLATE);
return this._setBox(this.get(ID), node, this.BOUNDING_TEMPLATE, true);
},

/**
Expand All @@ -775,7 +775,7 @@ Y.extend(Widget, Y.Base, {
* @return Node
*/
_setCB: function(node) {
return (this.CONTENT_TEMPLATE === null) ? this.get(BOUNDING_BOX) : this._setBox(null, node, this.CONTENT_TEMPLATE);
return (this.CONTENT_TEMPLATE === null) ? this.get(BOUNDING_BOX) : this._setBox(null, node, this.CONTENT_TEMPLATE, false);
},

/**
Expand All @@ -801,13 +801,27 @@ Y.extend(Widget, Y.Base, {
* @param {String} id The node's id attribute
* @param {Node|String} node The node reference
* @param {String} template HTML string template for the node
* @param {boolean} true if this is the boundingBox, false if it's the contentBox
* @return {Node} The node
*/
_setBox : function(id, node, template) {
node = Node.one(node) || Node.create(template);
_setBox : function(id, node, template, isBounding) {

node = Node.one(node);

if (!node) {
node = Node.create(template);

if (isBounding) {
this._bbFromTemplate = true;
} else {
this._cbFromTemplate = true;
}
}

if (!node.get(ID)) {
node.set(ID, id || Y.guid());
}

return node;
},

Expand Down
20 changes: 17 additions & 3 deletions src/widget/js/WidgetHTMLParser.js
View file
Expand Up @@ -6,7 +6,6 @@
* @for Widget
*/


var Widget = Y.Widget,
Node = Y.Node,
Lang = Y.Lang,
Expand Down Expand Up @@ -91,7 +90,7 @@ Y.mix(Widget.prototype, {
},

/**
* Utilitity method used to apply the <code>HTML_PARSER</code> configuration for the
* Utility method used to apply the <code>HTML_PARSER</code> configuration for the
* instance, to retrieve config data values.
*
* @method _applyParser
Expand All @@ -101,7 +100,7 @@ Y.mix(Widget.prototype, {
_applyParser : function(config) {

var widget = this,
srcNode = widget.get(SRC_NODE),
srcNode = this._getNodeToParse(),
schema = widget._getHtmlParser(),
parsedConfig,
val;
Expand Down Expand Up @@ -132,6 +131,21 @@ Y.mix(Widget.prototype, {
config = widget._applyParsedConfig(srcNode, config, parsedConfig);
},

/**
* Determines whether we have a node reference which we should try and parse.
*
* The current implementation does not parse nodes generated from CONTENT_TEMPLATE,
* only explicitly set srcNode, or contentBox attributes.
*
* @method _getNodeToParse
* @return {Node} The node reference to apply HTML_PARSER to.
* @private
*/
_getNodeToParse : function() {
var srcNode = this.get("srcNode");
return (!this._cbFromTemplate) ? srcNode : null;
},

/**
* Gets the HTML_PARSER definition for this instance, by merging HTML_PARSER
* definitions across the class hierarchy.
Expand Down
52 changes: 43 additions & 9 deletions src/widget/tests/widget.html
View file
Expand Up @@ -4,7 +4,7 @@
<title>Widget Test Suite</title>

<script src="../../../build/yui/yui.js"></script>
<!--script src="http://yui.yahooapis.com/3.5.0pr2/build/yui/yui.js"></script-->
<!--script src="http://yui.yahooapis.com/3.5.1/build/yui/yui.js"></script-->

<style type="text/css">
#testConsole .yui3-console-entry {
Expand Down Expand Up @@ -1713,10 +1713,7 @@
// FIXME: Ideally we should be able to do a this.get("titleNode") here. However,
// HTML_PARSER is called during widget initializer, when sub-class attrs aren't yet configured
// Moving all attribute set up to before all initializers, should allow us to do this.
var labelHolder = srcNode.one("> h1");
if (labelHolder) {
return labelHolder.get("text");
}
return srcNode.one("> h1").get("text");
},
listNodes: ["> ul > li"],
titleNode: "> h1"
Expand Down Expand Up @@ -1851,6 +1848,35 @@
w.destroy();
},

"testHTMLParserWithContentBox" : function() {

// HTML to Parse
var markup = Y.Node.create('<div id="customWidgetContent"><h1>My Label</h1><ul><li>Item1</li><li>Item2</li></ul></div>');
Y.Node.one("body").append(markup);

var w = new MyCustomWidget({
contentBox: "#customWidgetContent"
});

Y.Assert.areEqual("My Label", w.get("label"), "label not picked up from markup");
Y.Assert.areEqual("h1", w.get("titleNode").get("tagName").toLowerCase(), "titleNode not picked up from markup");

Y.Assert.areEqual(2, w.get("listNodes").size(), "listNodes count does not match markup");

Y.Assert.areSame("li", w.get("listNodes").item(0).get("tagName").toLowerCase(), "listNode 0 not picked up from markup");
Y.Assert.areSame("li", w.get("listNodes").item(1).get("tagName").toLowerCase(), "listNode 1 not picked up from markup");

Y.Assert.areSame("Item1", w.get("listNodes").item(0).get("text"), "listNode 0 not picked up from markup");
Y.Assert.areSame("Item2", w.get("listNodes").item(1).get("text"), "listNode 1 not picked up from markup");

w.destroy();
},

"testHTMLParserWithNoSrcNodeOrContentBox" : function() {
var w = new MyCustomWidget();
w.destroy();
},

"testHTMLParserWithEmptyConfig" : function() {

// See http://yuilibrary.com/projects/yui3/ticket/2531501
Expand Down Expand Up @@ -2143,6 +2169,7 @@
w1,
w2;

// Should be sync, because widget is already on the page
Y1 = YUI().use('widget', function (Y1) {

w1 = new Y1.Widget({render:true});
Expand All @@ -2152,6 +2179,7 @@
actualEvents.push("clickOuter");
});

// Should be sync, because widget is already on the page
Y2 = YUI().use('widget', function (Y2) {

w2 = new Y2.Widget({render:".w2-container"});
Expand All @@ -2171,11 +2199,17 @@
w1.destroy();
w2.destroy();

Y1.destroy();
Y2.destroy();
// Destroying Y instances throws errors when re-using modules which have conditionally loaded parts.
// Looks like a setTimeout() is being introduced to the use. Need to look into, but it's not
// related to this test. The lines below are effectively an async tearDown.
this.wait(function() {
Y1.destroy();
Y2.destroy();
}, 0);
},

testPublishDefaultFn : function() {

var w = new Y.Widget({
render:true
}),
Expand Down Expand Up @@ -2340,10 +2374,10 @@
runButton.set("value", "Run Tests");
runButton.set("disabled", false).on("click", function() {
if (!testConsole) {

// This test should be ignored if Console is instantiated.
coreTests._should.ignore.testDetachFocusOnLastWidgetDestroy = true;

testConsole = new Y.Console({
id:"testConsole",
width:"100%",
Expand Down

0 comments on commit 712600e

Please sign in to comment.