Added HTML support for Button labels #1163

derek · 2013-09-04T22:25:46Z

Currently Y.Button widgets do not support HTML labels. This is an issue because is isn't uncommon to use nested HTML structures inside of a <button> element. For example...

<button>
    <p>I Am A</p>
    <p>Button</p>
</button>

<button>
    <div>Image Button</div>
    <img src="path/to/image.jpg">
</button>

To fix this issue, this pull request introduces a labelHTML ATTR, and relegates the label ATTR as secure sugar around labelHTML.

A separate attribute is required for the following reasons:

To not introduce security vulnerabilities to existing implementations who use the label ATTR
To convey the insecurity of this attribute by including "HTML" in the name

Additional notes

Using @config instead of @attribute in ButtonCore documentation because @attribute will include the Change events to the API documentation, which ButtonCore (derived from AttributeCore) does not fire.
Fixed unrelated breakage in Selleck tests

Test Results

✓ Agent completed: Internet Explorer (9.0) / Windows 7
✓ Agent completed: Chrome (29.0.1547.65) / Mac OS
✓ Agent completed: Safari (6.0.5) / Mac OS
✓ Agent completed: Internet Explorer (10.0) / Windows 8
✓ Agent completed: Internet Explorer (8.0) / Windows XP
✓ Agent completed: Internet Explorer (6.0) / Windows XP
✓ Agent completed: Internet Explorer (7.0) / Windows XP
✓ Agent completed: Firefox (23.0) / Mac OS
✓ Agent completed: Safari (4.0) / Linux  (Android 2.3.4)
✓ Agent completed: Safari (4.0) / Linux  (Android 3.0.1)

✓ 352 tests passed! (1 minutes, 2 seconds)

… label ATTR to non-HTML.

…of its host node.

ericf · 2013-09-04T22:38:27Z

src/button/js/button.js

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


Does this mean this change will break backwards compatibility?

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.

ericf · 2013-09-04T22:40:11Z

What happens when the labelHTML is set on an <input type="submit" value="Save"> button node?

rgrove · 2013-09-04T23:00:59Z

src/button/js/button.js

+         * IMPORTANT: Sanitize all input passed into this attribute
+         *
+         * @attribute labelHTML
+         * @type {HTML|String}


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

Fixed in next push

derek · 2013-09-04T23:12:31Z

@ericf The provided text string is applied to the <input> element's value attribute and renders as a string.

So, with your provided HTML, executing this:

button.set('labelHTML', '<div>foo bar</div>');

Renders as

I just reviewed some of this behavior with @msweeney. He suggested some additional documentation to clarify that if you are dealing with an <input> element that you should only use label, but didn't see any issues to address.

rgrove · 2013-09-04T23:13:59Z

src/button/js/core.js

-     * @method _uiSetLabel
+     * @method _getLabel
+     * @description Getter for a button's 'label' ATTR
+     * @return {String} The label for a given node


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

See: #1163 (comment)

…y removing most of the now unneccesary prototype properties. * `Y.Button.ATTRS` now points to `Y.ButtonCore.ATTRS` * Improved code documentation

derek · 2013-09-05T19:26:02Z

As a result of an investigation into one of @rgrove's questions, I was able to fix the issue where _host was not set until Y.Button's initializer. Once that was fixed, it allowed Y.Button to use Y.ButtonCore's ATTRs, which allowed the removal of all the Y.Button-specific code to sync the ATTRs. As a result, Y.Button is now just a thin layer of Widget-specific properties which tie together methods available in Y.ButtonCore, which was originally the intended design for Y.Button.

P.S. All hidden diff comments have either been addressed, or are still a WIP.

rgrove · 2013-09-09T22:28:39Z

src/button/js/button.js

+     * user provided content, appropriate sanitization steps should first be
+     * conducted to ensure the content is safe for insertion into the DOM.
+     * This process typically involves removing any &lt;script&gt; or
+     * &lt;iframe&gt; tags, or other HTML that can be considered malicious.


