Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added HTML support for Button labels #1163

Merged
merged 22 commits into from
Sep 28, 2013
Merged

Added HTML support for Button labels #1163

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
584a81f
Improved ButtonCore's handling and preservation of HTML labels
derek Aug 29, 2013
45c04fa
Added tests to ensure nested labels behave correctly
derek Aug 29, 2013
48a6b63
Added labelHTML ATTR to Y.Button and Y.ButtonCore, and restricted the…
derek Sep 4, 2013
aa44a3a
Updated HTML label tests to reference the labelHTML ATTR
derek Sep 4, 2013
44e2abe
Added escape to ButtonCore dependencies
derek Sep 4, 2013
1b4758e
Documentation updates. get('label') now returns the 'text' attribute…
derek Sep 4, 2013
379bc02
Added test
derek Sep 4, 2013
f1c0683
Fixed failing Selleck tests
derek Sep 4, 2013
c6210fc
Fixed failing tests in IE
derek Sep 4, 2013
53291c7
Merge branch 'dev-3.x' into button-labels
derek Sep 4, 2013
672d56f
* Enhancements to ButtonCore allowed for simplification of Y.Button b…
derek Sep 5, 2013
5dfafaa
Updated HISTORY
derek Sep 5, 2013
3e3bfdd
Locally scoped some objects hanging off Y
derek Sep 5, 2013
2aad2f3
Moved ButtonCore private utility methods to static privates.
derek Sep 5, 2013
91518bb
Added additional tests
derek Sep 5, 2013
b582cf5
Improved labelHTML ATTR's security warning.
derek Sep 9, 2013
e2c46c5
Updated Y.ButtonCore's 'label' ATTR getter to HTML escape returned value
derek Sep 9, 2013
af22f98
Improved labelHTML ATTR's security warning.
derek Sep 10, 2013
652fd97
Using nodeName instead of tagName. See: http://www.quirksmode.org/do…
derek Sep 10, 2013
08ae388
Removed unneccesary escaping in Y.ButtonCore's ATTR getter.
derek Sep 10, 2013
12d93ac
Added semicolons to resolve lint warnings
derek Sep 10, 2013
77340c8
Removed unneccesary instanceof check, and updated ButtonCore to fetch…
derek Sep 27, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/button/docs/assets/button-basic-tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ YUI.add('button-basic-tests', function(Y){

var SELECTED_CLASS = 'yui3-button-selected',
WRONG_TEXT = ' has the wrong text.',
SELECTED_TEXT = 'this pressed button :)',
SELECTED_TEXT = 'this pressed button :)',
UNSELECTED_TEXT = 'this depressed button :(',
IS_SELECTED = ' is selected.',
IS_NOT_SELECTED = ' is not selected.';
Expand Down
2 changes: 1 addition & 1 deletion src/button/docs/assets/cssbutton-tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
YUI.add('cssbutton-tests', function(Y){

Y.Test.Runner.add(new Y.Test.Case({
new: 'CSS Button Tests',
name: 'CSS Button Tests',

'should contain 14 buttons': function() {
Y.Assert.areSame(14, Y.all('.example .yui3-button').size());
Expand Down
46 changes: 29 additions & 17 deletions src/button/js/button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ Y.extend(Button, Y.Widget, {
* @type {String}
* @default null
*/
CONTENT_TEMPLATE : null,
CONTENT_TEMPLATE : null,

/**
* @method initializer
Expand All @@ -58,6 +58,10 @@ Y.extend(Button, Y.Widget, {
if (config.disabled) {
this.set('disabled', config.disabled);
}

if (config.label) {
this.set('label', config.label);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why this needs to be here? And, for that matter, the config.disabled block above as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally label isn't a lazy attribute, but label's setter can't properly execute until this._host is set, which doesn't occur until a few lines before this one in the initializer....

... and nevermind, I just found the solution. ButtonCore should try to populate this._host with this.get('boundingBox') if it isn't already set.

Oh, and the disabled setter can also be removed. After some experimentation, #406 did fix targeted issue, but executing disabled's setter in the initializer wasn't required. I think at least. Regardless, it works as expected now.

So looks like I can actually remove this entire method now. Update coming soon.

Update: Resolved.

}
},

/**
Expand All @@ -68,7 +72,7 @@ Y.extend(Button, Y.Widget, {
*/
bindUI: function() {
var button = this;
button.after('labelChange', button._afterLabelChange);
button.after('labelHTMLChange', button._afterLabelHTMLChange);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean this change will break backwards compatibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bindUI is actually being removed from Y.Button as I believe it is no longer necessary now with the proper setters in place.

FWIW, an intent of this pull request is to not break backwards compat in Button. I'm only introducing new features, as well as fixes to meet expected behavior.

button.after('disabledChange', button._afterDisabledChange);
},

Expand All @@ -78,28 +82,24 @@ Y.extend(Button, Y.Widget, {
*/
syncUI: function() {
var button = this;
Y.ButtonCore.prototype._uiSetLabel.call(button, button.get('label'));
Y.ButtonCore.prototype._uiSetDisabled.call(button, button.get('disabled'));
Y.ButtonCore.prototype._setLabelHTML.call(button, button.get('labelHTML'));
Y.ButtonCore.prototype._setDisabled.call(button, button.get('disabled'));
},

/**
* @method _afterLabelChange
* @method _afterLabelHTMLChange
* @private
*/
_afterLabelChange: function(e) {
Y.ButtonCore.prototype._uiSetLabel.call(this, e.newVal);
_afterLabelHTMLChange: function(e) {
Y.ButtonCore.prototype._setLabelHTML.call(this, e.newVal);
},

/**
* @method _afterDisabledChange
* @private
*/
_afterDisabledChange: function(e) {
// Unable to use `this._uiSetDisabled` because that points
// to `Y.Widget.prototype._uiSetDisabled`.
// This works for now.
// @TODO Investigate most appropriate solution.
Y.ButtonCore.prototype._uiSetDisabled.call(this, e.newVal);
Y.ButtonCore.prototype._setDisabled.call(this, e.newVal);
}

}, {
Expand Down Expand Up @@ -127,15 +127,27 @@ Y.extend(Button, Y.Widget, {
* @static
*/
ATTRS: {

/**
* The text of the button (the `value` or `text` property)
*
* @attribute label
* @type String
*/
label: {
value: Y.ButtonCore.ATTRS.label.value
getter: Y.ButtonCore.ATTRS.label.getter,
setter: Y.ButtonCore.ATTRS.label.setter
},

/**
* The HTML of the button's label
*
* IMPORTANT: Sanitize all input passed into this attribute
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please explain what this means. As is, it's a little vague and I can imagine someone glossing over it because they're not sure it applies to them, or because they don't know how to sanitize input.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated with a more detailed notice. https://github.com/yui/yui3/pull/1163/files#L4R246

*
* @attribute labelHTML
* @type {HTML|String}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If labelHTML is always treated as HTML, then its type should be just HTML.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in next push

*/
labelHTML: {
setter: Y.ButtonCore.ATTRS.labelHTML.setter
}
},

Expand All @@ -146,9 +158,9 @@ Y.extend(Button, Y.Widget, {
* @static
*/
HTML_PARSER: {
label: function(node) {
this._host = node; // TODO: remove
return this._getLabel();

labelHTML: function(node) {
return this._getLabelFromNode(node, true);
},

disabled: function(node) {
Expand Down
105 changes: 80 additions & 25 deletions src/button/js/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,6 @@ ButtonCore.prototype = {
* @private
*/
_initAttributes: function(config) {
var host = this._host,
node = host.one('.' + ButtonCore.CLASS_NAMES.LABEL) || host;

config.label = config.label || this._getLabel(node);
Y.AttributeCore.call(this, ButtonCore.ATTRS, config);
},

Expand All @@ -97,7 +93,7 @@ ButtonCore.prototype = {

/**
* @method enable
* @description Sets the button's `disabled` DOM attribute to false
* @description Sets the button's `disabled` DOM attribute to `false`
* @public
*/
enable: function() {
Expand All @@ -106,7 +102,7 @@ ButtonCore.prototype = {

/**
* @method disable
* @description Sets the button's `disabled` DOM attribute to true
* @description Sets the button's `disabled` DOM attribute to `true`
* @public
*/
disable: function() {
Expand All @@ -115,59 +111,105 @@ ButtonCore.prototype = {

/**
* @method getNode
* @description Gets the host DOM node for this button instance
* @description Gets the host's DOM node for this button instance
* @return {Node} The host node instance
* @public
*/
getNode: function() {
return this._host;
},

/**
* @method _getLabel
* @description Getter for a button's 'label' ATTR
* @method _getLabelFromNode
* @param node {Node} The Y.Node instance to obtain the label from
* @return {HTML|String} The label for a given node
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method should return either HTML or plain text, and should be documented as such. Currently, it appears to return HTML sometimes and plain text sometimes, which is unsafe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed all {HTML|String} return types from the code.

* @private
*/
_getLabel: function () {
var node = this.getNode(),
tagName = node.get('tagName').toLowerCase(),
_getLabelFromNode: function (node, html) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The html parameter isn't documented, but it appears to be a boolean? Please don't use boolean arguments. If necessary, use an options object with a boolean property instead. This will provide some context to readers of the calling code, which is especially important in this case given that misunderstanding the meaning of the second parameter could result in a security issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In next push, _getLabelFromNode will be replaced with getTextLabelFromNode and getHTMLFromNode to serve the purpose of returning explicit data types. The 2nd arg has been removed all together.

var tagName = node.get('tagName').toLowerCase(),
label;

if (tagName === 'input') {
label = node.get('value');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if label is treated as HTML, then this is unsafe, as it could result in unsanitized user input being used as an HTML label.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As of 08ae388, _getLabelFromNode has been split into two different methods now. One returning HTML and the other returning a String.

}
else {
label = (node.one('.' + ButtonCore.CLASS_NAMES.LABEL) || node).get('text');
node = (node.one('.' + ButtonCore.CLASS_NAMES.LABEL) || node);
if (html) {
label = node.getHTML();
}
else {
label = node.get('text');
}
}

return label;
},

/**
* @method _uiSetLabel
* @method _getLabel
* @description Getter for a button's 'label' ATTR
* @return {String} The label for a given node
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since _getLabelFromNode() might return unsanitized HTML, this method must also be considered unsafe until _getLabelFromNode() is updated to always return something safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See: #1163 (comment)

* @private
*/
_getLabel: function () {
var node = this.getNode(),
label = this._getLabelFromNode(node);

return label;
},

/**
* @method _setLabel
* @description Getter for a button's 'label' ATTR
* @private
*/
_setLabel: function (value) {
var labelHTML = Y.Escape.html(value);

this.set('labelHTML', labelHTML);

return labelHTML;
},

/**
* @method _getLabelHTML
* @description Getter for a button's 'label' ATTR
* @return {HTML|String} The label for a given node
* @private
*/
_getLabelHTML: function () {
var node = this.getNode();

return this._getLabelFromNode(node, true);
},

/**
* @method _setLabelHTML
* @description Setter for a button's 'label' ATTR
* @param label {string}
* @param label {HTML|String} The label to set
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type is always HTML, never String.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in next push

* @return {String} The label for a given node
* @private
*/
_uiSetLabel: function (label) {
_setLabelHTML: function (label) {
var node = this.getNode(),
tagName = node.get('tagName').toLowerCase();

if (tagName === 'input') {
node.set('value', label);
} else {
(node.one('.' + ButtonCore.CLASS_NAMES.LABEL) || node).set('text', label);
(node.one('.' + ButtonCore.CLASS_NAMES.LABEL) || node).setHTML(label);
}

return label;
},

/**
* @method _uiSetDisabled
* @method _setDisabled
* @description Setter for the 'disabled' ATTR
* @param value {boolean}
* @private
*/
_uiSetDisabled: function(value) {
_setDisabled: function(value) {
var node = this.getNode();

node.getDOMNode().disabled = value; // avoid rerunning setter when this === node
Expand All @@ -191,26 +233,39 @@ Y.mix(ButtonCore.prototype, Y.AttributeCore.prototype);
ButtonCore.ATTRS = {

/**
* The text of the button (the `value` or `text` property)
* The text of the button's label
*
* @attribute label
* @type String
* @config label
* @type {String}
*/
label: {
setter: '_uiSetLabel',
setter: '_setLabel',
getter: '_getLabel',
lazyAdd: false
},
/**
* The HTML of the button's label
*
* IMPORTANT: Sanitize all input passed into this attribute
*
* @config labelHTML
* @type {HTML|String}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type is always HTML.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in next push

*/
labelHTML: {
setter: '_setLabelHTML',
getter: '_getLabelHTML',
lazyAdd: false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this attribute lazyAdd: false?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the Attribute docs state

The only reason you should need to disable lazyAdd is if your setter is doing more than normalizing the attribute value. For example if your setter is storing some other state, in this._someProp, which is being used by other parts of your component. In this case, since you're not getting or setting the attribute to access this._someProp, it won't get set up lazily, until someone actually calls get or set for the related attribute.

This applies to labelHTML because its setter also updates the label ATTR since they need to be in-sync. By invoking this ATTR's setter upon instantiation, it ensures that any value for labelHTML that is passed in with the configuration object will (almost) immediately update the label ATTR value as well.

This snippet demonstrates the difference.

var button = new Y.Button({
    srcNode: Y.Node.create('<button>Some old value</button>'),
    labelHTML: 'Some new value'
}).render();

// With lazyAdd = true
console.log(button.get('label')); // Some old value
console.log(button.get('labelHTML')); // Some new value

// With lazyAdd = false
console.log(button.get('label')); // Some new value
console.log(button.get('labelHTML')); // Some new value

},

/**
* The button's enabled/disabled state
*
* @attribute disabled
* @config disabled
* @type Boolean
*/
disabled: {
value: false,
setter: '_uiSetDisabled',
setter: '_setDisabled',
lazyAdd: false
}
};
Expand Down
3 changes: 2 additions & 1 deletion src/button/meta/button.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
"requires": [
"attribute-core",
"classnamemanager",
"node-base"
"node-base",
"escape"
]
},

Expand Down
28 changes: 26 additions & 2 deletions src/button/tests/unit/assets/button-core-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ YUI.add('button-core-test', function (Y) {
name: 'Attributes',

setUp : function () {
Y.one("#container").setContent('<button id="testButton">Hello</button>');
Y.one("#container").setContent('<button id="testButton"><div>Hello</div><div>World</div></button>');
this.button = new Y.ButtonCore({
host: Y.one("#testButton")
});
Expand Down Expand Up @@ -97,7 +97,31 @@ YUI.add('button-core-test', function (Y) {
Assert.areEqual(1, eventsTriggered);
button.set('label', 'somethingElse');
Assert.areEqual(2, eventsTriggered);
}
},

'Getting the `label` attribute should NOT respect nested HTML': function () {
var button = this.button,
expected = 'HelloWorld',
actual = button.get('label').replace(/\r\n/g, '');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are expected and actual different? Shouldn't this test be testing the real result of get('label'), not the result of get('label') after newlines are removed? If newlines only occur in some browsers, then isn't that a signal that they should be filtered in button itself to ensure consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test has been updated to longer use replace. While I originally did consider newline filtering inside of ButtonCore, I opted for the more hands-off approach to preserve current behavior. If someone would like to file a bug/enhancement ticket, I think we can look into it a bit more. One consideration is that because this isn't something ButtonCore-specific, the appropriate place for the normalization might be inside of Node.


Assert.areSame(expected, actual);
},

'Getting the `labelHTML` attribute should respect nested HTML': function () {
var button = this.button,
expected = '<div>hello</div><div>world</div>',
actual = button.get('labelHTML').toLowerCase().replace(/\r\n/g, '');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTML is obviously likely to differ across browsers so this is less unexpected than the plaintext label test, but it would still be better to generate the expected value in such a way that you can reliably compare it to the actual value without doing a regex replace.

For example, you could generate expected by setting an element's contents and pulling the HTML out of it. That should ensure a reliable comparison with actual based on the actual behavior of the current browser rather than on assumptions baked into tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So something like...

var button = this.button,
    expected = button.getNode().getHTML(),
    actual = button.get('labelHTML');

Which essentially points to the same DOM method, one is just passed through ButtonCore before the test gets the result?

I guess that makes sense though considering this unit test isn't to ensure the DOM's behavior is correct, but rather that ButtonCore does the right thing with what the DOM (and Y.Node) gives it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in next push.


Assert.areSame(expected, actual);
},

'Setting the `label` attribute with HTML should escape the value': function () {
var button = this.button;

button.set('label', '<div>foo</div>');
Assert.areSame('<div>foo</div>', button.get('label'));
Assert.areSame('&lt;div&gt;foo&lt;/div&gt;', button.getNode().get('innerHTML'));
}
}));

suite.add(new Y.Test.Case({
Expand Down
Loading