Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

TabView shouldn't make assumptions about labels #198

Closed
wants to merge 1 commit into from

4 participants

Juan Ignacio Dopazo Matt Sweeney Adrian Lanning peacemoon
Juan Ignacio Dopazo
Collaborator

This is a very common question in the forums.

At first I thought it was agreed that components should escape content to prevent XSS vulnerabilities, but tabview.add({ label: '<span>foo</span>' }) seems to be working. However, progressive enhancement is still removing HTML.

This patch changes the way tabs are added from markup. Instead of reading the content of the label and resetting it (which resets the label HTML and breaks existing node references) it just sets the contentBox. Then Tab is changed to have a getter that returns the html inside the content box, just like the content attribute does with the panel node.

This question is still open though: do we allow HTML by default or do we add an extra HTMLTab subclass of Tab?

Matt Sweeney msweeney was assigned
Matt Sweeney
Collaborator

Thanks Juan. I plan to roll this in for the next release, along with some other TabView-related work.

Juan Ignacio Dopazo
Collaborator

I think we should discuss this at some point anyway. What would be the correct way to set content in widgets? Maybe this is an issue of HTML_PARSER setting attributes too late and it can be solved by making it set attributes before listeners are added.

Adrian Lanning

Re: allowing HTML vs escaping, I believe having HTML_PARSER allow HTML by default is appropriate since it would be consistent with the behavior when using a config obj. IIRC, there are many instances where the docs describe the parser as just an alternate way to config the widget.

Regarding the implicit question of escaping content, templating libraries like Handlebars use a switch to indicate escape behavior. For example, using "{{&variable}}" or "{{{variable}}}" to indicate unsafe substitution rather than the safe-by-default behavior of "{{variable}}".

Perhaps it would be appropriate to add a switch to Widget to indicate a kind of global escape "on/off".

peacemoon

Is there any update with this issue? I also want to use html for labels of the tabview

Matt Sweeney
Collaborator

Apologies for not commenting sooner. In the near-term we should fix the issue with tabview that this patch addresses.

Longer-term we can reconsider the appropriate behavior for HTML_PARSER and template escaping.

Juan Ignacio Dopazo
Collaborator

I think we should close this pull request. I want to understand a little bit better the lifecycle of Widget+HTML_PARSER and see if there's a better way of dealing with this.

Juan Ignacio Dopazo juandopazo closed this
Matt Sweeney
Collaborator

#404 addresses all but the removal of the label parsing from _renderTabs(). I would prefer to remove that and address this issue as well.

My only (slight) concern is backcompat for the edge case of having existing HTML inside the Tab's contentBox, which would return the innerHTML of the contentBox, rather than the text via tab.get('label') when rendering from existing markup. I imagine most would prefer to keep the existing markup intact, so this is only a slight concern.

@juandopazo Do you have other reservations at this point?

Juan Ignacio Dopazo
Collaborator

No, I'm working on other things and #404 looks good so far. I'll probably get back to this in the future.

Matt Sweeney
Collaborator

I went ahead and removed the label parsing from #404.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 20, 2012
  1. Juan Ignacio Dopazo
This page is out of date. Refresh to see the latest.
Showing with 8 additions and 4 deletions.
  1. +8 −3 src/tabview/js/tab.js
  2. +0 −1  src/tabview/js/tabview.js
11 src/tabview/js/tab.js
View
@@ -110,17 +110,21 @@ Y.Tab = Y.Base.create('tab', Y.Widget, [Y.WidgetChild], {
},
_defLabelSetter: function(label) {
- this.get('contentBox').setContent(label);
+ this.get('contentBox').setHTML(label);
return label;
},
+ _defLabelGetter: function(label) {
+ return this.get('contentBox').getHTML();
+ },
+
_defContentSetter: function(content) {
- this.get('panelNode').setContent(content);
+ this.get('panelNode').setHTML(content);
return content;
},
_defContentGetter: function(content) {
- return this.get('panelNode').getContent();
+ return this.get('panelNode').getHTML();
},
// find panel by ID mapping from label href
@@ -167,6 +171,7 @@ Y.Tab = Y.Base.create('tab', Y.Widget, [Y.WidgetChild], {
*/
label: {
setter: '_defLabelSetter',
+ getter: '_defLabelGetter',
validator: Lang.isString
},
1  src/tabview/js/tabview.js
View
@@ -129,7 +129,6 @@ var _queries = Y.TabviewBase._queries,
tabview.add({
boundingBox: node,
contentBox: node.one(DOT + _classNames.tabLabel),
- label: node.one(DOT + _classNames.tabLabel).get('text'),
panelNode: panelNode
});
});
Something went wrong with that request. Please try again.