This isn't actually good advice, since it implies that blacklist-based tag filtering is a good approach, and also assumes that sanitization (rather than escaping) is the way to go, which is very rarely true in client-side JS.

This comment should just advocate escaping (using Y.Escape.html()) and leave it at that. Actual HTML sanitization is way out of scope.

To avoid clobbering the diff discussion with another commit, how about this instead?

/** * The HTML of the button's label * * **IMPORTANT** * This attribute accepts HTML and inserts it into the DOM **without** * sanitization. This attribute should only be used with HTML that has * either been escaped (using `Y.Escape.html`), or sanitized according to * the requirements of your application. * * If all you need is support for text labels, please use the `label` * attribute instead. * * @attribute labelHTML * @type HTML */

Looks good! Feel free to omit the "IMPORTANT" though. That might be taking it too far. :)

Pushed update.

derek · 2013-09-10T00:36:20Z

src/button/js/core.js

+        tagName = labelNode.get('tagName').toLowerCase(),
+        label = labelNode.get(tagName === 'input' ? 'value' : 'text');
+
+    label = Y.Escape.html(label);


@rgrove This escape was added to address this comment, though due to this getter now only returning a String and not mixed HTML|String, I believe the escapement is no longer necessary. Can you confirm and I'll remove it?

Removed, but happy to add it back if neccesary.

…/w3c_core.html#t23

derek · 2013-09-10T02:22:08Z

I reviewed all the outdated diffs and believe all questions/issues have been addressed. Let me know if there's anything I missed.

rgrove · 2013-09-10T16:42:11Z

Looks good to me. Thanks @derek. 👍

ericf · 2013-09-27T18:26:09Z

src/button/js/core.js

     * @public
     */
    getNode: function() {
+        if (!this._host && this instanceof Y.Widget) {


instanceof will fail across YUI instances. Duck-typing would be more robust in this case.

Rethinking that line, checking if this is an instance of Y.Widget is a little over-cautious. ButtonCore inherits from AttributeCore, so it will always have a get method, so falling into the condition can't result in a script error. If the instance is a Widget, get('boundingBox') will return the node, if not, it will return undefined, which was already the value of this._host going into the branch. Update coming.

ericf · 2013-09-27T18:33:24Z

@derek This should have tests for input elements, because there are many code branches on whether a node is an input.

What is the coverage for the button modules? And what browsers was this tested in?

derek · 2013-09-27T18:52:40Z

What is the coverage for the button modules? And what browsers was this tested in?

Button has 100% coverage (S, B, F, & L) and tested browsers are noted in the PR description. But another round of testing will be done, assuming additional tests are added and code changes occur.

ericf · 2013-09-27T21:28:17Z

@derek I added line comments earlier today as well…

… the `disabled` ATTR instead of the node's attribute property.

derek · 2013-09-27T23:32:45Z

After the additional commit

✓ Agent completed: Safari (6.0.5) / Mac OS
✓ Agent completed: Chrome (29.0.1547.76) / Mac OS
✓ Agent completed: Firefox (24.0) / Mac OS
✓ Agent completed: Safari (7.0) / iOS 7.0
✓ Agent completed: Internet Explorer (10.0) / Windows 8
✓ Agent completed: Internet Explorer (9.0) / Windows 7
✓ Agent completed: Internet Explorer (8.0) / Windows XP
✓ Agent completed: Internet Explorer (6.0) / Windows XP
✓ Agent completed: Internet Explorer (7.0) / Windows XP
✓ 423 tests passed! (14 seconds)

derek · 2013-09-28T00:06:14Z

This should have tests for input elements, because there are many code branches on whether a node is an input.

There are existing tests for <input> elements to cover all code branches, but I'll look into adding some more to strengthen Button's suite of unit tests. I added a line item for it on Button's Wishlist.

The reason there are no unit tests addressing <input> behavior for this pull request is because this PR adds support for HTML labels, which <input> elements do not support.

derek added 10 commits August 29, 2013 15:49

Improved ButtonCore's handling and preservation of HTML labels

584a81f

Added tests to ensure nested labels behave correctly

45c04fa

Added labelHTML ATTR to Y.Button and Y.ButtonCore, and restricted the…

48a6b63

… label ATTR to non-HTML.

Updated HTML label tests to reference the labelHTML ATTR

aa44a3a

Added escape to ButtonCore dependencies

44e2abe

Documentation updates. get('label') now returns the 'text' attribute …

1b4758e

…of its host node.

Added test

379bc02

Fixed failing Selleck tests

f1c0683

Fixed failing tests in IE

c6210fc

Merge branch 'dev-3.x' into button-labels

53291c7

This was referenced Sep 4, 2013

Added HTML support for Button labels #1162

Closed

Various issues with Button #973

Open

ericf reviewed Sep 4, 2013
View reviewed changes

rgrove reviewed Sep 4, 2013
View reviewed changes

derek added 4 commits September 5, 2013 00:50

* Enhancements to ButtonCore allowed for simplification of Y.Button b…

672d56f

…y removing most of the now unneccesary prototype properties. * `Y.Button.ATTRS` now points to `Y.ButtonCore.ATTRS` * Improved code documentation

Updated HISTORY

5dfafaa

Locally scoped some objects hanging off Y

3e3bfdd

Moved ButtonCore private utility methods to static privates.

2aad2f3

Added additional tests

91518bb

Improved labelHTML ATTR's security warning.

b582cf5

rgrove reviewed Sep 9, 2013
View reviewed changes

derek added 2 commits September 9, 2013 15:30

Updated Y.ButtonCore's 'label' ATTR getter to HTML escape returned value

e2c46c5

Improved labelHTML ATTR's security warning.

af22f98

derek reviewed Sep 10, 2013
View reviewed changes

derek added 2 commits September 9, 2013 17:45

Using nodeName instead of tagName. See: http://www.quirksmode.org/dom…

652fd97

…/w3c_core.html#t23

Removed unneccesary escaping in Y.ButtonCore's ATTR getter.

08ae388

Added semicolons to resolve lint warnings

12d93ac

ericf reviewed Sep 27, 2013
View reviewed changes

Removed unneccesary instanceof check, and updated ButtonCore to fetch…

77340c8

… the `disabled` ATTR instead of the node's attribute property.

ericf merged commit 77340c8 into yui:dev-3.x Sep 28, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added HTML support for Button labels #1163

Added HTML support for Button labels #1163

derek commented Sep 4, 2013

ericf Sep 4, 2013

derek Sep 5, 2013

ericf commented Sep 4, 2013

rgrove Sep 4, 2013

derek Sep 5, 2013

derek commented Sep 4, 2013

rgrove Sep 4, 2013

derek Sep 10, 2013

derek commented Sep 5, 2013

rgrove Sep 9, 2013

derek Sep 9, 2013

rgrove Sep 9, 2013

derek Sep 10, 2013

derek Sep 10, 2013

derek Sep 10, 2013

derek commented Sep 10, 2013

rgrove commented Sep 10, 2013

ericf Sep 27, 2013

derek Sep 27, 2013

ericf commented Sep 27, 2013

derek commented Sep 27, 2013

ericf commented Sep 27, 2013

derek commented Sep 27, 2013

derek commented Sep 28, 2013

Added HTML support for Button labels #1163

Added HTML support for Button labels #1163

Conversation

derek commented Sep 4, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericf commented Sep 4, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derek commented Sep 4, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derek commented Sep 5, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derek commented Sep 10, 2013

rgrove commented Sep 10, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericf commented Sep 27, 2013

derek commented Sep 27, 2013

ericf commented Sep 27, 2013

derek commented Sep 27, 2013

derek commented Sep 28, 2013